Inpector's overlay lingers after quickly quitting Responsive Design Mode

VERIFIED FIXED in Firefox 53

Status

defect
P1
normal
VERIFIED FIXED
2 years ago
11 months ago

People

(Reporter: Fanolian+bugzilla, Assigned: jryans)

Tracking

({regression, reproducible})

53 Branch
Firefox 55
Dependency tree / graph

Firefox Tracking Flags

(firefox51 unaffected, firefox52 unaffected, firefox-esr52 unaffected, firefox53 verified, firefox54 verified, firefox55 verified)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

2 years ago
str
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170222030329

Steps to reproduce:

(Please refer to attached video.)

1. Open Inspector and RDM.
2. Select an element with Inspector in the page and quickly close RDM.


Actual results:

The Inspector overlay remains on the page even after closing all dev tools. It goes away after a refresh.
Reporter

Updated

2 years ago
Has STR: --- → yes
Component: Untriaged → Developer Tools: Responsive Design Mode
Reporter

Comment 1

2 years ago
regression-window
Last good Nightly: 2016-11-28
First bad Nightly: 2016-11-29
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=05328d3102efd4d5fc0696489734d7771d24459f&tochange=adcc39e3cad0f32aba0efb478cc4a023a5dfc43f

Further bisection:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=a105cf2d33a5643d63c516d5ecfe917d8eb5eeee&tochange=fb4f5c7e082a4176cd1b8f3c784e5f424417e3fa

Regressed by bug 1151909.
Has Regression Range: --- → yes
Component: Developer Tools: Responsive Design Mode → Developer Tools: Inspector
Reporter

Updated

2 years ago
Blocks: 1151909
Reporter

Updated

2 years ago
Version: Trunk → 53 Branch
Assignee

Comment 2

2 years ago
:ochameau, maybe you can take a look since you worked on bug 1151909?  Let me know if you need help / if the problem is something in RDM.
Flags: needinfo?(poirot.alex)
The STR seems to be really about the RDM.

The picker stays on while the RDM is turned on. But the content document is suffled around, which ends up breaking the highlighter. The highlighter markup stays there for ever.

My patch from bug 1151909 starts listening for window-ready instead of navigate.
I imagine the RDM triggers navigate event when turning it ON?
And also will-navigate which are used in some other places in highlighter galaxy.

I tried to cleanup things on window-destroyed, but it seems to be tricky.
I'm wondering if we should just cancel the picker before toggling RDM?
Flags: needinfo?(poirot.alex)
Here is something on top of bug 1151909, but with that, the picker button on the top left of the toolbox stays ON whereas it has been cancelled.
May be we should also change the navigate to window-ready to try to restore it?

Before going deeper into these actors I would like you feedback on just cancelling picker on RDM toggle.
Assigning this to you Alex since you already took the lead on it. 

Gonna needinfo zer0 just in case he has other ideas on the fix.
Assignee: nobody → poirot.alex
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: qe-verify+
Flags: needinfo?(poirot.alex)
Priority: -- → P3
Assignee

Comment 6

2 years ago
I think :gl meant to ni? :zer0 here.
Flags: needinfo?(poirot.alex) → needinfo?(zer0)
I agreed with Alex that this seems to be really about the RDM. I think the culprit here is the swap frame loaders, that "confuses" the highlighter. I tried to apply the patch attached, but I got a `this._highlighter is null` if I try to reuse the highlighter once I exit from RDM (assuming I've the inspector enabled in RDM before close it); or vice versa (entering in RDM with the inspector enabled on the page, and then try to re-enable again in RDM).
Flags: needinfo?(zer0)
Assignee

Comment 8

2 years ago
I'll take a look at what RDM is doing here.
Flags: needinfo?(jryans)
jryans: is this likely to be upliftable to 53?  Should we fix-optional or wontfix it for 53?
Assignee

Comment 10

2 years ago
(In reply to Randell Jesup [:jesup] from comment #9)
> jryans: is this likely to be upliftable to 53?  Should we fix-optional or
> wontfix it for 53?

I think we'll be able to uplift to 53, at least so far.

I'll take this over from :ochameau.
Assignee: poirot.alex → jryans
Flags: needinfo?(jryans)
Assignee

Updated

2 years ago
Priority: P3 → P1
Assignee

Comment 11

2 years ago
It seems like the root cause here is that `swapFrameLoaders` path used by moving tabs between windows, toggling RDM, etc. fires `pageshow` events, which is one input we used to emit `window-created`.

We could try to distinguish or ignore _these_ pageshow events from _other_ pageshow events, but that feels a little more complex than needed.

I think we can attempt to remove any inserted highlighter on `window-created`, and that's probably good enough, but I might be missing a user flow here.
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8840359 - Attachment is obsolete: true

Comment 14

2 years ago
mozreview-review
Comment on attachment 8845170 [details]
Bug 1341756 - Clear any highlighters on window-ready.

https://reviewboard.mozilla.org/r/118366/#review121154

Looks good to me! Sorry if I wasn't quick to review it, but I wanted to check if this patch was fixing also some other scenario I was working on, or simplified existing patch.
Attachment #8845170 - Flags: review?(zer0) → review+
Assignee

Comment 15

2 years ago
mozreview-review-reply
Comment on attachment 8845170 [details]
Bug 1341756 - Clear any highlighters on window-ready.

https://reviewboard.mozilla.org/r/118366/#review121154

No worries, thanks for checking it over! :)

Comment 16

2 years ago
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4c0d4f924023
Clear any highlighters on window-ready. r=zer0
I am a bit concerned that the states in highlighter-overlays might not be cleared as a result of the current fix. zer0, can you check if this is the case? If so, possibly file a follow up.
Flags: needinfo?(zer0)

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4c0d4f924023
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Assignee

Comment 19

2 years ago
(In reply to Gabriel [:gl] (ΦωΦ) from comment #17)
> I am a bit concerned that the states in highlighter-overlays might not be
> cleared as a result of the current fix. zer0, can you check if this is the
> case? If so, possibly file a follow up.

Can I proceed to uplift this current fix to 54 and 53?  Or do you want more investigation first?
Flags: needinfo?(gl)
Having Matteo double check this and clearing it for uplift.
Flags: needinfo?(gl)
To me it's fine as is. There was a slightly doubt about some highlighters in highlighter-overlays, but I don't think it would trigger any major issues.
Flags: needinfo?(zer0)
Assignee

Comment 22

2 years ago
Comment on attachment 8845170 [details]
Bug 1341756 - Clear any highlighters on window-ready.

Approval Request Comment
[Feature/Bug causing the regression]: Changes from bug 1151909
[User impact if declined]: If declined, the DevTools highlighter remains stuck on when toggling Responsive Design Mode
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Yes, QE should verify, see STR in comment 0.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only affect DevTools highlighter
[String changes made/needed]: None
Attachment #8845170 - Flags: approval-mozilla-beta?
Attachment #8845170 - Flags: approval-mozilla-aurora?
Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(brindusa.tot)
Hi,
I tested this on Windows 10 with the latest FF Nightly 55.0a1(2017-03-14) and I can confirm the fix, I'm no longer able to reproduce this issue. 
Please note that I tried to reproduce this on Mac and Ubuntu but is not reproducible. 
I will mark this as verified fix.
Status: RESOLVED → VERIFIED
Flags: needinfo?(brindusa.tot)
Comment on attachment 8845170 [details]
Bug 1341756 - Clear any highlighters on window-ready.

Fix a regression related to DevTools highlighter and was verified in nightly. Beta53+ & Aurora54+.
Attachment #8845170 - Flags: approval-mozilla-beta?
Attachment #8845170 - Flags: approval-mozilla-beta+
Attachment #8845170 - Flags: approval-mozilla-aurora?
Attachment #8845170 - Flags: approval-mozilla-aurora+
I have reproduced this issue using Firefox 54.0a1 (ID=20170222030329) on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 53.0b2, 54.0a2, 55.0a1 on Win 8.1 x64, Mac OS X 10.11 and Ubuntu 16.04 x64.

Updated

2 years ago
Depends on: 1365209

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.