`.data()` doesn't handle the attributes with digits in the name well #1751

Closed
mgol opened this Issue Oct 21, 2014 · 16 comments

Comments

Projects
None yet
5 participants
@mgol
Member

mgol commented Oct 21, 2014

Originally reported by zerkms@… at: http://bugs.jquery.com/ticket/14376

version affected: any (1.10.1) browser: any (chrome) OS: any (windows 7 64bit)

For the following html

<div data-foo-42="bar"></div>

and

var div = $('div');

console.log(div.data());


javascript code I expect to see the object with one property.

Actual result: an empty object

jsfiddle:  http://jsfiddle.net/nWCKt/

Issue reported for jQuery 1.10.2

@mgol mgol added this to the 3.0.0 milestone Oct 21, 2014

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

Comment author: ameyms

Seems to be happening because we camelize the key before passing it to dataAttr() function. dataAttr() then tries to reconstruct the hyphenated attribute name and does so incorrectly.

Consider following example:

data-foo-bar --> fooBar --> data-foo-bar (correct)

data-foo-42 --> foo42 --> data-foo42 (incorrect)

Member

mgol commented Oct 21, 2014

Comment author: ameyms

Seems to be happening because we camelize the key before passing it to dataAttr() function. dataAttr() then tries to reconstruct the hyphenated attribute name and does so incorrectly.

Consider following example:

data-foo-bar --> fooBar --> data-foo-bar (correct)

data-foo-42 --> foo42 --> data-foo42 (incorrect)

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

Comment author: dmethvin

Ref:  http://www.w3.org/TR/2011/WD-html5-20110525/elements.html#embedding-custom-non-visible-data-with-the-data-attributes

It seems like we shouldn't take out the hyphen unless the following char is /[a-z]/. However that's inside jQuery.camelCase() so making that change could affect many other situations outside data attributes.

Member

mgol commented Oct 21, 2014

Comment author: dmethvin

Ref:  http://www.w3.org/TR/2011/WD-html5-20110525/elements.html#embedding-custom-non-visible-data-with-the-data-attributes

It seems like we shouldn't take out the hyphen unless the following char is /[a-z]/. However that's inside jQuery.camelCase() so making that change could affect many other situations outside data attributes.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

Comment author: rwaldron

 https://code.google.com/p/chromium/issues/detail?id=315125&thanks=315125&ts=1383668374

...was closed as a duplicate of this:  https://code.google.com/p/chromium/issues/detail?id=171175

Based on the spec:

For each name in list, for each U+002D HYPHEN-MINUS character (-) in the name that is followed by a lowercase ASCII letter, remove the U+002D HYPHEN-MINUS character (-) and replace the character that followed it by the same character converted to ASCII uppercase.

the solution would be to _not_ remove hyphens when they are immediately followed by numbers, so that data-foo-42 would produce a key called foo-42. Unfortunately, this is a compatibility breaking change—one that we actually have unit tests to support.

Member

mgol commented Oct 21, 2014

Comment author: rwaldron

 https://code.google.com/p/chromium/issues/detail?id=315125&thanks=315125&ts=1383668374

...was closed as a duplicate of this:  https://code.google.com/p/chromium/issues/detail?id=171175

Based on the spec:

For each name in list, for each U+002D HYPHEN-MINUS character (-) in the name that is followed by a lowercase ASCII letter, remove the U+002D HYPHEN-MINUS character (-) and replace the character that followed it by the same character converted to ASCII uppercase.

the solution would be to _not_ remove hyphens when they are immediately followed by numbers, so that data-foo-42 would produce a key called foo-42. Unfortunately, this is a compatibility breaking change—one that we actually have unit tests to support.

@mgol

This comment has been minimized.

Show comment
Hide comment
Member

mgol commented Oct 21, 2014

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

Comment author: gnarf

This is one of those weird situations where we "cantfix" but we probably should... Re-opening to discuss the ultimate fate of this one at the next meeting.

Member

mgol commented Oct 21, 2014

