Bug Tracker

Opened 6 years ago

Closed 6 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 6 years ago by m_gol

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

comment:2 Changed 6 years ago by Rick Waldron

Owner: set to 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

comment:3 Changed 6 years ago by m_gol

Status: pendingnew

comment:4 Changed 6 years ago by m_gol

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.

Last edited 6 years ago by m_gol (previous) (diff)

comment:5 Changed 6 years ago by m_gol

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 6 years ago by Rick Waldron

Status: newpending

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

comment:7 Changed 6 years ago by m_gol

Status: pendingnew

For me it's false. I suppose jQuery 2.0.0b2 haven't make it to jsFiddle "jQuery edge" yet.

comment:8 Changed 6 years ago by Rick Waldron

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.

comment:9 Changed 6 years ago by Rick Waldron

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 6 years ago by m_gol

Status: pendingnew

jQuery edge points to 1.9.2pre, not 2.0.0pre.

comment:11 Changed 6 years ago by m_gol

With 2.0.0pre your test case returns false: http://jsfiddle.net/m_gol/HDmdq/6/

comment:12 Changed 6 years ago by m_gol

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 Changed 6 years ago by Rick Waldron

Owner: changed from m_gol to Rick Waldron
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

comment:14 in reply to:  13 Changed 6 years ago by m_gol

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 6 years ago by Michał Gołębiowski

Resolution: fixed
Status: assignedclosed

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

Changeset: 761b96c3016e09613f356bd6070be04edec74d0f

Note: See TracTickets for help on using tickets.