752851 - [Responsive Mode] if the inspector is open, the inspector should close first (when hitting Escape)

Status: VERIFIED FIXED

(DevTools :: General, defect)

Comment 1

[Kumar McMillan [:kumar]]

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]]

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]]

Attachment: v1

Comment 4

[Paul Rouget [:paul]]

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)]

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)]

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]]

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]]

Attachment: v1.1

Comment 9

[Heather Arthur [:harth]]

Comment on attachment 638641 [details] [diff] [review]
v1.1

looks good, makes sense.

Comment 10

[Paul Rouget [:paul]]

We'll need that in Aurora too.

Comment 11

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/11aa96777b29

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/11aa96777b29

Comment 13

[Paul Rouget [:paul]]

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]]

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 16

[Paul Rouget [:paul]]

Attachment: v1.1 - Aurora

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/50963e16d1dc

Comment 18

[Paul Silaghi, QA [:pauly]]

Verified fixed on FF 15b4 on Win 7, Ubuntu 12.04 and Mac OS X 10.6 using the STR in comment 1.