Closed
Bug 1331971
Opened 7 years ago
Closed 4 years ago
[a11y] Can't navigate out of style editor content window using keyboard
Categories
(DevTools :: Style Editor, defect, P3)
DevTools
Style Editor
Tracking
(Not tracked)
RESOLVED
WORKSFORME
People
(Reporter: npang, Assigned: npang, Mentored)
Details
Attachments
(1 file, 1 obsolete file)
No description provided.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → npang
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•7 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)
Comment hidden (mozreview-request) |
Comment 6•7 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•7 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•7 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•7 years ago
|
Attachment #8828111 -
Flags: review?(gl)
Comment 11•7 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•7 years ago
|
Attachment #8828111 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment 14•7 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•7 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•7 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•7 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•6 years ago
|
Product: Firefox → DevTools
Comment 19•4 years ago
|
||
F6 and Shift + F6 seems to do the trick, Marco I think we can close it ?
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: needinfo?(mzehe)
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•