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)
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)
(deleted),
video/mp4
|
Details | |
(deleted),
text/x-review-board-request
|
zer0
:
review+
gchang
:
approval-mozilla-aurora+
gchang
:
approval-mozilla-beta+
|
Details |
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
Keywords: regression,
reproducible
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
Assignee | ||
Comment 2•8 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.
status-firefox52:
--- → unaffected
status-firefox53:
--- → affected
status-firefox54:
--- → affected
Flags: needinfo?(poirot.alex)
status-firefox51:
--- → unaffected
Comment 3•8 years ago
|
||
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)
Comment 4•8 years ago
|
||
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.
Comment 5•8 years ago
|
||
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•8 years ago
|
||
I think :gl meant to ni? :zer0 here.
Flags: needinfo?(poirot.alex) → needinfo?(zer0)
Comment 7•8 years ago
|
||
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•8 years ago
|
||
I'll take a look at what RDM is doing here.
Flags: needinfo?(jryans)
Comment 9•8 years ago
|
||
jryans: is this likely to be upliftable to 53? Should we fix-optional or wontfix it for 53?
Assignee | ||
Comment 10•8 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•8 years ago
|
Priority: P3 → P1
Assignee | ||
Comment 11•8 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 | ||
Comment 13•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8840359 -
Attachment is obsolete: true
Comment 14•8 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•8 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•8 years ago
|
||
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4c0d4f924023
Clear any highlighters on window-ready. r=zer0
Comment 17•8 years ago
|
||
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•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Assignee | ||
Comment 19•8 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)
Comment 20•8 years ago
|
||
Having Matteo double check this and clearing it for uplift.
Flags: needinfo?(gl)
Comment 21•8 years ago
|
||
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•8 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?
Updated•8 years ago
|
status-firefox-esr52:
--- → unaffected
Comment 23•8 years ago
|
||
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)
Comment 24•8 years ago
|
||
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.
Comment 25•8 years ago
|
||
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+
Comment 26•8 years ago
|
||
bugherder uplift |
Comment 27•8 years ago
|
||
bugherder uplift |
Comment 28•8 years ago
|
||
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•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•