Brian Grinstead [:bgrins] Reporter
	Description • 11 years ago

If you have a node in the markup view like so:

<div id="one" class="two"></div>

And you click into the id attribute to edit it and then press Tab it should focus and begin editing the class attribute.  If you press tab again, it should focus and being editing the new attribute field.  If you Shift+Tab back all the way past ID, it should take you to the tag editing field.

	Athena Assignee
	Comment 2 • 10 years ago

There are two cases being addressed here:

* activating an editor and then tabbing through without making any changes (inplace-editor)

* making changes to attributes while tabbing through (markup-view.js) -- because modifying an attribute regenerates the entire list of attribute elements


And one possibly weird thing, you'll need to tab or shift-tab twice to get to the next element because the tab is being caught first by autocomplete-handling logic.

	Athena Assignee
	Comment 3 • 10 years ago

AH yeah, and before these changes, when you edited an attribute, keyboard focus position would be lost and you'd end up at the top of the markup view. This should help with that.

	Joe Walker [:jwalker]
	Comment 4 • 10 years ago

Comment on attachment 8475256 [details] [diff] [review]
markup-view-attribute-focus-bug-911148

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

I'm going to bounce this review to Patrick because I'm on PTO, sorry!

	Patrick Brosset <:pbro>
	Comment 5 • 10 years ago

Comment on attachment 8475256 [details] [diff] [review]
markup-view-attribute-focus-bug-911148

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

I haven't yet fully reviewed the code, but I have a question in markup-view.js which I think should be answered before I move on.

::: browser/devtools/markupview/markup-view.js
@@ +639,5 @@
>            requiresLayoutChange = true;
>          }
> +
> +        if (type === "attributes") {
> +          // Put back keyboard focus if it's gone

What about if the mutation came from content JS rather than from the user changing an attribute in the markupview?
In the former case, there probably was no focused field, so we shouldn't be re-focusing.

::: browser/devtools/markupview/test/browser_markupview_tag_edit_09.js
@@ +6,5 @@
> +
> +// Tests that focus position is kept when tabbing through and editing
> +// attributes
> +
> +const TEST_URL = "data:text/html,<div id='one' class='two'></div>";

Please use "data:text/html;charset=utf8,<div ...." to avoid warnings in the logs.

	Athena Assignee
	Comment 6 • 10 years ago

Sorry for the delay. Here's a new patch with minor changes (charset=utf8 + tweaks so it applies against the latest). Not marking for review because I need to figure out how to handle the first thing you mentioned.

> > +
> > +        if (type === "attributes") {
> > +          // Put back keyboard focus if it's gone
> 
> What about if the mutation came from content JS rather than from the user
> changing an attribute in the markupview?
> In the former case, there probably was no focused field, so we shouldn't be
> re-focusing.


I think that's a good point, but... not sure how to fix it.

Ideally, the inner attribute could be focused from the `done` handler for `editableField` in `_createAttribute`. (This works if you're in edit mode and you're tabbing through from one field to the next without making any changes).

However, once an attribute value is edited, the entire attribute list is wiped away and regenerated, and the current focused element is lost. I think that this behavior will wind up looking exactly like what happens when content JS has changed the attribute (since this is a step removed from the actual user action that triggered the mutation).

Only thing I can think of is to track last focused element from the `done` handler in `editableField`), and then refocus on this element in the mutation observer, but tracking state that way... it feels a bit dirty and prone to breakage.

The mutation observer is the first place I can find that a) is called soon after the new attributes are added; and b) is aware of which attribute was modified.

Any chance you know of a better place or way to keep track of the focus?

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 7 • 10 years ago

Setting ni? so this doesn't get lost.

	Patrick Brosset <:pbro>
	Comment 8 • 10 years ago

This looks a lot like what I'm doing with a patch to make tagnames editable in the markup-view, remotely. See bug 917696.
The approach I've chosen for this is:

- right before changing the tagname (sending the request to the server), get the parent container, and remember the index (I) of the child whose tagname is being changed
- then start listening for markupmutations
- then send the request
- on markupmutations, check if there's one that is about removing the node whose tagname was changed
- if yes, stop listening for mutations
- if the user hasn't selected another node in the meantime, move selection to the parent container's child at index I.

In the case where the tagname update failed, just stop listening for mutations.

See https://bugzilla.mozilla.org/attachment.cgi?id=8500314&action=diff#a/browser/devtools/markupview/markup-view.js_sec4

In your case it's pretty similar because once the attribute is changed, the whole list of attributes is refreshed (in my case the node is removed and re-created). So we need to store some state before that happens and then re-focus.

