Update val.js #1531
Update val.js #1531
Conversation
Seeing a behavior change in jQuery('option').val() as reported in http://bugs.jquery.com/ticket/14858. Behavior of .val() was changed which may return insignificant white space when a value attribute is not used.
Seems to have been changed because of this ticket:
http://bugs.jquery.com/ticket/14686
in this commit:
92cbf53#diff-ac2456f6444b226fd09f497031b2f7d2
Since the problem is in IE10/11 and only for an empty value, can we simply return element.text. If an exception is caught, return an empty string?
|
Thanks for investigating this! Usually we like to steer clear of try/catch, they can mask other problems and they also defeat optimization in several browsers. You can see the Travis build failed, looks like you need to brush on the style guide. In most cases it's easy to avoid those problems by being really observant about the surrounding style, but some older untouched areas of the code may not be good examples. The guide at http://contribute.jquery.org/style-guide/js/ is the best way to go. |
|
This will also require new tests to prove the patch addresses the reported issue. |
|
I am wondering if the bug that caused us to originally change the code like this is now gone from IE10/11, because I can't seem to reproduce in either using bare DOM: http://jsfiddle.net/2avmY/1/ @markelog maybe we can back out this patch? |
|
I can reproduce the originial bug on my laptop here, although I don't have IE10 set to auto-update on that PC. |
|
@johnhoven Run an update and see what happens. I'm running 11.0.3 (KB2909921) |
Using style guide to update my pull, add a comment as to why I'm using try/catch.
|
Checking for updates now. I noticed the 2.x branch is still using .text (not jQuery.text()). Regarding preferring not to use try/catch. The only alternative I can see is to continue to use jQuery.text and trimming the result, however I'm not sure that is equivalent. Thoughts? I'll look in to adding test cases in case you can't pull this update. |
Style guide updates.
|
We should keep using jQuery.text and make needed adjustments for IE11 there. |
|
"value = string Maybe I'm backwards. Maybe 1.11.0+ is the only version returning the correct value for this option? Opinion? Given |
|
As far as I can tell I'm on latest update of IE 10. Check for updates isn't offering anything. I'll check again in the morning. |
From #1531, currently discussing if the result from val() should be equivalent to textContent in this scenario. This test is to be consistent with 1.10.1 and 2.x branches. It may be that 1.11.0 is the correct branch. Given that the browser would show an empty option value for this (no white-space long), and that the w3.org spec on option has the same definition for an empty label and value, and that the length of label for the following snippet would be zero, I'll argue that the value shoudd also be a length of zero.
|
@dmethvin, I also tried on IE11 at home (11.0.3) and the same problem persists. I looked at your fiddle, and it wasn't value that was the problem, but element.text (which is what lead to the commit which switched to jQuery.text). I've updated your fiddle to use text: http://jsfiddle.net/2avmY/3/ |
|
Duh! Yes, I can repro now. |
| return val; | ||
| } else { | ||
|
|
||
| /* In IE10/11 (at least before a certain version) |
There was a problem hiding this comment.
This doesn't match our comment style. Multiple lines should start with //; also, ther should be a // Support: IE10-11+ comment just before the workaround code.
|
Maybe we're missing a guide here. I meant without enclosing /* */, just //. Michał Gołębiowski |
|
Actually, the style guide specifically says you can use As far as I can tell, this was copied from the original style guide on MediaWiki. I think we should remove this since we never use it, but this isn't the best place to discuss that change. |
|
@johnhoven can we just change |
|
Yes, I was thinking trim would be sufficient to avoid the try/catch. I'll On Wed, Mar 19, 2014 at 2:37 PM, Dave Methvin notifications@github.comwrote:
John Hoven |
|
I'll make the code change and land the unit test under your name @johnhoven |
Fixes #14858 Ref #14686 Closes gh-1531
Fixes #14858 Ref #14686 Closes jquerygh-1531
Seeing a behavior change in jQuery('option').val() as reported in http://bugs.jquery.com/ticket/14858. Behavior of .val() was changed which may return insignificant white space when a value attribute is not used.
Seems to have been changed because of this ticket:
http://bugs.jquery.com/ticket/14686
in this commit:
92cbf53#diff-ac2456f6444b226fd09f497031b2f7d2
Since the problem is in IE10/11 and only for an empty value, can we simply return element.text. If an exception is caught, return an empty string?