Attributes: strip/collapse whitespace for set values on selects #3002

timmywil · 2016-03-15T22:16:31Z

gibson042 · 2016-03-15T22:21:25Z

src/attributes/val.js

@@ -4,7 +4,8 @@ define( [
 	"../core/init"
 ], function( jQuery, support ) {

-var rreturn = /\r/g;
+var rreturn = /\r/g,
+	rspaces = /\s/g;


HTML whitespace is a smaller set than that matched by \s, and this doesn't account for longer runs. I think you want /[\x20\t\r\n\f]+/g, matching various other regular expressions in our codebase.

Ha, I was just going off your fiddle.

gibson042 · 2016-03-15T22:26:38Z

src/attributes/val.js

+					// https://html.spec.whatwg.org/#strip-and-collapse-whitespace
+					values = jQuery.map( jQuery.makeArray( value ), function( val ) {
+						return jQuery.trim( val ).replace( rspaces, " " );
+					} ),


I'm confused... what is the point of this? Fixing gh-2978 requires an update to valHooks.option.get, not to valHooks.select.set: #2978 (comment) . Specifically, strip and collapse whitespace is necessary to get the value of an option when it has no value attribute, and AFAICT nowhere else.

The value coming in has to match the value coming out. get seems to be doing the right thing already.

Note the new tests to confirm. elem.value does indeed return text when there is no value content attribute.

To summarize, jQuery.valHooks.select.set should be left alone (the changes here reverted), but jQuery.valHooks.option.get as it exists in master (and here) has two problems:

It is erroneously trimming value when derived from a content attribute.

It is misimplementing strip and collapse whitespace when value is not derived from a content attribute.

gibson042 · 2016-03-15T22:40:39Z

I'd like to see some more testing, both for non-HTML whitespace included in \s and/or jQuery.trim(), but more importantly proving a fix for the real regression revealed by gh-2978, e.g.:

assert.equal(
    jQuery( "<option value='&#13;foo&#9;bar&#10;'>foo bar</option>" ).val(),
    "\rfoo\tbar\n"
);

timmywil · 2016-03-15T22:59:26Z

@gibson042 The regression was not the only thing reported, but I see you're right about not stripping/collapsing the value.

timmywil · 2016-03-15T23:36:02Z

@gibson042 "non-space" whitespace tests added, and content attr/text behavior separated. This still isn't perfect as jQuery.trim is stripping more than we want, but perhaps it's not worth being that nitpicky.

timmywil · 2016-03-15T23:38:14Z

I was thinking we could normalize setting the value whether or not the option value was from a content attribute or text, but after the adjustments, I think we'd need a 2nd .inArray check, and I don't think it's worth it.

gibson042 · 2016-03-16T03:37:10Z

This still isn't perfect as jQuery.trim is stripping more than we want, but perhaps it's not worth being that nitpicky.

I get what you're saying, but I just checked, and think all our internal uses of jQuery.trim should actually use HTML's "strip and collapse whitespace", because there are other latent edge case bugs (example). Note also that we're already close, so taking the final step probably won't be that big a hit:

var rhtmlSpace = /[\x20\t\r\n\f]+/g,
    stripAndCollapse = function( match, pos, str ) {
        return pos === 0 || pos === str.length - match.length ? "" : " ";
    };
…
// formerly jQuery.trim( value )
value.replace( rhtmlSpace, stripAndCollapse );

or

var rhtmlSpace = /[\x20\t\r\n\f]+/g;
…
// formerly jQuery.trim( value )
(" " + value + " ").replace( rhtmlSpace, " " ).slice( 1, -1 );

markelog · 2016-03-16T09:43:12Z

test/unit/attributes.js

@@ -1107,6 +1107,71 @@ QUnit.test( "val(select) after form.reset() (Bug #2551)", function( assert ) {
 	jQuery( "#kk" ).remove();
 } );

+QUnit.test( "select.val(space characters) (gh-2978)", function( assert ) {


I guess the old way of writing tests stays forever, oh well :/

Well, there's the old way, the really old way, and the ancient way. I suspect you're talking about a newer way?

Ah, I remember now you're talking about #2886. I guess I just stuck with what I was used to.

I'm just sad about this, you know? We write tests like we did 10 years ago, tests should be obvious, stateless, and isolated, now it is just ugly, like -

assert.equal( $select.val(), "attr" + val, "Value ending with space character (" + key + ") selected (attr)" );

I can't isolate that, i don't understand what is attr or key and that thing depend on other actions, while violating every standard there is about testing.

Like ember project uses the same old QUnit, but they write them in much more structured way, like compare - https://github.com/emberjs/ember.js/blob/ed298ced885b2f4b4435fbccb26b24b9a24bb68f/tests/node/app-boot-test.js
with what we have here.

I guess I don't see the similarity in purpose between those app-boot tests and this. Looks like they have lots of helpers and they are establishing that HTML is generated as expected, or that certain callbacks are called/not called. We have some tests in ajax that are similar. I think it would help me if I could see how you'd like to write some of our existing tests to compare something closer to home.

That said, I'll land this for now so we can get 1.12.2/2.2.2 out, but I'm still interested in what you're proposing.

Yeah, ajax looks better, i have shown that before, right now though, i'm low on motivation about this, since most of the team don't see as importance in it, which is a shame.

Cleaning up the tests would be great, @markelog, I don't think anyone disagrees although we may not see the same degree of urgency. Are you looking for someone else on the team to show that it is important by taking it on? We all have limited resources to spend here. It seems like #2886 stalled waiting for you to give a good example of what a refactor would look like. If we had such an example and it were a "good project for a beginner" kind of thing we could keep a ticket open for that, but it probably will take someone with experience to do it.

As I said in #2886 if it's not a clear bug or feature, I'd prefer to not have an open ticket and instead put it on our wish list in the roadmap as @timmywil has been doing lately. We can create a ticket when we have a volunteer and a plan. Long lists of "it works now but wouldn't it be nice to do this one day" tickets are often demotivating, at least for me.

It seems like #2886 stalled waiting for you to give a good example of what a refactor would look like

I was waiting for the @mgol changes on removal tests for unsupported environments, that happened only very recently, i was planning to do that after the show/hide changes, which now never come.

not see the same degree of urgency

The more tests we write the harder it is to change things, i would advise to check out some articles about tests writing technics (i can come with some if anyone would need that), because this should feel important, personally, i think tests sometimes even more important then the code itself.

All and all, i'm not trying to be preachy about these things without doing anything about it but it is kinda hard to fight for it when it seems no one sees why this should matter that much, like am tenager who is never understood by their parents or some stuff like that.

timmywil · 2016-03-16T14:17:40Z

I'd like to finish this one up today or tomorrow. @gibson042 Would you say this is good to go for now and we can address the #3003 later?

gibson042 · 2016-03-16T15:01:42Z

@gibson042 Would you say this is good to go for now and we can address the #3003 later?

👍

Fixes gh-2978 Close gh-3002

Attributes: strip/collapse whitespace for set values on selects

7e0e730

Fixes jquerygh-2978

jquerybot added the CLA: Valid label Mar 15, 2016

gibson042 reviewed Mar 15, 2016
View reviewed changes

timmywil added 2 commits March 15, 2016 18:21

fixup! add test for space in the middle

429ec0a

fixup! limit regex to space characters

e642f97

gibson042 reviewed Mar 15, 2016
View reviewed changes

fixup! separate attr and text

5a335b7

fixup! name argument

009394f

gibson042 mentioned this pull request Mar 16, 2016

Internal use of jQuery.trim erroneously strips too much #3003

Closed

markelog reviewed Mar 16, 2016
View reviewed changes

timmywil closed this in 7052698 Mar 17, 2016

timmywil deleted the whitespace branch March 17, 2016 16:26

timmywil added a commit that referenced this pull request Mar 17, 2016

Attributes: strip/collapse whitespace for set values on selects

e7af951

Fixes gh-2978 Close gh-3002

timmywil added a commit that referenced this pull request Mar 17, 2016

Attributes: strip/collapse whitespace for set values on selects

da015d9

Fixes gh-2978 Close gh-3002

lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attributes: strip/collapse whitespace for set values on selects #3002

Attributes: strip/collapse whitespace for set values on selects #3002

timmywil commented Mar 15, 2016

gibson042 Mar 15, 2016

timmywil Mar 15, 2016

gibson042 Mar 15, 2016

timmywil Mar 15, 2016

timmywil Mar 15, 2016

gibson042 Mar 15, 2016

gibson042 commented Mar 15, 2016

timmywil commented Mar 15, 2016

timmywil commented Mar 15, 2016

timmywil commented Mar 15, 2016

gibson042 commented Mar 16, 2016

markelog Mar 16, 2016

timmywil Mar 16, 2016

timmywil Mar 16, 2016

markelog Mar 16, 2016

timmywil Mar 16, 2016

timmywil Mar 16, 2016

markelog Mar 16, 2016

dmethvin Mar 16, 2016

markelog Mar 16, 2016

timmywil commented Mar 16, 2016

gibson042 commented Mar 16, 2016

Attributes: strip/collapse whitespace for set values on selects #3002

Attributes: strip/collapse whitespace for set values on selects #3002

Conversation

timmywil commented Mar 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibson042 commented Mar 15, 2016

timmywil commented Mar 15, 2016

timmywil commented Mar 15, 2016

timmywil commented Mar 15, 2016

gibson042 commented Mar 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timmywil commented Mar 16, 2016

gibson042 commented Mar 16, 2016