[a11y] Can't navigate out of style editor content window using keyboard

NEW
Assigned to

Status

DevTools
Style Editor
P3
normal
2 years ago
a month ago

People

(Reporter: npang, Assigned: npang, Mentored)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Updated

2 years ago
Assignee: nobody → npang
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

2 years ago
mozreview-review
Comment on attachment 8828111 [details]
Bug 1331971 - Can't navigate out of style editor content window using keyboard.

https://reviewboard.mozilla.org/r/105620/#review106910

Just a couple of comments and questions :)

::: devtools/client/sourceeditor/editor.js:371
(Diff revision 2)
>  
>        cm.getScrollerElement().scrollBy(deltaX, deltaY);
>      });
>  
> +    // Make code conatiner focusable
> +    cm.getWrapperElement().tabIndex = 0;

If I tab after I'm focused on the wrapper element? do I step into the editor or just jump to the next focusable item? We would want the latter.

::: devtools/client/sourceeditor/editor.js:505
(Diff revision 2)
> +   * - Focuses on editor code text area when Enter is pressed
> +   */
> +  onKeyDown: function (e) {
> +    let {key, target} = e;
> +    let codeWrapper = this.codeMirror.getWrapperElement();
> +    let textArea = codeWrapper.querySelector("textArea");

s/textArea/textarea

Could you also double check if, by any chance, the text area inside the codemirror widget has a getter similar to getWrapperElement() ? so we don't have to query for it in the dom?

::: devtools/client/styleeditor/test/browser_styleeditor_sourceditor_keyboard_navigation.js:22
(Diff revision 2)
> +
> +  let codeContainer = doc.querySelector(".CodeMirror");
> +  let textArea = doc.querySelector("textarea");
> +
> +  info("Select the first style sheet");
> +  yield ui.selectStyleSheet(editor.styleSheet, LINE_NO, COL_NO);

Lets also check what's an activeElement after we select stylesheet as well?
Attachment #8828111 - Flags: review?(yzenevich)
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P3
Comment hidden (mozreview-request)

Comment 6

2 years ago
mozreview-review
Comment on attachment 8828111 [details]
Bug 1331971 - Can't navigate out of style editor content window using keyboard.

https://reviewboard.mozilla.org/r/105620/#review108930

::: devtools/client/sourceeditor/editor.js:509
(Diff revision 3)
> +   * - Tabbing while focused on code container moves to next focusable item
> +   */
> +  onKeyDown: function (e, nextItem) {
> +    let {key, target} = e;
> +    let codeWrapper = this.codeMirror.getWrapperElement();
> +    let textArea = codeWrapper.querySelector("textArea");

textArea -> textarea
Attachment #8828111 - Flags: review?(yzenevich)
Comment hidden (mozreview-request)

Comment 8

2 years ago
mozreview-review
Comment on attachment 8828111 [details]
Bug 1331971 - Can't navigate out of style editor content window using keyboard.

https://reviewboard.mozilla.org/r/105620/#review109784

::: devtools/client/styleeditor/StyleEditorUI.jsm:125
(Diff revision 4)
>    get selectedStyleSheetIndex() {
>      return this.selectedEditor ?
>             this.selectedEditor.styleSheet.styleSheetIndex : -1;
>    },
>  
> +  _isCondition: function (e, keyCode, isShiftKey, targetElm) {

If I understand correctly, _isCondition is a leftover from previous patch?

::: devtools/client/styleeditor/StyleEditorUI.jsm:217
(Diff revision 4)
>        this._panelDoc.getElementById("context-openlinknewtab");
>      this._openLinkNewTabItem.addEventListener("command",
>                                                this._openLinkNewTab);
>  
> +    this._detailContent = this._panelDoc.querySelector(
> +      ".splitview-side-details.devtools-main-content");

See if we really need such a strict selector. Perhaps we can get away with using just 1 class name.

::: devtools/client/styleeditor/StyleEditorUI.jsm:361
(Diff revision 4)
>      if (savedFile && !file) {
>        file = savedFile;
>      }
>  
>      let editor = new StyleSheetEditor(styleSheet, this._window, file, isNew,
> -                                      this._walker, this._highlighter);
> +                                      this._walker, this._highlighter, this._target);

If I understand correctly, this._target argument is a leftover from previous patch?

::: devtools/client/styleeditor/StyleEditorUI.jsm:584
(Diff revision 4)
>  
>          let sidebar = details.querySelector(".stylesheet-sidebar");
> +        let list = details.querySelector(".stylesheet-media-list");
>          sidebar.setAttribute("width",
>              Services.prefs.getIntPref(PREF_SIDEBAR_WIDTH));
> +        sidebar.hidden ? list.tabIndex = -1 : list.tabIndex = 0;

I don't think this should be part of the patch since it does not prevent us from stepping out of the editor.

But in general, in cases like that, when container is hidden but internal stuff is tabbable, it is better to use CSS to set visibility: hidden or display: none on the conatiner when it has [hidden] attribute. This way we don't need to deal with JS and tabindexes. So this case here would be:

```
.stylesheet-sidebar[hidden] {
  display: none;
}
```

::: devtools/client/styleeditor/StyleEditorUI.jsm:924
(Diff revision 4)
>          cond.textContent = rule.conditionText;
>          cond.className = "media-rule-condition";
>          if (!rule.matches) {
>            cond.classList.add("media-condition-unmatched");
>          }
> -        if (this._target.tab.tagName == "tab") {
> +        if (this._target._tab.tagName == "tab") {

_tab and tab seem to be the same. Is there a reason for this change?
Attachment #8828111 - Flags: review?(yzenevich)
(Assignee)

Comment 9

2 years ago
mozreview-review
Comment on attachment 8828111 [details]
Bug 1331971 - Can't navigate out of style editor content window using keyboard.

https://reviewboard.mozilla.org/r/105620/#review109884

::: devtools/client/styleeditor/StyleEditorUI.jsm:125
(Diff revision 4)
>    get selectedStyleSheetIndex() {
>      return this.selectedEditor ?
>             this.selectedEditor.styleSheet.styleSheetIndex : -1;
>    },
>  
> +  _isCondition: function (e, keyCode, isShiftKey, targetElm) {

woops, yep that's right

::: devtools/client/styleeditor/StyleEditorUI.jsm:361
(Diff revision 4)
>      if (savedFile && !file) {
>        file = savedFile;
>      }
>  
>      let editor = new StyleSheetEditor(styleSheet, this._window, file, isNew,
> -                                      this._walker, this._highlighter);
> +                                      this._walker, this._highlighter, this._target);

Again woops
Comment hidden (mozreview-request)

Updated

2 years ago
Attachment #8828111 - Flags: review?(gl)

Comment 11

2 years ago
mozreview-review
Comment on attachment 8828111 [details]
Bug 1331971 - Can't navigate out of style editor content window using keyboard.

https://reviewboard.mozilla.org/r/105620/#review109896

Looks good, r? Gabriel for the devtools peer review.
Attachment #8828111 - Flags: review?(yzenevich) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8828111 - Attachment is obsolete: true
Comment hidden (mozreview-request)

Comment 14

2 years ago
mozreview-review
Comment on attachment 8828111 [details]
Bug 1331971 - Can't navigate out of style editor content window using keyboard.

https://reviewboard.mozilla.org/r/105620/#review109906

::: devtools/client/styleeditor/StyleEditorUI.jsm:213
(Diff revision 6)
>      this._openLinkNewTabItem.addEventListener("command",
>                                                this._openLinkNewTab);
>  
> +    this._detailContent = this._panelDoc.querySelector(
> +      ".devtools-main-content");
> +    this._detailContent.addEventListener("keydown", (e) => this._onKeyDown(e));

this can just be this._onKeyDown

::: devtools/client/styleeditor/test/browser_styleeditor_sourceditor_keyboard_navigation.js:4
(Diff revision 6)
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +"use strict";

Add a new line before "use strict";

::: devtools/client/styleeditor/test/browser_styleeditor_sourceditor_keyboard_navigation.js:29
(Diff revision 6)
> +  EventUtils.synthesizeKey("VK_ESCAPE", {});
> +  is(codeContainer, editordoc.activeElement, "Container is focused");
> +
> +  info("Enter editor");
> +  EventUtils.synthesizeKey("VK_RETURN", {});
> +  is(textArea, editordoc.activeElement, "Editor is enabled");

I think you mean the Editor is focused

Comment 15

2 years ago
mozreview-review
Comment on attachment 8828111 [details]
Bug 1331971 - Can't navigate out of style editor content window using keyboard.

https://reviewboard.mozilla.org/r/105620/#review109930

Comment 16

2 years ago
mozreview-review
Comment on attachment 8832482 [details]
Bug 1331971 - Can't navigate out of style editor content window using keyboard.

https://reviewboard.mozilla.org/r/108748/#review109932

::: devtools/client/styleeditor/StyleEditorUI.jsm:213
(Diff revision 1)
>      this._openLinkNewTabItem.addEventListener("command",
>                                                this._openLinkNewTab);
>  
> +    this._detailContent = this._panelDoc.querySelector(
> +      ".devtools-main-content");
> +    this._detailContent.addEventListener("keydown", this._onKeyDown());

This should be this._onKeyDown without the ()

::: devtools/client/styleeditor/test/browser_styleeditor_sourceditor_keyboard_navigation.js:4
(Diff revision 1)
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +"use strict";

Add a new line before "use strict";
Attachment #8832482 - Flags: review?(gl) → review+

Comment 17

2 years ago
mozreview-review
Comment on attachment 8832482 [details]
Bug 1331971 - Can't navigate out of style editor content window using keyboard.

https://reviewboard.mozilla.org/r/108748/#review109934

::: devtools/client/styleeditor/test/browser_styleeditor_sourceditor_keyboard_navigation.js:29
(Diff revision 1)
> +  EventUtils.synthesizeKey("VK_ESCAPE", {});
> +  is(codeContainer, editordoc.activeElement, "Container is focused");
> +
> +  info("Enter editor");
> +  EventUtils.synthesizeKey("VK_RETURN", {});
> +  is(textArea, editordoc.activeElement, "Editor is enabled");

Editor is focused
Comment hidden (mozreview-request)

Updated

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