STRs: - go to any page - open the inspector - select the computed view - in the boxmodel widget, click on any value to switch it to edit mode (top margin for instance) - press TAB ER: Focus should go to the next value of the boxmodel widget, and activate edit mode on it AR: Focus goes to the computed properties section Regression from Bug 1336198.

A first note: we closed Bug 1351589 as a duplicate of this one but the current changeset is not addressing the specific issue mentioned in this bug.

Comment on attachment 8853851 [details] Bug 1343167 - Add tab navigation in the box model view. https://reviewboard.mozilla.org/r/125880/#review128646 Thanks for restoring the functionality! Only one thing I'd like to check with you before r+ is the aria-descendant/lack of id. Other than this, just a few nits. ::: devtools/client/inspector/boxmodel/components/BoxModelEditable.js:64 (Diff revision 1) > className: "boxmodel-editable", > "data-box": box, > + tabIndex, > title: property, > - ref: "span", > + ref: span => { > + this.span = span; Let's use a member name more descriptive than span? boxmodelEditable ? ::: devtools/client/inspector/boxmodel/components/BoxModelMain.js:190 (Diff revision 1) > > return value; > }, > > /** > + * Keyboard navigation of edit boxes wraps around on edge Seems to describe L209-213, but not the rest of the method. "Move the focus to the next/previous editable element of the current layout" ? ::: devtools/client/inspector/boxmodel/components/BoxModelMain.js:231 (Diff revision 1) > + * Active aria-level set to current layout. > + * > + * @param {Element} nextLayout > + * Element of next layout that user has navigated to > + * @param {Element} target > + * Node to be observed That doesn't seem accurate. The target is simply blurred if it's editable. This last bit of the method also really feels unrelated to setting the aria active descendant, I'd prefer if it was moved out of here. ::: devtools/client/inspector/boxmodel/components/BoxModelMain.js:235 (Diff revision 1) > + * @param {Element} target > + * Node to be observed > + */ > + setAriaActive(nextLayout, target) { > + let { boxModelContainer } = this.props; > + // We set this attribute for testing purposes. This is also used in onKeyDown, so not only for testing purposes. ::: devtools/client/inspector/boxmodel/components/BoxModelMain.js:236 (Diff revision 1) > + * Node to be observed > + */ > + setAriaActive(nextLayout, target) { > + let { boxModelContainer } = this.props; > + // We set this attribute for testing purposes. > + boxModelContainer.setAttribute("aria-activedescendant", nextLayout.className); aria-activedescendant is supposed to be used with element ids. I understand here we are using it in order to know which "layout" should receive the focus next, but it might confuse screen readers if we don't have the expected ids in the UI. Since we can have the same widget displayed twice in the inspector, using ids would make this complicated. So maybe use simply "activedescendant" instead of aria-activedescendant, since we are not aria compliant here? ::: devtools/client/inspector/boxmodel/components/BoxModelMain.js:355 (Diff revision 1) > + event.preventDefault(); > + this.moveFocus(event, level); > + } > + break; > + case KeyCodes.DOM_VK_ESCAPE: > + if (isEditable && target._editable) { this is the same as if (target._editable) ::: devtools/client/inspector/boxmodel/components/BoxModelMain.js:366 (Diff revision 1) > + break; > + default: > + break; > + } > + > + if (nextLayout) { this could be moved to the UP/DOWN case ::: devtools/client/inspector/boxmodel/components/BoxModelMain.js:380 (Diff revision 1) > + * The event triggered by a mouse click on the box model > + */ > + onLevelClick(event) { > + let { target } = event; > + let { layout } = this.props.boxModel; > + let displayPosition = layout.position && layout.position != "static"; The logic to get displayPosition and isContentBox is duplicated here, in getAriaActiveDescendant and in componentDidUpdate. Maybe better to have shared helpers? ::: devtools/client/inspector/boxmodel/components/BoxModelMain.js:429 (Diff revision 1) > let marginLeft = this.getMarginValue("margin-left", "left"); > > height = this.getHeightValue(height); > width = this.getWidthValue(width); > > + let focusable = this.state.focusable; nit: use let { focusable } = this.state; ::: devtools/client/inspector/boxmodel/components/BoxModelMain.js:443 (Diff revision 1) > box: "content", > property: "width", > + ref: editable => { > + this.contentEditable = editable; > + }, > + tabIndex: level === "content" && focusable ? 0 : -1, Not an issue but I can't help wondering if we should not pass level & focusable down to BoxModelEditable. There we could do `tabIndex: this.props.box === this.props.level && this.props.focusable ? 0 : -1` instead of repeating this ternary every time we use a BoxModelEditable

Comment on attachment 8853851 [details] Bug 1343167 - Add tab navigation in the box model view. https://reviewboard.mozilla.org/r/125880/#review129022

Comment on attachment 8853851 [details] Bug 1343167 - Add tab navigation in the box model view. https://reviewboard.mozilla.org/r/125880/#review128646 Ya, we do have a lack of id as you have commented. I have changed the attribute to be activedescendant from aria-descendant. Maybe this can be fixed assuming we remove the box model from the computed view. > This is also used in onKeyDown, so not only for testing purposes. This is actually different from the component state that is used in onKeyDown.

Comment on attachment 8853851 [details] Bug 1343167 - Add tab navigation in the box model view. https://reviewboard.mozilla.org/r/125880/#review129100 LGTM if try is green! ::: devtools/client/inspector/boxmodel/components/BoxModelMain.js:46 (Diff revision 2) > > mixins: [ addons.PureRenderMixin ], > > + getInitialState() { > + return { > + "aria-activedescendant": null, consistency: aria-activedescendant -> activedescendant ::: devtools/client/inspector/boxmodel/components/BoxModelMain.js:96 (Diff revision 2) > + ]) > + }; > + }, > + > + getAriaActiveDescendant() { > + let activeDescendant = this.state["aria-activedescendant"]; consistency: aria-activedescendant -> activedescendant ::: devtools/client/inspector/boxmodel/components/BoxModelMain.js:445 (Diff revision 2) > > height = this.getHeightValue(height); > width = this.getWidthValue(width); > > + let { focusable } = this.state; > + let level = this.state["aria-activedescendant"]; let's use activedescendant instead of aria-activedescendant for consistency.

Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/98d4fa0f8bf8 Add navigation for the box model's position, padding, border, margin and content layout. r=jdescottes

Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6aa495cd9908 Fix eslint errors in BoxModelMain.js. r=me

https://hg.mozilla.org/mozilla-central/rev/98d4fa0f8bf8 https://hg.mozilla.org/mozilla-central/rev/6aa495cd9908

Backed out in https://hg.mozilla.org/mozilla-central/rev/10ea10d9993c The percentages vary per-platform and e10s-or-not, but my favorite, Win8 debug, that gave about a 50% chance of hitting https://treeherder.mozilla.org/logviewer.html#?job_id=89316245&repo=mozilla-inbound

Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/94720e6beec3 Add navigation for the box model's position, padding, border, margin and content layout. r=jdescottes

Backed out again in https://hg.mozilla.org/integration/mozilla-inbound/rev/25bf4a4f071c for still failing too often the same way.

Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/57d5c6bdba4c Add navigation for the box model's position, padding, border, margin and content layout. r=jdescottes

I have reproduced this bug following the str from comment #0 with Firefox Nightly 54.0a1 (2017-02-28) in 64bit Ubuntu 16.04 This bug is now verified as fixed in Latest Firefox Nightly 55.0a1 Build ID 20170414100243 User Agent Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0 [bugday-20170412]

While tabbing through the same kind of values works again now, I am wondering how you can get to the inner values via the keyboard? Firebug was smarter in that case, because it switched to the inner values when you tabbed through all outer values, e.g. when you tabbed through all margin values the inline editor opened for the border-top value. Sebastian

Suppose you were tabbing through margins, you would press ESC to refocus the container and then you would press UP/DOWN to cycle through the possible box model properties followed by ENTER to select the properties. This is what a11y has determined to be most effective and that is what was readded here. I would ping yzen if you have further questions about how this should work.

Should we consider uplifting this to 54 (soon to be on Beta) or can it ride the trains?

(In reply to Ryan VanderMeulen [:RyanVM] from comment #21) > Should we consider uplifting this to 54 (soon to be on Beta) or can it ride > the trains? We can probably let this ride the trains.

I have reproduced this bug with Nightly 54.0a1 (2017-02-28) on Windows 8.1 , 64 Bit ! This bug's fix is Verified with latest Nightly ! Build ID 20170419030223 User Agent Mozilla/5.0 (Windows NT 6.3; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0 [bugday-20170419]

(In reply to Gabriel [:gl] (ΦωΦ) from comment #20) > Suppose you were tabbing through margins, you would press ESC to refocus the > container and then you would press UP/DOWN to cycle through the possible box > model properties followed by ENTER to select the properties. This is what > a11y has determined to be most effective and that is what was readded here. > I would ping yzen if you have further questions about how this should work. Sorry for the delay in response! I believe this is a rather poor UX, as there is no indication about this behavior (and to me it feels rather unexpected), especially because there is no focus ring around the field you are going to edit. yzen seems to be away until the All Hands, so I'm wondering whether a needinfo would be answered in time. And this change can still be made afterwards, so I've created the follow-up bug . Sebastian