Side navigation
#13550 closed bug (fixed)
Opened March 02, 2013 01:15AM UTC
Closed March 02, 2013 06:07PM UTC
Regression: jQuery 2.0.0b2 bugs with $.fn.data handling parameters with dashes - next part
Reported by: | m_gol | Owned by: | rwaldron |
---|---|---|---|
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');
Attachments (0)
Change History (15)
Changed March 02, 2013 01:21AM UTC by comment:1
Changed March 02, 2013 01:50AM UTC by comment:2
owner: | → 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
Changed March 02, 2013 01:55AM UTC by comment:3
status: | pending → new |
---|
Sure, here it is: http://jsfiddle.net/m_gol/6Rqwj/9/
Changed March 02, 2013 02:03AM UTC by comment:4
_comment0: | 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: \ {{{ \ 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, we 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, we 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. → 1362189876286803 |
---|---|
_comment1: | 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, we 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, we 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. → 1362189966968934 |
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.
Changed March 02, 2013 02:08AM UTC by comment:5
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?
Changed March 02, 2013 02:12AM UTC by comment:6
status: | new → pending |
---|
Looks like it works correctly: http://jsfiddle.net/rwaldron/HDmdq/
Changed March 02, 2013 02:20AM UTC by comment:7
status: | pending → new |
---|
For me it's false. I suppose jQuery 2.0.0b2 haven't make it to jsFiddle "jQuery edge" yet.
Changed March 02, 2013 02:22AM UTC by comment:8
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.
Changed March 02, 2013 02:24AM UTC by comment:9
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
Changed March 02, 2013 02:27AM UTC by comment:10
status: | pending → new |
---|
jQuery edge points to 1.9.2pre, not 2.0.0pre.
Changed March 02, 2013 02:28AM UTC by comment:11
With 2.0.0pre your test case returns false:
Changed March 02, 2013 08:39AM UTC by comment:12
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.
Changed March 02, 2013 01:52PM UTC by comment:13
owner: | m_gol → rwaldron |
---|---|
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
Changed March 02, 2013 01:56PM UTC by comment:14
Replying to [comment:13 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!
Changed March 02, 2013 06:07PM UTC by comment:15
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...