Ctrl+Shift+C doesn't cancel element picker if devtools aren't focused (inconsistency)

VERIFIED FIXED in Firefox 50

Status

defect
P2
minor
VERIFIED FIXED
4 years ago
Last year

People

(Reporter: arni2033, Assigned: moby, Mentored)

Tracking

({regression})

Trunk
Firefox 50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 affected, firefox42 affected, firefox43 affected, firefox44 affected, firefox-esr38 unaffected, firefox50 verified)

Details

(Whiteboard: [polish-backlog][difficulty=easy][lang=js][good first bug], )

Attachments

(1 attachment, 3 obsolete attachments)

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]
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.
Has STR: --- → yes
Inspector bug triage (filter on CLIMBING SHOES).
Mentor: pbrosset
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.
Flags: needinfo?(arni2033)
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.
Flags: needinfo?(pbrosset)
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.
Flags: needinfo?(pbrosset)
Posted patch shortcut-fix.patch (obsolete) — Splinter Review
Attachment #8760474 - Flags: review?(pbrosset)
Assignee: nobody → mvonbriesen
Looks like you figured it out, so clearing the NI? request. I will take a look at the patch now.
Flags: needinfo?(pbrosset)
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";
Attachment #8760474 - Flags: review?(pbrosset)
Posted patch shortcut-fix.patch (obsolete) — Splinter Review
Attachment #8760474 - Attachment is obsolete: true
Attachment #8760938 - Flags: review?(pbrosset)
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?
Flags: needinfo?(bgrinstead)
(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.
Flags: needinfo?(bgrinstead)
Posted patch shortcut-fix.patch (obsolete) — Splinter Review
This should fix the errors with try push
Attachment #8760938 - Attachment is obsolete: true
Attachment #8761369 - Flags: review?(pbrosset)
Attachment #8761369 - Flags: review?(pbrosset) → review+
Keywords: checkin-needed
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
Flags: needinfo?(mvonbriesen)
Keywords: checkin-needed
Attachment #8761369 - Attachment is obsolete: true
Flags: needinfo?(mvonbriesen)
Attachment #8762659 - Flags: review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/c0548c6cba49
Allow Ctrl+Shift+C to cancel element picker even if page becomes unfocused; r=pbro
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c0548c6cba49
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
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
QA Whiteboard: [bugday-20160622]
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]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.