The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in Firefox 15

Status

()

Firefox
Developer Tools
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: paul, Unassigned)

Tracking

Trunk
Firefox 16
x86
All
Points:
---

Firefox Tracking Flags

(firefox15 verified)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
STR:
- enter responsive mode
- inspect an element
- finish inspecting, click escape

Expected:
The inspector should close first

Actual:
Responsive mode exits
(Reporter)

Comment 2

5 years ago
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.
(Reporter)

Comment 3

5 years ago
Created attachment 631364 [details] [diff] [review]
v1
(Reporter)

Comment 4

5 years ago
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.
Attachment #631364 - Flags: review?(dcamp)

Comment 5

5 years ago
When editing in the rule view with responsive mode enabled, Escape is closing responsive mode where it should be cancelling the edit.

Comment 6

5 years ago
The rule view/html panel both prevent propagation of the event after hitting escape, so we should cancel responsive mode in the bubble phase.
(Reporter)

Comment 7

5 years ago
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).
(Reporter)

Comment 8

5 years ago
Created attachment 638641 [details] [diff] [review]
v1.1
(Reporter)

Updated

5 years ago
Attachment #631364 - Attachment is obsolete: true
Attachment #631364 - Flags: review?(dcamp)
(Reporter)

Updated

5 years ago
Attachment #638641 - Flags: review?(dcamp)
(Reporter)

Updated

5 years ago
Attachment #638641 - Flags: review?(dcamp) → review?(fayearthur)
Comment on attachment 638641 [details] [diff] [review]
v1.1

looks good, makes sense.
Attachment #638641 - Flags: review?(fayearthur) → review+
(Reporter)

Comment 10

5 years ago
We'll need that in Aurora too.
(Reporter)

Updated

5 years ago
Whiteboard: [land-in-fx-team]
(Reporter)

Comment 11

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/11aa96777b29
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/11aa96777b29
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 16
(Reporter)

Comment 13

5 years ago
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
Attachment #638641 - Flags: approval-mozilla-aurora?
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.
Attachment #638641 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Reporter)

Updated

5 years ago
Whiteboard: [land-in-aurora]
(Reporter)

Updated

5 years ago
Duplicate of this bug: 773841
(Reporter)

Comment 16

5 years ago
Created attachment 642249 [details] [diff] [review]
v1.1 - Aurora
(Reporter)

Updated

5 years ago
Attachment #638641 - Attachment is obsolete: true
https://hg.mozilla.org/releases/mozilla-aurora/rev/50963e16d1dc
status-firefox15: --- → fixed
Whiteboard: [land-in-aurora]
Verified fixed on FF 15b4 on Win 7, Ubuntu 12.04 and Mac OS X 10.6 using the STR in comment 1.
Status: RESOLVED → VERIFIED
status-firefox15: fixed → verified
You need to log in before you can comment on or make changes to this bug.