So I'd say, right before requesting the attribute change, store enough state information to know what to do after it's been done. Then listen for attributes mutations. If the attribute change failed, stop listening. If it does get carried out, your mutation observer will be called, stop listening, and focus the right editor, based on the state information you previously stored.

Does that make sense?

	Athena Assignee
	Comment 9 • 10 years ago

That makes a lot of sense, thank you! I'll see what I can do.

	Athena Assignee
	Comment 10 • 10 years ago

Here's the patch redone as described.

Two things: 
1) Can't figure out how to test when attribute editing is cancelled (entering invalid characters just removes the attribute)

2) Causes some other tests to fail. Ugh!

Specifically, these three fail because they can't get an inplaceEditor for the new attributes field:
devtools/markupview/test/browser_markupview_tag_edit_01.js
devtools/markupview/test/browser_markupview_tag_edit_08.js
devtools/markupview/test/browser_markupview_tag_edit_09.js

Throwing it up anyway because I can't decide if this means I need to tweak those tests for the new behavior (of going to the next element after the attribute is edited), or whether this means my existing / proposed code is doing something wrong.

	Patrick Brosset <:pbro>
	Comment 11 • 10 years ago

(In reply to Athena from comment #10)
> Created attachment 8505272 [details] [diff] [review]
> markup-view-attribute-focus-state-bug-911148
> 
> Here's the patch redone as described.
> 
> Two things: 
> 1) Can't figure out how to test when attribute editing is cancelled
> (entering invalid characters just removes the attribute)
You're right, the parser will just remove the attribute if it can't be parsed.
I think there are 4 different cases:
- the editing is fine, the new attribute is set,
- the editing is incorrect, the attribute goes away altogether,
- the user removes the whole text in the attribute editor, the attribute goes away
- the user doesn't change any characters in the attribute editor,
In the first 3 cases, a markup mutation will occur and so your mutation observer in refocusOnEdit will be called.
In the fourth case, I think we detect the value is unchanged on the frontend and just don't modify the attribute. Tabbing does focus the next attribute, so that's fine.

So it looks like there's no way for an actual attribute edition to fail. Actually changing the value in an attribute editor field will *always* result in a markup mutation.
Which, in turn, means we don't need to have code to cancel the refocus after a failed change.

Can you ensure that the attribute modification protocol method on the actor cannot fail? I don't think it can.

> 2) Causes some other tests to fail. Ugh!
> 
> Specifically, these three fail because they can't get an inplaceEditor for
> the new attributes field:
> devtools/markupview/test/browser_markupview_tag_edit_01.js
> devtools/markupview/test/browser_markupview_tag_edit_08.js
> devtools/markupview/test/browser_markupview_tag_edit_09.js
> 
> Throwing it up anyway because I can't decide if this means I need to tweak
> those tests for the new behavior (of going to the next element after the
> attribute is edited), or whether this means my existing / proposed code is
> doing something wrong.
At first sight, the failures seem like test changes would be necessary. I haven't dug into details yet, but it looks like http://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/test/head.js#321 is having trouble properly focusing the field or something.

	Patrick Brosset <:pbro>
	Comment 12 • 10 years ago

Comment on attachment 8505272 [details] [diff] [review]
markup-view-attribute-focus-state-bug-911148

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

Thanks for the patch. Please see my comments below.
I've had issues when playing with it whereby the wrong field would sometimes get focused. I haven't yet been able to get good steps to reproduce, but it's got to with the way you're calculating the attribute index that needs to be refocused.

We have to take into considerations all these cases:

Say we have <div a="1" b="2" c="3">
and we edit the second attribute: b="2"
below is the list of cases, with [...] marking which field should be focused

1) edit to b="3"
<div a="1" b="3" [c="3"]>

2) edit to ""
<div a="1" [c="3"]>

3) edit to d="4"
<div a="1" [c="3"] d="4">
not sure about this one though, if the order of the attributes changes, we could just as well just focus the first one, or the last "newattr" one. I don't think this would be a big deal.

4) edit to _a="2"
<div _a="2" [a="1"] c="3">
same here, maybe we should always refocus the first or last attribute when the order changes, this would simplify the logic a lot.

