Opened 10 years ago
Closed 10 years ago
#13550 closed bug (fixed)
Regression: jQuery 2.0.0b2 bugs with $.fn.data handling parameters with dashes - next part
Reported by: | m_gol | Owned by: | Rick Waldron |
---|---|---|---|
Priority: | undecided | Milestone: | None |
Component: | unfiled | Version: | git |
Keywords: | Cc: | ||
Blocked by: | Blocking: |
Description
A followup to bug #13548. The fix didnt't handle the situation where we use .data(key)
between setting the attribute and setting the data. Currently the following still behaves incorrectly:
$('body').attr('data-long-param', 'test'); $('body').data('long-param'); $('body').data('long-param', {a: 2}); $('body').data('long-param');
Change History (15)
comment:1 Changed 10 years ago by
comment:2 Changed 10 years ago by
Owner: | set to m_gol |
---|---|
Status: | new → pending |
Before jumping into another patch, can you provide a fiddle that illustrates the problem behaviour? The new data system was written in conformance with all of our pre-existing expectations for the documented API, I want to make sure we're not pushing into "new turf"—which can happen if an API was "accidentally" allowing some use case
comment:3 Changed 10 years ago by
Status: | pending → new |
---|
Sure, here it is: http://jsfiddle.net/m_gol/6Rqwj/9/
comment:4 Changed 10 years ago by
As for where I stumbled upon this problem - in the codebase I'm talking about there's a jQuery plugin which is initialized in the following way (I mean, the code is much bigger, I extracted relevant parts):
function Tabs(element) { var that = this; this.$element = $(element); this.$element.find('.action-switch-tab').each(function (i, link) { var $link = $(link); var targetSelector = $link.data('tabs-target'); $link.data('tabs-target', that.$element.find(targetSelector)); }); }
After that, I expect to have a copy of the target jQuery object in $link.data('tabs-target'). However, just reading this data using this parameter causes the code to create a 'tabs-target' data field which causes the tabsTarget field to not even be read.
Sure, I could write var targetSelector = $link.attr('data-tabs-target')
instead but current code is more flexible - we can have elements set for this plugin via JS and it works even if there was no data-tabs-target
attribute.
comment:5 Changed 10 years ago by
Anyway, regardless of what's expected of the data API and what's not, without using a very hacky code (which I'd argue it's not in this fiddle and the example from my previous comment), there shouldn't be a situation where setting the data and reading it later returns the initial result, not the last one, am I right?
comment:6 Changed 10 years ago by
Status: | new → pending |
---|
Looks like it works correctly: http://jsfiddle.net/rwaldron/HDmdq/
comment:7 Changed 10 years ago by
Status: | pending → new |
---|
For me it's false. I suppose jQuery 2.0.0b2 haven't make it to jsFiddle "jQuery edge" yet.
comment:8 Changed 10 years ago by
Status: | new → pending |
---|
jQuery edge
This is the code on github.
Anyway, regardless of what's expected of the data API and what's not, without using a very hacky code (which I'd argue it's not in this fiddle and the example from my previous comment), there shouldn't be a situation where setting the data and reading it later returns the initial result, not the last one, am I right?
We only read the data-* attributes once, but I was certain we had all the edge cases covered.
comment:9 Changed 10 years ago by
For me it's false. I suppose jQuery 2.0.0b2 haven't make it to jsFiddle "jQuery edge" yet.
I just realized the confusion: the patch I landed won't be in jQuery 2.0.0b2, it will be in 2.0.0b3, to test against the latest code: http://code.jquery.com/jquery-git2.js
comment:10 Changed 10 years ago by
Status: | pending → new |
---|
jQuery edge points to 1.9.2pre, not 2.0.0pre.
comment:11 Changed 10 years ago by
With 2.0.0pre your test case returns false: http://jsfiddle.net/m_gol/HDmdq/6/
comment:12 Changed 10 years ago by
My latest test case updated to point to jquery-git2.js: http://jsfiddle.net/m_gol/6Rqwj/13/
Note that there are both long-param
and longParam
keys in the printed object and .data('long-param')
picks up the former. My pull request fixes that.
It also makes the code more consistent. In current jQuery master if you invoke $object.data()
, the HTML5 data-* attributes are set using the following code:
if ( name.indexOf( "data-" ) === 0 ) { name = jQuery.camelCase( name.substring(5) ); dataAttr( elem, name, data[ name ] ); }
(lines 235-238 of src/data.js). However, if you use $object.data('long-param')
, it's invoked in this way:
data = dataAttr( elem, key, undefined );
Changing it from key
to camelKey
makes it consistent with the first code snippet, thus my pull request.
comment:13 follow-up: 14 Changed 10 years ago by
Owner: | changed from m_gol to Rick Waldron |
---|---|
Status: | new → assigned |
jQuery edge points to 1.9.2pre, not 2.0.0pre.
Right, I completely forgot that there is a special jquery-git2.js—also explains why the bug wasn't present. I'll take a look at this today, thanks for the report and the follow up
comment:14 Changed 10 years ago by
Replying to rwaldron:
jQuery edge points to 1.9.2pre, not 2.0.0pre.
Right, I completely forgot that there is a special jquery-git2.js—also explains why the bug wasn't present.
Maybe it's worth to ping jsFiddle guys to provide the "jQuery 2 edge" option, it's needed for tests like that.
I'll take a look at this today, thanks for the report and the follow up
Great, thanks!
comment:15 Changed 10 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Fixes #13550. .data should not miss attr() set data-* with hyphenated property names. Closes gh-1189
Changeset: 761b96c3016e09613f356bd6070be04edec74d0f
I think the key is the
data_user.set( elem, key, data )
invocation where key should be changed to camelKey. I reported the pull request and added a new test against that: https://github.com/jquery/jquery/pull/1189 Bare with me, this is my first pull request on to jQuery so I might not know all the procedures...