Closed Bug 842367 Opened 11 years ago Closed 8 years ago

Firefox reverses the order that jQuery.data() returns data-attributes

Categories

(Core :: DOM: Core & HTML, defect)

18 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1264270

People

(Reporter: epinal99-bugzilla2, Unassigned, Mentored)

References

()

Details

(Keywords: testcase, Whiteboard: [lang=c++])

Attachments

(3 files)

STR:
Open the testcase http://codepen.io/stowball/pen/yGxrH

Result: order is reversed.
Compare the results with Chrome 24, Opera 12.5, IE10 and Safari 5.
http://browsershots.org/http://codepen.io/stowball/pen/yGxrH

NB: bug from https://twitter.com/stowball/status/303302865445867520
And this is a bug because?
It could be an issue if the dev wants to use the order returned by the command (of course applying sorting is safer)
Presumably a duplicate of the "attribute enumeration enumerates them in the opposite order of the order they appear in the source" thing.
(In reply to Boris Zbarsky (:bz) from comment #3)
> Presumably a duplicate of the "attribute enumeration enumerates them in the
> opposite order of the order they appear in the source" thing.

We no longer rely on that for uniqueness, so we could flip the order.
Ah, and the XML parser doesn't have the uniqueness problem, right?

So yeah, we should probably flip the order in the parsers and serializers...

Henri, do you have the bandwidth for this?
Whiteboard: [mentor=bz][lang=c++]
Can anyone point out where to start?
You probably want to change http://hg.mozilla.org/mozilla-central/annotate/06935f2db267/parser/html/nsHtml5TreeOperation.cpp#l297 and reverse the loop in nsHTMLContentSerializer::SerializeHTMLAttributes

Not sure whether anything else depends on this order.  nsXMLContentSerializer::SerializeAttributes doesn't seem to...
Flags: needinfo?(hsivonen)
I changed the code in both files this way:

-  for (int32_t index = count; index > 0;) {
-    --index;
+  for (int32_t index = 0; index < count; index++) {

but it haven't changed anything
You may need to also change the eTreeOpCreateElementNetwork/NotNetwork case.

But that case does in fact seem to rely on the ordering for uniqueness, no?  Henri?
Yes.  Probably worth looking at all uses of GetAttrNameAt.
(In reply to Boris Zbarsky (:bz) from comment #9)
> You may need to also change the eTreeOpCreateElementNetwork/NotNetwork case.
> 
> But that case does in fact seem to rely on the ordering for uniqueness, no? 
> Henri?

Uniqueness is handled in nsHtml5Tokenizer. As far as the HTML5 parser is concerned, it's fine to flip the order of the loop that adds the attributes to the DOM.
Flags: needinfo?(hsivonen)
When flipping the order, be sure to take care of how the attributes on the embed element get exposed to Flash Player.
Attached patch bugfix attemptSplinter Review
Should I add some tests?
Attachment #727800 - Flags: review?(bzbarsky)
Comment on attachment 727800 [details] [diff] [review]
bugfix attempt

Tests are good, yes.

In particular, please test .innerHTML and XMLSerializer, which I expect this patch breaks as written.  Note comment 7 and comment 10...

Have you audited the other consumers of GetAttrNameAt?
Attachment #727800 - Flags: review?(bzbarsky) → review-
(In reply to Boris Zbarsky (:bz) from comment #15)
> Tests are good, yes.

Can you point out to any tests guides? I don't understand which tests should I use? Mochitest?

> Have you audited the other consumers of GetAttrNameAt?
Changing Element.cpp didn't introduce any changes in jQuery.data() return value, other GetAttrNameAt uses haven't looked like what I need to change.
> I don't understand which tests should I use? Mochitest?

Yes, mochitests are the ideal tests to use here.  You'll want to test the things from comment 15 plus whatever it is that jQuery actually does under the hood for data() ordering.

> Changing Element.cpp didn't introduce any changes in jQuery.data() return value

Sure; that change is needed to not break .innerHTML.
And in particular, you're changing the order in which attributes are stored on elements.  Which means that you need to look at all callers of GetAttrNameAt, and if any of them depend on the order of attributes and currently walk them backwards switch them to walking forwards.
Khodzha, any progress here? Do you require further assistance in order to write a test?
Flags: needinfo?(khodzha.sh)
Flags: needinfo?(khodzha.sh)
Assignee: nobody → overholt
Attached file non-jQuery test case
Here's a test case that isn't using jQuery.  It seems to reproduce the problem as reported.  How does it seem to you, Loic?
Flags: needinfo?(epinal99-bugzilla)
Same as you:

Expected: 1, 2, 3, 4, 5
5, 4, 3, 2, 1

Expected: 5, 4, 3, 2, 1
1, 2, 3, 4, 5
Flags: needinfo?(epinal99-bugzilla)
Attached patch bug842367.patchSplinter Review
Here's a patch that fixes the issue and a mochitest.  I still have to test innerHTML and XMLSerializer (comment 13).  I have audited all consumers of GetAttrNameAt (comment 11) and am pretty confident there are no problems with reversing the order of attribute storage.  Comment 10 seems to not be dependent upon the attribute order.

I've run into an issue trying to maintain the attribute order sent to plugins (comment 13).  Example:

<object 
 style="visibility: visible;" 
 id="swf" 
 data="example.swf" 
 type="application/x-shockwave-flash" 
 height="300" 
 width="500"> 
</object>

Patching nsPluginInstanceOwner [1] (thanks for the pointer, :johns) I can retain the order of the style, id, data, and type attributes but the height and width attributes end up reversed and at the beginning of the attributes list.  I'm debugging to track this down but if anyone has an idea of where those are processed (in nsHtml5TreeOperation.cpp?), I'd appreciate a pointer :)

[1]
http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginInstanceOwner.cpp?mark=982-1000#1010
Andrew, height and width are actually stored separately from the other attributes in that case, in the mapped attributes.

If you look at nsAttrAndChildArray::AttrNameAt() you'll see that it always goes through the mapped attrs, in order, then through the non-mapped ones. And mapped attributes are always stored sorted, in numerical nsIAtom* order.  So I would expect it to always store "height" before "width" in the mapped attr list, and for both of those to come before all other attrs.  That's both with and without your patch.
I also have a slightly hard time reconciling all the consumers of GetAttrNameAt having been audited and innerHTML/XMLSerializer not having been checked, since those use GetAttrNameAt.
(In reply to Boris Zbarsky [:bz] from comment #23)
> Andrew, height and width are actually stored separately from the other
> attributes in that case, in the mapped attributes.

Thanks!

(In reply to Boris Zbarsky [:bz] from comment #24)
> I also have a slightly hard time reconciling all the consumers of
> GetAttrNameAt having been audited and innerHTML/XMLSerializer not having
> been checked, since those use GetAttrNameAt.

I should have said "other than innerHTML/XMLSerializer".  I will go over all consumers one more time before asking for review, though :)
I spent some time looking into maintaining the order of the attributes that we send to plugins like Flash via <object>.  I don't think it's going to be easy to maintain source order since nsPluginInstanceOwner has no knowledge that nsAttrAndChildArray is storing mapped attributes "before" non-mapped attributes.  With the code at [2] we seem to presently send attributes in reverse alphabetical mapped attribute order followed by reverse non-mapped attribute order.  I wrote some tests [1] to see and these are the results:

 https://tbpl.mozilla.org/php/getParsedLog.php?id=36408911&tree=Try

I think this means we can probably drop the code trying to handle this at [2] and likely assume Flash is now (some time since bug 234675) handling attributes regardless of the order they're sent.

What do you think, Boris?

In the meantime I will get back to innerHTML and XMLSerializer (and will re-check other callers of GetAttrAt).

[1]
https://hg.mozilla.org/try/rev/f2b2bcbbd416

[2]
http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginInstanceOwner.cpp?mark=982-1000#1010
Flags: needinfo?(bzbarsky)
Why can't we just keep the order we have right now, for the plugin stuff?  That seems most sensible to me.  I just don't trust plug-ins to not be broken.  :(

Seems like to do that we just need to reverse the order in which we ask for the indices, reverse the order in which the non-mapped attributes are stored (this bug), put the mapped attrs after the non-mapped ones when enumerating, instead of before, and reverse the sorting of the mapped attrs....
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #27)
> Why can't we just keep the order we have right now, for the plugin stuff? 

We can :)

> put the mapped attrs after the non-mapped ones when enumerating,
> instead of before, and reverse the sorting of the mapped attrs....

This is the part that seemed risky to me.  When I mentioned the situation to Ehsan, he was concerned that storing the mapped attributes first was maybe an optimization.  I will work on a patch for this since it seems you think it's not a problem to change it.

Thanks again.
The other thing is that the existing code to do the "right" thing for plugins in broken.  For example, if you have a test case such as:

<object
  foo=bar
  baz=huzzah
  width=100
  height=200>

the plugin will see the attributes in the order they are declared in the markup, but if you change the markup like this:

<object
  width=200
  foo=bar
  height=100
  baz=huzzah>

then the plugin will see things in a totally random order.

I may be screwing the above example up, but the point is, the existing code will only do what the comment advertizes *if* the markup groups all non-mapped attrs before the mapped ones.  If you do things intertwined like the second markup above, then what the plugin will see is essentially a random order.

In other words, we have already broken bug 234675, presumably when we first added the notion of mapped and non-mapped attrs.  The fact that we have not known about that until now suggests to me that Flash has been fixed here.
Mapped attrs predate bug 234675.

None of attributes involved in bug 234675 were mapped.

But more to the point, I think we should pretty much assume that plug-ins are broken.  If there's a simple way to not change plug-in-observable behavior, we should do that.
Mentor: bzbarsky
Whiteboard: [mentor=bz][lang=c++] → [lang=c++]
Assignee: overholt → nobody
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: