Closed Bug 1264270 Opened 8 years ago Closed 8 years ago

Parser should output attributes in source order, not reversed

Categories

(Core :: DOM: HTML Parser, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: ayg, Assigned: ayg)

References

Details

(Whiteboard: btpp-active)

Attachments

(1 file)

We want UAs to converge on DOM properties reflecting attributes in source order.  Currently our parser reverses the order.  Henri said via e-mail:

"""
The old parser added the attributes to the DOM in reverse order in
order to have the first attribute in source order win. To keep the
same order, the new parser adds the attributes in reverse order, too,
even though it's not needed for making the first attribute when, since
that already happens inside the parser itself. The embed element then
again reverses the order, because some ancient version of the Flash
plug-in wanted the attributes in the source order.

Since the old parser is gone, feel free to flip the order in which the
HTML parser sets the attributes if you are willing to deal with the
fallout for unit tests.
"""

Even after this is fixed, we will still not always get the order right because of mapped vs. unmapped attributes, but at least we'll be better off.

Loop that needs reversing is: https://mxr.mozilla.org/mozilla-central/source/parser/html/nsHtml5TreeOperation.cpp#413
Flags: in-testsuite+
Whiteboard: btpp-active
This is causing test failures in inspector:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e3943d85f43

Presumably inspector is reversing the DOM attributes for display somewhere internally.  Where is that so I can un-reverse it?  (Don't know who the right person is to ask, so I picked the last three reviewers of devtools/*/inspector/.)
Flags: needinfo?(jryans)
Flags: needinfo?(jdescottes)
Flags: needinfo?(bgrinstead)
My first guess would be the reverse() performed here :

https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/markup/markup.js#3175

The comment seems inline with what you updated.
Leaving the ni? for Ryan and Brian as they might know more.
Flags: needinfo?(jdescottes)
Thanks!  I updated those two lines, but I still seem to be getting some try failures.  Any further ideas?
When running the tests locally I had a failure with : browser_markup_html_edit_03.js . From what I can see this test has to be updated here.

I also had 3 test failures with the font inspector, need to investigate more. Did you see any other test failure?
Here's a new try run with the two .reverse()s removed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=791954122758

Completed try run without .reverse() removal: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e3943d85f43

There are a lot of devtools failures, and it looked to me like they indicate the change broke something in inspector.  If you tell me that I should just update the tests, though, I'll go ahead and do that.
(In reply to :Aryeh Gregor (working until May 8) from comment #6)
> 
> There are a lot of devtools failures, and it looked to me like they indicate
> the change broke something in inspector.  If you tell me that I should just
> update the tests, though, I'll go ahead and do that.

That comment was only related to browser_markup_html_edit_03.js .

I will have a look at the other failures from your try tests in a bit.
I think :bgrins or :pbro would know more, redirecting to them.
Flags: needinfo?(jryans) → needinfo?(pbrosset)
(In reply to Julian Descottes [:jdescottes] from comment #5)
> When running the tests locally I had a failure with :
> browser_markup_html_edit_03.js . From what I can see this test has to be
> updated here.

Do you understand why it failed?  It looks like it's testing outerHTML, and this patch is designed so it shouldn't change outerHTML output at all.  Tests should only need to be updated if they're testing the DOM (this patch reverses the attributes' order), or testing that .setAttribute() and similar prepend attributes to .innerHTML and friends rather than appending, or something like that.  I don't want to just update the test if it looks to me like it shouldn't have changed.  I'm also not sure about the devtools/client/shared/test/browser_outputparser.js failure, or devtools/client/webconsole/test/browser_webconsole_output_dom_elements_01.js (although the expectation for that one looks like it should have always been wrong).

For devtools/server/tests/mochitest/test_inspector-search.html, I just don't understand the test and don't know how to fix the test to not fail.

Those are the only four failures with the .reverse()s removed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=791954122758&selectedJob=19618164

Thanks for the help!
TLDR: I've tried to understand all of the failures, and it seems to me tests should be updated here. I've explained my reasons below. I would really prefer if we got some feedback from either bgrins or pbro here. Hopefully they can simply confirm my investigations. Maybe you can separate the devtools test modifications in a second patch and flag :bgrins or :pbro for review on this one? They're both out this week, though. Can it wait til next week? 

(In reply to :Aryeh Gregor (working until May 8) from comment #9)
> (In reply to Julian Descottes [:jdescottes] from comment #5)
> > When running the tests locally I had a failure with :
> > browser_markup_html_edit_03.js . From what I can see this test has to be
> > updated here.
> 
> Do you understand why it failed?  It looks like it's testing outerHTML, and
> this patch is designed so it shouldn't change outerHTML output at all. 
> Tests should only need to be updated if they're testing the DOM (this patch
> reverses the attributes' order), or testing that .setAttribute() and similar
> prepend attributes to .innerHTML and friends rather than appending, or
> something like that.  I don't want to just update the test if it looks to me
> like it shouldn't have changed.

This test is updating the outerHTML of a documentElement. For such elements we can't simply update the outerHTML, but we loop through all attributes in order to update them : https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/inspector.js#2609

Since the id attribute for this <html> element is created in the previous test method (testDocumentElement), in this second step only "class" is added to the HTML element (using setAttribute). With your patch, the outerHTML should respect the order in which the attributes have been created, so it makes sense that "id" is rendered before "class".

> I'm also not sure about the
> devtools/client/shared/test/browser_outputparser.js failure, or
> devtools/client/webconsole/test/browser_webconsole_output_dom_elements_01.js
> (although the expectation for that one looks like it should have always been
> wrong).

browser_outputparser.js : Here the output parser is creating span elements and using setAttribute to set the classname and the style. The test failures are linked to the reversed order of style and class in the expected innerHTML for the spans we create. 

Basically doing :

var sp = document.createElement("span");
sp.setAttribute("class", "my-class");
sp.setAttribute("style", "color:red");
console.log(sp.outerHTML);

outputs "<span style="color:red" class="my-class"></span>" before your patch and "<span class="my-class" style="color:red"></span>". If this is expected, then the test has to be updated.

browser_webconsole_output_dom_elements_01.js : This test fails because the representation of DOM Nodes in the webconsole is done by looping on the attributes from a node. See https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/object.js#1606 . In this particular case, the devtools were still displaying the reverse order for the attributes. I think in this case the test should be updated as well (you can easily see the behavior change by opening "data:text/html;charset=utf-8,<div a b c d e f g h>test</div>" and logging the div in the webconsole)

> 
> For devtools/server/tests/mochitest/test_inspector-search.html, I just don't
> understand the test and don't know how to fix the test to not fail.

This one is a bit more tricky. 

The test fails because we are searching for 

"<div class="emoji" id="emoji" emoji="emoji"></div>" (emojis have been replaced by strings here)

The walker-search is building a search index by looping on attributes (see https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/utils/walker-search.js#108). Before your patch, the first attribute retrieved for the indexing was `emoji="emoji"`. We then index the attribute name, and then the value. 

Search index before your patch:
attributeName:emoji
attributeValue:emoji
attributeName:id
attributeValue:emoji
attributeName:class
attributeValue:emoji

After your patch:
attributeName:class
attributeValue:emoji
attributeName:id
attributeValue:emoji
attributeName:emoji
attributeValue:emoji

In the test, we lookup "emoji" using our walker search. Before your patch, the first match was "attributeName:emoji", but now it is "attributeValue:emoji". So in my opinion, it should be safe to update this test to expect a match on "attributeValue" instead of "attributeName" at line 159.

> 
> Those are the only four failures with the .reverse()s removed:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=791954122758&selectedJob=19618164
> 
> Thanks for the help!
Attached patch PatchSplinter Review
Review requests from bgrins for devtools/, and from hsivonen for the rest.  (pbro currently isn't accepting review requests, so I didn't ask him right now.)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=93975a224845
Attachment #8742863 - Flags: review?(hsivonen)
Attachment #8742863 - Flags: review?(bgrinstead)
Attachment #8742863 - Flags: review?(hsivonen) → review+
Comment on attachment 8742863 [details] [diff] [review]
Patch

Review of attachment 8742863 [details] [diff] [review]:
-----------------------------------------------------------------

Devtools changes look good, thanks
Attachment #8742863 - Flags: review?(bgrinstead) → review+
Flags: needinfo?(pbrosset)
Flags: needinfo?(bgrinstead)
https://hg.mozilla.org/mozilla-central/rev/45cde9858aa0
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1271439
Depends on: 1287588
Depends on: 1310865
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: