Change precise increment to Ctrl instead of Alt for Linux compat

RESOLVED FIXED in Firefox 60

Status

P3
normal
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: jryans, Assigned: abhinav.koppula)

Tracking

({dev-doc-complete})

Trunk
Firefox 60
dev-doc-complete

Firefox Tracking Flags

(firefox58 wontfix, firefox59 wontfix, firefox60 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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
Comment hidden (mozreview-request)
(Assignee)

Comment 2

a year ago
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?
(Reporter)

Updated

a year ago
Keywords: dev-doc-needed
(Reporter)

Comment 3

a year ago
mozreview-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

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 hidden (mozreview-request)
(Assignee)

Comment 5

a year ago
mozreview-review-reply
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?
(Reporter)

Comment 6

a year ago
mozreview-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/#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)

Updated

a year ago
Assignee: nobody → abhinav.koppula
Status: NEW → ASSIGNED
(Reporter)

Comment 8

a year ago
Great, appears to work on Windows 10.  I'll push the land button in MozReview.  Thanks for working on this!

Comment 9

a year ago
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
Last Resolved: a year ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
status-firefox58: affected → wontfix
status-firefox59: --- → wontfix
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)
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 12

a year ago
Yes, looks fine to me.
Flags: needinfo?(abhinav.koppula)

Updated

9 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.