1093593 - [markup-view] Order of attributes in markup view depends on input order

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Athena]

The attribute list in markup-view is expected to go in the order: id, class, all other attributes in alphabetical order.

However, the output order depends on the input order. Compare:

  data:text/html,<div c b a></div>
  <div a="" b="" c=""></div>

  data:text/html,<div a b c></div>
  <div c="" b="" a=""></div>

Comment 1

[Athena]

Lots more information in https://bugzilla.mozilla.org/show_bug.cgi?id=911148#c27

Comment 2

[Patrick Brosset <:pbro>]

Extract from bug 911148 comment 27 about attributes order:

"- node.attributes returns a NamedNodeMap (https://developer.mozilla.org/en-US/docs/Web/API/NamedNodeMap)
- a NamedNodeMap is in no particular order, but its items may still be accessed by indexes, like arrays (which is weird, but hey)
- it turns out firefox's implementation does seem to return the attributes exactly in the opposite order they appear in the markup: with <div a b c d e>, iterating through the node.attributes array-like will return e, d, c, b, a, in order.

[...]

Also, when modifying attributes on a DOM node, this changes the order of the NamedNodeMap. So, a map that was previously in the order [a,b,c], will now be in the order [b,c,a] if a's value is changed."

My suggestion for fixing this bug is:
- keep id always first (if any)
- keep class always second (if any)
- sort all other attributes alphabetically.

This way, no weird jumping around behavior when modifying attributes.

Comment 3

[Anurag Chaudhury]

Hi Patrick, I'd like to work on this.

Comment 4

[Patrick Brosset <:pbro>]

(In reply to Anurag Chaudhury from comment #3)
> Hi Patrick, I'd like to work on this.

Sure, thanks for your interest. I will assign the bug to you.
Do you already have the dev environment set up? Are my explanations in comment 2 clear enough to get started?

For info, the attributes are being retrieved from the node here: http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js#356
This is the "actor-side" part, which has access to the actual DOM nodes and sends information about them to the client-side.

Then, on the client-side (the code that actually creates the toolbox UI): 
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/markup-view.js#2103 is the place where elements are added to the markup-view panel,
and there are 2 places where attributes are created:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/markup-view.js#2192
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/markup-view.js#2333

These 2 loops just iterate over an array of attribute objects {name, value}.
I suggest adding a sorting step before these 2 loops that would sort the provided array with the id first, then the class and then the rest of the attributes sorted alphabetically.

This means the fix should be implemented on the client-side. Indeed, I don't think the output of the actor should be sorted, each consumer of its API is free to display attributes in any order it wants. It just happens that we want the markup-view to display them in a specific order for better UX.

Let me know if you have further questions.

Comment 5

[Patrick Brosset <:pbro>]

Unassigning since 1 month with no activity.

Comment 6

[Yash Mehrotra]

hi patrick, I would like to work on this bug. I have set up my dev environment and already fixed a few bugs in devtools.
Based on the comments, I think I understood most of it. But the code links you posted are outdated ( If you could send one with the revision number, that would be of great help) .
Thanks.

Comment 7

[Patrick Brosset <:pbro>]

Right, the code has changed since. Let me give you new pointers:

> For info, the attributes are being retrieved from the node here:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> inspector.js#356
> This is the "actor-side" part, which has access to the actual DOM nodes and
> sends information about them to the client-side.
writeAttrs method in NodeActor class, in inspector.js

> Then, on the client-side (the code that actually creates the toolbox UI): 
> http://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/
> markup-view.js#2103 is the place where elements are added to the markup-view
> panel,
_createAttribute method in ElementEditor class in markup-view.js

> and there are 2 places where attributes are created:
> http://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/
> markup-view.js#2192
> http://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/
> markup-view.js#2333
_applyAttributes method in ElementEditor class in markup-view.js and
update method in ElementEditor class in markup-view.js.

> These 2 loops just iterate over an array of attribute objects {name, value}.
> I suggest adding a sorting step before these 2 loops that would sort the
> provided array with the id first, then the class and then the rest of the
> attributes sorted alphabetically.

Comment 8

[Grisha Pushkov]

Attachment: 1093593-attributes-order.patch

I take out attributes ordering logic to new method insertAttribute, which is called in _createAttribute now. Disadvantage of this approach is that instead of sorting attributes at once, we do it for every attribute on creation.

Sorting of all attributes can work only in update method with no previously rendered attributes. And for cases such as adding new attribute or changing already rendered one, we still need to walk through DOM to get position for attribute and duplicate ordering logic.

Also tests from bug 911148 were changed, because they depends on order of attributes.

Comment 9

[Grisha Pushkov]

Attachment: 1093593-attributes-order-simple.patch

Here is another patch which is simpler than previous one and it does one thing - preserves original attributes order (id and class go first as before).

With alphabetic ordering there is a problem with lost focus. For example, if you change attribute name, it will change position and another attribute will be focused. It could be fixed in another bug, but I think it will be much more complex as it is now. And from user percepective, it is not convenient when you change attribute name and it "flyes away".

As I understand, this bug was created because of strange behaviour of NamedNodeMap for not preserving original attributes order (1) and changing order in test cases while adding / removing attributes (2).

(1) Is solved, initial attributes order is same as original.
(2) Test cases are using getAttributesFromEditor method and don't depend on NamedNodeMap order.

Comment 10

[Patrick Brosset <:pbro>]

I did some more researching on this and realized I was wrong about something I said in comment 2: when you set a different value on an attribute, that doesn't change the position of the attribute in the node.attributes NamedNodeMap.

For instance:

- open this URL data:text/html,<div a b c d e>
- in the console, enter $("div").attributes
- this returns NamedNodeMap [ e="", d="", c="", b="", a="" ] , so the attributes exactly in the reverse order
- in the console, enter $("div").setAttribute("d", "foo") and then $("div").attributes again
- this returns NamedNodeMap [ e="", d="foo", c="", b="", a="" ]

So changing an attribute's value doesn't alter the order of the map.

So with this in mind, I agree with your approach to simple reverse the map, that way making sure attributes appear in the order they were written in the source.

BUT, I still have a few comments about it:

- I think it makes a lot of sense to keep id and class always first (your 2nd patch seems to leave this unchanged anyway, so that's good)
- if you do modify an attribute via the console, and have the markup-view opened at the same time, then you'll see that attribute moves to the end of the list. That's because the ElementEditor.update method loops over attributes when this happens, and skips over the ones it already knows, and then add the new/changed ones. We could possibly fix this quite easily.
- If we're going to reverse attributes only because Firefox's NamedNodeMap implementation happens to order them in reverse order to how they were defined, then I'd suggest doing this on the server. I know I said previously that this should be a client-side only fix, but that was when the plan was to sort attributes alphabetically. So please move the reverse to devtools/server/actors/inspector.js in the writeAttrs method.

Comment 11

[Grisha Pushkov]

Attachment: 1093593-attributes-order-simple.patch

Good catch for position change in update method! Now current element is passed as parameter to _createAttribute and position remains unchanged.

There is new setAttribute method in testActor as I remember from bug 994555 that using content.document is deprecated.

To get testActor in test, openInspector function was slightly modified. Code in inspector's head.js looks much nicer, why generators aren't used in markup-view's head.js?

Also I faced a bug with attributes position on editing. Example:
> <div a="a" c="c">
Double click to edit "a" attribute:
> <div [a="a" b="b"] c="c">
And it will be:
> <div a="a" c="c" b="b">
But we can do better:
> <div a="a" b="b" c="c">

In _applyAttributes aAttrNode is null right after the first attribute is created and all other attributes are inserted to the end. 

How do you think, does it worth a separate bug?

Comment 12

[Patrick Brosset <:pbro>]

(In reply to Grisha Pushkov from comment #11)
> Also I faced a bug with attributes position on editing. Example:
> > <div a="a" c="c">
> Double click to edit "a" attribute:
> > <div [a="a" b="b"] c="c">
> And it will be:
> > <div a="a" c="c" b="b">
> But we can do better:
> > <div a="a" b="b" c="c">
> 
> In _applyAttributes aAttrNode is null right after the first attribute is
> created and all other attributes are inserted to the end. 
> 
> How do you think, does it worth a separate bug?
Not sure about this one. When adding attributes in the markup-view, users have to know they're not changing the source file, but the living DOM tree.
It's good that the markup-view displays attributes in the order they were written in in the source file (which your patch does), but once the source file becomes a DOM tree in the browser, inserting attributes doesn't mean the same thing anymore. node.setAttribute("b", "b") in javascript will append b to the end of the map, and I think the markup-view should reflect this.
I guess you could argue that it'd be better UX to insert b in between a and c, to avoid having things jump around in the UI, but I'd answer that attributes editors are primarily used to edit the attribute's value, not to add an entirely new one.

Comment 13

[Patrick Brosset <:pbro>]

Comment on attachment 8684419 [details] [diff] [review]
1093593-attributes-order-simple.patch

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

Just a few minor comments, nothing that should block R+.

::: devtools/client/markupview/markup-view.js
@@ +2634,5 @@
>          el.style.removeProperty("display");
>        } else {
>          // Create a new editor, because the value of an existing attribute
>          // has changed.
> +        let attribute = this._createAttribute(attr, el);

Great that it's such a simple fix!

::: devtools/client/markupview/test/head.js
@@ +142,5 @@
>      let inspector = toolbox.getCurrentPanel();
>      let eventId = "inspector-updated";
>      return inspector.once("inspector-updated").then(() => {
>        info("The inspector panel is active and ready");
> +      return registerTestActor(toolbox.target.client);

Thanks for adding the test-actor to the markup-view tests, it's a much better way to write tests and is compatible with our future strategy to run tests on remote devices.

The thing howerver is that it'd be good now that it's there to migrate *all* markup-view tests to use it! That's a huge task that should definitely be done in a separate bug (could you file one?).

I'd normally say that we shouldn't introduce the test-actor here if we're not migrating the other tests at the same time because it creates more ways to implement tests and adds confusion for new people trying to write tests in the meantime.

But I'm ok to do it because markup-view tests really need some cleanup (a lot of them access elements in the page directly). This will get us started.

Would you be interested in working on this test refactoring bug?

I think we need to send an email to the dev-developer-tools mailing list to let people know that if they're writing new markup-view tests, they should use the test-actor to access elements in the page. I can do this if you want, unless you'd like to.

Finally, to answer your question: "why generators aren't used in markup-view's head.js?": just because it wasn't done :) no other reason. So please feel free to use a generator instead.

@@ +676,5 @@
> + *         (e.g. ["id", "class", "href"])
> + */
> +var getAttributesFromEditor = Task.async(function*(selector, inspector) {
> +  let nodeList = (yield getContainerForSelector(selector, inspector))
> +    .tagLine.querySelectorAll('[data-attr]');

nit: please only use double-quotes "

@@ +678,5 @@
> +var getAttributesFromEditor = Task.async(function*(selector, inspector) {
> +  let nodeList = (yield getContainerForSelector(selector, inspector))
> +    .tagLine.querySelectorAll('[data-attr]');
> +
> +  return [...nodeList].map(node => node.getAttribute('data-attr'));

nit: please only use double-quotes "

@@ +679,5 @@
> +  let nodeList = (yield getContainerForSelector(selector, inspector))
> +    .tagLine.querySelectorAll('[data-attr]');
> +
> +  return [...nodeList].map(node => node.getAttribute('data-attr'));
> +})

nit: please add a ;

::: devtools/server/actors/inspector.js
@@ +418,5 @@
>    writeAttrs: function() {
>      if (!this.rawNode.attributes) {
>        return undefined;
>      }
> +    return [...this.rawNode.attributes].reverse().map(attr => {

Ok this needs a comment. Future readers of the code won't understand why we do this.
Can you add something like:

// The NamedNodeMap implementation in Firefox (returned by
// node.attributes) gives attributes in the reverse order compared
// to the source file when iterated. So reverse the list here.

Comment 14

[Grisha Pushkov]

Attachment: 1093593-attributes-order-simple.patch

Comment added and semicolons with quotes corrected. I hope now with ESLint I don't miss these meaningless typos.

Also I added "r=pbro" to the end of commit message and going to mark this patch as R+ by myself. All seems right :)

Comment 15

[Grisha Pushkov]

> Would you be interested in working on this test refactoring bug?
Yes, I just created bug 1223527 for this.

Comment 16

[Grisha Pushkov]

> It's good that the markup-view displays attributes in the order they were written in in the source
> file (which your patch does), but once the source file becomes a DOM tree in the browser,
> inserting attributes doesn't mean the same thing anymore. node.setAttribute("b", "b") in javascript
> will append b to the end of the map, and I think the markup-view should reflect this.

I found another example:
> <div a="a" d="d">
Double click to edit "a" attribute:
> <div [b="b" c="c"] d="d">
And it will be:
> <div c="c" b="b" d="d">
But should be to reflect NamedNodeMap:
> <div d="d" b="b" c="c">
Or maybe this is more intuitively:
> <div b="b" c="c" d="d">

Looks more bug-like than previous one.

Comment 17

[Patrick Brosset <:pbro>]

(In reply to Grisha Pushkov from comment #16)
> > It's good that the markup-view displays attributes in the order they were written in in the source
> > file (which your patch does), but once the source file becomes a DOM tree in the browser,
> > inserting attributes doesn't mean the same thing anymore. node.setAttribute("b", "b") in javascript
> > will append b to the end of the map, and I think the markup-view should reflect this.
> 
> I found another example:
> > <div a="a" d="d">
> Double click to edit "a" attribute:
> > <div [b="b" c="c"] d="d">
> And it will be:
> > <div c="c" b="b" d="d">
> But should be to reflect NamedNodeMap:
> > <div d="d" b="b" c="c">
> Or maybe this is more intuitively:
> > <div b="b" c="c" d="d">
> 
> Looks more bug-like than previous one.
Yes it does. Did you investigate why we're not getting <div d="d" b="b" c="c"> like the NamedNodeMap?

Comment 18

[Grisha Pushkov]

> Yes it does. Did you investigate why we're not getting <div d="d" b="b" c="c"> like the NamedNodeMap?
I created bug 1225591 with detailed description.

And about this bug, do I need something to do additionally here?

Comment 19

[Patrick Brosset <:pbro>]

(In reply to Grisha Pushkov from comment #18)
> > Yes it does. Did you investigate why we're not getting <div d="d" b="b" c="c"> like the NamedNodeMap?
> I created bug 1225591 with detailed description.
Thanks
> And about this bug, do I need something to do additionally here?
Should be good to go. We do need to run the patch on TRY though, to make sure it doesn't cause regressions that other tests would pick up.
Since you're relatively new to bugzilla, I'm guessing you don't yet have TRY push access?
I suggest you request commit access level 1 to get it, it will be very useful for you to test out patches on all OSs before requesting to check them in (I'm going to push this patch to TRY myself here, and if/after the results are green, we can just add the "checkin-needed" keyword to the bug on bugzilla, and a code sheriff will come and push the change to mercurial).
https://www.mozilla.org/en-US/about/governance/policies/commit/
https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/

Comment 20

[Patrick Brosset <:pbro>]

Pending try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cbb46f1c583
Please keep an eye on the results (it may take a few hours to complete all tests on all platforms).
Let me know if you see anything red of orange that looks suspicious. Interpreting try tests results is hard at first, I can help detect if things are OK or not.

Comment 21

[Grisha Pushkov]

Looking at results browser_webconsole_output_dom_elements_02.js test failed. But passes on my local. I'm going to do "hg pull" and try again.

Also found a table with transcripts for try:
https://developer.mozilla.org/en-US/docs/Mozilla/QA/Automated_testing

Comment 22

[Grisha Pushkov]

> I'm guessing you don't yet have TRY push access?
Yes. I created request for it in bug 1226043. Could you comment there?
I don't fully understand do I need to give my account on mozillians.org. So in case it's needed:
https://mozillians.org/en-US/u/reepush/

Comment 23

[Grisha Pushkov]

Fails in browser_webconsole_output_dom_elements_02.js are caused by changes in this bug.
> Got class, expected id
> Got body-id, expected body-class
> Got ltr, expected en-US

Will check how expected attributes are depends on the order.

Comment 24

[Grisha Pushkov]

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

Comment 25

[Grisha Pushkov]

Attachment: 1093593-attributes-order-simple.patch

Updated patch, which was tested on try.

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/cc2af13f040c

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cc2af13f040c