[a11y] Box model inspector is not accessible

RESOLVED FIXED in Firefox 53

Status

defect
P2
normal
RESOLVED FIXED
3 years ago
10 months ago

People

(Reporter: yzen, Assigned: npang)

Tracking

(Blocks 2 bugs, {access})

unspecified
Firefox 53
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

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

Comment 2

3 years ago
Actually not tab order, arrow order
(Reporter)

Comment 3

3 years ago
Activation on any level will activate the top input for the property. Sequential tabbing switches to right-bottom-left property inputs.
Filter on CLIMBING SHOES
Priority: -- → P3
(Assignee)

Updated

3 years ago
Assignee: nobody → npang
(Reporter)

Comment 5

3 years ago
\o/
(Reporter)

Comment 6

3 years ago
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).
Flags: needinfo?(mzehe)
(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.
Flags: needinfo?(mzehe)
(Reporter)

Updated

3 years ago
Depends on: 1247729
(Reporter)

Comment 9

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

as per in person conversation, removing an r? for now.
Attachment #8773105 - Flags: review?(yzenevich)
(Assignee)

Comment 10

3 years ago
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/
Attachment #8773105 - Flags: review?(yzenevich)
(Assignee)

Comment 11

3 years ago
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/
(Reporter)

Comment 12

3 years ago
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.
(Reporter)

Updated

3 years ago
Attachment #8773105 - Flags: review?(yzenevich)
(Assignee)

Comment 13

3 years ago
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/
Attachment #8773105 - Flags: review?(yzenevich)
(Reporter)

Comment 14

3 years ago
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
Attachment #8773105 - Flags: review?(yzenevich)
(Assignee)

Comment 15

3 years ago
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/
Attachment #8773105 - Flags: review?(yzenevich)
(Assignee)

Comment 16

3 years ago
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/
(Reporter)

Comment 17

3 years ago
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]
(Assignee)

Comment 18

3 years ago
https://reviewboard.mozilla.org/r/65744/#review64134

> Just _editable will not suffice?

need the input check for tab sequence for users editing the fields
(Assignee)

Comment 19

3 years ago
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
(Assignee)

Comment 20

3 years ago
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/
(Assignee)

Comment 21

3 years ago
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/
(Reporter)

Comment 22

3 years ago
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.
(Reporter)

Comment 23

3 years ago
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
(Assignee)

Comment 24

3 years ago
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/
(Assignee)

Comment 25

3 years ago
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/
(Reporter)

Comment 26

3 years ago
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.
(Reporter)

Comment 27

3 years ago
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.
(Reporter)

Comment 28

3 years ago
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.
(Reporter)

Comment 29

3 years ago
mozreview-review
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.
(Reporter)

Comment 30

3 years ago
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
Attachment #8773105 - Flags: review?(yzenevich)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 33

3 years ago
Hi Nancy, I noticed you pushed to review again, are those ready for re-review? Thanks
Flags: needinfo?(npang)
(Assignee)

Comment 34

3 years ago
Hey Yura, they should be ready to review. I rebased my branch and made a couple of changes.
Flags: needinfo?(npang)
(Reporter)

Comment 35

2 years ago
mozreview-review
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
Attachment #8773105 - Flags: review?(yzenevich)
(Reporter)

Comment 36

2 years ago
One more question, Nancy: are those 2 review requests complimentary? Should we merge them into 1?
(Reporter)

Comment 37

2 years ago
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.
Attachment #8810152 - Flags: review?(yzenevich)
(Assignee)

Comment 38

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8810152 - Attachment is obsolete: true
(Reporter)

Comment 40

2 years ago
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 :)
(Reporter)

Comment 41

2 years ago
Also, dimension box should probably be supporting aria-activedescendant too.
(Reporter)

Comment 42

2 years ago
(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..
(Reporter)

Comment 43

2 years ago
(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.
(Reporter)

Comment 44

2 years ago
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).
(Reporter)

Comment 45

2 years ago
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.
(Reporter)

Comment 46

2 years ago
mozreview-review
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.
I've had people report to me that this was also a Firebug gap. So marking it as such.
Blocks: firebug-gaps
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!
Priority: P3 → P2
(Reporter)

Comment 49

2 years ago
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
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 53

2 years ago
mozreview-review
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.
Attachment #8773105 - Flags: review?(yzenevich)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 56

2 years ago
mozreview-review-reply
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
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 :)
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Reporter)

Comment 59

2 years ago
mozreview-review
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.
Attachment #8773105 - Flags: review?(yzenevich) → review+

Comment 60

2 years ago
mozreview-review
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.
(Assignee)

Comment 61

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 63

2 years ago
mozreview-review
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.
Attachment #8773105 - Flags: review?(gl)
Comment hidden (mozreview-request)

Comment 65

2 years ago
mozreview-review
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.
Attachment #8773105 - Flags: review?(gl) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
Autoland can't push this due to unresolved issues in MozReview.
Keywords: checkin-needed
(Assignee)

Comment 69

2 years ago
Been resolved
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
Comment hidden (mozreview-request)

Comment 71

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c622a6c181bf
Added navigation for padding, border and margin. r=gl,yzen
Keywords: checkin-needed
(Assignee)

Comment 73

2 years ago
(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
Flags: needinfo?(npang)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 75

2 years ago
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/92b0af77c610
Added navigation for padding, border and margin. r=gl,yzen
Keywords: checkin-needed

Comment 76

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/92b0af77c610
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
No longer blocks: top-inspector-bugs
See Also: → 1343167

Updated

10 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.