Comment author: gnarf

This is one of those weird situations where we "cantfix" but we probably should... Re-opening to discuss the ultimate fate of this one at the next meeting.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

Comment author: rwaldron

This has been fixed in Chrome  https://code.google.com/p/chromium/issues/detail?id=171175

Member

mgol commented Oct 21, 2014

Comment author: rwaldron

This has been fixed in Chrome  https://code.google.com/p/chromium/issues/detail?id=171175

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

Comment author: dmethvin

This is a behavior change so it won't go into 1.11/2.1, but I think we'll need to deal with it.

Member

mgol commented Oct 21, 2014

Comment author: dmethvin

This is a behavior change so it won't go into 1.11/2.1, but I think we'll need to deal with it.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

Comment author: malsup

This is a breaking change when migrating from 1.9 to 2.x.

Member

mgol commented Oct 21, 2014

Comment author: malsup

This is a breaking change when migrating from 1.9 to 2.x.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

Comment author: dmethvin

@malsup, the two branches seem to work the same with the OP's test case. Is there some other test case?

Member

mgol commented Oct 21, 2014

Comment author: dmethvin

@malsup, the two branches seem to work the same with the OP's test case. Is there some other test case?

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

Comment author: malsup

Replying to dmethvin:

@malsup, the two branches seem to work the same with the OP's test case. Is there some other test case?

@dmethvin See thread:  https://github.com/jquery/api.jquery.com/issues/383

Member

mgol commented Oct 21, 2014

Comment author: malsup

Replying to dmethvin:

@malsup, the two branches seem to work the same with the OP's test case. Is there some other test case?

@dmethvin See thread:  https://github.com/jquery/api.jquery.com/issues/383

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Dec 20, 2014

Member

Related to http://bugs.jquery.com/ticket/14799 :

assert.equal( jQuery("<div>").data({ "a-b": 1 }).data( "a-b", 2 ).data( "a-b" ), 2 )

And per #1515 (comment) , we want the "Embrace HTML5" option:

  • camelCase data-* attributes into the data object on first access
  • set only camelCase key on .data( key, val ) (setter)
  • camelCase all the added keys on .data( obj ) (bulk setter)
  • look for only camelCase key on .data( key ) (getter)
  • ignore direct manipulation from operations on .data()
Member

gibson042 commented Dec 20, 2014

Related to http://bugs.jquery.com/ticket/14799 :

assert.equal( jQuery("<div>").data({ "a-b": 1 }).data( "a-b", 2 ).data( "a-b" ), 2 )

And per #1515 (comment) , we want the "Embrace HTML5" option:

  • camelCase data-* attributes into the data object on first access
  • set only camelCase key on .data( key, val ) (setter)
  • camelCase all the added keys on .data( obj ) (bulk setter)
  • look for only camelCase key on .data( key ) (getter)
  • ignore direct manipulation from operations on .data()
@markelog

This comment has been minimized.

Show comment
Hide comment
Member

markelog commented Jan 5, 2015

/cc @rwaldron

@rwaldron

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Jan 5, 2015

Member

@markelog what's up? I was following this, but there hasn't been any activity lately.

Member

rwaldron commented Jan 5, 2015

@markelog what's up? I was following this, but there hasn't been any activity lately.

@timmywil timmywil assigned timmywil and unassigned rwaldron Jan 30, 2015

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Jan 30, 2015

Member

I'll grab this for 3.0, if that's alright with Rick.

Member

timmywil commented Jan 30, 2015

I'll grab this for 3.0, if that's alright with Rick.

@rwaldron

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Jan 30, 2015

Member

@timmywil sure, it shouldn't be too much work TBH. Mostly deleting code paths and associated tests. The tricky part is making sure that manipulation tests pass as well, since data tests exist there also

Member

rwaldron commented Jan 30, 2015

@timmywil sure, it shouldn't be too much work TBH. Mostly deleting code paths and associated tests. The tricky part is making sure that manipulation tests pass as well, since data tests exist there also

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