Alexandre Poirot [:ochameau] Assignee
	Description • 8 years ago

Instead of manually listening for DOM key event, we should use key-shortcuts module to listen for events:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/rules/rules.js#1502
https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/computed/computed.js#500

	Alexandre Poirot [:ochameau] Assignee
	Comment 1 • 8 years ago

I'll take this one, I think that's the last key-shortcut related work around the inspector.

	Alexandre Poirot [:ochameau] Assignee
	Comment 2 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d79723f2d53

	Alexandre Poirot [:ochameau] Assignee
	Comment 4 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea9712fc7bcc

	Alexandre Poirot [:ochameau] Assignee
	Comment 6 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7028546f14d

	Brian Grinstead [:bgrins]
	Comment 7 • 8 years ago

Comment on attachment 8758215 [details] [diff] [review]
rule view patch v2

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

LGTM generally, but browser_computed_keybindings_01.js and browser_computed_keybindings_02.js are failing on the try push (and might be more reason to do both tools at once)

::: devtools/client/inspector/rules/rules.js
@@ -192,5 @@
>    this.element.addEventListener("copy", this._onCopy);
>    this.element.addEventListener("contextmenu", this._onContextMenu);
>    this.addRuleButton.addEventListener("click", this._onAddRule);
>    this.searchField.addEventListener("input", this._onFilterStyles);
> -  this.searchField.addEventListener("keypress", this._onFilterKeyPress);

It's interesting that this was keypress before - I'm sure the computed view uses that also.  I can't remember the history behind that.  But it seems to work fine with the patch applied using only keydown

