Closed Bug 1203079 Opened 6 years ago Closed 5 years ago
Ctrl+Shift+C doesn't cancel element picker if devtools aren't focused (inconsistency)
STR: (Win7_64, Nightly 43, 32bit, ID 20150907030206, new profile, safe mode) 1. Open any page, e.g. data:text/html,<div style="background:gray;width:100px;height:100px;"> 2. Press Ctrl+Shift+C to initiate element picker 3. Hover mouse over gray square on the page (any element on the page from Step 1) 4. Press Ctrl+Shift+C Result: Element picker is still active Expectations: Element picker should be canceled Compare this behavior with resilt of following steps: 1. Open any page, e.g. data:text/html,<div style="background:gray;width:100px;height:100px;"> 2. Press Ctrl+Shift+C to initiate element picker 3.(*) Click <body> element in markup-view 4. Hover mouse over gray square on the page (any element on the page from Step 1) 5. Press Ctrl+Shift+C Result == Expectations: Element picker was canceled
I figured out that if in Step 3.(*) I click "Inspector" tab in devtools or dock devtools to another side of window, or click ruleview (or another sidebar tab), then I still get expected result. Looks like the problem is that devtools lose focus when element picker is initiated.
Severity: normal → minor
Summary: Ctrl+Shift+C doesn't cancel element picker if markup-view isn't focused (inconsistency) → Ctrl+Shift+C doesn't cancel element picker if devtools aren't focused (inconsistency)
Yes devtools looses the focus when we enter the element pick mode, but that's because the picker requires to have focus on the page to work properly (also in the case where you're debugging another window, you'd want the debuggee window to receive focus anyway once you click the element picker, or used the shortcut). So, we should make sure the shortcut also works when the page is focused. For info, we already have the ESCAPE key shortcut that works. Maybe we should also have ctrl+shift+C for consistency.
Whiteboard: [polish-backlog] → [polish-backlog][difficulty=easy]
This is regression between 2015-03-11 and 2015-03-12. I knew that it's a regression when reported it (because it used to work OK) but just didn't know how to deal w/ regressions yet: > pushlog_url: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fd8e079d6335&tochange=58c9d079f318 Bug 1120111 just prevents "keyup" and "keydown" events in content. So Ctrl+Shift+C could just be added there, in the list of hotkeys, w/o extra investigation. And, please, somebody, file a bug about implementing Up/Down hotkeys to switch through siblings. Bug 1120111 is useless while it doesn't have hotkeys to switch through siblings and while it lags when you (first) change highlighted node with Left/Right (then) accidentally move mouse by ~1px.
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P2
Whiteboard: [polish-backlog][difficulty=easy] → [polish-backlog][difficulty=easy][lang=js][good first bug]
this appears to be fixed in firefox 46
Yes, I noticed that it has greatly improved, but it still happens on Firefox 49 if the page is focused (but priority is most likely lower than before). I'll update info on this one later.
Bug 1206559 changed this. Setting NI for Patrick, because it's probably not "good first bug" anymore. STR_2 1. Open bug 1257815 2. Press Ctrl+Shift+C 3. Scroll the page by dragging scrollbar at the right side 4. Press Ctrl+Shift+C 5. Press Escape AR: Element picker is still active after Step 4. Element picked is canceled after Step 5. ER: Element picker should be canceled after Step 4.
Flags: needinfo?(arni2033) → needinfo?(pbrosset)
See Also: → 1206559
I think the fix is still the same as before. If we're in a situation where devtools doesn't have focus anymore, then we want ctrl+shift+C to still work. And that means listening to this key shortcut in content. Luckily, we already listen to keyboard event when the picker mode is ON: https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/highlighters.js#289-337 So I suggest we add a new entry in there that takes care of ctrl+shift+C and does the same as ESCAPE.
Patrick, how would you include a case for a shortcut in that switch statement? Because it seems that it is only triggered for a single keypress. I'm not sure how you could check the keypress event against a three-button shortcut.
Looks like you figured it out, so clearing the NI? request. I will take a look at the patch now.
Comment on attachment 8760474 [details] [diff] [review] shortcut-fix.patch Review of attachment 8760474 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. This will need a new test case in a test. No need for a new test file, I think you can just add a case in: devtools\client\inspector\test\browser_inspector_highlighter-keybinding_03.js since this test already deals with the ESC key. ::: devtools/server/actors/highlighters.js @@ +333,5 @@ > this.cancelPick(); > events.emit(this._walker, "picker-node-canceled"); > return; > + case Ci.nsIDOMKeyEvent.DOM_VK_C: > + let isOSX = Services.appinfo.OS === "Darwin"; No need to access this everytime C is pressed, you could just do this once near the top of the module file and store it as a const: const IS_OSX = Services.appinfo.OS === "Darwin";
Comment on attachment 8760938 [details] [diff] [review] shortcut-fix.patch Review of attachment 8760938 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Thanks Moby.
Attachment #8760938 - Flags: review?(pbrosset) → review+
It seems that there are significantly more errors than usual on this try push, but I do not think it is related to my changes. Do you have any idea why those are happening Brian?
(In reply to Maximillian von Briesen [:moby] from comment #16) > It seems that there are significantly more errors than usual on this try > push, but I do not think it is related to my changes. Do you have any idea > why those are happening Brian? I'm actually seeing errors on the test that was changed on Linux and Windows, like https://treeherder.mozilla.org/#/jobs?repo=try&revision=70e23af59d4b&selectedJob=22183108. The test is probably not synthesizing the right key combo in those environments. Best way to debug it would be to set up a build VM for linux and run the test there: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Using_the_VM.
This should fix the errors with try push
Attachment #8761369 - Flags: review?(pbrosset) → review+
has problems to apply: renamed 1203079 -> shortcut-fix.patch applying shortcut-fix.patch patching file devtools/server/actors/highlighters.js Hunk #1 FAILED at 3 1 out of 3 hunks FAILED -- saving rejects to file devtools/server/actors/highlighters.js.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and qrefresh shortcut-fix.patch
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/fx-team/rev/c0548c6cba49 Allow Ctrl+Shift+C to cancel element picker even if page becomes unfocused; r=pbro
I have reproduced this bug on Nightly 43.0a1 (2015-09-09) on ubuntu 16.04 LTS, 64 bit! The bug's fix is now verified on Latest Nightly 50.0a1! Build ID: 20160622030210 User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0
I have reproduced this bug with Nightly 43.0a1 (2015-09-10) on Windows 7, 64 Bit! The Bug's fix is now verified on Latest Nightly 50.0a1. Build ID 20160722030235 User Agent Mozilla/5.0 (Windows NT 6.1; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [bugday-20160622] → [bugday-20160622][bugday-20160720]
You need to log in before you can comment on or make changes to this bug.