Closed Bug 1331971 Opened 5 years ago Closed 1 year ago

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

Categories

(DevTools :: Style Editor, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: npang, Assigned: npang, Mentored)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Assignee: nobody → npang
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 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 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)
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 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+
Attachment #8828111 - Attachment is obsolete: true
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 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 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 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
Product: Firefox → DevTools

F6 and Shift + F6 seems to do the trick, Marco I think we can close it ?

Status: NEW → RESOLVED
Closed: 1 year ago
Flags: needinfo?(mzehe)
Resolution: --- → WORKSFORME

Correct.

Flags: needinfo?(mzehe)
You need to log in before you can comment on or make changes to this bug.