@@ +1484,5 @@
>      if (!event.target.closest("#sidebar-panel-ruleview")) {
>        return;
>      }
>  
> +    if (name == "CmdOrCtrl+F") {

Nit: === throughout

@@ +1491,5 @@
> +    } else if ((name == "Return" || name == "Space") &&
> +               this.element.classList.contains("non-interactive")) {
> +      event.preventDefault();
> +    } else if (name == "Escape" &&
> +               event.target.closest("#ruleview-searchbox") &&

I feel like this should be done at the same time as the computed view (either in this bug, or in a bug that's a quick follow-on).  Dince IIRC the key shortcut implementation for the filter boxes are identical.

	Alexandre Poirot [:ochameau] Assignee
	Comment 8 • 8 years ago

Just fixed the ===.
The try push from comment 6 if from a wip patch to convert the computed view...
Try push for this patch is green from comment 4.

	Alexandre Poirot [:ochameau] Assignee
	Comment 9 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dbd9f421e0a9

	Alexandre Poirot [:ochameau] Assignee
	Comment 10 • 8 years ago

Here is a tweak to key-shortcuts to allow listening to only one given dom node.
It helps supported the computed view.

	Brian Grinstead [:bgrins]
	Comment 12 • 8 years ago

Comment on attachment 8759101 [details] [diff] [review]
support targets in key-shortcuts - v1

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

::: devtools/client/shared/test/browser_key_shortcuts.js
@@ +351,5 @@
> +    window,
> +    target
> +  });
> +  let onKey = once(shortcuts, "0", (key, event) => {
> +    is(event.key, "0");

Should also check event.target here

	Brian Grinstead [:bgrins]
	Comment 13 • 8 years ago

Comment on attachment 8759093 [details] [diff] [review]
rule view patch v3

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

Still seeing == throughout this file

::: devtools/client/inspector/rules/rules.js
@@ +1490,5 @@
>        event.preventDefault();
> +    } else if ((name == "Return" || name == "Space") &&
> +               this.element.classList.contains("non-interactive")) {
> +      event.preventDefault();
> +    } else if (name == "Escape" &&

This could use the new 'target' option instead of relying on closest + the selector

	Brian Grinstead [:bgrins]
	Comment 14 • 8 years ago

(In reply to Brian Grinstead [:bgrins] from comment #13)
> Comment on attachment 8759093 [details] [diff] [review]
> rule view patch v3
> 
> Review of attachment 8759093 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Still seeing == throughout this file
> 
> ::: devtools/client/inspector/rules/rules.js
> @@ +1490,5 @@
> >        event.preventDefault();
> > +    } else if ((name == "Return" || name == "Space") &&
> > +               this.element.classList.contains("non-interactive")) {
> > +      event.preventDefault();
> > +    } else if (name == "Escape" &&
> 
> This could use the new 'target' option instead of relying on closest + the
> selector

Or use event.target === this.searchField that would be a little less code.  Don't really care either way as long as we don't end up having the hardcoded selector

	Brian Grinstead [:bgrins]
	Comment 15 • 8 years ago

Comment on attachment 8759103 [details] [diff] [review]
computed view patch v1

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

Nit: == should be === throughout

::: devtools/client/inspector/computed/computed.js
@@ +1103,5 @@
>          textContent: selector.source
>        });
>        link.addEventListener("click", selector.openStyleEditor, false);
> +      let shortcuts = new KeyShortcuts({
> +        window: this.tree.styleWindow,

Should window be required if target is passed? Looks like the only reason it'd be needed is so parseElectronKey can access window.KeyboardEvent, but that should be accessible from target.ownerDocument.defaultView.

	Alexandre Poirot [:ochameau] Assignee
	Comment 16 • 8 years ago

When for target === this.searchField

	Alexandre Poirot [:ochameau] Assignee
	Comment 18 • 8 years ago

> Should window be required if target is passed? Looks like the only reason
> it'd be needed is so parseElectronKey can access window.KeyboardEvent, but
> that should be accessible from target.ownerDocument.defaultView.

Yes. I can followup be converting all to only pass target which could be a window, document or element.
So that you would only pass a target. Makes sense?

	Brian Grinstead [:bgrins]
	Comment 19 • 8 years ago

(In reply to Alexandre Poirot [:ochameau] from comment #18)
> Created attachment 8759245 [details] [diff] [review]
> computed view patch v2
> 
> > Should window be required if target is passed? Looks like the only reason
> > it'd be needed is so parseElectronKey can access window.KeyboardEvent, but
> > that should be accessible from target.ownerDocument.defaultView.
> 
> Yes. I can followup be converting all to only pass target which could be a
> window, document or element.
> So that you would only pass a target. Makes sense?

Yes, that works

	Alexandre Poirot [:ochameau] Assignee
	Comment 20 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=490029c61934

	Alexandre Poirot [:ochameau] Assignee
	Comment 21 • 8 years ago

https://hg.mozilla.org/integration/fx-team/rev/6cb4ec731f499d21cd1409d421f29aa824ecafab
Bug 1271988 - Convert rule view to use key-shortcuts module. r=bgrins

https://hg.mozilla.org/integration/fx-team/rev/e5edc0ec18b9fdea1b830853441b9ccda4ea2014
Bug 1271988 - Support targeting a specific element on key shortcut module. r=bgrins

https://hg.mozilla.org/integration/fx-team/rev/10371d73bb08fa730e311885433bd147bf2a3576
Bug 1271988 - Convert computed view to use key-shortcuts module. r=bgrins

	Carsten Book [:Tomcat]
	Comment 22 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/6cb4ec731f49
https://hg.mozilla.org/mozilla-central/rev/e5edc0ec18b9
https://hg.mozilla.org/mozilla-central/rev/10371d73bb08

	Alexandre Poirot [:ochameau] Assignee
	Comment 23 • 8 years ago

Same thing over here. These shortcuts are not documented.

We may contribute to this page:
https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_CSS

Or contribute to this one and link to it:
https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/Keyboard_shortcuts

On the computed view you can do:
- Ctrl+F or Cmd+F(mac) to focus the searh field
- When the search field is focused: Escape will clear its content
- when a rule is selected, F1 is going to open a mdn page about this rule
- when a rule is selected, Return or Space is going to toggle the rule to see/hide more info
- if you manage to focus a css file name (select a rule, toggle it to see more info, pass tab to focus the file name) and press Return, it will open this file in the style editor

On the rule view you can do:
- Ctrl+F or Cmd+F(mac) to focus the searh field (same than computed)
- When the search field is focused: Escape will clear its content (same than computed)
- when you manage to focus some rule (select search field and hit tab 3 or more times) hit Enter or Space and it will start editing.

	Ada [:adalucinet]
	Comment 24 • 8 years ago

Verified with latest Aurora 49.0a2, across platforms [1]. 
Found 1 issue - when performing the following steps:
> - if you manage to focus a css file name (select a rule, toggle it to see
> more info, pass tab to focus the file name) and press Return, it will open
> this file in the style editor
_only_ under Mac OS X (with both latest 49.0a2 and 50.0a1) when pressing Tab the .css file is skipped, unable to gain focus. Any ideas?


[1] Windows 10 64-bit, Ubuntu 16.04 64-bit and Mac OS X 10.11.1

	Brian Grinstead [:bgrins]
	Comment 25 • 8 years ago

(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #24)
> Verified with latest Aurora 49.0a2, across platforms [1]. 
> Found 1 issue - when performing the following steps:
> > - if you manage to focus a css file name (select a rule, toggle it to see
> > more info, pass tab to focus the file name) and press Return, it will open
> > this file in the style editor
> _only_ under Mac OS X (with both latest 49.0a2 and 50.0a1) when pressing Tab
> the .css file is skipped, unable to gain focus. Any ideas?
> 
> 
> [1] Windows 10 64-bit, Ubuntu 16.04 64-bit and Mac OS X 10.11.1

Was this a problem before the patches?  I'm unable to focus the file names even in a 48 dev edition build on mac.

This might just be OSX not wanting to focus certain elements.  If you switch to full keyboard access in: Preferences -> Keyboard -> Shortcuts -> Full Keyboard Access -> All controls does it begin to work?

	Alexandre Poirot [:ochameau] Assignee
	Comment 26 • 8 years ago

Alexandra, It would be great to confirm if that's related to this patch and see if Brian's trick in OSX preferences changes anything. I don't have a Mac off hand to easily check that.

I'm wondering if we should keep this cryptic feature. I imagine it was introduce for accessibility? It would be great to have some feedback from a11y expert to confirm it is any useful.

	Ada [:adalucinet]
	Comment 27 • 8 years ago

(In reply to Alexandre Poirot [:ochameau] from comment #26)
> Alexandra, It would be great to confirm if that's related to this patch and
> see if Brian's trick in OSX preferences changes anything. I don't have a Mac
> off hand to easily check that.

Now, this issue is reproducible across platforms [1] with latest builds and plus the Aurora 49.0a2 build mentioned in comment 24. Any ideas what could have happen here? thanks in advance!

[1] Windows 10 64-bit, Ubuntu 16.04 64-bit and Mac OS X 10.11.1

	Julian Descottes [:jdescottes]
	Comment 28 • 8 years ago

Alexandra: Alex is on PTO so I'll try to handle it from here. I have not been able to focus the css filename on any version between release and nightly, using OSX and Linux. 

Looking at the markup the CSS filename is not a focusable element, so could you post specific STRs to be able to reproduce the issue here?

Thanks!

	Ada [:adalucinet]
	Comment 29 • 8 years ago

(In reply to Julian Descottes [:jdescottes] from comment #28)
> Alexandra: Alex is on PTO so I'll try to handle it from here. I have not
> been able to focus the css filename on any version between release and
> nightly, using OSX and Linux. 
> 
> Looking at the markup the CSS filename is not a focusable element, so could
> you post specific STRs to be able to reproduce the issue here?
> 
> Thanks!

As I mentioned in comment 27, this behavior is no longer reproducible for me. Couldn't figure it out why, but i think we are safe to call this report verified. If this issue will pop up in the future, I'll file a report with detailed STR. Thanks, Julian!

	Chris Mills [:cmills]
	Comment 30 • 8 years ago

(In reply to Alexandre Poirot [:ochameau] PTO, back on 1st from comment #23)
> Same thing over here. These shortcuts are not documented.
> 
> We may contribute to this page:
> https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/
> Examine_and_edit_CSS
> 
> Or contribute to this one and link to it:
> https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/
> Keyboard_shortcuts
> 
> On the computed view you can do:
> - Ctrl+F or Cmd+F(mac) to focus the searh field
> - When the search field is focused: Escape will clear its content
> - when a rule is selected, F1 is going to open a mdn page about this rule
> - when a rule is selected, Return or Space is going to toggle the rule to
> see/hide more info
> - if you manage to focus a css file name (select a rule, toggle it to see
> more info, pass tab to focus the file name) and press Return, it will open
> this file in the style editor
> 
> On the rule view you can do:
> - Ctrl+F or Cmd+F(mac) to focus the searh field (same than computed)
> - When the search field is focused: Escape will clear its content (same than
> computed)
> - when you manage to focus some rule (select search field and hit tab 3 or
> more times) hit Enter or Space and it will start editing.

I believe I've documented all these shortcuts successfully now. I have added them all to the table at

https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/Keyboard_shortcuts#CSS_pane

I've also updated the howto page to make sure they are all mentioned in context, as both docs are useful at different times. I also made sure there was a link to the shortcut reference.

https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_CSS 

Let me know if that looks ok to you. Thanks!