1343167 - Can not tab through inputs of the new boxmodel view

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Julian Descottes [:jdescottes]]

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.

Comment 2

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

Attachment: Bug 1343167 - Add tab navigation in the box model view.

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

Comment 3

[Julian Descottes [:jdescottes]]

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 4

[Julian Descottes [:jdescottes]]

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 5

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

Comment on attachment 8853851 [details]
Bug 1343167 - Add tab navigation in the box model view.

https://reviewboard.mozilla.org/r/125880/#review129022

Comment 6

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

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 7

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

Comment on attachment 8853851 [details]
Bug 1343167 - Add tab navigation in the box model view.

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

Comment 8

[Julian Descottes [:jdescottes]]

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.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6aa495cd9908
Fix eslint errors in BoxModelMain.js. r=me

Comment 11

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

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

Comment 12

[Phil Ringnalda (:philor)]

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

Comment 13

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

Attachment: 1343167.patch

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

Comment 15

[Phil Ringnalda (:philor)]

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

Comment 17

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/57d5c6bdba4c

Comment 18

[Sayed [:LiquidO2]]

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]

Comment 19

[Sebastian Zartner [:sebo]]

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

Comment 20

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

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.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

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

(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.

Comment 23

[Sajedul Islam]

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]

Comment 24

[Sebastian Zartner [:sebo]]

(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