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)
Tracking
()
RESOLVED
DUPLICATE
of bug 1264270
People
(Reporter: epinal99-bugzilla2, Unassigned, Mentored)
References
()
Details
(Keywords: testcase, Whiteboard: [lang=c++])
Attachments
(3 files)
1.90 KB,
patch
|
bzbarsky
:
review-
|
Details | Diff | Splinter Review |
829 bytes,
text/html
|
Details | |
3.48 KB,
patch
|
Details | Diff | Splinter Review |
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
Keywords: testcase
Comment 1•11 years ago
|
||
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)
Comment 3•11 years ago
|
||
Presumably a duplicate of the "attribute enumeration enumerates them in the opposite order of the order they appear in the source" thing.
Comment 4•11 years ago
|
||
(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.
Comment 5•11 years ago
|
||
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++]
Comment 6•11 years ago
|
||
Can anyone point out where to start?
Comment 7•11 years ago
|
||
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)
Comment 8•11 years ago
|
||
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
Comment 9•11 years ago
|
||
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?
Comment 10•11 years ago
|
||
http://mxr.mozilla.org/mozilla-central/source/content/base/src/Element.cpp?rev=a8a73639eb9b&mark=2994-2995#2982 too?
Comment 11•11 years ago
|
||
Yes. Probably worth looking at all uses of GetAttrNameAt.
Comment 12•11 years ago
|
||
(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)
Comment 13•11 years ago
|
||
When flipping the order, be sure to take care of how the attributes on the embed element get exposed to Flash Player.
Comment 15•11 years ago
|
||
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-
Comment 16•11 years ago
|
||
(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.
Comment 17•11 years ago
|
||
> 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.
Comment 18•11 years ago
|
||
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.
Comment 19•11 years ago
|
||
Khodzha, any progress here? Do you require further assistance in order to write a test?
Flags: needinfo?(khodzha.sh)
Updated•11 years ago
|
Flags: needinfo?(khodzha.sh)
Updated•10 years ago
|
Assignee: nobody → overholt
Comment 20•10 years ago
|
||
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)
Reporter | ||
Comment 21•10 years ago
|
||
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)
Comment 22•10 years ago
|
||
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
Comment 23•10 years ago
|
||
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.
Comment 24•10 years ago
|
||
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.
Comment 25•10 years ago
|
||
(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 :)
Comment 26•10 years ago
|
||
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)
Comment 27•10 years ago
|
||
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)
Comment 28•10 years ago
|
||
(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.
Comment 29•10 years ago
|
||
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.
Comment 30•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Mentor: bzbarsky
Whiteboard: [mentor=bz][lang=c++] → [lang=c++]
Updated•8 years ago
|
Assignee: overholt → nobody
Updated•8 years ago
|
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.
Description
•