Closed Bug 1268450 Opened 9 years ago Closed 9 years ago

Convert toolbox.xul to use new key shortcut API

Categories

(DevTools :: Framework, defect, P1)

Product:
DevTools
Component:
Framework
Type:
defect

Tracking

(firefox49 verified)

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

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

(Whiteboard: [devtools-html])

Attachments

(4 files, 12 obsolete files)

Fix 'Plus' key shortcut support.
(deleted), patch
ochameau
: review+
Details | Diff | Splinter Review
tests fixes - v1
(deleted), patch
jryans
: review+
Details | Diff | Splinter Review
patch v7
(deleted), patch
ochameau
: review+
Details | Diff | Splinter Review
Accept loose digits for azerty keyboards.
(deleted), patch
ochameau
: review+
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.
Whiteboard: [devtools-html][triage]
Attached patch toolbox.xul - patch v1 (obsolete) (deleted) — — Splinter Review
This patch focuses on toolbox.xul only and ignore edit keys for now.
Blocks: devtools-html-1
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [devtools-html][triage] → [devtools-html]
Attached patch toolbox.xul - patch v2 (obsolete) (deleted) — — Splinter Review
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
Flags: qe-verify? → qe-verify+
QA Contact: alexandra.lucinet
Assignee: nobody → poirot.alex
Status: NEW → ASSIGNED
Iteration: --- → 49.1 - May 9
Attached patch toolbox.xul - patch v3 (obsolete) (deleted) — — Splinter Review
With various test fixes.
Attachment #8748255 - Attachment is obsolete: true
Iteration: 49.1 - May 9 → 49.2 - May 23
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.
Attached patch patch v3 (obsolete) (deleted) — — Splinter Review
Rebased.
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.
Flags: needinfo?(pbrosset)
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
Flags: needinfo?(pbrosset)
Attached patch patch v4 (obsolete) (deleted) — — Splinter Review
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)
Attached patch fix target window for synthesizeKeyShortcut (obsolete) (deleted) — — Splinter Review
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-
Attached patch patch v5 (obsolete) (deleted) — — Splinter 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
Attached patch Accept loose digits for azerty keyboards - v1 (obsolete) (deleted) — — Splinter Review
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)
Attached patch Fix 'Plus' key shortcut support - v1 (obsolete) (deleted) — — Splinter Review
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)
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.
Attached patch patch v6 (obsolete) (deleted) — — Splinter Review
Rebased + merged.
Attachment #8754576 - Flags: review+
Attachment #8752910 - Attachment is obsolete: true
Attachment #8752911 - Attachment is obsolete: true
Attachment #8753947 - Attachment is obsolete: true
Attached patch Accept loose digits for azerty keyboards. (obsolete) (deleted) — — Splinter Review
Rebased.
Attachment #8754577 - Flags: review+
Attachment #8753951 - Attachment is obsolete: true
Rebased.
Attachment #8754578 - Flags: review+
Attachment #8753955 - Attachment is obsolete: true
Trees are closed, but this is ready to land.
Keywords: checkin-needed
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+
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.
Attached patch tests fixes - v1 (deleted) — — Splinter Review
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)
Attached patch patch v7 (deleted) — — Splinter Review
Fix eslint.
Attachment #8756310 - Flags: review+
Attachment #8754576 - Attachment is obsolete: true
Fix eslint.
Attachment #8756311 - Flags: review+
Attachment #8754577 - Attachment is obsolete: true
Iteration: 49.2 - May 23 → 49.3 - Jun 6
Verified fixed with latest Aurora 49.0a2, across platforms [1] - keyboard shortcuts are working as expected based on MDN document [2]. [1] Windows 10 64-bit, Ubuntu 16.04 64-bit and Mac OS X 10.11.1 [2] https://developer.mozilla.org/en-US/docs/Tools/Keyboard_shortcuts#Toolbox
Status: RESOLVED → VERIFIED
QA Whiteboard: [qe-dthtml]
status-firefox49: fixed → verified
Flags: qe-verify+
Depends on: 1292081
Depends on: 1297288
Depends on: 1300458
Depends on: 1328018
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: