1243045 - [a11y] Box model inspector is not accessible

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Yura Zenevich [:yzen] (please use "needinfo")]

The box model tab is currently not keyboard accessible and lacks some description. We need to think of the way to let the screen reader user perceive the box model.

Comment 1

[Yura Zenevich [:yzen] (please use "needinfo")]

For example firebug makes each level: position, margin, border etc a grouping, and puts them in sequential tab order. It works pretty great.

Comment 2

[Yura Zenevich [:yzen] (please use "needinfo")]

Actually not tab order, arrow order

Comment 3

[Yura Zenevich [:yzen] (please use "needinfo")]

Activation on any level will activate the top input for the property. Sequential tabbing switches to right-bottom-left property inputs.

Comment 4

[Patrick Brosset <:pbro>]

Filter on CLIMBING SHOES

Comment 5

[Yura Zenevich [:yzen] (please use "needinfo")]

\o/

Comment 6

[Yura Zenevich [:yzen] (please use "needinfo")]

So here are the details we chatted about:

* Box model widget can be tabbed to from the tablist or shift-tabbed from its relative next element
* By default box model widget only allows arrow navigation where UpArrow will move 1 level up  in the box model (if available) and down arrow will move one level down (if available). Tabbing at this stage will move the focus away from the box model widget
* Labeling for each level at this point should be reading the level of the box model and the values starting from top and going clockwise
* To get access to the values the user has to press enter. This should put focus on the top value
* Consequent tabbing will move the focus clockwise between values on the same level
* Escape will move focus back onto the widget itself
* Pressing enter will make the current focused value editable
* Consequent tabbing will move the focus to the next clockwise value and preserve the edit mode
* Escape will unset the edit mode at this point.

Marco, what do you think of this interactions? I was thinking to keep them consistent between all widgets (similar to markup view).

Comment 7

[Marco Zehe (:MarcoZ)]

(In reply to Yura Zenevich [:yzen] from comment #6)
> Marco, what do you think of this interactions? I was thinking to keep them
> consistent between all widgets (similar to markup view).
I think this sounds absolutely fantastic! The more consistency we'll have, the less steep the learning curve will be, and the more engaging for new web developers and testers who rely on assistive technologies or the keyboard alone.

Comment 8

[Nancy Pang [:npang]]

Attachment: Bug 1243045 - Added navigation for padding, border and margin.

Review commit: https://reviewboard.mozilla.org/r/65744/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65744/

Comment 9

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

as per in person conversation, removing an r? for now.

Comment 10

[Nancy Pang [:npang]]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65744/diff/1-2/

Comment 11

[Nancy Pang [:npang]]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65744/diff/2-3/

Comment 12

[Yura Zenevich [:yzen] (please use "needinfo")]

https://reviewboard.mozilla.org/r/65744/#review63186

This is just a fist attempt at reivew, I haven't actually tried it yet. I will with comments addressed.

::: devtools/client/inspector/inspector.xul:130
(Diff revision 2)
>                    <html:button class="devtools-button" id="layout-geometry-editor" title="&geometry.button.tooltip;"></html:button>
>                    <html:span id="layout-element-position"></html:span>
>                  </html:section>
>                </html:p>
>  
> -              <html:div id="layout-main">
> +              <html:div id="layout-main" tabindex="0">

This probably needs a role="group" and a label (perhaps using aria-label) naming the widget meaningully.

::: devtools/client/inspector/layout/layout.js:452
(Diff revision 2)
>      }
>  
>      let nodeGeometry = this.doc.getElementById("layout-geometry-editor");
>      nodeGeometry.removeEventListener("click", this.onGeometryButtonClick);
>  
> +    this.layoutBox.removeEventListener("keydown", this._onMarginLayoutFocus, true);

Does this need to be renamed? It seems like an artifact from a previous version.

::: devtools/client/inspector/layout/layout.js:547
(Diff revision 2)
>    onMarkupViewNodeHover: function () {
>      this.hideGeometryEditor(false);
>    },
>  
> +  _onMoveFocus: function (event) {
> +    this._onMoveLevelFocus = this._onMoveLevelFocus.bind(this);

I would not do this binding on every event, why now do it once were onMoveFocus is bound. (for all 4)

::: devtools/client/inspector/layout/layout.js:557
(Diff revision 2)
> +    let {target, keyCode, shiftKey} = event;
> +    let level = target.dataset.box;
> +    let isEditable = target.tagName === "input" || target._editable;
> +
> +    if (target == this.layoutBox
> +      && keyCode === KeyEvent.DOM_VK_RETURN) {

&& at the end of the line

::: devtools/client/inspector/layout/layout.js:558
(Diff revision 2)
> +    let level = target.dataset.box;
> +    let isEditable = target.tagName === "input" || target._editable;
> +
> +    if (target == this.layoutBox
> +      && keyCode === KeyEvent.DOM_VK_RETURN) {
> +      let marginLayout = this.layoutBox.querySelectorAll("#layout-margins")[0];

Here and below, since you are quering for unique elements (by ID) why not use querySelector.

::: devtools/client/inspector/layout/layout.js:631
(Diff revision 2)
> +      e.initEvent("click", true, true);
> +      editBox.dispatchEvent(e);
> +    }
> +  },
> +
> +  _onMoveLevelFocus(editLevel, key) {

"on" part of the name implies an event. I think here we can simply call it moveLevelFocus

::: devtools/client/inspector/layout/layout.js:632
(Diff revision 2)
> +      editBox.dispatchEvent(e);
> +    }
> +  },
> +
> +  _onMoveLevelFocus(editLevel, key) {
> +    let marginLayout = this.layoutBox.querySelectorAll("#layout-margins")[0];

same here querySelector

::: devtools/client/inspector/layout/layout.js:640
(Diff revision 2)
> +
> +    if (!editLevel) {
> +      return null;
> +    }
> +
> +    if (key === KeyEvent.DOM_VK_DOWN) {

Here and below, lets do if else..
It looks like all the stuff inside every if statament is very similar, I would take it out into a separate function.

Comment 13

[Nancy Pang [:npang]]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65744/diff/3-4/

Comment 14

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

Hey Nancy, could you rebase with latest mozilla-central, it doesn't apply if I download from mozreview.. Thanks

Comment 15

[Nancy Pang [:npang]]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65744/diff/4-5/

Comment 16

[Nancy Pang [:npang]]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65744/diff/5-6/

Comment 17

[Yura Zenevich [:yzen] (please use "needinfo")]

https://reviewboard.mozilla.org/r/65744/#review64134

More suggestions that we can take a look at together.

::: devtools/client/inspector/inspector.xul:110
(Diff revision 6)
>                  <html:span>&layoutViewTitle;</html:span>
>                  <html:button class="devtools-button" id="layout-geometry-editor" title="&geometry.button.tooltip;"></html:button>
>                </html:div>
>  
>                <html:div id="layout-container">
> -                <html:div id="layout-main">
> +                <html:div id="layout-main" tabindex="0" role="group" aria-label="Box Model Widget">

We need to make sure this is internationalized and also I would drop "Widget"

::: devtools/client/inspector/inspector.xul:121
(Diff revision 6)
> -                      <html:div id="layout-padding" data-box="padding" title="&padding.tooltip;">
> -                        <html:div id="layout-content" data-box="content" title="&content.tooltip;">
> +                      <html:div id="layout-padding" data-box="padding" title="&padding.tooltip;" tabindex="-1">
> +                        <html:div id="layout-content" data-box="content" title="&content.tooltip;" tabindex="-1">
>                          </html:div>
>                        </html:div>
>                      </html:div>
>                 	  </html:div>

Some weird whitespace I guess.

::: devtools/client/inspector/layout/layout.js:228
(Diff revision 6)
>      this.onPickerStarted = this.onPickerStarted.bind(this);
>      this.onMarkupViewLeave = this.onMarkupViewLeave.bind(this);
>      this.onMarkupViewNodeHover = this.onMarkupViewNodeHover.bind(this);
>      this.onWillNavigate = this.onWillNavigate.bind(this);
>  
> +    this._onMoveFocus = this._onMoveFocus.bind(this);

looks like convention in this file is to not use _ to prefix the method.

::: devtools/client/inspector/layout/layout.js:469
(Diff revision 6)
>      header.removeEventListener("dblclick", this.onToggleExpander);
>  
>      let nodeGeometry = this.doc.getElementById("layout-geometry-editor");
>      nodeGeometry.removeEventListener("click", this.onGeometryButtonClick);
>  
> +    this.layoutBox.removeEventListener("keydown", this._onMoveFocus, true);

I guess since it's handling more than just focus move (enter, escape etc) I would rename to onKeyDown (shocker :))

::: devtools/client/inspector/layout/layout.js:579
(Diff revision 6)
>    onMarkupViewNodeHover: function () {
>      this.hideGeometryEditor(false);
>    },
>  
> +  _onMoveFocus: function (event) {
> +    this._updateLayoutTitle = this._updateLayoutTitle.bind(this);

This binding should happen outside of the event handler.

::: devtools/client/inspector/layout/layout.js:582
(Diff revision 6)
>  
> +  _onMoveFocus: function (event) {
> +    this._updateLayoutTitle = this._updateLayoutTitle.bind(this);
> +    let {target, keyCode, shiftKey} = event;
> +    let level = target.dataset.box;
> +    let isEditable = target.tagName === "input" || target._editable;

Just _editable will not suffice?

::: devtools/client/inspector/layout/layout.js:586
(Diff revision 6)
> +    let level = target.dataset.box;
> +    let isEditable = target.tagName === "input" || target._editable;
> +
> +    let nextLayout;
> +
> +    if (target == this.boxModel &&

So I would refactor this as a switch between different keyCodes

::: devtools/client/inspector/layout/layout.js:672
(Diff revision 6)
> +      editBox.dispatchEvent(e);
> +    }
> +  },
> +
> +  _moveLevelFocus(editLevel, key) {
> +    let marginLayout = this.layoutBox.querySelector("#layout-margins");

These should probably be taken out into init if they do not change and also changed to getElementById

::: devtools/client/inspector/layout/layout.js:677
(Diff revision 6)
> +    let marginLayout = this.layoutBox.querySelector("#layout-margins");
> +    let borderLayout = this.layoutBox.querySelector("#layout-borders");
> +    let paddingLayout = this.layoutBox.querySelector("#layout-padding");
> +    let layouts = [marginLayout, borderLayout, paddingLayout];
> +
> +    if (!editLevel) {

When is it the case that editLevel is null? perhaps we should handle it outside of this function?

::: devtools/client/inspector/layout/layout.js:683
(Diff revision 6)
> +      return null;
> +    }
> +
> +    let index;
> +
> +    for (let i = 0; i < layouts.length; i++) {

yeah so here I would do something like this:

when creating layouts structure, we can have it as an object like this:

{
  xLayoutTitle: {elm: selector, DOM_VK_DOWN: selector, DOM_VK_UP: selector},
  yLayoutTitle: ...
  ...
}

Then we do not need to deal with indexes and can just access the right selector by: layouts[editLevel]

Comment 18

[Nancy Pang [:npang]]

https://reviewboard.mozilla.org/r/65744/#review64134

> Just _editable will not suffice?

need the input check for tab sequence for users editing the fields

Comment 19

[Nancy Pang [:npang]]

https://reviewboard.mozilla.org/r/65744/#review64134

> yeah so here I would do something like this:
> 
> when creating layouts structure, we can have it as an object like this:
> 
> {
>   xLayoutTitle: {elm: selector, DOM_VK_DOWN: selector, DOM_VK_UP: selector},
>   yLayoutTitle: ...
>   ...
> }
> 
> Then we do not need to deal with indexes and can just access the right selector by: layouts[editLevel]

That's a cool way to do it

Comment 20

[Nancy Pang [:npang]]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65744/diff/6-7/

Comment 21

[Nancy Pang [:npang]]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65744/diff/7-8/

Comment 22

[Yura Zenevich [:yzen] (please use "needinfo")]

https://reviewboard.mozilla.org/r/65744/#review64606

still looking just a quick feedback, getting this error when trying to navigate the widget:
resource://devtools/client/inspector/layout/layout.js, line 714: TypeError: this.layouts[editLevel] is undefined

::: devtools/client/inspector/inspector.xul:110
(Diff revision 8)
>                  <html:span>&layoutViewTitle;</html:span>
>                  <html:button class="devtools-button" id="layout-geometry-editor" title="&geometry.button.tooltip;"></html:button>
>                </html:div>
>  
>                <html:div id="layout-container">
> -                <html:div id="layout-main">
> +                <html:div id="layout-main" tabindex="0" role="group" aria-label="&layoutViewTitle;">

Curious if this is being reused somehow or you need to add an entry in the message bundle?

::: devtools/client/inspector/layout/layout.js:239
(Diff revision 8)
> +    this.wrapFocus = this.wrapFocus.bind(this);
> +    this.updateLayoutTitle = this.updateLayoutTitle.bind(this);
> +
> +    this.boxModel = this.doc.getElementById("layout-wrapper");
> +    this.layoutBox = this.doc.getElementById("layout-main");
> +    this.boxModel.addEventListener("keydown", this.onKeyDown, true);

nit: separate add event listener part from the the group of element declarations.

Comment 23

[Yura Zenevich [:yzen] (please use "needinfo")]

https://reviewboard.mozilla.org/r/65744/#review64614

more feedback

::: devtools/client/inspector/layout/layout.js:604
(Diff revision 8)
>    },
>  
> +  onKeyDown: function (event) {
> +    let {target, keyCode, shiftKey} = event;
> +    let level = target.dataset.box;
> +

nit: not need for space

::: devtools/client/inspector/layout/layout.js:606
(Diff revision 8)
> +  onKeyDown: function (event) {
> +    let {target, keyCode, shiftKey} = event;
> +    let level = target.dataset.box;
> +
> +    // If focused on editable value or in editing mode
> +    let isEditable = target._editable || target.editor;

nit: not need for space

::: devtools/client/inspector/layout/layout.js:611
(Diff revision 8)
> +    let isEditable = target._editable || target.editor;
> +
> +    let nextLayout;
> +
> +    switch (keyCode) {
> +

nit: not need for space here and around all case clauses

::: devtools/client/inspector/layout/layout.js:622
(Diff revision 8)
> +        }
> +        break;
> +
> +      case KeyEvent.DOM_VK_DOWN:
> +        this.makeUnfocasable(level);
> +        nextLayout = this.moveLevelFocus(level, keyCode);

here and for up: not preventing default will lead to scrolling of the whole tab panel.

::: devtools/client/inspector/layout/layout.js:655
(Diff revision 8)
> +        break;
> +    }
> +
> +    if (nextLayout) {
> +      this.updateLayoutTitle(nextLayout);
> +      nextLayout.focus();

I think in cases when we do not need to shift focus we should not do it and instead use aria-activedescendant

Comment 24

[Nancy Pang [:npang]]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65744/diff/8-9/

Comment 25

[Nancy Pang [:npang]]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65744/diff/9-10/

Comment 26

[Yura Zenevich [:yzen] (please use "needinfo")]

https://reviewboard.mozilla.org/r/65744/#review65158

Couple more comments :)

::: devtools/client/inspector/layout/layout.js:243
(Diff revision 10)
> +    this.boxModel.addEventListener("keydown", this.onKeyDown, true);
> +    this.boxModel.addEventListener("focus", this.onFocus, true);
> +    this.marginLayout = this.doc.getElementById("layout-margins");
> +    this.borderLayout = this.doc.getElementById("layout-borders");
> +    this.paddingLayout = this.doc.getElementById("layout-padding");
> +    this.marginLayout.addEventListener("keydown", this.onKeyDown, true);

this feels a little funky. Seems like we are handling keyboard events in the same way for each level. Is ther a way to just have 1 listener on the top level container and use capturing (3rd arg of addEventListener) ?

::: devtools/client/inspector/layout/layout.js:610
(Diff revision 10)
>    onMarkupViewNodeHover: function () {
>      this.hideGeometryEditor(false);
>    },
>  
> +  onFocus: function () {
> +    let activeDescendant = this.boxModel.getAttribute("aria-activedescendant");

This might not do what you'd expect if you start clicking around different levels. After the first click, aria-activedescendant will be set, but if we consequently click inside another container, the attribute will not get updated, AFAIK.

Comment 27

[Yura Zenevich [:yzen] (please use "needinfo")]

https://reviewboard.mozilla.org/r/65742/#review65166

::: devtools/client/inspector/layout/layout.js:610
(Diff revision 10)
>    onMarkupViewNodeHover: function () {
>      this.hideGeometryEditor(false);
>    },
>  
> +  onFocus: function () {
> +    let activeDescendant = this.boxModel.getAttribute("aria-activedescendant");

We also want to: if the click happens over non ediatable element (not over the number value for any level) we want to preserve focus on the box model container.

Comment 28

[Yura Zenevich [:yzen] (please use "needinfo")]

https://reviewboard.mozilla.org/r/65742/#review65168

::: devtools/client/inspector/layout/layout.js:686
(Diff revision 10)
> +    let first = true;
> +
> +    let selector = ".layout-" + editLevel;
> +    let editBoxes = this.boxModel.querySelectorAll(selector);
> +
> +    for (let edit of editBoxes) {

I feel like this has a high capacity to get regressed easily if the markup changes. Maybe we can do something similar to that Map stuff and define related elements explicitely.

::: devtools/client/inspector/layout/layout.js:697
(Diff revision 10)
> +    }
> +  },
> +
> +  wrapFocus: function (element, back) {
> +    let editingMode = element.tagName === "input";
> +    let elementLevel = editingMode ? element.parentElement.children[1].dataset.box : element.dataset.box;

Not sure if this is the same but the level is already calculated in the parent function that calls wrapFocus, you should just pass it as an arument then.

Comment 29

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

https://reviewboard.mozilla.org/r/65744/#review71726

::: devtools/client/themes/layout.css:347
(Diff revision 10)
>  }
> +
> +#layout-margins.layout-active-elm,
> +#layout-borders.layout-active-elm,
> +#layout-padding.layout-active-elm {
> +  outline: 1px dotted red;

This probably needs ux-review from Helen. She will likely have better ideas about this.

Comment 30

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

I'll remove this for now, it probably needs rebasing + some comments addressed

Comment 31

[Nancy Pang [:npang]]

Attachment: Bug 1243045 - Added navigation for padding, border and margin.

Review commit: https://reviewboard.mozilla.org/r/92560/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/92560/

Comment 32

[Nancy Pang [:npang]]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65744/diff/10-11/

Comment 33

[Yura Zenevich [:yzen] (please use "needinfo")]

Hi Nancy, I noticed you pushed to review again, are those ready for re-review? Thanks

Comment 34

[Nancy Pang [:npang]]

Hey Yura, they should be ready to review. I rebased my branch and made a couple of changes.

Comment 35

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

https://reviewboard.mozilla.org/r/65744/#review94196

::: devtools/client/inspector/components/box-model.js:536
(Diff revision 11)
> +    let activeDescendant = this.boxModel.getAttribute("aria-activedescendant");
> +
> +    if (!activeDescendant) {
> +      let nextLayout = this.marginLayout;
> +      this.boxModel.setAttribute("aria-activedescendant", nextLayout.id);
> +      nextLayout.className = "layout-active-elm";

There might be other classes on the element and this would override them. I would only use {some_element}.classList.add/remove for these purposes

::: devtools/client/inspector/components/box-model.js:573
(Diff revision 11)
> +            this.wrapFocus(target, false);
> +          }
> +        }
> +        break;
> +      case KeyEvent.DOM_VK_ESCAPE:
> +        if (target._editable) {

aren't we supposed to be using isEditable flag here?

::: devtools/client/inspector/components/box-model.js:579
(Diff revision 11)
> +          event.preventDefault();
> +          event.stopPropagation();
> +          this.makeUnfocasable(level);
> +          this.boxModel.focus();
> +        }
> +        break;

I think ESLint will complain if you do not have default: case (even though it just has break; in it)

::: devtools/client/inspector/components/box-model.js:595
(Diff revision 11)
> +  },
> +
> +  makeUnfocasable: function (editLevel) {
> +    let datalevel = this.doc.getElementById(editLevel).getAttribute("data-box");
> +    let selector = "[data-box='" + datalevel + "']" + ".boxmodel-editable";
> +    let editBoxes = this.doc.querySelectorAll(selector);

similar to makeUnfocusable:

```
let editBoxes = [...this.doc.querySelectorAll(
  `[data-box="${dataLevel}"].boxmodel-editable`)];
editBoxes.forEach(editBox => editBox.setAttribute("tabindex", "-1"));
```

::: devtools/client/inspector/components/box-model.js:603
(Diff revision 11)
> +      editBox.setAttribute("tabindex", "-1");
> +    }
> +  },
> +
> +  makeFocusable: function (editLevel) {
> +    let first = true;

this stuff is not needed, see below:

::: devtools/client/inspector/components/box-model.js:607
(Diff revision 11)
> +  makeFocusable: function (editLevel) {
> +    let first = true;
> +
> +    let datalevel = this.doc.getElementById(editLevel).getAttribute("data-box");
> +    let selector = "[data-box='" + datalevel + "']" + ".boxmodel-editable";
> +    let editBoxes = this.doc.querySelectorAll(selector);

I think this first logic might be confusing for the reader, why don't we just do

```
let editBoxes = [...this.doc.querySelectorAll(
  `[data-box="${dataLevel}"].boxmodel-editable`)];
editBoxes.forEach(editBox => editBox.setAttribute("tabindex", "0"));
editBoxes[0].focus();
```

::: devtools/client/inspector/components/box-model.js:609
(Diff revision 11)
> +
> +    let datalevel = this.doc.getElementById(editLevel).getAttribute("data-box");
> +    let selector = "[data-box='" + datalevel + "']" + ".boxmodel-editable";
> +    let editBoxes = this.doc.querySelectorAll(selector);
> +
> +    for (let editBox of editBoxes) {

not needed, see above

::: devtools/client/inspector/components/box-model.js:619
(Diff revision 11)
> +      }
> +    }
> +  },
> +
> +  wrapFocus: function (element, back) {
> +    let editingMode = element.tagName === "input";

do you know if element._editable is not sufficient?

::: devtools/client/inspector/components/box-model.js:625
(Diff revision 11)
> +
> +    let selector = ".boxmodel-editing > .boxmodel-editable";
> +    let editLevel = editingMode ? this.doc.querySelector(selector).getAttribute("data-box") : element.getAttribute("data-box");
> +    let next = back ? "left" : "top";
> +
> +    selector = ".boxmodel-" + editLevel + ".boxmodel-" + next + "> .boxmodel-editable";

nit:

```
`.boxmodel-${editLevel}.boxmodel-${next} > .boxmodel-editable`
```

::: devtools/client/inspector/components/box-model.js:631
(Diff revision 11)
> +    let editBox = this.boxModel.querySelector(selector);
> +    editBox.focus();
> +
> +    if (editingMode) {
> +      let e = this.doc.createEvent("Event");
> +      e.initEvent("click", true, true);

can we use element._trigger instead of "click"? if it is present of course

Comment 36

[Yura Zenevich [:yzen] (please use "needinfo")]

One more question, Nancy: are those 2 review requests complimentary? Should we merge them into 1?

Comment 37

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8810152 [details]
Bug 1243045 - Added navigation for padding, border and margin.

As we talked in person, lets merge the two into 1.

Comment 38

[Nancy Pang [:npang]]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

https://reviewboard.mozilla.org/r/65744/#review94196

> do you know if element._editable is not sufficient?

the element is a dynamically created style inspector with input tag has a class field but not editable field

> can we use element._trigger instead of "click"? if it is present of course

used element._click()

Comment 39

[Nancy Pang [:npang]]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65744/diff/11-12/

Comment 40

[Yura Zenevich [:yzen] (please use "needinfo")]

One thing I noticed is that when trying to move between levels of box model, the focus always moves onto a container. Then when I step in again, it is at the correct level. Also, I was wondering if there's any way to, when entering the box model, switch into focused state and not edit mode? Sort of similar to how the inspector works right now? More review bits are coming up :)

Comment 41

[Yura Zenevich [:yzen] (please use "needinfo")]

Also, dimension box should probably be supporting aria-activedescendant too.

Comment 42

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Yura Zenevich [:yzen] from comment #40)
> One thing I noticed is that when trying to move between levels of box model,
> the focus always moves onto a container. Then when I step in again, it is at
> the correct level. Also, I was wondering if there's any way to, when
> entering the box model, switch into focused state and not edit mode? Sort of
> similar to how the inspector works right now? More review bits are coming up
> :)

Duh, scratch that, now that I'm looking at active descendant..

Comment 43

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Yura Zenevich [:yzen] from comment #42)
> (In reply to Yura Zenevich [:yzen] from comment #40)
> > One thing I noticed is that when trying to move between levels of box model,
> > the focus always moves onto a container. Then when I step in again, it is at
> > the correct level. Also, I was wondering if there's any way to, when
> > entering the box model, switch into focused state and not edit mode? Sort of
> > similar to how the inspector works right now? More review bits are coming up
> > :)
> 
> Duh, scratch that, now that I'm looking at active descendant..

But re: edit mode, I think on first enter it should still be just focused, not in edit mode.

Comment 44

[Yura Zenevich [:yzen] (please use "needinfo")]

I think it's also important to make sure that click acts the same as before after the stuff in comment 43 is addressed (e.g. edit mode right away).

Comment 45

[Yura Zenevich [:yzen] (please use "needinfo")]

Ah, another interesting caveat:
STR:

* When clicking on the box model value we enter edit mode for that field
* Press tab
Expected: move to next editable value in clockwise order
Result: jump out of the box model

I think we need to make the widget focusable in the same way it would be if we got into edit mode with keyboard.

Comment 46

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

https://reviewboard.mozilla.org/r/65742/#review96006

Overall looks good. Lets try to clean this up one more time and you should start thinking of a bunch of tests for this with difference scenarios of keyboard navigation.

::: devtools/client/inspector/components/box-model.js:247
(Diff revision 12)
> +
> +    this.boxModel = this.doc.getElementById("boxmodel-wrapper");
> +    this.boxModel.addEventListener("keydown", this.onKeyDown, true);
> +    this.boxModel.addEventListener("focus", this.onFocus, true);
> +    this.marginLayout = this.doc.getElementById("boxmodel-margins");
> +    this.marginLayout.addEventListener("click", this.onLevelClick, true);

I think similarly to how we handle keydown and focus, we should handle click for the whole box model container.

::: devtools/client/inspector/components/box-model.js:546
(Diff revision 12)
> +      let nextLayout = this.marginLayout;
> +      this.setAriaActive(nextLayout);
> +    }
> +  },
> +
> +  setAriaActive: function (nextLayout, level = null, target = null) {

null is an unnessasary default since undefined is sort of similar.

::: devtools/client/inspector/components/box-model.js:552
(Diff revision 12)
> +    this.boxModel.setAttribute("aria-activedescendant", nextLayout.id);
> +    if (target && target._editable) {
> +      target.blur();
> +    }
> +    // Remove Current level border outline
> +    if (level) {

I think insead of having the level argument. Lets have "clear all" logic here (removing layout-active-elm from everything) and setting a new one for nextLevel afterwards.

::: devtools/client/inspector/components/box-model.js:553
(Diff revision 12)
> +    if (target && target._editable) {
> +      target.blur();
> +    }
> +    // Remove Current level border outline
> +    if (level) {
> +      this.doc.getElementById(level).classList.remove("layout-active-elm");

I guess we don't have the style for this yet, feel free to ask Helen for feedback regarding it.

::: devtools/client/inspector/components/box-model.js:617
(Diff revision 12)
> +      this.setAriaActive(nextLayout, level, target);
> +    }
> +  },
> +
> +  makeUnfocasable: function (editLevel) {
> +    let dataLevel = this.doc.getElementById(editLevel).getAttribute("data-box");

calculation of editBoxes here and in the other function bellow should be taken out as a getter.

::: devtools/client/inspector/components/box-model.js:632
(Diff revision 12)
> +    editBoxes.forEach(editBox => editBox.setAttribute("tabindex", "0"));
> +    editBoxes[0].focus();
> +  },
> +
> +  wrapFocus: function (element, back) {
> +    let editingMode = element.tagName === "input";

We should simplify this logic as well:
it looks like in markup, the things are listed in order: top,right,bottom,left. If we stick them in a temporary array via [...querySelectorAll(...)] we can just wrap based on index (if back and index == 0 - go to last element, if forward and index == length - 1 go to first element). Something like that.

::: devtools/client/inspector/components/box-model.js:649
(Diff revision 12)
> +    if (editingMode) {
> +      editBox.click();
> +    }
> +  },
> +
> +  moveLevelFocus(editLevel, key) {

name is misleading since we do not actually move focus in this function. Also since it's only called in one place and does not do much, we should probably get rid of it and do this stuff in the handler.

Comment 47

[Patrick Brosset <:pbro>]

I've had people report to me that this was also a Firebug gap. So marking it as such.

Comment 48

[Patrick Brosset <:pbro>]

It would be really awesome if we could land this patch. I see that a lot of work has been done already and there has been a few reviews too, so it looks like we're not too far.
Nancy: are you planning to continue working on this bug?
Thanks for your work so far!

Comment 49

[Yura Zenevich [:yzen] (please use "needinfo")]

Hi Patrick, Nancy and I chatter a couple of weeks ago and it is definitely something she is working towards (landing this) in the near future. I believe she has exams right now and will likely complete it in January. Cheers

Comment 50

[Patrick Brosset <:pbro>]

(In reply to Yura Zenevich [:yzen] from comment #49)
> Hi Patrick, Nancy and I chatter a couple of weeks ago and it is definitely
> something she is working towards (landing this) in the near future. I
> believe she has exams right now and will likely complete it in January.
> Cheers
This is great news. Thanks for the heads up Yura.

Comment 51

[Nancy Pang [:npang]]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65744/diff/12-13/

Comment 52

[Nancy Pang [:npang]]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65744/diff/13-14/

Comment 53

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

https://reviewboard.mozilla.org/r/65744/#review104584

::: devtools/client/inspector/components/box-model.js:593
(Diff revisions 12 - 13)
>          this.boxModel.focus();
>          break;
>        case KeyEvent.DOM_VK_TAB:
>          if (isEditable) {
>            let editName = target.parentElement.className;
> -          if (editName.includes("top") && shiftKey) {
> +          if(shiftKey && editName.includes("top")){

I think we are doing the same check twice: here and in the wrapFocus wunction as well. Perhaps when we get a tab we just call this.wrapFocusIfNeeded(event, level); and implement it as such:

wrapFocusIfNeeded({target, shiftKey, preventDefault}, level) {
  // calc position
  if (position === length - 1 && !shiftKey)
    // update position to 0
  else if (position === 0 && shiftKey)
    // update position to length - 1
    
  // focus on new and click if needed
}

::: devtools/client/inspector/components/box-model.js:630
(Diff revisions 12 - 13)
> -      `[data-box="${dataLevel}"].boxmodel-editable`)];
>      editBoxes.forEach(editBox => editBox.setAttribute("tabindex", "0"));
>      editBoxes[0].focus();
>    },
>  
> -  wrapFocus: function (element, back) {
> +  wrapFocus: function (element, level, dir) {

I think dir can still be just a boolean (like it was) since there are 2 possible values

::: devtools/client/inspector/components/box-model.js:633
(Diff revisions 12 - 13)
>    },
>  
> -  wrapFocus: function (element, back) {
> +  wrapFocus: function (element, level, dir) {
> +    let editBoxes = this.getEditBoxes(level);
>      let editingMode = element.tagName === "input";
> +    let position = editingMode ? editBoxes.indexOf(element.parentElement.children[1])

nit: indentation - generally:

```
a ? b :
    c
```

or 

```
a ?
  b :
  C
```

::: devtools/client/inspector/components/box-model.js:633
(Diff revisions 12 - 13)
>    },
>  
> -  wrapFocus: function (element, back) {
> +  wrapFocus: function (element, level, dir) {
> +    let editBoxes = this.getEditBoxes(level);
>      let editingMode = element.tagName === "input";
> +    let position = editingMode ? editBoxes.indexOf(element.parentElement.children[1])

Without knowing the details about the markup I think we can make this more readable but writing something like: element.parentElement.closest("input"). Also since we use "input" in more than one place, lets take it as a variable - something like: editElementTagName

::: devtools/client/inspector/components/box-model.js:652
(Diff revisions 12 - 13)
>      }
>    },
>  
> -  moveLevelFocus(editLevel, key) {
> -    let datalevel = this.doc.getElementById(editLevel).getAttribute("data-box");
> -    return this.layouts[datalevel].get(key);
> +  getEditBoxes: function(editLevel){
> +    let dataLevel = this.doc.getElementById(editLevel).getAttribute("data-box");
> +    let editBoxes = [...this.doc.querySelectorAll(

nice! nit - just return here.

Comment 54

[Nancy Pang [:npang]]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65744/diff/14-15/

Comment 55

[Nancy Pang [:npang]]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65744/diff/15-16/

Comment 56

[Nancy Pang [:npang]]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

https://reviewboard.mozilla.org/r/65744/#review104584

> Without knowing the details about the markup I think we can make this more readable but writing something like: element.parentElement.closest("input"). Also since we use "input" in more than one place, lets take it as a variable - something like: editElementTagName

Unfortunately the closest function queries ancestors and the input and span are in the same level. Switched to using nextSibling

Comment 57

[Gabriel Luong [:gl] (ΦωΦ)]

I think a devtools peer should also review this code before it lands. Feel free to flag me for a review when it is r+ from Yura, thanks :)

Comment 58

[Nancy Pang [:npang]]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65744/diff/16-17/

Comment 59

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

https://reviewboard.mozilla.org/r/65744/#review104752

This is great, just final comments and also note that devtools/client/inspector/components/test/browser_boxmodel_editablemodel.js test seems to time out. Once all issues are addressed, please mark gl for review :) Nice job!

::: devtools/client/inspector/components/box-model.js:592
(Diff revisions 14 - 15)
>          nextLayout = this.layouts[datalevel].get(keyCode);
>          this.boxModel.focus();
>          break;
>        case KeyEvent.DOM_VK_TAB:
> -        if (isEditable) {
> -          let editName = target.parentElement.className;
> +        if(isEditable){
> +          this.wrapFocus(event, event, level);

OK this is the final one, I promise :):
* lets rename wrapFocus into moveFocus
* 2 arguments event and level
* before calling wrapFocus lets prevent default (if isEditable is true). So we always prevent default and will handle move of the focus inside our function.
* For move focus comments see below

::: devtools/client/inspector/components/box-model.js:623
(Diff revisions 14 - 15)
>      let editBoxes = this.getEditBoxes(editLevel);
>      editBoxes.forEach(editBox => editBox.setAttribute("tabindex", "0"));
>      editBoxes[0].focus();
>    },
>  
> -  wrapFocus: function (element, level, dir) {
> +  wrapFocus: function ({target, shiftKey}, e, level) {

just 2 args ({target, shiftHey}, level)

::: devtools/client/inspector/components/box-model.js:626
(Diff revisions 14 - 15)
>    },
>  
> -  wrapFocus: function (element, level, dir) {
> +  wrapFocus: function ({target, shiftKey}, e, level) {
>      let editBoxes = this.getEditBoxes(level);
> -    let editingMode = element.tagName === "input";
> -    let position = editingMode ? editBoxes.indexOf(element.parentElement.children[1])
> +    let editingMode = target.tagName === "input";
> +    let position = editingMode ? editBoxes.indexOf(target.nextSibling)

yeah this is better, lets just add a comment here to explain that the next sibling is the input field.

::: devtools/client/inspector/components/box-model.js:630
(Diff revisions 14 - 15)
> -    let editingMode = element.tagName === "input";
> -    let position = editingMode ? editBoxes.indexOf(element.parentElement.children[1])
> -                    : editBoxes.indexOf(element);
> +    let editingMode = target.tagName === "input";
> +    let position = editingMode ? editBoxes.indexOf(target.nextSibling)
> +                    : editBoxes.indexOf(target);
>  
> -    if(position === editBoxes.length - 1 && dir === "forward") {
> +    if(position === editBoxes.length - 1 && !shiftKey) {
> +      e.preventDefault();

no need will call outside of function

::: devtools/client/inspector/components/box-model.js:633
(Diff revisions 14 - 15)
>  
> -    if(position === editBoxes.length - 1 && dir === "forward") {
> +    if(position === editBoxes.length - 1 && !shiftKey) {
> +      e.preventDefault();
>        position = 0;
> -    } else if (position === 0 && dir === "back") {
> +    } else if (position === 0 && shiftKey) {
> +      e.preventDefault();

same

::: devtools/client/inspector/components/box-model.js:636
(Diff revisions 14 - 15)
>        position = 0;
> -    } else if (position === 0 && dir === "back") {
> +    } else if (position === 0 && shiftKey) {
> +      e.preventDefault();
>        position = editBoxes.length - 1;
> +    } else {
> +      return;

instead of return we do:
position = position + (shiftKey ? -1 : 1);

so now we handle all focus movements inside of this function and everything is contained nicely.

::: devtools/client/inspector/components/test/browser_boxmodel_navigation.js:38
(Diff revision 17)
> +
> +  is(boxmodel.getAttribute("aria-activedescendant"), "boxmodel-margins", "Should be set to the margin layout.");
> +}
> +
> +function* testChangingLevels(inspector, view) {
> +  info("Test that using arrow keys updates level");

Lines 38 to 42 repeat in every test function, it makes sense to take it out into its own function.

Comment 60

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

https://reviewboard.mozilla.org/r/65744/#review104818

Runby review of nits.

::: devtools/client/inspector/components/box-model.js
(Diff revision 17)
>    this.expander = this.doc.getElementById("boxmodel-expander");
>    this.sizeLabel = this.doc.querySelector(".boxmodel-size > span");
>    this.sizeHeadingLabel = this.doc.getElementById("boxmodel-element-size");
>    this._geometryEditorHighlighter = null;
>    this._cssProperties = getCssProperties(inspector.toolbox);
> -

Don't remove the new line here.

::: devtools/client/inspector/components/box-model.js:242
(Diff revision 17)
> +    this.makeUnfocasable = this.makeUnfocasable.bind(this);
> +    this.wrapFocus = this.wrapFocus.bind(this);
> +    this.onFocus = this.onFocus.bind(this);
> +
> +    this.boxModel = this.doc.getElementById("boxmodel-wrapper");
> +    this.boxModel.addEventListener("keydown", this.onKeyDown, true);

Reorder these event listeners so that the type is in alphabetical order (click, focus, keydown).

::: devtools/client/inspector/components/box-model.js:244
(Diff revision 17)
> +    this.onFocus = this.onFocus.bind(this);
> +
> +    this.boxModel = this.doc.getElementById("boxmodel-wrapper");
> +    this.boxModel.addEventListener("keydown", this.onKeyDown, true);
> +    this.boxModel.addEventListener("focus", this.onFocus, true);
> +    this.boxModel.addEventListener("click", this.onLevelClick, true);

Add a new line after these event listener assignments.

::: devtools/client/inspector/components/box-model.js:247
(Diff revision 17)
> +    this.boxModel.addEventListener("keydown", this.onKeyDown, true);
> +    this.boxModel.addEventListener("focus", this.onFocus, true);
> +    this.boxModel.addEventListener("click", this.onLevelClick, true);
> +    this.marginLayout = this.doc.getElementById("boxmodel-margins");
> +    this.borderLayout = this.doc.getElementById("boxmodel-borders");
> +    this.paddingLayout = this.doc.getElementById("boxmodel-padding");

Add a new line after variable assignments.

::: devtools/client/inspector/components/box-model.js:622
(Diff revision 17)
> +      this.setAriaActive(nextLayout, target);
> +    }
> +  },
> +
> +  /**
> +   * Make previous layout's elements unfocusable

JSDocs are passing @param

::: devtools/client/inspector/components/test/browser_boxmodel_navigation.js:7
(Diff revision 17)
> +/* Any copyright is dedicated to the Public Domain.
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Test that keyboard and mouse navigation updates aria-active and focus

s/Test/Tests

::: devtools/client/inspector/components/test/browser_boxmodel_navigation.js:8
(Diff revision 17)
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Test that keyboard and mouse navigation updates aria-active and focus
> +// of elements

s/elements/elements.

::: devtools/client/inspector/components/test/browser_boxmodel_navigation.js:11
(Diff revision 17)
> +
> +// Test that keyboard and mouse navigation updates aria-active and focus
> +// of elements
> +
> +add_task(function* () {
> +  let style = "div { position: absolute; top: 42px; left: 42px; " +

We should make use of multiline strings. See tests in /inspector/rules/test/

::: devtools/client/inspector/components/test/browser_boxmodel_navigation.js:17
(Diff revision 17)
> +              "height: 100.111px; width: 100px; border: 10px solid black; " +
> +              "padding: 20px; margin: 30px auto;}";
> +  let html = "<style>" + style + "</style><div></div>";
> +
> +  yield addTab("data:text/html," + encodeURIComponent(html));
> +  let {inspector, view, testActor} = yield openBoxModelView();

Remove testActor since it is never used.

::: devtools/client/inspector/components/test/browser_boxmodel_navigation.js:29
(Diff revision 17)
> +});
> +
> +function* testInitialFocus(inspector, view) {
> +  info("Test that the focus is on margin layout");
> +  let viewdoc = view.doc;
> +

Remove this new line.

::: devtools/client/inspector/components/test/browser_boxmodel_navigation.js:30
(Diff revision 17)
> +
> +function* testInitialFocus(inspector, view) {
> +  info("Test that the focus is on margin layout");
> +  let viewdoc = view.doc;
> +
> +  let boxmodel = viewdoc.getElementById("boxmodel-wrapper");

Add a new line after the variable assignments.

Comment 61

[Nancy Pang [:npang]]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

https://reviewboard.mozilla.org/r/65744/#review104818

> s/Test/Tests

What does this mean?

> s/elements/elements.

What does this mean?

Comment 62

[Nancy Pang [:npang]]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65744/diff/17-18/

Comment 63

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

https://reviewboard.mozilla.org/r/65744/#review104980

::: devtools/client/inspector/components/box-model.js:15
(Diff revision 18)
>  const {InplaceEditor, editableItem} =
>        require("devtools/client/shared/inplace-editor");
>  const {ReflowFront} = require("devtools/shared/fronts/reflow");
>  const {LocalizationHelper} = require("devtools/shared/l10n");
>  const {getCssProperties} = require("devtools/shared/fronts/css-properties");
> +const {KeyEvent} = require("gcli/util/util");

I don't think we should be using the KeyEvent from the gcli/util here. 

Instead use:
const {KeyCodes} = require("devtools/client/shared/keycodes");

::: devtools/client/inspector/components/box-model.js:246
(Diff revision 18)
> +
> +    this.borderLayout = this.doc.getElementById("boxmodel-borders");
> +    this.boxModel = this.doc.getElementById("boxmodel-wrapper");
> +    this.marginLayout = this.doc.getElementById("boxmodel-margins");
> +    this.paddingLayout = this.doc.getElementById("boxmodel-padding");
> +    this.layouts = {

Add a new line above this.layouts

::: devtools/client/inspector/components/box-model.js:549
(Diff revision 18)
> +      this.setAriaActive(nextLayout);
> +    }
> +  },
> +
> +  /**
> +   * Active aria-level set to current layout

End JSDoc comments with a period.

::: devtools/client/inspector/components/box-model.js:550
(Diff revision 18)
> +    }
> +  },
> +
> +  /**
> +   * Active aria-level set to current layout
> +   * @param {Element} nextLayout

The JSDoc styles are a bit inconsistent in this file, but we should be adding a new line to separate the JSDoc and the @param.

::: devtools/client/inspector/components/box-model.js:584
(Diff revision 18)
> +
> +    this.setAriaActive(nextLayout, target);
> +  },
> +
> +  /**
> +   * Handle keyboard navigation and focus for box model layouts

This function could use some more in-depth description of the expected behavior for various key down events.

::: devtools/client/inspector/components/test/browser_boxmodel_navigation.js:10
(Diff revision 18)
> +"use strict";
> +
> +// Tests that keyboard and mouse navigation updates aria-active and focus
> +// of elements
> +
> +const TEST_URI = "<style>" +

Use multiline strings as seen in 
/inspector/rules/test/browser_rules_edit-property_07.js

::: devtools/client/inspector/components/test/browser_boxmodel_navigation.js:91
(Diff revision 18)
> +  let borderLayout = viewdoc.getElementById("boxmodel-borders");
> +  let paddingLayout = viewdoc.getElementById("boxmodel-padding");
> +  let layouts = [paddingLayout, borderLayout, marginLayout];
> +
> +  layouts.forEach(layout => {
> +                              layout.click();

We should only indent once in this function block.

Comment 64

[Nancy Pang [:npang]]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65744/diff/18-19/

Comment 65

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

https://reviewboard.mozilla.org/r/65744/#review105172

::: devtools/client/inspector/components/box-model.js:496
(Diff revision 19)
>      header.removeEventListener("dblclick", this.onToggleExpander);
>  
>      let nodeGeometry = this.doc.getElementById("layout-geometry-editor");
>      nodeGeometry.removeEventListener("click", this.onGeometryButtonClick);
>  
> +    this.boxModel.removeEventListener("focus", this.onFocus, true);

Reorder these event types to be alphabetical order.

::: devtools/client/inspector/components/test/browser_boxmodel_navigation.js:8
(Diff revision 19)
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Tests that keyboard and mouse navigation updates aria-active and focus
> +// of elements

Add a period to the end of the sentence.

::: devtools/client/inspector/components/test/browser_boxmodel_navigation.js:11
(Diff revision 19)
> +
> +// Tests that keyboard and mouse navigation updates aria-active and focus
> +// of elements
> +
> +const TEST_URI = `
> +  "<style>"

You don't need quotations in multiline strings, and you should format this properly. See devtools/client/inspector/components/test/browser_boxmodel_editablemodel_bluronclick.js

::: devtools/client/inspector/components/test/browser_boxmodel_navigation.js:61
(Diff revision 19)
> +  EventUtils.synthesizeKey("VK_UP", {});
> +  is(boxmodel.getAttribute("aria-activedescendant"), "boxmodel-margins", "Should be set to the margin layout.");
> +}
> +
> +function* testTabbingWrapAround(inspector, view) {
> +  info("Test that using arrow keys updates level");

Add a period to end the sentence for info.

::: devtools/client/inspector/components/test/browser_boxmodel_navigation.js:75
(Diff revision 19)
> +    `[data-box="${dataLevel}"].boxmodel-editable`)];
> +
> +  EventUtils.synthesizeKey("VK_ESCAPE", {});
> +  editBoxes[3].focus();
> +  EventUtils.synthesizeKey("VK_TAB", {});
> +  is(editBoxes[0], viewdoc.activeElement, "Top edit box should have focus");

We should be consistent with your periods in these is()/ok() checks. Add a period to all of them since you have done so before.

Comment 66

[Nancy Pang [:npang]]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65744/diff/19-20/

Comment 67

[Nancy Pang [:npang]]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65744/diff/20-21/

Comment 68

[Ryan VanderMeulen [:RyanVM]]

Autoland can't push this due to unresolved issues in MozReview.

Comment 69

[Nancy Pang [:npang]]

Been resolved

Comment 70

[Nancy Pang [:npang]]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65744/diff/21-22/

Comment 71

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c622a6c181bf
Added navigation for padding, border and margin. r=gl,yzen

Comment 72

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

I had to back this out for eslint failures like https://treeherder.mozilla.org/logviewer.html#?job_id=69758488&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/077be424eb16

Comment 73

[Nancy Pang [:npang]]

(In reply to Wes Kocher (:KWierso) from comment #72)
> I had to back this out for eslint failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=69758488&repo=autoland
> 
> https://hg.mozilla.org/integration/autoland/rev/077be424eb16

Will fix the lint errors

Comment 74

[Nancy Pang [:npang]]

Comment on attachment 8773105 [details]
Bug 1243045 - Added navigation for padding, border and margin.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65744/diff/22-23/

Comment 75

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/92b0af77c610
Added navigation for padding, border and margin. r=gl,yzen

Comment 76

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

https://hg.mozilla.org/mozilla-central/rev/92b0af77c610