::: browser/devtools/markupview/markup-view.js
@@ +2153,5 @@
>            this._applyAttributes(aVal, attr, doMods, undoMods);
>            this.container.undo.do(() => {
>              doMods.apply();
>            }, () => {
> +            this.cancelRefocusOnEdit();

Why do you want to cancel the refocus here?
The undo helper api is a little weird, but what it does here is it defines 2 callbacks: one that is executed immediately to "do" the action, and one that is stored and only executed if the user presses ctrl+Z to "undo" the action.
So once the attribute changes have been done, there won't any refocus thing to cancel anymore.

@@ +2228,5 @@
>    },
>  
>    /**
> +   * Listen to mutations, and when the attribute list is regenerated
> +   * try to focus on the attribute that was in the same position

Shouldn't this say "try to focus the attribute after the one that's being edited now"?

@@ +2230,5 @@
>    /**
> +   * Listen to mutations, and when the attribute list is regenerated
> +   * try to focus on the attribute that was in the same position
> +   */
> +  refocusOnEdit: function(aAttrName, aAttrNode) {

The devtools code base isn't big on hungarian notation like this. just 'attrName' and 'attrNode' will be fine.

@@ +2241,5 @@
> +      return;
> +    }
> +
> +    let container = this.markup.getContainer(this.node);
> +    var childIndex = Array.prototype.indexOf.call(this.attrList.childNodes, aAttrNode);

One edge case we need to think of is if the user changes the attribute *name* so that it ends up changing the sorting of the attributes.
If you have: <div a="1" b="2">
And you change a="1" to c="1"
After the change, you'll have: <div b="2" c=1">

Also, this line can be changed to:
[...this.attrList.childNodes].indexOf(aAttrNode);

Also, rename childIndex to attributeIndex.

@@ +2255,5 @@
> +          isDeletedAttribute = mutation.newValue === null;
> +          break;
> +        }
> +      }
> +      if (!isAttributeEditedMutation) {

As said earlier, I don't think isAttributeEditedMutation can ever be false, so this condition should be removed.

@@ +2259,5 @@
> +      if (!isAttributeEditedMutation) {
> +        return;
> +      }
> +
> +      this.markup._inspector.off("markupmutation", onMutations);

Since we can use 'once' instead of 'on', you can get rid of this.

@@ +2263,5 @@
> +      this.markup._inspector.off("markupmutation", onMutations);
> +      this._editedAttributeObserver = null;
> +
> +      // Don't try to refocus if the user has focused on something in the meantime
> +      if (!focusManager.focusedElement) {

Can you reverse this condition and return if true. This help avoid too much indentation.
if (focusManager.focusedElement) {
  return;
}

let visibleAttrs ....

@@ +2267,5 @@
> +      if (!focusManager.focusedElement) {
> +        let visibleAttrs = Array.prototype.filter.call(this.attrList.childNodes,
> +                          function(element) {
> +                            return element.style.display != "none";
> +                          });

Shorter this way:
let visibleAttrs = [...this.attrList.childNodes].filter(el => el.style.display != "none");

@@ +2269,5 @@
> +                          function(element) {
> +                            return element.style.display != "none";
> +                          });
> +        childIndex = (childIndex > visibleAttrs.length - 1 ? visibleAttrs.length - 1 : childIndex);
> +        let attrEditor = visibleAttrs[childIndex].querySelector("[tabindex]");

maybe using ".editable" would be more self-explanator than "[tabindex]"

@@ +2272,5 @@
> +        childIndex = (childIndex > visibleAttrs.length - 1 ? visibleAttrs.length - 1 : childIndex);
> +        let attrEditor = visibleAttrs[childIndex].querySelector("[tabindex]");
> +
> +        // focus on the attribute
> +        attrEditor.focus();

Focusing the same node again and *then* moving the focus seems a little strange. Isn't there a way to focus directly the right editor?

@@ +2282,5 @@
> +        if (elementToActivate && elementToActivate.ownerDocument === this.doc
> +            && elementToActivate._editable) {
> +          let e = this.doc.createEvent('Event');
> +          e.initEvent("dblclick", true, true);
> +          elementToActivate.dispatchEvent(e);

I think the logic to actually turn the inplaceEditor into edit mode should be handled by the inplaceEditor itself, not by dispatching an even here.
The function here should just be responsible for the logic of which field we want to focus and should then delegate to it.

Here's one way to do this:

See http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/inplace-editor.js#85, this is how we create the editable field in the first place. It returns what http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/inplace-editor.js#107 returns, which is nothing.

We could make 'editableItem' return a function (or an API object that contains a function) that can be used to turn the field into edit mode.
This function would basically just call 'aCallback(element)'.

@@ +2291,5 @@
> +    };
> +
> +    // start listening for mutations until we find an attributes change
> +    // that modifies this attribute
> +    this.markup._inspector.on("markupmutation", onMutations);

In your case, I don't actually think there can be several mutations before the attributes mutation occurs.
So we can safely use 'this.markup._inspector.once("markupmutation", onMutations);' and get rid of the 'this.markup._inspector.off("markupmutation", onMutations);' inside 'onMutations'.

@@ +2296,5 @@
> +  },
> +
> +  /**
> +   * Make sure to stop listening for attribute change markupmutations
> +   * Useful when the attribute change failed

nit: periods at the end of sentences please.

@@ +2302,5 @@
> +  cancelRefocusOnEdit: function() {
> +    if (this._editedAttributeObserver) {
> +      this.markup._inspector.off("markupmutation", this._editedAttributeObserver);
> +      this._editedAttributeObserver = null;
> +      this.markup.emit("canceledrefocusedonedit");

It doesn't look like you need this event in your patch. You should remove it for now.
And since we only need to cancel the refocus at the top of the 'refocusOnEdit' function, let's get rid of the 'cancelRefocusOnEdit' function altogether and just move the lines in 'refocusOnEdit' at the top.

	Athena Assignee
	Comment 13 • 10 years ago

Biggest change is using a method returned when we create editableItem to trigger the editor, instead of triggering an event and moving focus around. I've made new tests for the various cases you mentioned (though I merged the "_a" and "d" ones since I've gone with having both of these just move you to the beginning of the attribute list).

To address a couple specific points:

(1)
> Why do you want to cancel the refocus here?
Heh, my bad. I misunderstood what was happening and thought this would also be triggered when the attribute is invalid. And since I had no way to test before... removed.

(2)
> Also, this line can be changed to:
> [...this.attrList.childNodes].indexOf(aAttrNode);

Thanks, that looks pretty nice. Is there a name to this pattern? I'm curious and would like to find out more about it (and unfortunately "[...]" is not easily googleable!)

(3)
> Focusing the same node again and *then* moving the focus seems a little strange. Isn't there a way to focus directly the right editor?

This also handled the case when you were at the end of the attributes list and wanted the next to go to the new attribute editor. Got a better way in the patch now.

(4) Think I figured out what was going on with the wrong attribute position sometimes happening after edit: was calculating the original attribute index off all attributes, and this included deleted (hidden) ones. Fixed by looking for visible attributes in the original calculation as well

(5) Of note, removed this code from the original patch:

      // Don't try to refocus if the user has focused on something in the meantime.
      if (focusManager.focusedElement) {
        return;
      }

It was causing an inconsistency when you'd edit the name of the last attribute, because the focus would go to the .newattr instead of the first attribute.

Also removed this code:

    // Nothing was focused in the first place, so let's not try to refocus.
    if (!focusManager.focusedElement) {
      return;
    }

Because, at this point, I'm not sure that having the focus manager has any further use. Originally, it was only there to check that (a) we had a previously focused element before trying to refocus; and (b) we were able to move to the next focusable item

(a) should no longer be an issue because we only trigger the mutation listener on actual user input
(b) should no longer be an issue because we no longer track the current position using the focus element

(6) other test failures still happening, am digging into it now but that might have to wait until I have another free evening/weekend

	Athena Assignee
	Comment 14 • 10 years ago

Figured out what was wrong with the tests. Old behavior was that after editing, we'd automatically close the active editor, so the next "return" would open a new editor.

New behavior, however, keeps open an active editor; the next "return" moves you to the next field.

I've added escapes before all attempts at setting a new value, and that fixes most things.

However I found out something horrible: this patch breaks the undo/redo API. I'll try to see what I can do, but would appreciate pointers as well.

	Athena Assignee
	Comment 15 • 10 years ago

Ahh I guess it's not as broken as I thought. The undo action still works, but only once you've hit escape/exited from the editor (the next editor that was automatically activated).

The question now is whether this is a reasonable change in behavior or not.

If it seems reasonable, I'll go and see what I can do to fix the devtools/markupview/test/browser_markupview_tag_edit_01.js test which is failing on this issue.

	Patrick Brosset <:pbro>
	Comment 16 • 10 years ago

(In reply to Athena from comment #13)
> (2)
> > Also, this line can be changed to:
> > [...this.attrList.childNodes].indexOf(aAttrNode);
> 
> Thanks, that looks pretty nice. Is there a name to this pattern? I'm curious
> and would like to find out more about it (and unfortunately "[...]" is not
> easily googleable!)
The spread operator: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_operator

	Patrick Brosset <:pbro>
	Comment 17 • 10 years ago

Thanks for this new patch, much better as far as I can see.
I'm currently playing with it, trying to find remaining problems. So far, everything seems to work fine.

I did find one problem though, but it exists without your patch too. It's just more noticeable with it:

- focus the tagname
- press tab to move to the next attribute field
- there are 2 cases:
  1 - either the tagname was changed
  2 - or the tagname wasn't changed

For (1), a mutation will occur, the node will be removed and recreated again, and then re-selected (using 'reselectOnRemoved'). But the focus will be lost. We need to jump directly to the next editable field. Filed bug 1090871 for this.

For (2), we currently have a bug whereby we still recreate the node even if the tagname didn't change.
Filed bug 1090874 for this. Once fixed, we should make sure tabbing out of an unchanged tagname jumps to the next editable field too.

	Patrick Brosset <:pbro>
	Comment 18 • 10 years ago

Comment on attachment 8511925 [details] [diff] [review]
markup-view-attribute-focus-state-bug-911148

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

Nice, almost there, I think the tests need some work.

::: browser/devtools/markupview/markup-view.js
@@ +2369,5 @@
>  XPCOMUtils.defineLazyGetter(this, "clipboardHelper", function() {
>    return Cc["@mozilla.org/widget/clipboardhelper;1"].
>      getService(Ci.nsIClipboardHelper);
>  });
> +XPCOMUtils.defineLazyGetter(this, "focusManager", function() {

Let's get rid of this dependency now that you don't need the focusManager anymore.

::: browser/devtools/markupview/test/browser_markupview_tag_edit_08.js
@@ +39,5 @@
>    let {editor} = yield getContainerForSelector("#node24", inspector);
>    let attr = editor.attrs["data-long"].querySelector(".editable");
>  
>    // Check to make sure it has expanded after focus
> +  EventUtils.sendKey("escape", inspector.panelWin);

Can you explain why you need to do this? (here and in 2 other places in the same file).
The field isn't focused when you do this, so why is this needed?
Surely there is a fix we can do in the code to avoid this, it seems weird to have to make tests do this to pass.

::: browser/devtools/markupview/test/browser_markupview_tag_edit_09.js
@@ +27,5 @@
>  
>    info("Focusing the viewBox attribute editor");
>    let {editor} = yield getContainerForSelector("svg", inspector);
>    let attr = editor.attrs["viewBox"].querySelector(".editable");
> +  EventUtils.sendKey("escape", inspector.panelWin);

Same remark for this as in file browser_markupview_tag_edit_08.js

::: browser/devtools/markupview/test/browser_markupview_tag_edit_11.js
@@ +6,5 @@
> +
> +// Tests that focus position is kept when tabbing through and editing
> +// attributes
> +
> +// Generated attribute order (except for id) is the reverse of original order.

Hm, this command is a bit misleading I think.
I think we put id and class attributes first in the node, because they are the most used. Then other attributes are alphabetically ordered. But, as soon as you add new attributes, they end up at the end (except id and class), even if they would normally come an existing attribute alphabetically.
So either clarify this comment, or remove it altogether (I don't know if it helps understanding the test).

@@ +17,5 @@
> +  yield testAttributeEditing(inspector);
> +  yield testAttributeDeletion(inspector);
> +});
> +
> +function* testAttributeTraversal(inspector) {

Can you add 'info("testing blah blah blah ...");' at the beginning of each test case function. It makes it a lot simpler to debug tests that fail.

@@ +69,5 @@
> +  is(focusedAttr.tagName, "input", "newattr is active");
> +}
> +
> +function tabToNext(inspector) {
> +  EventUtils.sendKey("tab", inspector.panelWin); // popup

What does popup mean here?
I think the first tab is used to collapse the selection and move the caret to the end of the input field, is that right? If so, the comment should instead read something like: "collapse selection and move caret to end".

@@ +75,5 @@
> +}
> +
> +function tabToPrevious(inspector) {
> +  EventUtils.synthesizeKey("VK_TAB", { shiftKey: true },
> +    inspector.panelWin); // popup

Same remark here.

::: browser/devtools/markupview/test/head.js
@@ +318,5 @@
>   * @param {InspectorPanel} inspector The instance of InspectorPanel currently
>   * loaded in the toolbox
>   */
>  function setEditableFieldValue(field, value, inspector) {
> +  EventUtils.sendKey("escape", inspector.panelWin);

Same remark as in 08/09 tests.

	Athena Assignee
	Comment 19 • 10 years ago

> ::: browser/devtools/markupview/test/browser_markupview_tag_edit_08.js
> @@ +39,5 @@
> >    let {editor} = yield getContainerForSelector("#node24", inspector);
> >    let attr = editor.attrs["data-long"].querySelector(".editable");
> >  
> >    // Check to make sure it has expanded after focus
> > +  EventUtils.sendKey("escape", inspector.panelWin);
> 
> Can you explain why you need to do this? (here and in 2 other places in the
> same file).
> The field isn't focused when you do this, so why is this needed?
> Surely there is a fix we can do in the code to avoid this, it seems weird to
> have to make tests do this to pass.

It's because the behavior when you hit "return" has changed.

Old behavior:
- focus on attribute
- return opens attribute editor
- return saves changes and closes attribute editor
(clean state for next test)

New behavior:
- focus on attribute
- return opens attribute editor
- return saves changes, closes current attribute editor, opens the next attribute editor
(need extra escape to close attribute editor)

This is what we want when actually interacting with markupview so we need that extra escape. However, looking at the tests again, it's clearer to have the escape at the end, to make it obvious it's the test resetting to the original state, rather than the next test trying to "fix" the state from the previous.


> I think we put id and class attributes first in the node, because they are
> the most used. Then other attributes are alphabetically ordered. But, as
> soon as you add new attributes, they end up at the end (except id and
> class), even if they would normally come an existing attribute
> alphabetically.
> So either clarify this comment, or remove it altogether (I don't know if it
> helps understanding the test).

I'm actually not sure what's going on here. But when I go to:

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

That shows up in the inspector as:

   <div a="" b="" c=""></div>


If I instead use this:

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

I get this:

  <div c="" b="" a=""></div>

So I figured the comment would be useful because otherwise the order of traversal in testAttributeTraversal looks like it should be the other way around.

Edited comment, and moved to just above the relevant section in testAttributeTraversal.

	Patrick Brosset <:pbro>
	Comment 20 • 10 years ago

(In reply to Athena from comment #19)
> It's because the behavior when you hit "return" has changed.
> 
> Old behavior:
> - focus on attribute
> - return opens attribute editor
> - return saves changes and closes attribute editor
> (clean state for next test)
> 
> New behavior:
> - focus on attribute
> - return opens attribute editor
> - return saves changes, closes current attribute editor, opens the next
> attribute editor
> (need extra escape to close attribute editor)
> 
> This is what we want when actually interacting with markupview so we need
> that extra escape. However, looking at the tests again, it's clearer to have
> the escape at the end, to make it obvious it's the test resetting to the
> original state, rather than the next test trying to "fix" the state from the
> previous.
Yes, having the escape at the end is way clearer, BUT, I still don't understand one thing, the new behavior should only happen when using TAB, not ENTER.
I think the old behavior still makes the most sense when using ENTER.

> I'm actually not sure what's going on here. But when I go to:
> 
>   data:text/html,<div c b a></div>
> 
> That shows up in the inspector as:
> 
>    <div a="" b="" c=""></div>
> 
> 
> If I instead use this:
> 
>    data:text/html,<div a b c></div>
> 
> I get this:
> 
>   <div c="" b="" a=""></div>
> 
> So I figured the comment would be useful because otherwise the order of
> traversal in testAttributeTraversal looks like it should be the other way
> around.
> 
> Edited comment, and moved to just above the relevant section in
> testAttributeTraversal.
Right, this is weird. I suggest you file a bug so we can investigate why |data:text/html,<div a b c></div>| would render as |<div c="" b="" a=""></div>| in the markup-view.

	Patrick Brosset <:pbro>
	Comment 21 • 10 years ago

Also, there's something I missed in the previous reviews, sorry I should have caught this earlier:
- focus a field
- press enter
- edit the value
- press shift-TAB
==> The focus goes to the *next* field, not the *previous* one as expected.

In fact, as long as the value doesn't change, pressing ENTER, TAB, and shift-TAB works as expected. inplace-editor.js handles the navigation there and gets it ok.
As soon as the value is changed though, we get our mutation event, and handle the navigation from markup-view.js but we do miss a few edge cases.

	Patrick Brosset <:pbro>
	Comment 22 • 10 years ago

Comment on attachment 8513468 [details] [diff] [review]
markup-view-attribute-focus-state-bug-911148

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

The code changes look good to me.
I think we need to sort out the edge cases I described above, but apart from that, this is getting ready to land soon.

::: browser/devtools/markupview/test/browser_markupview_tag_edit_11.js
@@ +39,5 @@
> +  tabToNext(inspector);
> +  checkFocusedAttribute("c", inspector, true);
> +
> +  info("Tabbing backward through attributes in edit mode");
> +  tabToPrevious(inspector);

We miss a test that tabs to next and previous and that modifies the values at each step.

::: browser/devtools/shared/inplace-editor.js
@@ +151,5 @@
> +
> +  // Save the trigger type so we can dispatch this later
> +  element._trigger = trigger;
> +
> +  return function turnOnEditMode() {

Please update the editableItem function's jsdoc comment (add a @return comment).

@@ +964,5 @@
>          // move the focus as requested.
>          let next = moveFocus(this.doc.defaultView, direction);
>  
>          // If the next node to be focused has been tagged as an editable
>          // node, send it a click event to trigger

Please adapt this comment now that it's not always a click event.

	Athena Assignee
	Comment 23 • 10 years ago

> Yes, having the escape at the end is way clearer, BUT, I still don't
> understand one thing, the new behavior should only happen when using TAB,
> not ENTER.
> I think the old behavior still makes the most sense when using ENTER.

(plus various edge cases with tab/shift-tab/enter)
I see what you mean now. Might be the only way to fix it is to have an "onFocusMove" callback passed into the editablefield options. Let me play with that and get back to you.

> Right, this is weird. I suggest you file a bug so we can investigate why
> |data:text/html,<div a b c></div>| would render as |<div c="" b=""
> a=""></div>| in the markup-view.

Filed bug 1093593.

	Athena Assignee
	Comment 24 • 10 years ago

This should handle the direction-related edge cases. Note that there's some additional complexity because we've had to move this to after the .done callback is called (when the attribute is hidden).

Additional changes to tests have been removed; manually playing with it it all looks fine.

But (ugh) I'm having issues with the test which combines traversal + editing. Exact same input, exact same everything, as far as I can tell up until doMods are applied (exact same modifications to be applied there) but the node's attributes are ordered differently depending on whether you're doing the change manually or running the test.

You start with this.node.attributes in the order ["id","a","b","c"].

When manually tabbing through, after doMods.apply() the node attributes are in the same order.

When running the test, after doMods.apply(), the node attributes are suddenly in the order ["id","b","c","a"] -- as if I'd removed the old attribute and then created a new one.

My best guess is a timing issue, but it doesn't quite make sense in this context; there's no extra input between when the tab key is sent and when we handle the mutations event.

	Patrick Brosset <:pbro>
	Comment 25 • 10 years ago

Comment on attachment 8526540 [details] [diff] [review]
markup-view-attribute-focus-state-bug-911148

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

Thanks for working on this. It's proving a little bit hard to get the details right, while keeping the code as simple as possible.
I've made a couple of comments below about potentially using |done| instead of introducing a new callback. Please let me know what you think of this approach.

::: browser/devtools/markupview/markup-view.js
@@ +2243,5 @@
> +
> +    let container = this.markup.getContainer(this.node);
> +
> +    // First we filter to visible (not deleted) or the current attribute.
> +    // The current attribute may be hidden if we had just modified its value, triggering a mutation.

This is the added complexity you mentioned in your last comment. Wouldn't that become unneeded with my comment in inplace-editor.js? (using the done callback to call this instead).
Maybe I'm wrong, you've worked with this part of the code more than me at this stage. I'd really love to keep this change as simple as possible, the markup-view is already quite complex as it is.

::: browser/devtools/shared/inplace-editor.js
@@ +66,5 @@
>   *       commit the change.  This function is called before the editor
>   *       has been torn down.
>   *    {function} destroy:
>   *       Called when the editor is destroyed and has been torn down.
> + *    {function} focusChange:

I like this approach, but do you think it would be possible to remove the need for yet another configuration option by making |done| or |destroy| have an extra |direction| argument?
I think |done| would be a good candidate for this. It already has 2 arguments (value and commit), but there's not much harm in adding a 3rd one which would be the direction.
This would help keep this widget simple and would also simplify the caller in markup-view.

@@ +111,2 @@
>   */
> +

nit: no need for this empty line

	Athena Assignee
	Comment 26 • 10 years ago

I do like how that looks compared to before. Removed the extra call and reverted the filtering of activeEditors to the original simpler version.

I'm still stuck on getting the test to work though. Forcing an order to the attributes now seems more and more tempting (but it's unlikely to be the desired effect, especially when adding new attributes where it makes sense they stay at the end as they're added).

	Patrick Brosset <:pbro>
	Comment 27 • 10 years ago

Comment on attachment 8528540 [details] [diff] [review]
markup-view-attribute-focus-state-bug-911148

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

Thanks for changing the patch to handle the direction change in the |done| callback. Looks good.

I did find one bug while playing with the feature, and this bug doesn't appear without the patch:
- focus an attribute field and hit enter to turn it to edit mode
- remove the whole text (everything in the field, name="value")
- hit enter:
TypeError: activeEditor.querySelector(...) is null: ElementEditor.prototype.refocusOnEdit/this._editedAttributeObserver@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/markupview/markup-view.js:2397:9

This doesn't look too hard to fix.

About the attributes order crazyness, I did some research and found the following:
- 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.

This at least explains why the markup-view displays the attributes in reverse order (except from id and class which appear first). If you set a breakpoint in the MarkupElementContainer constructor and look at node._form.attributes, you'll see the reverse ordered array there.

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.
So, knowing this, it's only normal that your test would fail. In fact, what I discovered is that in the test, after the mutation has been handled, a new call to the walker's children method is made, and that returns the new version of the NodeFronts, and therefore the markup-view is updated, with the attributes order. When testing manually, this extra call isn't made, so the order doesn't change (and doesn't reflect the reality anymore).
The extra call seems to be made by breadcrumbs.js, which is the widget we use to display the breadcrumbs above the markup-view. It probably listens to markup-mutations to update itself too. 
In fact, the main difference when running the test or testing manually, is that when doing the change manually, the node is selected, while in your test, body is selected by default. And that seems to make the breadcrumbs do the call in one case and not in the other (I haven't investigated further why).

So, to fix this, you should make sure the test node is selected first.

In any case, I *think* the clean way of fixing this would be to alphabetically order all non id/class attributes. I don't see why a user would be interested in seeing attributes jump around just to reflect the internal implementation details of the ordering of a NamedNodeMap.
But since this bug has been going on for a while already, we can just file a follow-up bug, and just add a comment above the selectNode fix in the test to explain this situation and gives the bug number.

Next, I think it would help splitting this new test in several smaller tests. I'll start something and attach a patch for you to take a look.
Also, instead of having the test assume any specific order, why not access the DOM element's attributes and just compare that to the markup-view.

I also found that in your testAttributeEditing test-case, simulating escape the first time activateFirstAttribute is called actually opens up the split console panel! And focuses its input field, therefore breaking the rest of the test. Does this happen for you too?
Tip: when debugging tests, it's useful to add delays here and there to see what the browser is doing (otherwise it goes too fast): 'yield wait(1000)' will make the async test wait for 1sec for instance.

::: browser/devtools/markupview/test/browser_markupview_tag_edit_11.js
@@ +4,5 @@
> +
> +"use strict";
> +
> +// Tests that focus position is kept when tabbing through and editing
> +// attributes

See the new patch I'm going to attach in a minute for how to solve the test failures here and make the test a bit more robust too.

::: browser/devtools/shared/inplace-editor.js
@@ +63,5 @@
>   *    {function} done:
>   *       Called when input is committed or blurred.  Called with
> + *       current value, a boolean telling the caller whether to
> + *       commit the change, and the direction of the next element.
> + *       This function is called before the editor has been torn down.

Cool, I like this version of this file better. Simpler.
Just one last minor remark: could you improve a little bit this comment? In particular, I think it should explain what is "direction" here and what values it can have?

	Patrick Brosset <:pbro>
	Comment 28 • 10 years ago

This splits the test in 2 and makes the main traversal/editing test pass.

	Athena Assignee
	Comment 29 • 10 years ago

Much thanks for the huge tips for the tests. I've fixed the error on deletion (it apparently only happens when the last thing is deleted!), and have tweaked the comments as requested.

I've also looked at the advice you had around tests, but I've not done many changes in that respect because it seems like the new tests you wrote already address the comments. 

(Let me know if I missed something though!)


Bug 1093593 should be good for handling the attribute ordering issues we've run into.

	Patrick Brosset <:pbro>
	Comment 30 • 10 years ago

Comment on attachment 8549344 [details] [diff] [review]
markup-view-attribute-focus-state-bug-911148

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

\o/ looks good to me. Good job Athena.
Let's try this on TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=71db11b2338e

	Patrick Brosset <:pbro>
	Comment 31 • 10 years ago

Try is green. Asking for check-in.

	Ryan VanderMeulen [:RyanVM]
	Comment 32 • 10 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 33 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/7140e09999c8