Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
timmywil wants to merge 5 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7e0e730
Attributes: strip/collapse whitespace for set values on selects
timmywil Mar 15, 2016
429ec0a
fixup! add test for space in the middle
timmywil Mar 15, 2016
e642f97
fixup! limit regex to space characters
timmywil Mar 15, 2016
5a335b7
fixup! separate attr and text
timmywil Mar 15, 2016
009394f
fixup! name argument
timmywil Mar 15, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions src/attributes/val.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ define( [
"../core/init"
], function( jQuery, support ) {

var rreturn = /\r/g;
var rreturn = /\r/g,
rspaces = /[\x20\t\r\n\f]+/g;

jQuery.fn.extend( {
val: function( value ) {
Expand Down Expand Up @@ -80,9 +81,15 @@ jQuery.extend( {
option: {
get: function( elem ) {

// Support: IE<11
// option.value not trimmed (#14858)
return jQuery.trim( elem.value );
var val = jQuery.find.attr( elem, "value" );
return val != null ?
val :

// Support: IE10-11+
// option.text throws exceptions (#14686, #14858)
// Strip and collapse whitespace
// https://html.spec.whatwg.org/#strip-and-collapse-whitespace
jQuery.trim( jQuery.text( elem ) ).replace( rspaces, " " );
}
},
select: {
Expand Down Expand Up @@ -134,7 +141,7 @@ jQuery.extend( {
while ( i-- ) {
option = options[ i ];
if ( option.selected =
jQuery.inArray( jQuery.valHooks.option.get( option ), values ) > -1
jQuery.inArray( jQuery.valHooks.option.get( option ), values ) > -1
) {
optionSet = true;
}
Expand Down
82 changes: 76 additions & 6 deletions test/unit/attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ) {
Copy link
Member

@markelog markelog Mar 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@timmywil timmywil Mar 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@timmywil timmywil Mar 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@markelog markelog Mar 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@timmywil timmywil Mar 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@timmywil timmywil Mar 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@markelog markelog Mar 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@dmethvin dmethvin Mar 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@markelog markelog Mar 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

assert.expect( 35 );

var $select = jQuery( "<select/>" ).appendTo( "#qunit-fixture" ),
spaces = {
"\\t": {
html: "&#09;",
val: "\t"
},
"\\n": {
html: "&#10;",
val: "\n"
},
"\\r": {
html: "&#13;",
val: "\r"
},
"\\f": "\f",
"space": " ",
"\\u00a0": "\u00a0",
"\\u1680": "\u1680"
},
html = "";
jQuery.each( spaces, function( key, obj ) {
var value = obj.html || obj;
html += "<option value='attr" + value + "'></option>";
html += "<option value='at" + value + "tr'></option>";
html += "<option value='" + value + "attr'></option>";
} );
$select.html( html );

jQuery.each( spaces, function( key, obj ) {
var val = obj.val || obj;
$select.val( "attr" + val );
assert.equal( $select.val(), "attr" + val, "Value ending with space character (" + key + ") selected (attr)" );

$select.val( "at" + val + "tr" );
assert.equal( $select.val(), "at" + val + "tr", "Value with space character (" + key + ") in the middle selected (attr)" );

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

jQuery.each( spaces, function( key, obj ) {
var value = obj.html || obj,
val = obj.val || obj;
html = "";
html += "<option>text" + value + "</option>";
html += "<option>te" + value + "xt</option>";
html += "<option>" + value + "text</option>";
$select.html( html );

$select.val( "text" );
assert.equal( $select.val(), "text", "Value with space character at beginning or end is stripped (" + key + ") selected (text)" );

if ( /^\\u/.test( key ) ) {
$select.val( "te" + val + "xt" );
assert.equal( $select.val(), "te" + val + "xt", "Value with non-space whitespace character (" + key + ") in the middle selected (text)" );
} else {
$select.val( "te xt" );
assert.equal( $select.val(), "te xt", "Value with space character (" + key + ") in the middle selected (text)" );
}
} );
} );

var testAddClass = function( valueObj, assert ) {
assert.expect( 9 );

Expand Down Expand Up @@ -1523,17 +1588,22 @@ QUnit.test( "option value not trimmed when setting via parent select", function(
assert.equal( jQuery( "<select><option> 2</option></select>" ).val( "2" ).val(), "2" );
} );

QUnit.test( "Insignificant white space returned for $(option).val() (#14858)", function( assert ) {
assert.expect( 3 );
QUnit.test( "Insignificant white space returned for $(option).val() (#14858, gh-2978)", function( assert ) {
assert.expect( 16 );

var val = jQuery( "<option></option>" ).val();
assert.equal( val.length, 0, "Empty option should have no value" );

val = jQuery( "<option> </option>" ).val();
assert.equal( val.length, 0, "insignificant white-space returned for value" );
jQuery.each( [ " ", "\n", "\t", "\f", "\r" ], function( i, character ) {
var val = jQuery( "<option>" + character + "</option>" ).val();
assert.equal( val.length, 0, "insignificant white-space returned for value" );

val = jQuery( "<option>" + character + "test" + character + "</option>" ).val();
assert.equal( val.length, 4, "insignificant white-space returned for value" );
Copy link
Member

@gibson042 gibson042 Mar 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not assert.equal( val, "test", … )?

Copy link
Member Author

@timmywil timmywil Mar 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just kept the old. It's accomplishing the same thing.


val = jQuery( "<option> test </option>" ).val();
assert.equal( val.length, 4, "insignificant white-space returned for value" );
val = jQuery( "<option>te" + character + "st</option>" ).val();
assert.equal( val, "te st", "Whitespace is collapsed in values" );
} );
} );

QUnit.test( "SVG class manipulation (gh-2199)", function( assert ) {
Expand Down