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)
Core
DOM: HTML Parser
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: ayg, Assigned: ayg)
References
Details
(Whiteboard: btpp-active)
Attachments
(1 file)
31.67 KB,
patch
|
hsivonen
:
review+
bgrins
:
review+
|
Details | Diff | Splinter Review |
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+
Updated•8 years ago
|
Whiteboard: btpp-active
Assignee | ||
Comment 1•8 years ago
|
||
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)
Comment 2•8 years ago
|
||
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)
Comment 3•8 years ago
|
||
As well as : https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/inspector.js#429
Assignee | ||
Comment 4•8 years ago
|
||
Thanks! I updated those two lines, but I still seem to be getting some try failures. Any further ideas?
Comment 5•8 years ago
|
||
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?
Assignee | ||
Comment 6•8 years ago
|
||
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.
Comment 7•8 years ago
|
||
(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)
Assignee | ||
Comment 9•8 years ago
|
||
(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!
Comment 10•8 years ago
|
||
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!
Assignee | ||
Comment 11•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8742863 -
Flags: review?(hsivonen) → review+
Comment 12•8 years ago
|
||
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+
Updated•8 years ago
|
Flags: needinfo?(pbrosset)
Flags: needinfo?(bgrinstead)
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/45cde9858aa0
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in
before you can comment on or make changes to this bug.
Description
•