1413314 - Change precise increment to Ctrl instead of Alt for Linux compat

Status: RESOLVED FIXED

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

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 1

[Abhinav Koppula]

Attachment: Bug 1413314 - Change precise increment to Ctrl instead of Alt for Linux compat;

Review commit: https://reviewboard.mozilla.org/r/215976/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215976/

Comment 2

[Abhinav Koppula]

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 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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 {
  ...
}
```

Comment 4

[Abhinav Koppula]

Comment on attachment 8945870 [details]
Bug 1413314 - Change precise increment to Ctrl instead of Alt for Linux compat;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215976/diff/1-2/

Comment 5

[Abhinav Koppula]

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 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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?

Comment 7

[Abhinav Koppula]

TRY push on all 3 platforms:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a1e34f6f87f931c52c600c9d614d4042ced50a4

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ec90dfc15636
Change precise increment to Ctrl instead of Alt for Linux compat; r=jryans

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/ec90dfc15636

Comment 11

[Chris Mills [:cmills]]

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!

Comment 12

[Abhinav Koppula]

Yes, looks fine to me.