589375 - Inspector: style panel keyboard access

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Kevin Dangoor]

The Inspector Style panel should be navigable by keyboard.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

This was working before the UI refresh but the style inspector is now completely unusable via the keyboard.

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Keyboard access

I don't see any tests for keyboard access anywhere so hopefully we don't need to create any. Basically, Marco Zehe said that we need to be able to use the style inspector without using the mouse and now you can.

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Keyboard access WIP

Of course we need a test ... still struggling to focus elements in the style panel.

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Keybindings with test

Keys:
<tab> to navigate
<space> or <enter> expand / collapse property
When property name is focused ? takes you to the MDN help page

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I have just realized that I am not removing the focus & blur listeners on test finish. I won't have access to a PC to change this until Monday so anybody that feels the urge ... please go ahead ;o)

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Keybindings with test

Event listeners are now removed.

Comment 7

[Mihai Sucan [:msucan]]

Comment on attachment 574300 [details] [diff] [review]
Keybindings with test

Review of attachment 574300 [details] [diff] [review]:
-----------------------------------------------------------------

Patch looks good, thank you! 

The new test fails on my system:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser/browser_bug589375_keybindings.js | expander is focused
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser/browser_bug589375_keybindings.js | rules Table is populated - Didn't expect , but got it
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser/browser_bug589375_keybindings.js | rules Table is populated - Didn't expect , but got it
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser/browser_bug589375_keybindings.js | property name is focused
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser/browser_bug589375_keybindings.js | Test timed out
make: *** [mochitest-browser-chrome] Erreur 1

Giving the patch r- for the failures.

Marking optional suggestions/non-review comments with an * (asterisk) at the start.

*Usability concerns:

- In addition to / instead of Shift-? for loading the MDN link I would suggest F1. People are more likely to press F1 rather than Shift-?.

- The expander and the property name are both focusable when one uses Tab. This quickly becomes a usability problem. Why not have only the property name (or just the expander) focus? There's no benefit from being able to focus each individually, and it makes tabbing through props slower.

More comments below. Looking forward for the updated patch, thanks!

