Closed Bug 1413314 Opened 6 years ago Closed 6 years ago

Change precise increment to Ctrl instead of Alt for Linux compat

Categories

(DevTools :: Inspector: Rules, enhancement, P3)

enhancement

Tracking

(firefox58 wontfix, firefox59 wontfix, firefox60 fixed)

RESOLVED FIXED
Firefox 60
Tracking Status
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: jryans, Assigned: abhinav.koppula)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

We allow "precise" increment / decrement of values in the rules view using Alt-Up and Alt-Down.

This works well on Windows and macOS.

On Linux though (or at least with Ubuntu 16.04 LTS), pressing Alt changes focus to the window manager, so the shortcut doesn't work there.

(It seems like we have tests[1] that work on Linux, but I assume it's because we synthesize the event within Gecko only, so the window manager never sees it.)

Proposal:

We could change this shortcut to Ctrl-Up / Ctrl-Down instead.  This should work across all platforms, but it would mess with any muscle memory people may have for the old shortcut.

[1]: http://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/test/browser_rules_edit-property-increments.js
Priority: -- → P3
Hi Ryan,
This issue has been troubling me for some time since I am on Ubuntu.
I've given this a shot. Can you review my patch?
Comment on attachment 8945870 [details]
Bug 1413314 - Change precise increment to Ctrl instead of Alt for Linux compat;

https://reviewboard.mozilla.org/r/215976/#review221790

Thanks for looking into this! :)

I think, unfortunately, we'll need to make this OS-specific: on macOS, for example, Ctrl-Up Arrow is the default shortcut to show "Mission Control" (the view of all open windows).

So, let's try the following:

* On macOS, use Alt like we do now
* On Windows and Linux, change to Ctrl like you propose

To check the current OS, you can use the system module:

```
loader.lazyRequireGetter(this, "system", "devtools/shared/system");

...

if (system.constants.platform === "macosx") {
  ...
} else {
  ...
}
```
Attachment #8945870 - Flags: review?(jryans) → review-
Comment on attachment 8945870 [details]
Bug 1413314 - Change precise increment to Ctrl instead of Alt for Linux compat;

https://reviewboard.mozilla.org/r/215976/#review221790

Hi Ryan,
I've made the changes. Does it look fine now?
Comment on attachment 8945870 [details]
Bug 1413314 - Change precise increment to Ctrl instead of Alt for Linux compat;

https://reviewboard.mozilla.org/r/215976/#review222106

Thanks for working on this! :) I'd like to double-check that it works on Windows as well.  Could you push this to try so we can check Windows build before landing?
Attachment #8945870 - Flags: review?(jryans) → review+
Assignee: nobody → abhinav.koppula
Status: NEW → ASSIGNED
Great, appears to work on Windows 10.  I'll push the land button in MozReview.  Thanks for working on this!
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ec90dfc15636
Change precise increment to Ctrl instead of Alt for Linux compat; r=jryans
https://hg.mozilla.org/mozilla-central/rev/ec90dfc15636
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
I've documented this:

Description of new behavior added to 
https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_CSS#Edit_rules

Note about updated shortcut added to
https://developer.mozilla.org/en-US/docs/Tools/Keyboard_shortcuts#CSS_pane

Note added to Fx60 release notes:
https://developer.mozilla.org/en-US/Firefox/Releases/60#Developer_tools

Let me know if this looks OK. Thanks!
Flags: needinfo?(abhinav.koppula)
Yes, looks fine to me.
Flags: needinfo?(abhinav.koppula)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.