Closed Bug 1271988 Opened 4 years ago Closed 3 years ago

Convert computed and rule views to keyboard shortcut module

Categories

(DevTools :: Inspector, defect, P1)

defect

Tracking

(firefox49 verified)

VERIFIED FIXED
Firefox 49
Iteration:
49.3 - Jun 6
Tracking Status
firefox49 --- verified

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [devtools-html])

Attachments

(3 files, 5 obsolete files)

Whiteboard: [devtools-html] [triage]
Flags: qe-verify+
Priority: -- → P2
QA Contact: alexandra.lucinet
Whiteboard: [devtools-html] [triage] → [devtools-html]
I'll take this one, I think that's the last key-shortcut related work around the inspector.
Assignee: nobody → poirot.alex
Status: NEW → ASSIGNED
Iteration: --- → 49.3 - Jun 6
Priority: P2 → P1
Attached patch rule view patch v1 (obsolete) — Splinter Review
Attached patch rule view patch v2 (obsolete) — Splinter Review
Attachment #8758215 - Flags: review?(bgrinstead)
Attachment #8757998 - Attachment is obsolete: true
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.
Attachment #8758215 - Flags: review?(bgrinstead)
Summary: Convert computed and rule views to keyboard shorcut module → Convert computed and rule views to keyboard shortcut module
Attached patch rule view patch v3 (obsolete) — Splinter Review
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.
Attachment #8759093 - Flags: review?(bgrinstead)
Attachment #8758215 - Attachment is obsolete: true
Here is a tweak to key-shortcuts to allow listening to only one given dom node.
It helps supported the computed view.
Attachment #8759101 - Flags: review?(bgrinstead)
Attached patch computed view patch v1 (obsolete) — Splinter Review
Attachment #8759103 - Flags: review?(bgrinstead)
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
Attachment #8759101 - Flags: review?(bgrinstead) → review+
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
Attachment #8759093 - Flags: review?(bgrinstead)
(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
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.
Attachment #8759103 - Flags: review?(bgrinstead) → review+
When for target === this.searchField
Attachment #8759241 - Flags: review?(bgrinstead)
Attachment #8759093 - Attachment is obsolete: true
Attachment #8759101 - Attachment is obsolete: true
> 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?
Attachment #8759245 - Flags: review+
Attachment #8759103 - Attachment is obsolete: true
(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
Attachment #8759241 - Flags: review?(bgrinstead) → review+
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
https://hg.mozilla.org/mozilla-central/rev/6cb4ec731f49
https://hg.mozilla.org/mozilla-central/rev/e5edc0ec18b9
https://hg.mozilla.org/mozilla-central/rev/10371d73bb08
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
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.
Keywords: dev-doc-needed
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
QA Whiteboard: [qe-dthtml]
Flags: needinfo?(poirot.alex)
(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?
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.
Flags: needinfo?(poirot.alex) → needinfo?(alexandra.lucinet)
(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
Flags: needinfo?(alexandra.lucinet) → needinfo?(poirot.alex)
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!
Flags: needinfo?(poirot.alex) → needinfo?(alexandra.lucinet)
(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!
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: needinfo?(alexandra.lucinet)
(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!
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.