Skip to main content

Bug Tracker

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 m_gol comment:1

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...

Changed March 02, 2013 01:50AM UTC by rwaldron comment:2

owner: → m_gol
status: newpending

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 m_gol comment:3

status: pendingnew

Changed March 02, 2013 02:03AM UTC by m_gol 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 m_gol 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 rwaldron comment:6

status: newpending

Looks like it works correctly: http://jsfiddle.net/rwaldron/HDmdq/

Changed March 02, 2013 02:20AM UTC by m_gol comment:7

status: pendingnew

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 rwaldron comment:8

status: newpending

>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 rwaldron 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 m_gol comment:10

status: pendingnew

jQuery edge points to 1.9.2pre, not 2.0.0pre.

Changed March 02, 2013 02:28AM UTC by m_gol comment:11

With 2.0.0pre your test case returns false:

http://jsfiddle.net/m_gol/HDmdq/6/

Changed March 02, 2013 08:39AM UTC by m_gol 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 rwaldron comment:13

owner: m_golrwaldron
status: newassigned
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 m_gol 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 Michał Gołębiowski comment:15

resolution: → fixed
status: assignedclosed

Fixes #13550. .data should not miss attr() set data-* with hyphenated property names. Closes gh-1189

Changeset: 761b96c3016e09613f356bd6070be04edec74d0f