Closed Bug 1280360 Opened 8 years ago Closed 8 years ago

[a11y] After editing attribute, all attributes drop out of tab order

Categories

(DevTools :: Inspector, defect, P2)

Product:
DevTools
Component:
Inspector
Type:
defect

Tracking

(firefox50 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 50
Tracking Flags:
Tracking Status
firefox50 --- verified

People

(Reporter: Jamie, Assigned: jdescottes)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(1 file)

Bug 1280360 - markupview: a11y fix tabindex when modifying attributes;
58 bytes, text/x-review-board-request
pbro
: review+
Details 
STR:
1. Open this URL: data:text/html,<div data-a="a" data-b="b">content</div>
2. Open the Inspector.
3. Select the div in the tree view.
4. Press enter to edit it.
5. Tab to data-a="a".
6. Press enter to edit it.
7. Change something; e.g. change the final "a" to a "z".
8. Press enter to commit.
9. Tab through all of the controls.
Expected: The two attributes should be in the tab order.
Actual: Attributes are no longer in the tab order.
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P2
Having a look.
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Regression from Bug 1242694. The default template for attributes used to have tabindex="0". We now have tabindex="-1" and tabindex is set to "0" when the element is focused in the markup view.

When attributes are updated in the markup view, the markup for all the attributes is removed and replaced with new markup. We should check if the current container can be focused, and in this case, setAttribute("tabindex", "0")
Blocks: 1242694
Comment on attachment 8767337 [details]
Bug 1280360 - markupview: a11y fix tabindex when modifying attributes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61870/diff/1-2/
Attachment #8767337 - Attachment description: Bug 1280360 - markupview: a11y fix tabindex when modifying attributes → Bug 1280360 - markupview: a11y fix tabindex when modifying attributes;
Attachment #8767337 - Flags: review?(gl)
Attachment #8767337 - Flags: review?(gl) → review?(pbrosset)
No activity for a week, forwarding the review.
Comment on attachment 8767337 [details]
Bug 1280360 - markupview: a11y fix tabindex when modifying attributes;

https://reviewboard.mozilla.org/r/61870/#review61894

Only a few test cleanups. The actual code changes look good.

::: devtools/client/inspector/markup/test/browser_markup_accessibility_navigation_after_edit.js:7
(Diff revision 2)
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +/* global getContainerForSelector, openInspectorForURL */

Why did you need these 2 eslint globals?
Our custom rules should auto-import globals from head.js files and even go in dependencies from there.

::: devtools/client/inspector/markup/test/browser_markup_accessibility_navigation_after_edit.js:102
(Diff revision 2)
> +    key: "VK_TAB",
> +    options: { }
> +  },
> +];
> +
> +let elms = {};

You might need to nullify `elms` at the end of the main function in `add_task`.

::: devtools/client/inspector/markup/test/browser_markup_accessibility_navigation_after_edit.js:127
(Diff revision 2)
> +      updated = waitFor === "inspector-updated" ?
> +        inspector.once(waitFor) : markup.once(waitFor);

Looks like `waitFor` is only used for `inspector-updated` right now, so maybe simplify this whole `if/else` to:

```
let updated = waitFor ? inspector.once(waitFor) : Promise.resolve();
```

If in the future we need more exotic ways to wait, then we could make `waitFor` a generator function that takes `inspector` as an argument instead, so that each item in `TESTS` can implement its own `waitFor`. 
But for now, the event name is more than fine, since we have only this one case.

::: devtools/client/inspector/markup/test/browser_markup_accessibility_navigation_after_edit.js:140
(Diff revision 2)
> +function getElm(path) {
> +  let segments = path.split(".");
> +  return segments.reduce((prev, current) => prev[current], elms);
> +}

Oh nice. We do this kind of ugly lookup work in many tests in the inspector/markup-view, getting a container from a selector, then getting its editor, then input, etc...
I suggest moving this function to head.js so we can use it later.
Maybe renamed to `getMarkupViewElement` or something.
Attachment #8767337 - Flags: review?(pbrosset) → review+
Comment on attachment 8767337 [details]
Bug 1280360 - markupview: a11y fix tabindex when modifying attributes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61870/diff/2-3/
Comment on attachment 8767337 [details]
Bug 1280360 - markupview: a11y fix tabindex when modifying attributes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61870/diff/3-4/
Comment on attachment 8767337 [details]
Bug 1280360 - markupview: a11y fix tabindex when modifying attributes;

Thanks for the review! Flagging again because I made substantial changes.

Most of the test I added was actually based on a similar test devtools/client/inspector/markup/test/browser_markup_accessibility_navigation.js

I went ahead and extracted the common methods to a helper that is loaded by both tests. 

The lookup method is generic enough so I extracted it to shared-head and renamed it at the same time to lookupPath (feel free to suggest a better name though).
Attachment #8767337 - Flags: review+ → review?(pbrosset)
Comment on attachment 8767337 [details]
Bug 1280360 - markupview: a11y fix tabindex when modifying attributes;

https://reviewboard.mozilla.org/r/61870/#review62220

::: devtools/client/framework/test/shared-head.js:480
(Diff revision 4)
>      SpecialPowers.pushPrefEnv(options, resolve);
>    });
>  }
> +
> +/**
> + * Lookup the provided dotted path (".prop1.subprop2.myProp") in the provided object.

Shouldn't the path not start with an initial dot?
I think the example should be `("prop1.subprop2.myProp")` instead.

::: devtools/client/framework/test/shared-head.js:488
(Diff revision 4)
> + *        Dotted path to use to expand the object.
> + * @param {Object} obj
> + *        Object to expand.
> + * @return {?} anything that is found at the provided path in the object.
> + */
> +function lookupPath(path, obj) {

Nit: it makes more sense to me the other way around:
`lookupPath(obj, path)`

::: devtools/client/inspector/markup/test/browser.ini:58
(Diff revision 4)
> -skip-if = os == "mac" # Full keyboard navigation on OSX only works if Full Keyboard Access setting is set to All Control in System Keyboard Preferences
> +# skip-if = os == "mac" # Full keyboard navigation on OSX only works if Full Keyboard Access setting is set to All Control in System Keyboard Preferences
> +[browser_markup_accessibility_navigation_after_edit.js]
> +# skip-if = os == "mac" # Full keyboard navigation on OSX only works if Full Keyboard Access setting is set to All Control in System Keyboard Preferences

Wait, did I miss this on the first review round? Why are you commenting these skip-if?
The comment suggests that the tests won't work on mac and so should be disabled.

::: devtools/client/inspector/markup/test/helper_markup_accessibility_navigation.js:46
(Diff revision 4)
> + *        - {String} waitFor, optional: event to wait for if keyboard actions result in
> + *          asynchronous updates

This should say that we expect the event to come from the markup-view only, except if the event name is inspector-updated.
Attachment #8767337 - Flags: review?(pbrosset) → review+
Comment on attachment 8767337 [details]
Bug 1280360 - markupview: a11y fix tabindex when modifying attributes;

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

Thanks for the review. Try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=d85196cbd827

> Wait, did I miss this on the first review round? Why are you commenting these skip-if?
> The comment suggests that the tests won't work on mac and so should be disabled.

No you didn't miss it, I just committed it by mistake here. I needed to enable the tests to run them locally.
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/c0ab769be276
markupview: a11y fix tabindex when modifying attributes;r=pbro
https://hg.mozilla.org/mozilla-central/rev/c0ab769be276
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
 I have reproduced this bug with Nightly 50.0a1 (2016-06-16) on Windows 7, 64 Bit!

 This bug's fix is verified on latest Aurora


 Build ID      20160803004014

 User Agent    Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0

 [bugday-20160803]
Reproduced this bug in firefox nightly 50.0a1 (2016-06-16) as comment 0 with ubuntu 16.04 (64 bit)

Verified this bug as fixed with latest firefox aurora 50.0a2 (Build ID: 20160803004014)
Mozilla/5.0 (X11; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0
QA Whiteboard: [bugday-20160803]
Status: RESOLVED → VERIFIED
status-firefox50: fixed → verified
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: