Closed Bug 1460613 Opened 6 years ago Closed 6 years ago

The keyboard-only operation is not implemented for Shadow DOM DevTools

Categories

(DevTools :: Inspector, defect, P3)

Product:
DevTools
Component:
Inspector
Type:
defect

Tracking

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox60 unaffected, firefox61 disabled, firefox62 wontfix, firefox63 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox60 --- unaffected
firefox61 --- disabled
firefox62 --- wontfix
firefox63 --- verified

People

(Reporter: Ovidiu, Assigned: ladybenko)

References

Details

Attachments

(1 file)

Bug 1460613 - Add keyboard navigation for reveal link in slotted nodes.
59 bytes, text/x-review-board-request
jdescottes
: review+
yzen
: review+
Details 
[Affected versions]:

Tested on Nightly 62.0a1(2018-05-10)

[Affected platforms]:

Tested on Mac OS X 10.13, Ubuntu 16.04 and Windows 10 

[Steps to reproduce]:

Prerequisites:
 Go to about:config and enable this 2 preferences:

dom.webcomponents.shadowdom.enabled
dom.webcomponents.customelements.enabled (should already be true on Nightly)

 
1. Open Firefox and go to
https://juliandescottes.github.io/webcomponents-playground/next-sibling/
2. Open the Inspector
3. Right click on <body> and select "Expand All"
4. Use the down and up key to focus the <div> form the <slot id="slot1" name="slot1">
6. The "reveal" link is displayed.
7. Use the "Tab" key to focus the reveal link.
8. Press "Enter"



[Expected result]:

The selection is moved to the corresponding node.

[Actual result]:

The keyboard navigation is not implemented for Shadow DOM DevTools.
Blocks: devtools-webcomponents-63
Severity: normal → enhancement
Priority: -- → P3
Product: Firefox → DevTools
Assignee: nobody → balbeza
Status: NEW → ASSIGNED
Depends on: 1449954
Comment on attachment 8992957 [details]
Bug 1460613 - Add keyboard navigation for reveal link in slotted nodes.

https://reviewboard.mozilla.org/r/257764/#review264918

Thanks for the patch Belén, seems to work well! Some suggestions for the test and the implementation. 
Would like to see a new version with Telemetry implemented.

::: devtools/client/inspector/markup/test/browser_markup_shadowdom_clickreveal.js:59
(Diff revision 1)
>    await checkRevealLink(inspector, slotChildContainers[1].node);
>    is(inspector.selection.nodeFront.id, "el2", "The right node was selected");
>    is(hostContainer.getChildContainers()[2].node, inspector.selection.nodeFront);
>  });
>  
> +// Test reveal link with keyboard navigation (Enter key)

Could we try to mutualize the code here? The two add_task look identical except for calling  checkRevealLink or checkRevealLinkWithKeyboard.

And same comment for the two helper methods, they look similar except for using clickOnRevealLink or keydownEnterOnRevealLink.

::: devtools/client/inspector/markup/test/head.js:743
(Diff revision 1)
> + */
> +async function keydownEnterOnRevealLink(inspector, container) {
> +  const revealLink = container.elt.querySelector(".reveal-link");
> +  const win = inspector.markup.doc.defaultView;
> +
> +  await new Promise(r => setTimeout(r, 1000));

We should be able to remove this timeout (as a general rule, we try to avoid timeouts in mochitests)

::: devtools/client/inspector/markup/test/head.js:751
(Diff revision 1)
> +
> +  // we need to go through a ENTER + TAB  key sequence to focus on
> +  // the .reveal-link element with the keyboard
> +  const rootBlurred = once(root.elt, "blur");
> +  EventUtils.synthesizeKey("VK_RETURN", {}, win);
> +  await rootBlurred;

We don't need to wait for this event.

::: devtools/client/inspector/markup/test/head.js:755
(Diff revision 1)
> +  EventUtils.synthesizeKey("VK_RETURN", {}, win);
> +  await rootBlurred;
> +
> +  const revealFocused = once(revealLink, "focus");
> +  EventUtils.synthesizeKey("VK_TAB", {}, win);
> +  await revealFocused;

Same comment. 

However we might want to keep this one and log an info before we wait on the "focus". I am thinking that if a new feature introduces a new element that appears before the "reveal" element, it will be helpful to see in the logs something like "Waiting for .reveal-link to be focused"

::: devtools/client/inspector/markup/views/slotted-node-container.js:41
(Diff revision 1)
>  
> +  _onKeyDown: function(event) {
> +    if (event.target.classList.contains("reveal-link") && event.keyCode == 13) {
> +      const reason = "reveal-from-slot";
> +      this.markup.inspector.selection.setNodeFront(this.node, { reason });
> +      // TODO: telemetry?

Yes we should log the same probe as for the click. I don't think we need to distinguish clicks and keyboard events here. 

Maybe extract the relevant code in a method and call it from both the click and keydown handlers?

::: devtools/client/inspector/markup/views/slotted-node-editor.js:33
(Diff revision 1)
>      this.tag = doc.createElement("span");
>      this.tag.classList.add("tag");
>      this.elt.appendChild(this.tag);
>  
>      this.revealLink = doc.createElement("span");
> +    this.revealLink.tabIndex = -1;

It seems that DevTools use `setAttribute("tabindex", "-1");` to set the tabIndex, so we should keep doing that for consistency. 

I hope Yura can point which value we should use here. Normally -1 means we make the item focusable but exclude it from the sequential keyboard navigation. I assume the navigation is fully handled via a custom implementation so I'm not sure...
Attachment #8992957 - Flags: review?(jdescottes)
Comment on attachment 8992957 [details]
Bug 1460613 - Add keyboard navigation for reveal link in slotted nodes.

https://reviewboard.mozilla.org/r/257764/#review265152

Thanks, see comments inline.

::: devtools/client/inspector/markup/views/slotted-node-container.js:38
(Diff revision 1)
>    _onToggle: function(event) {
>      event.stopPropagation();
>    },
>  
> +  _onKeyDown: function(event) {
> +    if (event.target.classList.contains("reveal-link") && event.keyCode == 13) {

keyCode is deprecated, I'd suggest using key. Also Space should trigger this similarly to Enter.

::: devtools/client/inspector/markup/views/slotted-node-editor.js:33
(Diff revision 1)
>      this.tag = doc.createElement("span");
>      this.tag.classList.add("tag");
>      this.elt.appendChild(this.tag);
>  
>      this.revealLink = doc.createElement("span");
> +    this.revealLink.tabIndex = -1;

Reveal link must have semantics. Right now it's just a span and accessibility tools would not know that users can perform actions on it.

You can either use:
* anchor tag with specified href attribute (even if it's #)
* role="link" set on the element to indicate the link semantics.
Attachment #8992957 - Flags: review?(yzenevich)
Comment on attachment 8992957 [details]
Bug 1460613 - Add keyboard navigation for reveal link in slotted nodes.

https://reviewboard.mozilla.org/r/257764/#review265152

> Reveal link must have semantics. Right now it's just a span and accessibility tools would not know that users can perform actions on it.
> 
> You can either use:
> * anchor tag with specified href attribute (even if it's #)
> * role="link" set on the element to indicate the link semantics.

Thanks! I added `role=link`
Comment on attachment 8992957 [details]
Bug 1460613 - Add keyboard navigation for reveal link in slotted nodes.

https://reviewboard.mozilla.org/r/257764/#review264918

> Same comment. 
> 
> However we might want to keep this one and log an info before we wait on the "focus". I am thinking that if a new feature introduces a new element that appears before the "reveal" element, it will be helpful to see in the logs something like "Waiting for .reveal-link to be focused"

I kept it just in case, as you suggested, and added the logging message
Comment on attachment 8992957 [details]
Bug 1460613 - Add keyboard navigation for reveal link in slotted nodes.

https://reviewboard.mozilla.org/r/257764/#review265452

Works for me, thanks! A few nits and the suggestions for the test.

::: devtools/client/inspector/markup/test/browser_markup_shadowdom_clickreveal.js:59
(Diff revision 2)
>    is(inspector.selection.nodeFront.id, "el2", "The right node was selected");
>    is(hostContainer.getChildContainers()[2].node, inspector.selection.nodeFront);
> +}
> +
> +// Test reveal link with mouse navigation
> +add_task(async function() {

By convention we always put the add_task as the first method in mochitests.

::: devtools/client/inspector/markup/test/head.js:736
(Diff revision 2)
>  
>    await onSelection;
>  }
> +
> +/**
> + * Hit Enter on the reveal link the provided slotted container.

Update comment since ENTER is no longer used by default.

::: devtools/client/inspector/markup/test/head.js:739
(Diff revision 2)
> +
> +/**
> + * Hit Enter on the reveal link the provided slotted container.
> + * Will resolve when selection emits "new-node-front".
> + */
> +async function keydownOnRevealLink(key, inspector, container) {

If you prefer we can "only" support enter or space, and have a boolean parameter to switch between the two options.

::: devtools/client/inspector/markup/test/head.js:754
(Diff revision 2)
> +  EventUtils.synthesizeKey("KEY_Enter", {}, win);
> +  EventUtils.synthesizeKey("KEY_Tab", {}, win);
> +  info("Waiting for .reveal-link to be focused");
> +  await revealFocused;
> +
> +  // hit ENTER on the .reveal-link

Comment should be updated, since this supports other keys than ENTER.
Attachment #8992957 - Flags: review?(jdescottes) → review+
Comment on attachment 8992957 [details]
Bug 1460613 - Add keyboard navigation for reveal link in slotted nodes.

https://reviewboard.mozilla.org/r/257764/#review265442

Looks good, could you also add a check (in test) for the correct role attribute being set on the reveal link span? Thanks!

::: devtools/client/inspector/markup/views/slotted-node-container.js:44
(Diff revision 2)
> +    this.markup.inspector.selection.setNodeFront(this.node, { reason });
> +    this.markup.telemetry.scalarSet("devtools.shadowdom.reveal_link_clicked", true);
> +  },
> +
> +  _onKeyDown: function(event) {
> +    const isActionKey = event.code == "Enter" || event.code == "Space";

I believe it should be " " instead of "Space". Could you try with the keyboard to confirm?
Attachment #8992957 - Flags: review?(yzenevich) → review+
Comment on attachment 8992957 [details]
Bug 1460613 - Add keyboard navigation for reveal link in slotted nodes.

https://reviewboard.mozilla.org/r/257764/#review265452

> If you prefer we can "only" support enter or space, and have a boolean parameter to switch between the two options.

I'm not sure I'd prefer using a flag, since passing a key code is more telling than just `true`/`false`. Maybe it should be the value of an object acting as an enum, but I'm not sure if it's a bit overkilling for a function used in just one test –at least for now.
Comment on attachment 8992957 [details]
Bug 1460613 - Add keyboard navigation for reveal link in slotted nodes.

https://reviewboard.mozilla.org/r/257764/#review265442

Thanks! I'll add this check

> I believe it should be " " instead of "Space". Could you try with the keyboard to confirm?

I think it should be `"Space"` (see https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/code#Code_values). It works with `"Space"`, tried manually and with the mochitests
Pushed by balbeza@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2ee0f455b43b
Add keyboard navigation for reveal link in slotted nodes. r=jdescottes,yzen
https://hg.mozilla.org/mozilla-central/rev/2ee0f455b43b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
I tested this with Nightly 63.0a1(2018-07-26) and I have a question related to the Tab key functionality. If I click on the <div> element and then I press the Tab key the focus is moved, but if I press again on Tab key the focus is moved to the bottom, not to the next <div>. 
I think when you press Tab key the focus should go from one <div> to the next one. Please see the video for a better understanding: https://streamable.com/1p0kd
Please tell me your opinion.
I think to be consistent with the rest of the markupview, the focus should remain in the same line (and not got to the next one, or go to "more nodes" or go to the breadcrumbs). Indeed a bug.
Julian, should we reopen this one or a new bug should be filed for this issue?
Flags: needinfo?(jdescottes)
Sorry Ovidiu, forgot to ping you! 

We already filed and fixed Bug 1478686 for the new issue you reported.
You should be able to validate it on mozilla-central.
Flags: needinfo?(jdescottes)
Flags: qe-verify+
Flags: needinfo?(timea.zsoldos)
I have reproduced this issue using Firefox 62.0a1 (2018.05.1) on Windows 10 x64.
I can confirm this issue is fixed, I verified using Firefox 63.0b5 on Ubuntu 16.04 x64, Windows  10 x64 and Mac OS X 10.13.
Status: RESOLVED → VERIFIED
status-firefox63: fixed → verified
Flags: qe-verify+
Flags: needinfo?(timea.zsoldos)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: