Skip to content

Passed along any original 'target' attribute to the generated form tag in handleMethod()#207

Closed
gregjacobs wants to merge 1 commit intorails:masterfrom
gregjacobs:master
Closed

Passed along any original 'target' attribute to the generated form tag in handleMethod()#207
gregjacobs wants to merge 1 commit intorails:masterfrom
gregjacobs:master

Conversation

@gregjacobs
Copy link

Passed along any original 'target' attribute to the generated form tag in handleMethod(). Fixes rails/rails#2885 for jquery-ujs.

I don't seem to be able to write a unit test for this however with the current code in the test harness. The 'target' attribute on the generated form tag is overwritten in test/settings.js when the form is submitted, so there seems to be no way to confirm the expected behavior.

@JangoSteve
Copy link
Member

Interesting, I'll take a look at this. I don't like that there's no way to test it, as I would like to have two test cases, one that makes sure the target is passed along, and one that makes sure it uses the current window as the target when none exists.

@JangoSteve
Copy link
Member

BTW, this somewhat relates to #189, however, I think this one is a bit more straight forward.

@gregjacobs
Copy link
Author

Yeah, I tried to write tests for it, but then I didn't want to modify the code that generalized the submission of iframes within the tests (which is what overwrites the 'target' attribute). Unless perhaps that code could restore the original 'target' attribute after the form is submitted, so the form tag's 'target' attribute could then be tested afterward? I'll give that a try tomorrow.

@JangoSteve
Copy link
Member

Hey @Gjslick, thanks for your help. I had some time, so I went ahead and rewrote some of the test setup to make testing this possible. Also, I wanted to implement it slightly differently so that we're only adding the target attribute to the hidden form if the link actually has a target attribute, rather than just always adding it with an empty string.

I meant to pull in your commit and then make the changes on top of it, but somehow in all my branching and merging to get the test suite working I ended up pushing a clean commit before realizing it. Sorry about that, but thanks for submitting this.

Anyway, this is fixed now in 888be8a

@JangoSteve JangoSteve closed this Oct 3, 2011
@gregjacobs
Copy link
Author

Hey, awesome man, glad you got it working with tests! And np on the pull request :)

All the best!

Greg

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

link_to with method post and a target does not work

2 participants