Closed Bug 1341756 Opened 8 years ago Closed 8 years ago

Inpector's overlay lingers after quickly quitting Responsive Design Mode

Categories

(DevTools :: Inspector, defect, P1)

53 Branch
defect

Tracking

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

VERIFIED FIXED
Firefox 55
Tracking Status
firefox51 --- unaffected
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- verified
firefox54 --- verified
firefox55 --- verified

People

(Reporter: Fanolian+BMO, Assigned: jryans)

References

Details

(Keywords: regression, reproducible)

Attachments

(2 files, 1 obsolete file)

Attached video Inspector (pick an element) + RDM (deleted) —
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.
Has STR: --- → yes
Component: Untriaged → Developer Tools: Responsive Design Mode
Has Regression Range: --- → yes
Component: Developer Tools: Responsive Design Mode → Developer Tools: Inspector
Blocks: 1151909
Version: Trunk → 53 Branch
: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
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)
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?
(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)
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 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+
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! :)
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)
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
(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)
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.
Depends on: 1365209
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: