Last Comment Bug 752851 - [Responsive Mode] if the inspector is open, the inspector should close first (when hitting Escape)
: [Responsive Mode] if the inspector is open, the inspector should close first ...
Status: VERIFIED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: Firefox 16
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
: 773841 (view as bug list)
Depends on:
Blocks: 749628
  Show dependency treegraph
 
Reported: 2012-05-08 03:44 PDT by Paul Rouget [:paul]
Modified: 2012-08-13 05:30 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified


Attachments
v1 (1.95 KB, patch)
2012-06-08 05:52 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
v1.1 (3.46 KB, patch)
2012-07-03 03:53 PDT, Paul Rouget [:paul]
fayearthur: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
v1.1 - Aurora (3.46 KB, patch)
2012-07-14 10:05 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review

Description Paul Rouget [:paul] 2012-05-08 03:44:24 PDT

    
Comment 1 Kumar McMillan [:kumar] (needinfo all the things) 2012-06-03 22:30:36 PDT
STR:
- enter responsive mode
- inspect an element
- finish inspecting, click escape

Expected:
The inspector should close first

Actual:
Responsive mode exits
Comment 2 Paul Rouget [:paul] 2012-06-08 05:31:37 PDT
I guess the correct behavior is:

1) open tool A
2) open tool B
3) ESC -> close tool B
4> ESC -> close tool A

The last tool should close first.

I don't really want to have a "if inspector open first, then close" kind of thing.
I'd prefer to have a more generic and natural mechanism. But doing that with just the
DOM events is, I think, impossible.

This is how it works today:

Both the Inspector and the Responsive Mode attach a listener to the chrome window.
When the user press escape, the first to be notified is the first that attached the listener.

We can stop the propagation, but of course, this is useless because we use the same node.
We can use preventDefault() and check .defaultPrevented, but the listener are called in this
order:

1) attach A
2) attach B
3) call A
4) call B

So we can't preventDefault().

To get the right order, one could listen during the bubbling, but then, we start checking if another tool is open, and I'd like to avoid that.
Comment 3 Paul Rouget [:paul] 2012-06-08 05:52:56 PDT
Created attachment 631364 [details] [diff] [review]
v1
Comment 4 Paul Rouget [:paul] 2012-06-08 05:55:15 PDT
Comment on attachment 631364 [details] [diff] [review]
v1

This approach works because the inspector listens to keypress during the bubbling, while the Responsive Mode does not.
Comment 5 Dave Camp (:dcamp) 2012-06-19 07:45:42 PDT
When editing in the rule view with responsive mode enabled, Escape is closing responsive mode where it should be cancelling the edit.
Comment 6 Dave Camp (:dcamp) 2012-06-19 07:50:31 PDT
The rule view/html panel both prevent propagation of the event after hitting escape, so we should cancel responsive mode in the bubble phase.
Comment 7 Paul Rouget [:paul] 2012-07-03 02:51:34 PDT
if the responsive mode catch to the key event in the bubble phase, we are not guaranteed that its listener will be called before the inspector listener. I'll find a better way to do that (like listening to a deeper node).
Comment 8 Paul Rouget [:paul] 2012-07-03 03:53:05 PDT
Created attachment 638641 [details] [diff] [review]
v1.1
Comment 9 Heather Arthur [:harth] 2012-07-05 09:46:18 PDT
Comment on attachment 638641 [details] [diff] [review]
v1.1

looks good, makes sense.
Comment 10 Paul Rouget [:paul] 2012-07-07 04:28:56 PDT
We'll need that in Aurora too.
Comment 12 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-07-10 02:07:35 PDT
https://hg.mozilla.org/mozilla-central/rev/11aa96777b29
Comment 13 Paul Rouget [:paul] 2012-07-12 02:40:47 PDT
Comment on attachment 638641 [details] [diff] [review]
v1.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Responsive Design Tool: bug 749628
User impact if declined: hard to use the Responsive Tool with the Inspector (closing it doesn't do what's expected)
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): not high (code is pretty simple)
String or UUID changes made by this patch:  none
Comment 14 Alex Keybl [:akeybl] 2012-07-12 13:56:57 PDT
Comment on attachment 638641 [details] [diff] [review]
v1.1

[Triage Comment]
Low risk, and we can always backout/disable in the coming weeks if need be. Approving for Aurora 15.
Comment 15 Paul Rouget [:paul] 2012-07-14 01:59:01 PDT
*** Bug 773841 has been marked as a duplicate of this bug. ***
Comment 16 Paul Rouget [:paul] 2012-07-14 10:05:08 PDT
Created attachment 642249 [details] [diff] [review]
v1.1 - Aurora
Comment 18 Paul Silaghi, QA [:pauly] 2012-08-13 05:30:38 PDT
Verified fixed on FF 15b4 on Win 7, Ubuntu 12.04 and Mac OS X 10.6 using the STR in comment 1.

Note You need to log in before you can comment on or make changes to this bug.