::: browser/devtools/styleinspector/CssHtmlTree.jsm
@@ +629,5 @@
>      this.propertyHeader.appendChild(this.matchedExpander);
>      this.matchedExpander.setAttribute("class", "match expander");
> +    this.matchedExpander.setAttribute("tabindex", "0");
> +    this.matchedExpander.addEventListener("keydown", function(aEvent) {
> +      if (aEvent.keyCode == 13 || aEvent.keyCode == 32) {

Please do not use magic numbers.

Use constants like Ci.nsIDOMKeyboardEvent.DOM_VK_RETURN and so on.

@@ +643,5 @@
> +    name.addEventListener("keydown", function(aEvent) {
> +      if (aEvent.shiftKey && aEvent.keyCode == 191) {
> +        this.mdnLinkClick();
> +      }
> +      if (aEvent.keyCode == 13 || aEvent.keyCode == 32) {

Same as above.

::: browser/devtools/styleinspector/csshtmltree.xul
@@ +48,5 @@
>    <!ATTLIST li foreach CDATA #IMPLIED>
>    <!ATTLIST div foreach CDATA #IMPLIED>
>    <!ATTLIST loop foreach CDATA #IMPLIED>
>    <!ATTLIST a target CDATA #IMPLIED>
> +  <!ATTLIST a target CDATA #IMPLIED>

Why do you duplicate this line?

::: browser/devtools/styleinspector/test/browser/browser_bug589375_keybindings.js
@@ +12,5 @@
> +function createDocument()
> +{
> +  doc.body.innerHTML = '<style type="text/css"> ' +
> +    '.matches {color: #F00;}</style>' +
> +    '<span id="matches" class="matches">Some styled text</span>' +

Is the id needed?

@@ +54,5 @@
> +
> +  info("checking keybindings");
> +
> +  let iframe = stylePanel.iframe;
> +  let searchbar = iframe.contentDocument.querySelector(".searchfield");

let searchbar = stylePanel.cssHtmlTree.searchField;

@@ +109,5 @@
> +function checkHelpLinkKeybinding()
> +{
> +  info("checking help link keybinding");
> +  let iframe = stylePanel.iframe;
> +  let visibleProperty = iframe.contentDocument.querySelector(".property-view");

*Why not get the element from the PropertyView instance found inside the CssHtmlTree instance?

@@ +139,5 @@
> +      }
> +    },
> +  };
> +  gBrowser.addProgressListener(helpLinkOnProgress);
> +  EventUtils.synthesizeKey("?", {shiftKey: true}, iframe.contentWindow);

This is prone to testing errors. Probably it may be simpler to just check if mdnLinkClick() was called by the keyboard event handler - you just overwrie the method with an implementation provided by the test. See, for example, how browser_scratchpad_ui.js works.

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Keybindings with test 2

(In reply to Mihai Sucan [:msucan] from comment #7)
> Comment on attachment 574300 [details] [diff] [review] [diff] [details] [review]
> Keybindings with test
> 
> Review of attachment 574300 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> Patch looks good, thank you! 
> 
> The new test fails on my system:
> 
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/devtools/styleinspector/test/
> browser/browser_bug589375_keybindings.js | expander is focused
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/devtools/styleinspector/test/
> browser/browser_bug589375_keybindings.js | rules Table is populated - Didn't
> expect , but got it
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/devtools/styleinspector/test/
> browser/browser_bug589375_keybindings.js | rules Table is populated - Didn't
> expect , but got it
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/devtools/styleinspector/test/
> browser/browser_bug589375_keybindings.js | property name is focused
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/devtools/styleinspector/test/
> browser/browser_bug589375_keybindings.js | Test timed out
> make: *** [mochitest-browser-chrome] Erreur 1
> 
> Giving the patch r- for the failures.
> 

Still works fine for me ... because we are checking keybindings here focus is very important, could you have changed the focus when running the tests? This test has now been changed quite a bit due to this review so hopefully it will work for you now.

> Marking optional suggestions/non-review comments with an * (asterisk) at the
> start.
> 
> *Usability concerns:
> 
> - In addition to / instead of Shift-? for loading the MDN link I would
> suggest F1. People are more likely to press F1 rather than Shift-?.
> 

Changed it to F1

> - The expander and the property name are both focusable when one uses Tab.
> This quickly becomes a usability problem. Why not have only the property
> name (or just the expander) focus? There's no benefit from being able to
> focus each individually, and it makes tabbing through props slower.
> 

Changed to just tab through property names

> More comments below. Looking forward for the updated patch, thanks!
> 
> ::: browser/devtools/styleinspector/CssHtmlTree.jsm
> @@ +629,5 @@
> >      this.propertyHeader.appendChild(this.matchedExpander);
> >      this.matchedExpander.setAttribute("class", "match expander");
> > +    this.matchedExpander.setAttribute("tabindex", "0");
> > +    this.matchedExpander.addEventListener("keydown", function(aEvent) {
> > +      if (aEvent.keyCode == 13 || aEvent.keyCode == 32) {
> 
> Please do not use magic numbers.
> 
> Use constants like Ci.nsIDOMKeyboardEvent.DOM_VK_RETURN and so on.
> 

Aha, of course we have these constants? Done.

> @@ +643,5 @@
> > +    name.addEventListener("keydown", function(aEvent) {
> > +      if (aEvent.shiftKey && aEvent.keyCode == 191) {
> > +        this.mdnLinkClick();
> > +      }
> > +      if (aEvent.keyCode == 13 || aEvent.keyCode == 32) {
> 
> Same as above.
> 

Done

> ::: browser/devtools/styleinspector/csshtmltree.xul
> @@ +48,5 @@
> >    <!ATTLIST li foreach CDATA #IMPLIED>
> >    <!ATTLIST div foreach CDATA #IMPLIED>
> >    <!ATTLIST loop foreach CDATA #IMPLIED>
> >    <!ATTLIST a target CDATA #IMPLIED>
> > +  <!ATTLIST a target CDATA #IMPLIED>
> 
> Why do you duplicate this line?
> 

I never changed it ... must have been some patch funkiness.

> :::
> browser/devtools/styleinspector/test/browser/browser_bug589375_keybindings.js
> @@ +12,5 @@
> > +function createDocument()
> > +{
> > +  doc.body.innerHTML = '<style type="text/css"> ' +
> > +    '.matches {color: #F00;}</style>' +
> > +    '<span id="matches" class="matches">Some styled text</span>' +
> 
> Is the id needed?
> 

Nope, removed

> @@ +54,5 @@
> > +
> > +  info("checking keybindings");
> > +
> > +  let iframe = stylePanel.iframe;
> > +  let searchbar = iframe.contentDocument.querySelector(".searchfield");
> 
> let searchbar = stylePanel.cssHtmlTree.searchField;
> 
> @@ +109,5 @@
> > +function checkHelpLinkKeybinding()
> > +{
> > +  info("checking help link keybinding");
> > +  let iframe = stylePanel.iframe;
> > +  let visibleProperty = iframe.contentDocument.querySelector(".property-view");
> 
> *Why not get the element from the PropertyView instance found inside the
> CssHtmlTree instance?
> 

Done. Now that we are working via propertyView instances we need to save the property names node but this is useful for testing purposes.

> @@ +139,5 @@
> > +      }
> > +    },
> > +  };
> > +  gBrowser.addProgressListener(helpLinkOnProgress);
> > +  EventUtils.synthesizeKey("?", {shiftKey: true}, iframe.contentWindow);
> 
> This is prone to testing errors. Probably it may be simpler to just check if
> mdnLinkClick() was called by the keyboard event handler - you just overwrie
> the method with an implementation provided by the test. See, for example,
> how browser_scratchpad_ui.js works.

True, I didn't realize this until MDN was down yesterday. Done.

Comment 9

[Mihai Sucan [:msucan]]

Comment on attachment 574569 [details] [diff] [review]
Keybindings with test 2

Review of attachment 574569 [details] [diff] [review]:
-----------------------------------------------------------------

Patch looks good. Thanks for your fixes!

I would give the patch r+, but the test still fails:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser/browser_bug589375_keybindings.js | property name is focused
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser/browser_bug589375_keybindings.js | rules Table is populated - Didn't expect , but got it
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser/browser_bug589375_keybindings.js | rules Table is populated - Didn't expect , but got it
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser/browser_bug589375_keybindings.js | MDN link will be shown - Got false, expected true

This will probably fail on the try server as well.

Non-review comments:
- in the propertyHeaderClick() method maybe it would be appropriate to do this.nameNode.focus(). This would make the property name focused so I can continue to Tab from the property I clicked on - even if I don't click the name. Currently if I click the computed value, or if I click the expander, no focus change happens.
- the question mark should show on :focus as well, not just :hover - but I am not sure if everyone would agree with this change. I noticed I have "weird"/different tastes on such matters. :) (so please ping others or do as you see fit - let it as is, or add :focus).

Looking forward for the test fixes! Thanks!

::: browser/devtools/styleinspector/test/browser/browser_bug589375_keybindings.js
@@ +65,5 @@
> +  info("focusing window");
> +  stylePanel.window.focus();
> +
> +  info("focusing search field");
> +  searchbar.select();

shouldn't this be searchbar.focus()?

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Timing issues now fixed

(In reply to Mihai Sucan [:msucan] from comment #9)
> Comment on attachment 574569 [details] [diff] [review] [diff] [details] [review]
> Keybindings with test 2
> 
> Review of attachment 574569 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> Patch looks good. Thanks for your fixes!
> 
> I would give the patch r+, but the test still fails:
> 
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/devtools/styleinspector/test/
> browser/browser_bug589375_keybindings.js | property name is focused
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/devtools/styleinspector/test/
> browser/browser_bug589375_keybindings.js | rules Table is populated - Didn't
> expect , but got it
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/devtools/styleinspector/test/
> browser/browser_bug589375_keybindings.js | rules Table is populated - Didn't
> expect , but got it
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/devtools/styleinspector/test/
> browser/browser_bug589375_keybindings.js | MDN link will be shown - Got
> false, expected true
> 
> This will probably fail on the try server as well.
> 

I have [hopefully] fixed the timing issues now.

> Non-review comments:
> - in the propertyHeaderClick() method maybe it would be appropriate to do
> this.nameNode.focus(). This would make the property name focused so I can
> continue to Tab from the property I clicked on - even if I don't click the
> name. Currently if I click the computed value, or if I click the expander,
> no focus change happens.

Done ... there is obviously now a dotted outline on click that I believe we cannot remove for accessibility reasons.

> - the question mark should show on :focus as well, not just :hover - but I
> am not sure if everyone would agree with this change. I noticed I have
> "weird"/different tastes on such matters. :) (so please ping others or do as
> you see fit - let it as is, or add :focus).
> 

I tried this and it was just strange to see the ? appear on focus, I will leave it as it is.

> Looking forward for the test fixes! Thanks!
> 
> :::
> browser/devtools/styleinspector/test/browser/browser_bug589375_keybindings.js
> @@ +65,5 @@
> > +  info("focusing window");
> > +  stylePanel.window.focus();
> > +
> > +  info("focusing search field");
> > +  searchbar.select();
> 
> shouldn't this be searchbar.focus()?

Done

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 574836 [details] [diff] [review]
Timing issues now fixed

Patch fails on try due to panel focus issues. I wish it didn't work locally as it would make it easier to fix.

Comment 12

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Timing issues now fixed

I suspect that the test failures on try were caused by somebody elses bug ... in any case it works fine now.

Comment 13

[Mihai Sucan [:msucan]]

Does this patch depend on some other patch? It fails to apply cleanly and I see some code is missing.

Comment 14

[Mihai Sucan [:msucan]]

Comment on attachment 577876 [details] [diff] [review]
Timing issues now fixed

Review of attachment 577876 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me, r+!

Thanks for your test fixes. All tests pass now!

Comment 15

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 577876: Timing issues now fixed

Rebased

Comment 16

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/3c3f90925e20

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/3c3f90925e20