Closed Bug 1268450 Opened 4 years ago Closed 4 years ago
.xul to use new key shortcut API
3.47 KB, patch
|Details | Diff | Splinter Review|
4.00 KB, patch
|Details | Diff | Splinter Review|
37.15 KB, patch
|Details | Diff | Splinter Review|
3.59 KB, patch
|Details | Diff | Splinter Review|
We need to stop using xul for key shortcuts and instead use the upcoming module from bug 1260493. http://mxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox.xul#31 There is also the edit menu keys, which are inserted via an overlay: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/editMenuOverlay.xul#31 I think we should address that separately as it is yet another story.
There is also toolbox-window.xul using xul keys: http://mxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox-window.xul#24 And the browser toolbox window using xul keys: http://mxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox-process-window.xul#29
This patch focuses on toolbox.xul only and ignore edit keys for now.
Priority: -- → P1
Whiteboard: [devtools-html][triage] → [devtools-html]
Rebased against last changes made to key-shortcut.js. Also fix it against characters like "[", with an additional test.
Attachment #8746542 - Attachment is obsolete: true
With various test fixes.
Attachment #8748255 - Attachment is obsolete: true
Some tests only fail on try, which slow down my progress here. https://treeherder.mozilla.org/#/jobs?repo=try&revision=1090227cb38e
For some reason, when running: devtools/client/inspector/markup/test/browser_markup_dragdrop_autoscroll.js The markup view is already scolled down. And that, only on try, not locally.
Attachment #8750386 - Attachment is obsolete: true
Any random guess why this test: http://mxr.mozilla.org/mozilla-central/source/devtools/client/inspector/markup/test/browser_markup_dragdrop_autoscroll.js?force=1 Would have an already scrolled markup view when running the test, that, only on try (so mostly due to slower code)? It seems to be related to attachment 8752181 [details] [diff] [review], but I have some other patches (the first 5 patches are already landed or known to not make that test fail): https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9b731fc5c90&selectedJob=20798628 I can only think of a slightly slower/faster MarkupView constructor which introduces weird races. The markup view seems to be looking good while the test times out: http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/bace35542a7f51c13fc121049e498c6fd6cd96ad9ac58f828f1ff2577b7181b3702f9697b4488f415c4e46c0f53ae7c13bb64874fee948e736b1292a4b144830 (so there isn't any major UI glitch) If you don't know, I'm running out of idea and I think I'll just ensure the markupview is scrolled to top when the test starts.
Wow, that's really odd. Looking at the patch, I don't see how this could happen. In the screenshot however, the font-size is huge, so big in fact that the <body> node (which is the selection), is below the fold. The markup-view auto-scrolls to the selected node I think: http://mxr.mozilla.org/mozilla-central/source/devtools/client/inspector/markup/markup.js#1053
bgrins is out of town this week, so I'm asking you for review. Bug 1260493 introduced this new API. This patch has no other dependency that is not landed. This helper modules supports the electron strings for key shortcuts: https://github.com/electron/electron/blob/master/docs/api/accelerator.md Here, I'm replacing all xul:key's. I've explicitely let the manual keydown listener for split console for a followup, as it appears to break many tests. These changes are quite straightforward. The only trick is the preventDefault in `_buildOptions:selectOptions`, to prevent Ctrl+Shift+O to open the bookmarks diaglog when opening the options panel. Also, with this new way of defining shortcuts, there is no need to disable zoom shortcuts, instead we just do not set them... The rest of the patch is test fixes due to removal of xul:key. Note that I have to keep toolbox-keyset for other xul:key usages. I'll try to get rid of it in followups.
Attachment #8752909 - Flags: review?(jryans)
Attachment #8752181 - Attachment is obsolete: true
I'll merge that in the previous patch, but kept it separated to help the review. In various places we display the shortcuts. So I exposed an helper method in the key shortcut module to do that, and start using it in toolbox.js
Attachment #8752910 - Flags: review?(jryans)
Same for this patch. We now have a synthesizeKeyShortcut, similar to synthesizeFromKeyTag. In the first version I didn't allowed targeting a precise window, but it is useful for some tests...
Attachment #8752911 - Flags: review?(jryans)
Comment on attachment 8752909 [details] [diff] [review] patch v4 Review of attachment 8752909 [details] [diff] [review]: ----------------------------------------------------------------- This seems good overall. However, zooming seems to have regressed: 1. Open toolbox to console tool 2. Click into console tool to focus it 3. Zoom in (Cmd-+ on Mac) With this patch, both the toolbox and the content zoom. Only the toolbox should zoom. ::: devtools/client/locales/en-US/toolbox.properties @@ +167,5 @@ > + > +# LOCALIZATION NOTE (toolbox.reload*.key) > +# Key shortcuts used to reload the page > +toolbox.reload.key=CmdOrCtrl+R > + Nit: remove this blank line
Attachment #8752909 - Flags: review?(jryans) → review-
Attachment #8752910 - Flags: review?(jryans) → review+
Attachment #8752911 - Flags: review?(jryans) → review+
Just added a couple of event.preventDefault in zoom*() functions. But I also need two additional patches against key-shortcuts module... Note that I'll meld into this patch the previous two you r+ (attachment 8752910 [details] [diff] [review] and attachment 8752911 [details] [diff] [review]). But I'll keep the next two patch separated while landing this bug.
Attachment #8753947 - Flags: review?(jryans)
Attachment #8752909 - Attachment is obsolete: true
azerty keyboard, like french ones... requires Shift/Capslock to be pressed to do a digit without a keypad. So the xul:key is doing some magic about this. It doesn't require you to actually have shift/capslock pressed. I'm just redoing this magic here, by matching by keycode. It appears that, even if the default character for the key isn't a digit, its keycode relates to the digit \o/ So I'm also matching against the keycode for digits! http://static.commentcamarche.net/ccm.net/faq/images/0-Vuskup12-azerty-s-.png See, for example 0. By default it will output "à", with shift, "0". To get the keycode for a given char, we can use charCodeAt "0".charCodeAt(0) == 48 So whenever you press "à"/"0" key, the event keycode will always be 48, no matter which modifiers you also (not) pressed.
Attachment #8753951 - Flags: review?(jryans)
It looks like I also got 'Plus'/'+' key support wrong. It is an edge case as Electron shortcut strings are splited by "+", like Ctrl+A, if you want to target the '+' key, you have to use "Plus" string. But as "Plus" is behind another key, behind "=". It requires Shift to be pressed. The keycode of this key is the one for "=", i.e. 61. So, this is a special case. Plus isn't a non-character key like all others. It should be considered as a character one.
Attachment #8753955 - Flags: review?(jryans)
Attachment #8753947 - Flags: review?(jryans) → review+
Comment on attachment 8753951 [details] [diff] [review] Accept loose digits for azerty keyboards - v1 Review of attachment 8753951 [details] [diff] [review]: ----------------------------------------------------------------- Does this make the behavior differ from Electron? I started to look at https://github.com/electron/electron/blob/e05804848f98959d63337aacd9047cd579c5b23f/atom/common/keyboard_util.cc but not sure if that's the right place... Does it matter if we differ?
Attachment #8753951 - Flags: review?(jryans) → review+
Comment on attachment 8753955 [details] [diff] [review] Fix 'Plus' key shortcut support - v1 Review of attachment 8753955 [details] [diff] [review]: ----------------------------------------------------------------- Seems like Electron uses special support here too.
Attachment #8753955 - Flags: review?(jryans) → review+
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #24) > Comment on attachment 8753951 [details] [diff] [review] > Accept loose digits for azerty keyboards - v1 > > Review of attachment 8753951 [details] [diff] [review]: > ----------------------------------------------------------------- > > Does this make the behavior differ from Electron? I started to look at > https://github.com/electron/electron/blob/ > e05804848f98959d63337aacd9047cd579c5b23f/atom/common/keyboard_util.cc but > not sure if that's the right place... > > Does it matter if we differ? I'm not sure we do, the final code is deep down into platform specifics into chromium... https://chromium.googlesource.com/chromium/src/+/lkgr/ui/events/keycodes/keyboard_code_conversion_mac.mm#719 It looks like that do something with shift and digits. Hard to tell exactly what happens. Did you actually tested something on electron? I don't think we care much about 1:1 behaviors. I'm still not convinced devtools would have to use any electron API. At the end we don't need more than just Web features: keydown! I went for the electron syntax mostly as it looks more modern than old xul way of doing.
Rebased + merged.
Attachment #8754576 - Flags: review+
Attachment #8754577 - Flags: review+
Attachment #8753951 - Attachment is obsolete: true
Attachment #8754578 - Flags: review+
Attachment #8753955 - Attachment is obsolete: true
Trees are closed, but this is ready to land.
Backed out for browser_keybindings_03.js timeouts. https://hg.mozilla.org/integration/fx-team/rev/c62705a833ff https://treeherder.mozilla.org/logviewer.html#?job_id=9437576&repo=fx-team#L13043
And browser_toolbox_window_reload_target.js. Might want to give this a more thorough Try run next time around. https://treeherder.mozilla.org/logviewer.html#?job_id=9437577&repo=fx-team#L5284
Oops I pushed to linux, but it seems to be specific to mac. I'll get back to this un tuesday.
Comment on attachment 8756308 [details] [diff] [review] tests fixes - v1 Review of attachment 8756308 [details] [diff] [review]: ----------------------------------------------------------------- Makes sense. On Mac, Cmd-Shift-D is "Bookmark all tabs..." when outside of DevTools.
Attachment #8756308 - Flags: review?(jryans) → review+
https://hg.mozilla.org/integration/fx-team/rev/9bed18498d9dfb6e544d63008e371ba45abe3557 Bug 1268450 - Convert toolbox key shortcut to stop using XUL. r=jryans https://hg.mozilla.org/integration/fx-team/rev/550359b138498145d266ac265d4c16eb6ae73c15 Bug 1268450 - Accept loose digits for azerty keyboards. r=jryans https://hg.mozilla.org/integration/fx-team/rev/df627da479f868c9704d7ac0b1c6aa76fb298150 Bug 1268450 - Fix 'Plus' key shortcut support. r=jryans
Had this somewhat green try before pushing to fxteam: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f98c1b6871bb&selectedJob=21434779
browser_toolbox_window_reload_target.js was trivial to fix, but browser_keybindings_03.js isn't. I thought it was a timeout, but it is a crash. Seems to be due to Cmd+Shift+D already doing something else opening a window which end up crashing when also toggling the toolbox host mode. Finally got a green try, now checking on all platforms.
Here is some test tweaks. I'm asking review especially for the preventDefault on Cmd+Shift+D. - Fix window_reload_target by targetting toolbox document when firing key events, - Fix bindings_03 by preventing even on mac which seems to doing something else on Mac and introduces a crash. - Strengthen bindings_03 by listening for host-changed first.
Attachment #8756308 - Flags: review?(jryans)
Attachment #8756310 - Flags: review+
Attachment #8754576 - Attachment is obsolete: true
Attachment #8756311 - Flags: review+
Attachment #8754577 - Attachment is obsolete: true
Verified fixed with latest Aurora 49.0a2, across platforms  - keyboard shortcuts are working as expected based on MDN document .  Windows 10 64-bit, Ubuntu 16.04 64-bit and Mac OS X 10.11.1  https://developer.mozilla.org/en-US/docs/Tools/Keyboard_shortcuts#Toolbox
You need to log in before you can comment on or make changes to this bug.