Closed Bug 1273211 Opened 8 years ago Closed 8 years ago

[a11y] Add accessibility navigation(enter/space) to controls

Categories

(DevTools :: Inspector, defect, P2)

defect

Tracking

(firefox50 fixed)

RESOLVED FIXED
Firefox 50
Tracking Status
firefox50 --- fixed

People

(Reporter: npang, Assigned: npang)

References

Details

(Keywords: access)

Attachments

(1 file)

Some controls are only clickable not keyboard accessible.
Assignee: nobody → npang
Comment on attachment 8754540 [details]
Bug 1273211 - Added side panel enter and return controls, added keyboard space selection for controls in netmonitor and console key;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54026/diff/1-2/
Attachment #8754540 - Attachment description: MozReview Request: Bug 1273211 - Added side panel enter and return controls, added keyboard space selection for controls in netmonitorr; ?yzen → MozReview Request: Bug 1273211 - Added side panel enter and return controls, added keyboard space selection for controls in netmonitor; r?yzen
Attachment #8754540 - Flags: review?(yzenevich)
Comment on attachment 8754540 [details]
Bug 1273211 - Added side panel enter and return controls, added keyboard space selection for controls in netmonitor and console key;

https://reviewboard.mozilla.org/r/54026/#review50944

Looks good, just a couple of comments and the tests and I will try applying it for the final check. Thanks!

::: devtools/client/debugger/views/toolbar-view.js:174
(Diff revision 2)
>      for (let button of buttons) {
>        button.disabled = !enabled;
>      }
>    },
>  
> +  _onTogglePanesPressed: function(e) {

Here destructuring assignment can be very handy (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment)


_onTogglePanesPressed: function({keyCode}) {
...
}

We also need a jsdoc comment above the function just like _onTogglePanesClicked has

::: devtools/client/debugger/views/toolbar-view.js:175
(Diff revision 2)
>        button.disabled = !enabled;
>      }
>    },
>  
> +  _onTogglePanesPressed: function(e) {
> +    if(e.keyCode === e.DOM_VK_SPACE || e.keyCode === e.DOM_VK_RETURN){

nothing wrong with this check, you can also use this form:
[e.DOM_VK_SPACE, e.DOM_VK_RETURN].includes(keyCode)

::: devtools/client/inspector/inspector-panel.js:406
(Diff revision 2)
>    /**
>     * Add the expand/collapse behavior for the sidebar panel.
>     */
>    setupSidebarToggle: function() {
>      this._paneToggleButton = this.panelDoc.getElementById("inspector-pane-toggle");
> +    this._paneToggleButton.setAttribute("tooltiptext",

I'm guessing this is from another bug. I would probably start from the clean master version for this new one.

::: devtools/client/inspector/inspector-panel.js:1042
(Diff revision 2)
>      this._markupBox = null;
>  
>      return destroyPromise;
>    },
>  
> +  onPaneToggleButtonPressed: function(e) {

Same here, destructuring assignment and jsdoc

::: devtools/client/netmonitor/netmonitor-view.js:1001
(Diff revision 2)
>     *
>     * @param string type
>     *        Either "all", "html", "css", "js", "xhr", "fonts", "images", "media"
>     *        "flash", "ws" or "other".
>     */
> +  filterOnPress: function(type, e){

same here - destructuring assignment and its own jsdoc comment (mentioning the key)

::: devtools/client/netmonitor/netmonitor-view.js:1135
(Diff revision 2)
>     *
>     * @param string type
>     *        Either "status", "method", "file", "domain", "type", "transferred",
>     *        "size" or "waterfall".
>     */
> +  sortByPress: function(type, e){

destructuring assignment and its own jsdoc comment (mentioning the key)

::: devtools/client/netmonitor/netmonitor-view.js:3901
(Diff revision 2)
>   *
>   * @param function callback
>   *          Function to execute execute when data-key
>   *          is present in event.target.
>   * @return function
>   *          Wrapped function with the target data-key as the first argument.

Update this comment mentioning the event as the second argument.
Status: NEW → ASSIGNED
Keywords: access
FILTER ON CLIMBING SHOES
Priority: -- → P2
Comment on attachment 8754540 [details]
Bug 1273211 - Added side panel enter and return controls, added keyboard space selection for controls in netmonitor and console key;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54026/diff/2-3/
Attachment #8754540 - Flags: review?(yzenevich)
https://reviewboard.mozilla.org/r/54026/#review51392

::: devtools/client/debugger/views/toolbar-view.js:24
(Diff revision 3)
>    this.StackFrames = DebuggerController.StackFrames;
>    this.ThreadState = DebuggerController.ThreadState;
>    this.DebuggerController = DebuggerController;
>    this.DebuggerView = DebuggerView;
>  
> -  this._onTogglePanesPressed = this._onTogglePanesPressed.bind(this);
> +  this._onTogglePanesClicked= this._onTogglePanesClicked.bind(this);

Fly-by comment: Did you really want to make this change, e. g. just remove the space before the =? The line seems unchanged otherwise, and in a later file, you didn't make this change, either.

::: devtools/client/debugger/views/toolbar-view.js:179
(Diff revision 3)
>    /**
> +  * Listener handling the toggle button space and return key event.
> +  */
> +  _onTogglePanesPressed: function({ keyCode }) {
> +    let keyEvent = Ci.nsIDOMKeyEvent;
> +    if(keyCode === keyEvent.DOM_VK_SPACE || keyCode === keyEvent.DOM_VK_RETURN){

Not sure if this is code style in these files, but normally, there's a space between if and the opening parenthesis.

::: devtools/client/inspector/inspector-panel.js:1046
(Diff revision 3)
> +  * When the pane toggle button is pressed with space and return keys toggle
> +  * the pane, change the button state and tooltip.
> +  */
> +  onPaneToggleButtonPressed: function({keyCode}) {
> +    let keyEvent = Ci.nsIDOMKeyEvent;
> +    if(keyCode === keyEvent.DOM_VK_SPACE || keyCode === keyEvent.DOM_VK_RETURN){

Same here, space between if and (.
https://reviewboard.mozilla.org/r/54024/#review51490

Ok, gave it another round. Ill apply it after these comments are addressed. (Marco's as well that eslint should help with).

::: devtools/client/debugger/test/mochitest/browser.ini:287
(Diff revision 3)
>  [browser_dbg_iframes.js]
>  skip-if = e10s # TODO
>  [browser_dbg_instruments-pane-collapse.js]
>  skip-if = e10s && debug
> +[browser_dbg_instruments-pane-collapse_press.js]
> +skip-if = (os == 'mac' && e10s && debug)

Add the inline comment similar to what inspector's browser.ini has:
# Full keyboard navigation on OSX only works if Full Keyboard Access setting is set to All Control in System Keyboard Preferences

::: devtools/client/debugger/test/mochitest/browser_dbg_instruments-pane-collapse_press.js:1
(Diff revision 3)
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */

maybe ..._keyboard instead of ..._press ?

::: devtools/client/debugger/test/mochitest/browser_dbg_instruments-pane-collapse_press.js:12
(Diff revision 3)
> + * Tests that the debugger panes collapse properly.
> + */
> +
> +const TAB_URL = EXAMPLE_URL + "doc_recursion-stack.html";
> +
> +var gTab, gPanel, gDebugger;

I dont think we need these ouside of test() scope.

::: devtools/client/debugger/test/mochitest/browser_dbg_instruments-pane-collapse_press.js:17
(Diff revision 3)
> +var gTab, gPanel, gDebugger;
> +var gPrefs, gOptions;
> +
> +function test() {
> +  initDebugger(TAB_URL).then(([aTab,, aPanel]) => {
> +    gTab = aTab;

Unused variable here

::: devtools/client/debugger/test/mochitest/browser_dbg_instruments-pane-collapse_press.js:18
(Diff revision 3)
> +var gPrefs, gOptions;
> +
> +function test() {
> +  initDebugger(TAB_URL).then(([aTab,, aPanel]) => {
> +    gTab = aTab;
> +    gPanel = aPanel;

No need for this variable just use aPanel instead

::: devtools/client/debugger/test/mochitest/browser_dbg_instruments-pane-collapse_press.js:19
(Diff revision 3)
> +
> +function test() {
> +  initDebugger(TAB_URL).then(([aTab,, aPanel]) => {
> +    gTab = aTab;
> +    gPanel = aPanel;
> +    gDebugger = gPanel.panelWin;

See if we really need dDebugger declared outside of the test function scope. I think here 
```
let debugger = aPanel.panelWin;
```
should suffice.
In fact all you need is a document so maybe:
```
let doc = aPanel.panelWin.document;
```
?

::: devtools/client/debugger/test/mochitest/browser_dbg_instruments-pane-collapse_press.js:20
(Diff revision 3)
> +function test() {
> +  initDebugger(TAB_URL).then(([aTab,, aPanel]) => {
> +    gTab = aTab;
> +    gPanel = aPanel;
> +    gDebugger = gPanel.panelWin;
> +    gPrefs = gDebugger.Prefs;

Unused variable here

::: devtools/client/debugger/test/mochitest/browser_dbg_instruments-pane-collapse_press.js:21
(Diff revision 3)
> +  initDebugger(TAB_URL).then(([aTab,, aPanel]) => {
> +    gTab = aTab;
> +    gPanel = aPanel;
> +    gDebugger = gPanel.panelWin;
> +    gPrefs = gDebugger.Prefs;
> +    gOptions = gDebugger.DebuggerView.Options;

Unused variable here

::: devtools/client/debugger/test/mochitest/browser_dbg_instruments-pane-collapse_press.js:31
(Diff revision 3)
> +      let button =
> +          gDebugger.document.getElementById("instruments-pane-toggle");
> +      ok(pane.hasAttribute("pane-collapsed"),
> +          "The instruments panel is initially in collapsed state");
> +
> +      let onTransitionEnd = once(pane, "transitionend");

Lines 31-35 and 40-44 are a good codidate for their own function since they are essentially the same

::: devtools/client/debugger/test/mochitest/browser_dbg_instruments-pane-collapse_press.js:47
(Diff revision 3)
> +      button.focus();
> +      EventUtils.synthesizeKey("VK_SPACE", {});
> +      yield onTransitionEnd;
> +
> +      setTimeout(function(){
> +        ok(pane.hasAttribute("pane-collapsed"),

I think this check belongs outside of timeout after the last yield.

::: devtools/client/debugger/test/mochitest/browser_dbg_instruments-pane-collapse_press.js:49
(Diff revision 3)
> +      yield onTransitionEnd;
> +
> +      setTimeout(function(){
> +        ok(pane.hasAttribute("pane-collapsed"),
> +            "The instruments panel is in the collapsed state");
> +        closeDebuggerAndFinish(gPanel);

Do we really need a time out here? I see some other debugger ones without it. See if we can call closeDebuggerAndFinish as is (if the test still passes).

::: devtools/client/debugger/views/toolbar-view.js:24
(Diff revision 3)
>    this.StackFrames = DebuggerController.StackFrames;
>    this.ThreadState = DebuggerController.ThreadState;
>    this.DebuggerController = DebuggerController;
>    this.DebuggerView = DebuggerView;
>  
> -  this._onTogglePanesPressed = this._onTogglePanesPressed.bind(this);
> +  this._onTogglePanesClicked= this._onTogglePanesClicked.bind(this);

So I was thinking about the names for these functions a bit more and I think we should make "clicked" ones named more interaction type agnostic (e.g. click press). So could we rename _onTogglePanesClicked and all other ones that you renamed to ...Clicked into _onTogglePanesActivated and ...Activated respectively.

::: devtools/client/inspector/inspector-panel.js:95
(Diff revision 3)
>    this._resetNodeMenu = this._resetNodeMenu.bind(this);
>    this._updateSearchResultsLabel = this._updateSearchResultsLabel.bind(this);
>    this.onNewSelection = this.onNewSelection.bind(this);
>    this.onBeforeNewSelection = this.onBeforeNewSelection.bind(this);
>    this.onDetached = this.onDetached.bind(this);
>    this.onPaneToggleButtonClicked = this.onPaneToggleButtonClicked.bind(this);

so here as well feel free to rename to ...Clicked -> ...Activated

::: devtools/client/inspector/test/browser_inspector_pane-toggle-05.js:11
(Diff revision 3)
> +// Test that the inspector toggled panel is visible by default, is hidden after
> +// pressing space or the enter key on the toggle button and
> +//remains expanded/collapsed when switching hosts.
> +
> +add_task(function* () {
> +  info("Open the inspector in a side toolbox host");

nit: no need fot this info()

::: devtools/client/inspector/test/browser_inspector_pane-toggle-05.js:13
(Diff revision 3)
> +//remains expanded/collapsed when switching hosts.
> +
> +add_task(function* () {
> +  info("Open the inspector in a side toolbox host");
> +  let {toolbox, inspector} = yield openInspectorForURL("about:blank", "side");
> +

nit no need for empty line between lets, add one after all the lets instead.

::: devtools/client/inspector/test/browser_inspector_pane-toggle-05.js:18
(Diff revision 3)
> +
> +  let panel = inspector.panelDoc.querySelector("#inspector-sidebar");
> +  let button = inspector.panelDoc.getElementById("inspector-pane-toggle");
> +  ok(!panel.hasAttribute("pane-collapsed"), "The panel is in expanded state");
> +
> +  info("Listen to the end of the animation on the sidebar panel");

Same as the other test file, make a separate function for the key press and transition yield part.

::: devtools/client/netmonitor/netmonitor-view.js:1000
(Diff revision 3)
>     * Filters all network requests in this container by a specified type.
>     *
>     * @param string type
>     *        Either "all", "html", "css", "js", "xhr", "fonts", "images", "media"
>     *        "flash", "ws" or "other".
> +   * @param string keyCode

The argument here is actually an event object.

::: devtools/client/netmonitor/netmonitor-view.js:1003
(Diff revision 3)
>     *        Either "all", "html", "css", "js", "xhr", "fonts", "images", "media"
>     *        "flash", "ws" or "other".
> +   * @param string keyCode
> +   *        The event's keyCode triggered by the keypress
> +   */
> +  filterOnPress: function(type, {keyCode}){

Here and below, 2 spaces for indentation instead of 4.

::: devtools/client/netmonitor/netmonitor-view.js:1144
(Diff revision 3)
>     * Sorts all network requests in this container by a specified detail.
>     *
>     * @param string type
>     *        Either "status", "method", "file", "domain", "type", "transferred",
>     *        "size" or "waterfall".
> +   * @param string keyCode

Same here, this is an event object arg.
Comment on attachment 8754540 [details]
Bug 1273211 - Added side panel enter and return controls, added keyboard space selection for controls in netmonitor and console key;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54026/diff/3-4/
Comment on attachment 8754540 [details]
Bug 1273211 - Added side panel enter and return controls, added keyboard space selection for controls in netmonitor and console key;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54026/diff/4-5/
https://reviewboard.mozilla.org/r/54024/#review51560

Looks good, I think after this round it will likely be ready for devtools person to take a look. Thanks!

There are a couple of other controls that are not activatable that you can add to this patch:

In inspector:
#inspector-element-add-button is not keyboard accessible (can't tab to/from) it might also be not activatable

In the toolbox (top level toolbar):
#command-button-splitconsole 
#command-button-responsive

In network monitor:
#details-pane-toggle

::: devtools/client/debugger/test/mochitest/browser_dbg_instruments-pane-collapse_keyboard.js:13
(Diff revision 5)
> + */
> +
> +const TAB_URL = EXAMPLE_URL + "doc_recursion-stack.html";
> +
> +function test() {
> +  let gDebugger;

We should move this declaration inside the Task.spawn test function. Since we only need document object, just have initialize
```
let doc = aPanel.panelWin.document;
```

::: devtools/client/debugger/test/mochitest/browser_dbg_instruments-pane-collapse_keyboard.js:14
(Diff revision 5)
> +
> +const TAB_URL = EXAMPLE_URL + "doc_recursion-stack.html";
> +
> +function test() {
> +  let gDebugger;
> +  let onTransitionEnd;

Unuesed variable.

::: devtools/client/debugger/views/toolbar-view.js:68
(Diff revision 5)
>      this._stepInTooltip = L10N.getFormatStr("stepInTooltip", stepInKey);
>      this._stepOutTooltip = L10N.getFormatStr("stepOutTooltip", stepOutKey);
>  
> -    this._instrumentsPaneToggleButton.addEventListener("mousedown", this._onTogglePanesPressed, false);
> +    this._instrumentsPaneToggleButton.addEventListener("mousedown",
> +      this._onTogglePanesActivated, false);
> +    this._instrumentsPaneToggleButton.addEventListener(

nit: keep "keydown" on the same line (similar to the mousedown case above.

::: devtools/client/debugger/views/toolbar-view.js:181
(Diff revision 5)
>    },
>  
>    /**
> +  * Listener handling the toggle button space and return key event.
> +  */
> +  _onTogglePanesPressed: function ({ keyCode }) {

Something that was mentioned to me by the devtools folks, they would like to use event.DOM_VK_... for keys to match against. So I hope it's ok to go back to your original design where you used event as an argument.

::: devtools/client/debugger/views/toolbar-view.js:184
(Diff revision 5)
> +  * Listener handling the toggle button space and return key event.
> +  */
> +  _onTogglePanesPressed: function ({ keyCode }) {
> +    let keyEvent = Ci.nsIDOMKeyEvent;
> +    if (keyCode === keyEvent.DOM_VK_SPACE
> +      || keyCode === keyEvent.DOM_VK_RETURN) {

nit || generally goes at the end of the line

::: devtools/client/inspector/inspector-panel.js:1048
(Diff revision 5)
>  
>    /**
> +  * When the pane toggle button is pressed with space and return keys toggle
> +  * the pane, change the button state and tooltip.
> +  */
> +  onPaneToggleButtonPressed: function ({keyCode}) {

Same here

::: devtools/client/inspector/inspector-panel.js:1051
(Diff revision 5)
> +  * the pane, change the button state and tooltip.
> +  */
> +  onPaneToggleButtonPressed: function ({keyCode}) {
> +    let keyEvent = Ci.nsIDOMKeyEvent;
> +    if (keyCode === keyEvent.DOM_VK_SPACE
> +      || keyCode === keyEvent.DOM_VK_RETURN) {

Same here, re ||

::: devtools/client/inspector/test/browser_inspector_pane-toggle-05.js:14
(Diff revision 5)
> +
> +add_task(function* () {
> +  let {toolbox, inspector} = yield openInspectorForURL("about:blank", "side");
> +  let panel = inspector.panelDoc.querySelector("#inspector-sidebar");
> +  let button = inspector.panelDoc.getElementById("inspector-pane-toggle");
> +  ok(!panel.hasAttribute("pane-collapsed"), "The panel is in expanded state");

just a small nit here: new line between lets and ok

::: devtools/client/netmonitor/netmonitor-view.js:999
(Diff revision 5)
>     * Filters all network requests in this container by a specified type.
>     *
>     * @param string type
>     *        Either "all", "html", "css", "js", "xhr", "fonts", "images", "media"
>     *        "flash", "ws" or "other".
> +   * @param event keyCode

Same here.

::: devtools/client/netmonitor/netmonitor-view.js:1004
(Diff revision 5)
> +   * @param event keyCode
> +   *        The event's keyCode triggered by the keypress
> +   */
> +  filterOnPress: function (type, {keyCode}) {
> +    let keyEvent = Ci.nsIDOMKeyEvent;
> +    if (keyCode === keyEvent.DOM_VK_SPACE

Same here

::: devtools/client/netmonitor/netmonitor-view.js:1147
(Diff revision 5)
>     *        Either "status", "method", "file", "domain", "type", "transferred",
>     *        "size" or "waterfall".
> +   * @param event keyCode
> +   *        The event's keycode triggered by the keypress
> +   */
> +  sortByPress: function (type, {keyCode}) {

Same here and ||
Comment on attachment 8754540 [details]
Bug 1273211 - Added side panel enter and return controls, added keyboard space selection for controls in netmonitor and console key;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54026/diff/5-6/
Comment on attachment 8754540 [details]
Bug 1273211 - Added side panel enter and return controls, added keyboard space selection for controls in netmonitor and console key;

https://reviewboard.mozilla.org/r/54026/#review51908

Looks good, I added final nits and then you can move the r? to pbro for example. In terms of the other inaccessible controls, are you thinking of doing it in a separate bug?

::: devtools/client/debugger/test/mochitest/browser_dbg_instruments-pane-collapse_keyboard.js:15
(Diff revision 6)
> +const TAB_URL = EXAMPLE_URL + "doc_recursion-stack.html";
> +
> +function test() {
> +  initDebugger(TAB_URL).then(([aTab,, aPanel]) => {
> +    Task.spawn(function* () {
> +      let gDebugger = aPanel.panelWin.document;

nit: misleading variable name
change gDebugger to doc.

::: devtools/client/debugger/views/toolbar-view.js:24
(Diff revision 6)
>    this.StackFrames = DebuggerController.StackFrames;
>    this.ThreadState = DebuggerController.ThreadState;
>    this.DebuggerController = DebuggerController;
>    this.DebuggerView = DebuggerView;
>  
> -  this._onTogglePanesPressed = this._onTogglePanesPressed.bind(this);
> +  this._onTogglePanesActivated= this._onTogglePanesActivated.bind(this);

nit space before =

::: devtools/client/debugger/views/toolbar-view.js:25
(Diff revision 6)
>    this.ThreadState = DebuggerController.ThreadState;
>    this.DebuggerController = DebuggerController;
>    this.DebuggerView = DebuggerView;
>  
> -  this._onTogglePanesPressed = this._onTogglePanesPressed.bind(this);
> +  this._onTogglePanesActivated= this._onTogglePanesActivated.bind(this);
> +  this._onTogglePanesPressed= this._onTogglePanesPressed.bind(this);

nit space before =

::: devtools/client/debugger/views/toolbar-view.js:66
(Diff revision 6)
>      this._pausePendingTooltip = L10N.getStr("pausePendingButtonTooltip");
>      this._stepOverTooltip = L10N.getFormatStr("stepOverTooltip", stepOverKey);
>      this._stepInTooltip = L10N.getFormatStr("stepInTooltip", stepInKey);
>      this._stepOutTooltip = L10N.getFormatStr("stepOutTooltip", stepOutKey);
>  
> -    this._instrumentsPaneToggleButton.addEventListener("mousedown", this._onTogglePanesPressed, false);
> +    this._instrumentsPaneToggleButton.addEventListener("mousedown", this._onTogglePanesActivated, false);

nit: sorry what i meant here was a line break as follows:
```
this._instrumentsPaneToggleButton.addEventListener("mousedown", 
  this._onTogglePanesActivated, false);
```

::: devtools/client/debugger/views/toolbar-view.js:67
(Diff revision 6)
>      this._stepOverTooltip = L10N.getFormatStr("stepOverTooltip", stepOverKey);
>      this._stepInTooltip = L10N.getFormatStr("stepInTooltip", stepInKey);
>      this._stepOutTooltip = L10N.getFormatStr("stepOutTooltip", stepOutKey);
>  
> -    this._instrumentsPaneToggleButton.addEventListener("mousedown", this._onTogglePanesPressed, false);
> +    this._instrumentsPaneToggleButton.addEventListener("mousedown", this._onTogglePanesActivated, false);
> +    this._instrumentsPaneToggleButton.addEventListener("keydown", this._onTogglePanesPressed, false);

same here

::: devtools/client/debugger/views/toolbar-view.js:88
(Diff revision 6)
>     */
>    destroy: function () {
>      dumpn("Destroying the ToolbarView");
>  
> -    this._instrumentsPaneToggleButton.removeEventListener("mousedown", this._onTogglePanesPressed, false);
> +    this._instrumentsPaneToggleButton.removeEventListener("mousedown",
> +      this._onTogglePanesActivated, false);

Yeah like this

::: devtools/client/debugger/views/toolbar-view.js:179
(Diff revision 6)
>    },
>  
>    /**
> +  * Listener handling the toggle button space and return key event.
> +  */
> +  _onTogglePanesPressed: function (e) {

nice, just 1 nit: rename e to event.

::: devtools/client/inspector/inspector-panel.js:1048
(Diff revision 6)
>  
>    /**
> +  * When the pane toggle button is pressed with space and return keys toggle
> +  * the pane, change the button state and tooltip.
> +  */
> +  onPaneToggleButtonPressed: function (e) {

looks good as well , just change e to event

::: devtools/client/netmonitor/netmonitor-view.js:1002
(Diff revision 6)
>     *        Either "all", "html", "css", "js", "xhr", "fonts", "images", "media"
>     *        "flash", "ws" or "other".
> +   * @param event e
> +   *        The event triggered by the keypress
> +   */
> +  filterOnPress: function (type, e) {

same here (e->event)

::: devtools/client/netmonitor/netmonitor-view.js:1146
(Diff revision 6)
>     *        Either "status", "method", "file", "domain", "type", "transferred",
>     *        "size" or "waterfall".
> +   * @param event e
> +   *        The event triggered by the keypress
> +   */
> +  sortByPress: function (type, e) {

same here (e->event)
Comment on attachment 8754540 [details]
Bug 1273211 - Added side panel enter and return controls, added keyboard space selection for controls in netmonitor and console key;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54026/diff/6-7/
Attachment #8754540 - Attachment description: MozReview Request: Bug 1273211 - Added side panel enter and return controls, added keyboard space selection for controls in netmonitor; r?yzen → MozReview Request: Bug 1273211 - Added side panel enter and return controls, added keyboard space selection for controls in netmonitor and console key; r?pbro
Attachment #8754540 - Flags: feedback+ → review?(pbrosset)
https://reviewboard.mozilla.org/r/54026/#review52010

Added some comments as a fly by review

::: devtools/client/framework/toolbox.js:558
(Diff revision 7)
>          e.preventDefault();
>        }
>      }
>    },
> +
> +  _splitConsoleOnKeypressLocal: function (e) {

So i think this should be done slighly differently:
Since the escape handler is in here as well. You can listen to space and return key presses within the same event handler and do the same thing as what happens with escape if the event target is the consoleKey. No need to create additional handlers and keybindings.

::: devtools/client/inspector/inspector.xul:158
(Diff revision 7)
>          class="devtools-toolbar"
>          nowindowdrag="true">
>          <toolbarbutton id="inspector-element-add-button"
>            class="devtools-toolbarbutton"
>            tooltiptext="&inspectorAddNode.label;"
> -          oncommand="inspector.addNode()" />
> +          oncommand="inspector.addNode()"

I think you can just add
```
-moz-user-focus: normal;
```
style in CSS for that toolbarbutton instead of tabindex here.
https://reviewboard.mozilla.org/r/54026/#review52180

::: devtools/client/framework/toolbox.js:558
(Diff revision 7)
>          e.preventDefault();
>        }
>      }
>    },
> +
> +  _splitConsoleOnKeypressLocal: function (e) {

Actually, thinking of it a bit more, lets do a separate event listener for the button itself (so almost like yours). But lets take out the common bits (e.g. from line 560 to 566 as a separate functiona that will be used in both handlers). Also the if you dont mind, maybe a better name for this function would be something like _splitConsoleOnButtonKeypress?
Comment on attachment 8754540 [details]
Bug 1273211 - Added side panel enter and return controls, added keyboard space selection for controls in netmonitor and console key;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54026/diff/7-8/
This looks good :). Mind you, I think, you need to rebase your patch against latest master as it does not apply any more.
Comment on attachment 8754540 [details]
Bug 1273211 - Added side panel enter and return controls, added keyboard space selection for controls in netmonitor and console key;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54026/diff/8-9/
Comment on attachment 8754540 [details]
Bug 1273211 - Added side panel enter and return controls, added keyboard space selection for controls in netmonitor and console key;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54026/diff/9-10/
It would be nice to have a shared module instead of duplicating the code.

Something like this:

const { makeKeyboardAccessible } = require("devtools/client/shared/tool-accessibility"); (maybe a11y-helpers would be a better name ?)

and then you would simply use it like this:

makeKeyboardAccessible(this._instrumentsPaneToggleButton);

The API would simply trigger a click when the right keys are pressed.
(In reply to Tim Nguyen :ntim (not accepting requests) from comment #20)
> It would be nice to have a shared module instead of duplicating the code.
> 
> Something like this:
> 
> const { makeKeyboardAccessible } =
> require("devtools/client/shared/tool-accessibility"); (maybe a11y-helpers
> would be a better name ?)
or since we're already importing view-helpers, it could belong there as well.
Comment on attachment 8754540 [details]
Bug 1273211 - Added side panel enter and return controls, added keyboard space selection for controls in netmonitor and console key;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54026/diff/10-11/
Comment on attachment 8754540 [details]
Bug 1273211 - Added side panel enter and return controls, added keyboard space selection for controls in netmonitor and console key;

https://reviewboard.mozilla.org/r/54026/#review53086

::: devtools/client/debugger/test/mochitest/browser_dbg_instruments-pane-collapse_keyboard.js:21
(Diff revision 11)
> +      let pane = doc.getElementById("instruments-pane");
> +      let button = doc.getElementById("instruments-pane-toggle");
> +      ok(pane.hasAttribute("pane-collapsed"),
> +          "The instruments panel is initially in collapsed state");
> +
> +      yield togglePane(button, "Press on the toggle button to expand", pane);

You should add the keycode as a new argument to this function so you can test both return and space. Here you only test return.

::: devtools/client/framework/toolbox.js:555
(Diff revision 11)
>    },
>  
> -  _splitConsoleOnKeypress: function (e) {
> -    if (e.keyCode === e.DOM_VK_ESCAPE) {
> +  /**
> +  * Global handler for ESC shortcut to split the console
> +  */
> +  _splitConsoleOnKeypress: function (event) {

I would simply rename this to `_onKeyPress`. It is indeed used to toggle the split console, but it's a global listener in the toolbox. And we might add more event handling here in the future. So, renaming to `_onKeyPress` makes the intent clearer and makes it easier to choose names for your other functions.

::: devtools/client/framework/toolbox.js:564
(Diff revision 11)
> +  },
> +
> +  /**
> +  * Handler for split console button on the the toolbar
> +  */
> +  _splitConsoleOnButtonKeypress: function (event) {

Maybe rename to `_onSplitConsoleKeyPress`

::: devtools/client/framework/toolbox.js:571
(Diff revision 11)
> +  },
> +
> +  /**
> +  * Action that splits the console
> +  */
> +  _splitConsoleAction: function (event) {

Maybe rename to `_onSplitConsoleEvent`

::: devtools/client/framework/toolbox.js:634
(Diff revision 11)
> +  _addToolButtonListeners: function () {
> +    let consoleKey = this.doc.getElementById("command-button-splitconsole");
> +    consoleKey.addEventListener("keydown", this._splitConsoleOnButtonKeypress, false);
> +  },

So, why add keyboard support just for the splitconsole, and not for the other buttons in the toolbar?

I ask because I think it would be a lot simpler to manage keyboard events at the buttons level, not add code for it in toolbox.js.
Each button you see in the toolbar comes from a command-button file, in the splitconsole's case, it is: https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/console-commands.js#15

And, based on these commands, we then create the buttons in the toolbar.
This is done here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/developer-toolbar.js#75

So I'm thinking that we could maybe simply change this code to also listen to keyboard events. This way, it would work for all buttons, not just the splitconsole one, and it wouldn't require any code changes in toolbox.js.

Actually, after looking at the code more, it looks like the buttons are only appended here:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox.js#1038-1047
so you'd need to add the listener here too.

::: devtools/client/inspector/inspector-panel.js:12
(Diff revision 11)
>  /* eslint max-len: [2, 100, 2, {ignoreUrls: true, "ignorePattern": "\\s*require\\s*\\(|^\\s*loader\\.lazy|-\\*-"}] */ // eslint-disable-line
>  
>  "use strict";
>  
>  const {Cc, Ci} = require("chrome");
> +const { KeypressHelpers } = require("devtools/client/shared/keypress-helpers");

nit: in this file, other destructuring assignments do not have spaces after and before curly brackets. Like: `const {Cc, Ci} = require("chrome");`.
So, please be consistent with the rest of the file and use:
`const {KeypressHelpers} = require("...");`

::: devtools/client/inspector/inspector.xul:158
(Diff revision 11)
>          class="devtools-toolbar"
>          nowindowdrag="true">
>          <toolbarbutton id="inspector-element-add-button"
>            class="devtools-toolbarbutton"
>            tooltiptext="&inspectorAddNode.label;"
> -          oncommand="inspector.addNode()" />
> +          oncommand="inspector.addNode()"/>

nit: this seems like an unrelated whitespace change. Please revert it as it helps keeping mercurial history clean and easy to browse.

::: devtools/client/inspector/test/browser_inspector_pane-toggle-05.js:6
(Diff revision 11)
> +// Test that the inspector toggled panel is visible by default, is hidden after
> +// pressing space or the enter key on the toggle button and
> +// remains expanded/collapsed when switching hosts.

Please update this comment. See my other comments below about why.

::: devtools/client/inspector/test/browser_inspector_pane-toggle-05.js:15
(Diff revision 11)
> +add_task(function* () {
> +  let {toolbox, inspector} = yield openInspectorForURL("about:blank", "side");
> +  let panel = inspector.panelDoc.querySelector("#inspector-sidebar");
> +  let button = inspector.panelDoc.getElementById("inspector-pane-toggle");
> +
> +  ok(!panel.hasAttribute("pane-collapsed"), "The panel is in expanded state");

This is duplicated with other tests. This test shouldn't care about this, as this is tested elsewhere already. It should just focus the button and try to action it with enter and space.

::: devtools/client/inspector/test/browser_inspector_pane-toggle-05.js:17
(Diff revision 11)
> +  let panel = inspector.panelDoc.querySelector("#inspector-sidebar");
> +  let button = inspector.panelDoc.getElementById("inspector-pane-toggle");
> +
> +  ok(!panel.hasAttribute("pane-collapsed"), "The panel is in expanded state");
> +
> +  yield togglePane(button, "Press on the toggle button", panel);

You should pass the keycode as an argument to this function, so that you can call it again a second time with space. Indeed, here, you're only testing that return works.

::: devtools/client/inspector/test/browser_inspector_pane-toggle-05.js:22
(Diff revision 11)
> +  info("Switch the host to bottom type");
> +  yield toolbox.switchHost("bottom");
> +  ok(panel.hasAttribute("pane-collapsed"), "The panel is in collapsed state");
> +
> +  yield togglePane(button, "Press on the toggle button to expand the panel again", panel);
> +  ok(!panel.hasAttribute("pane-collapsed"), "The panel is in expanded state");

You're duplicating some logic from another test here. We don't need this test to do this, we only want this test to make sure that the space and enter keys actually do something when pressed on the toggle button. The rest is tested elsewhere already.

::: devtools/client/netmonitor/netmonitor-view.js:1150
(Diff revision 11)
> +   */
> +  sortByPress: function (type, event) {
> +    if (KeypressHelpers.checkForkey(event)) {
> +      this.sortBy(type);
> +    }
> +    // KeypressHelpers.onKeyboardButtonPress(event, this.sortBy, type);

If you don't need this line of code anymore, please remove it.

::: devtools/client/shared/keypress-helpers.js:10
(Diff revision 11)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +/**
> + * Helpers for creating and messaging between UI components.

This comment was copied from view-helpers.js which I guess you used as a base to create this new file. 
Please update this comment.

::: devtools/client/shared/keypress-helpers.js:12
(Diff revision 11)
> +"use strict";
> +
> +/**
> + * Helpers for creating and messaging between UI components.
> + */
> +const KeypressHelpers = exports.KeypressHelpers = {

I think `KeypressHelpers`, as a name, is too generic and doesn't really say what this does.
Even the module name is `keypress-helpers.js` which might lead people to believe that this contains general keyboard-related utilities.
However this really does 1 thing only, check if space or return was pressed and call a function.

Maybe adding the module with this name is fine, because we can add more general keyboard utilities in the future, but at least the 2 method names should be changed.

`checkForKey` might be better as `isSpaceOrReturn`.

I don't really understand why we need `onKeyboadButtonPress` in fact.
Everywhere you do something like
```
KeypressHelpers.onKeyboardButtonPress(event, this.doSomething);
```
you could do this instead:
```
if (KeypressHelpers.isSpaceOrReturn(event)) {
  this.doSomething(event);
}
```

So I'm not sure we should be adding a function just for this. It seems like an unnecessary level of complexity to just replace a if in the code.

So, if you remove it, then you'll end up with just `isSpaceOrReturn` in this new module, that just tests for space or return key codes.
I don't think we should be introducing a new module just for this. I know ntim suggested it, but I think he had in mind moving more code here.
Attachment #8754540 - Flags: review?(pbrosset)
In comment 20, I rather suggested a function that would make the button directly keyboard accessible :

makeKeyboardAccessible: function(button) {
  button.addEventListener("keypress", function() {
    // Check for the right keys...

  });
(In reply to Tim Nguyen :ntim from comment #24)
> In comment 20, I rather suggested a function that would make the button
> directly keyboard accessible :
> 
> makeKeyboardAccessible: function(button) {
>   button.addEventListener("keypress", function() {
>     // Check for the right keys...
> 
>   });

Hit enter too fast.

makeKeyboardAccessible: function(button) {
  button.addEventListener("keypress", function() {
    // Check for esc/enter
    // Trigger a click event on the button (new Event("click"))
  });
}

This way, you don't have to duplicate the logic everywhere.
A simple call like this: makeKeyboardAccessible(button); would fix make the button keyboard accessible, no extra keypress handler needed. This is what I suggested in comment 20, sorry if it wasn't clear. Patrick, what do you think ?
Flags: needinfo?(pbrosset)
Also, if you're worried about having a separate module for this, maybe the function could belong in view-helpers.js
Comment on attachment 8754540 [details]
Bug 1273211 - Added side panel enter and return controls, added keyboard space selection for controls in netmonitor and console key;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54026/diff/11-12/
Attachment #8754540 - Flags: review?(pbrosset)
(In reply to Tim Nguyen :ntim from comment #25)
> makeKeyboardAccessible: function(button) {
>   button.addEventListener("keypress", function() {
>     // Check for esc/enter
>     // Trigger a click event on the button (new Event("click"))
>   });
> }
> 
> This way, you don't have to duplicate the logic everywhere.
> A simple call like this: makeKeyboardAccessible(button); would fix make the
> button keyboard accessible, no extra keypress handler needed. This is what I
> suggested in comment 20, sorry if it wasn't clear. Patrick, what do you
> think ?
This looks good, but looking at the code in the patch, there are places where it might be weird to use it, like in the netmonitor-view.js file.
Also, how would you remove the event listener on destroy then?

(In reply to Tim Nguyen :ntim from comment #26)
> Also, if you're worried about having a separate module for this, maybe the
> function could belong in view-helpers.js
Yeah. that's a good idea. I'm not too comfortable with adding yet another module for something like this.
Flags: needinfo?(pbrosset)
Comment on attachment 8754540 [details]
Bug 1273211 - Added side panel enter and return controls, added keyboard space selection for controls in netmonitor and console key;

https://reviewboard.mozilla.org/r/54026/#review54280

This looks really good. I just have a few final comments so I'd like to do a final review after this, but then I think we should land the change.
Also, I noticed a bug (on windows, haven't tested on mac or linux): if you focus the split-console icon in the toolbar and press ENTER, then it works fine. But if you press SPACE, then the console opens and closes quickly (or closes and opens quickly if it was opened first).

::: devtools/client/framework/toolbox.js:135
(Diff revision 12)
>    this._toolUnregistered = this._toolUnregistered.bind(this);
>    this._refreshHostTitle = this._refreshHostTitle.bind(this);
>    this._toggleAutohide = this._toggleAutohide.bind(this);
>    this.selectFrame = this.selectFrame.bind(this);
>    this._updateFrames = this._updateFrames.bind(this);
> -  this._splitConsoleOnKeypress = this._splitConsoleOnKeypress.bind(this);
> +  this._onKeyPress = this._onKeyPress.bind(this);

Well, sorry to contradict myself with what I said in my last comment, but since you handled button keyboard events in developer-toolbar.js (which is great because it applies to all buttons), then you don't need to change anything in this file anymore.
In fact you should revert all changes in toolbox.js so it is unchanged in this patch.

::: devtools/client/netmonitor/netmonitor-view.js:28
(Diff revision 12)
>  const DevToolsUtils = require("devtools/shared/DevToolsUtils");
>  const {LocalizationHelper} = require("devtools/client/shared/l10n");
>  const {PrefsHelper} = require("devtools/client/shared/prefs");
>  const {ViewHelpers, Heritage, WidgetMethods, setNamedTimeout} =
>    require("devtools/client/shared/widgets/view-helpers");
> +const { KeypressHelpers } = require("devtools/client/shared/keypress-helpers");

nit: other imports in this file do not have spaces after/before curly braces in the destructuring assignment. Try and be consistent with the code style used in the file you are editing:

`const {KeypressHelpers} = require("....");`

::: devtools/client/netmonitor/netmonitor-view.js:447
(Diff revision 12)
>      this.widget.addEventListener("swap", this._onSwap, false);
>      this._splitter.addEventListener("mousemove", this._onResize, false);
>      window.addEventListener("resize", this._onResize, false);
>  
>      this.requestsMenuSortEvent = getKeyWithEvent(this.sortBy.bind(this));
> +    this.requestsMenuSortEventPress = getKeyWithEvent(this.sortByPress.bind(this));

Suggestion: don't introduce sortByPress and fitlerOnPress, they're not needed.
Instead, you could do this:

```
this.requestsMenuSortEvent = getKeyWithEvent(this.sortBy.bind(this));
this.requestsMenuSortKeyboardEvent = getKeyWithEvent(this.sortBy.bind(this), true);
this.requestsMenuFilterEvent = getKeyWithEvent(this.filterOn.bind(this));
this.requestsMenuFilterKeyboardEvent = getKeyWithEvent(this.filterOn.bind(this), true);
```

And then change getKeyWithEvent like this:

```
function getKeyWithEvent(callback, onlySpaceOrReturn) {
  return function (event) {
    let key = event.target.getAttribute("data-key");
    let filterKeyboardEvent = onlySpaceOrReturn
      ? KeypressHelpers.isSpaceOrReturn(event) : true
    if (key && filterKeyboardEvent) {
      callback.call(null, key);
    }
  };
}
```

This way, you avoid adding 2 new methods and just call sortBy and filterOn directly, but add the filtering logic in getKeyWithEvent.

::: devtools/client/netmonitor/netmonitor-view.js:3926
(Diff revision 12)
>   */
>  function getKeyWithEvent(callback) {
>    return function (event) {
>      let key = event.target.getAttribute("data-key");
>      if (key) {
> -      callback.call(null, key);
> +      callback.call(null, key, event);

Also, if you do what I suggested, no need to add this new event parameter here.

::: devtools/client/shared/keypress-helpers.js:13
(Diff revision 12)
> +
> +/**
> + * Helpers for checking space and return key presses
> + * and various keyboard utilities
> + */
> +const KeypressHelpers = exports.KeypressHelpers = {

I would like to go with Tim's suggestion in comment 26: please move this function to view-helpers.js, as a new method in the ViewHelpers object.
Attachment #8754540 - Flags: review?(pbrosset)
Comment on attachment 8754540 [details]
Bug 1273211 - Added side panel enter and return controls, added keyboard space selection for controls in netmonitor and console key;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54026/diff/12-13/
Attachment #8754540 - Attachment description: MozReview Request: Bug 1273211 - Added side panel enter and return controls, added keyboard space selection for controls in netmonitor and console key; r?pbro → Bug 1273211 - Added side panel enter and return controls, added keyboard space selection for controls in netmonitor and console key;
Attachment #8754540 - Flags: review?(pbrosset)
Comment on attachment 8754540 [details]
Bug 1273211 - Added side panel enter and return controls, added keyboard space selection for controls in netmonitor and console key;

https://reviewboard.mozilla.org/r/54026/#review58580

Looks good, thanks.
Please do address my final review comments and then feel free to land this once you have agreen TRY build.
Thanks!

::: devtools/client/framework/toolbox.js:96
(Diff revision 13)
>      }
>    },
>    { id: "command-button-splitconsole",
>      isTargetSupported: target => !target.isAddon },
> -  { id: "command-button-responsive" },
> +  { id: "command-button-responsive",
> +    isTargetSupported: target => !target.isAddon },

This change seems unrelated to this bug. You should revert this change.

::: devtools/client/inspector/inspector-panel.js:26
(Diff revision 13)
>  const {initCssProperties} = require("devtools/shared/fronts/css-properties");
>  const nodeConstants = require("devtools/shared/dom-node-constants");
>  
>  const Menu = require("devtools/client/framework/menu");
>  const MenuItem = require("devtools/client/framework/menu-item");
> +const {ViewHelpers} = require("devtools/client/shared/widgets/view-helpers");

Why did you remove the `loader.lazyRequireGetter` and added a `require` instead?
ViewHelpers is only used on user events, so it's best to lazy load it, i.e. you should revert this change and keep the lazyRequireGetter instead of the sync require.

::: devtools/client/netmonitor/netmonitor-view.js:4010
(Diff revision 13)
>   * present in event.target.
>   *
>   * @param function callback
>   *          Function to execute execute when data-key
>   *          is present in event.target.
>   * @return function

Please add a new `@param` in this jsdoc comment to explain what the `onlySpaceOrReturn` param does.

::: devtools/client/shared/widgets/view-helpers.js:228
(Diff revision 13)
> +    if (event.keyCode === event.DOM_VK_SPACE ||
> +          event.keyCode === event.DOM_VK_RETURN) {
> +      return true;
> +    }
> +    return false;

nit: this would be shorter written as:

```
isSpaceOrReturn: function (event) {
  return event.keyCode === event.DOM_VK_SPACE ||
         event.keyCode === event.DOM_VK_RETURN;
},
```
Attachment #8754540 - Flags: review?(pbrosset) → review+
Comment on attachment 8754540 [details]
Bug 1273211 - Added side panel enter and return controls, added keyboard space selection for controls in netmonitor and console key;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54026/diff/13-14/
Comment on attachment 8754540 [details]
Bug 1273211 - Added side panel enter and return controls, added keyboard space selection for controls in netmonitor and console key;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54026/diff/14-15/
Comment on attachment 8754540 [details]
Bug 1273211 - Added side panel enter and return controls, added keyboard space selection for controls in netmonitor and console key;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54026/diff/15-16/
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/352142c846c3
Add side panel enter and return controls, added keyboard space selection for controls in netmonitor and console key. r=pbro
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/352142c846c3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Changes related to the InspectorPanel & SidebarToggle seems to be incorrect in the patch.

There is new keydown event handler `onPaneToggleButtonPressed` implemented in InspectorPanel and passed into `SidebarToggle()` React component as a prop.  But, this component doesn't know anything about 'onKeyDown' prop and it isn't used.

It's also, unnecessary to register new keydown handler to the SidebarToggle button since it's based on <html:button> and handles pressing Space (32) and Return (13) by default. That's why the code works even if the new keydown event handler isn't actually used.

Or, am I missing something?
(if not, please file a bug and I can fix it)

Honza
Flags: needinfo?(pbrosset)
Bug 1287474 reported to cover comment #37

Thanks Nancy

Honza
Flags: needinfo?(pbrosset)
Depends on: 1208183
Depends on: 1327738
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: