1506751 - Firefox developer tools - css rules in devtools pane not updating after page refresh.

Status: VERIFIED FIXED

Description

[nuwanda]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/70.0.3538.77 Safari/537.36

Steps to reproduce:

Load page
Select element with media query max-width rule
Change screen width to trigger css border color changes on element
Refreshed using Ctrl + F5 (and also ctrl + R)

Video of problem here: https://i.redd.it/usia2q4c6mx11.gif

Tested also with other rules in media query. Same bug.

Tested on clean install of Firefox, Developer edition and Portable edition, and Nightly v65.x

Windows 7, 64bit, FF v63.0.1


Actual results:

Before refresh style updated correctly in devtools rules pane
After refresh style doesn't change to reflect border color change

Note: the style on the page is changing correctly, but the rule in the devtools pane is not after a page refresh. Closing and reopening tab/browser restores correct behavior. It's the refresh that causes the bug.


Expected results:

After page refresh rules should update in rules pane to reflect css style changes based on media query

Comment 1

[Timea Babos]

Hey nuwanda,

Can you please also attach the page you loaded?
Adding to a Component given that the reporter reproduced it on a clean profile.

There is also a bug report that looks similar to this, not sure if it is the same tho:  Bug 1108502

Comment 2

[nuwanda]

Attachment: Needinfo as requested: index2.html

To reproduce bug:

-Load index2.html
-In devtools, select container-1
-Resize window either side of 500px width to trigger media query
-container-1 border will change red/blue and rule will update in devtools styles pane
-Refresh page (ctrl + F5, or ctrl + R) and repeat window resize to trigger style changes
-Border color will red/blue on page but rule will NOT change in devtools styles pane
-Close and reopen tab
-devtools rules now update correctly

Comment 3

[nuwanda]

(In reply to Timea Babos from comment #1)
> Hey nuwanda,
> 
> Can you please also attach the page you loaded?
> Adding to a Component given that the reporter reproduced it on a clean
> profile.
> 
> There is also a bug report that looks similar to this, not sure if it is the
> same tho:  Bug 1108502

Attached.

Yes, it seems similar to 1108502 but that report involved an extension. This bug is with clean installs of all three FF versions and Nightly.

Comment 4

[Timea Babos]

Reproduced on latest Nightly 65.0a1 (2018-11-19), Beta 64.0b10 and Release 63.0.3. on Windows 10 x64.
Thanks again for the test case and steps!

Comment 5

[nuwanda]

(In reply to Timea Babos from comment #4)
> Reproduced on latest Nightly 65.0a1 (2018-11-19), Beta 64.0b10 and Release
> 63.0.3. on Windows 10 x64.
> Thanks again for the test case and steps!

No worries. It's been driving me nuts. This is a pretty major bug in my opinion as it goes directly to having trust in the devtools to show accurate information. I've been using Chrome for my media queries testing, which is pretty inconvenient to say the least. I look forward to a fix so I can come back to FF.

Here's another page I used for testing which will be easier to share:

http://www.webdesignerwall.com/demo/media-queries/

Comment 6

[Patrick Brosset <:pbro> (off until July 22nd)]

Thanks for filing. This is indeed a very frustrating issue.
I think I found the root cause for it. When DevTools starts, we listen for window resize so we can correctly show new rules when media queries kick in.
The problem is that after a page refresh (or navigation), we don't listen again for resize in the new window.
So we should listen for target navigation events and use those to start listening again to resize after the new window has been navigated to.

This should be done in the WindowResizeObserver class, in the /devtools/server/actors/reflow.js file.
Specifically, the targetActor constructor argument should be used to add an event listener like this:
targetActor.on("navigate", this.onNavigate);
and then the onNavigate callback should be used to stop and restart the window resize listener, so it points to the right window.

Comment 7

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1506751 - Re-assign listeners for Observable instances that monitor TargetActor windows on navigate. r=pbro

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0fa44cb3f4b6
Re-assign listeners for Observable instances that monitor TargetActor windows on navigate. r=pbro

Comment 9

[Razvan Caliman [:rcaliman]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ca435dba55048d92dbd71645f7d466e231123aa

Comment 10

[Narcis Beleuzu [:NarcisB]]

Backed out for xpcshell failures on reflow.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/f85343663811990b19cecb27b7c0beba0448309a
Push link: https://hg.mozilla.org/integration/autoland/rev/0fa44cb3f4b686913a397c6297c26d5f9c64bcb9
Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=213560705&repo=autoland&lineNumber=7666

Comment 11

[Razvan Caliman [:rcaliman]]

Fixed issue. Ran successfully on try for xpcshell tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b41930a92ff2aff6b0af875592bffcc44b76ec47

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/507a08aaccd5
Re-assign listeners for Observable instances that monitor TargetActor windows on navigate. r=pbro

Comment 13

[Bogdan Tara[:bogdan_tara]]

https://hg.mozilla.org/mozilla-central/rev/507a08aaccd5

Comment 14

[nuwanda]

Quick work.

This will be in the next Nightly? Currently I'm on 65.0a1.

Comment 15

[Timea Babos]

Verified - Fixed on Windows 10 x64, Mac OS 10.13 and Ubuntu 16.04. on the latest Nightly 65.0a1 (2018-11-25) (64-bit).

Nuwanda, can you please check if it is also fixed on your side? Please make sure to update to the latest Nightly. Thanks!

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Is this something we should nominate for Beta approval ahead of next week's RC builds?

Comment 17

[nuwanda]

(In reply to Timea Babos from comment #15)
> Nuwanda, can you please check if it is also fixed on your side? Please make
> sure to update to the latest Nightly. Thanks!

Verified FIXED on Nightly 65.0a1. Rules in media queries now update in devtools style pane after refresh.

Comment 18

[Razvan Caliman [:rcaliman]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)
> Is this something we should nominate for Beta approval ahead of next week's
> RC builds?

Yes, thank you for the reminder! This is an issue in current Beta and it will slip to Stable unless uplifted. Requesting that now.

Comment 19

[Razvan Caliman [:rcaliman]]

Comment on attachment 9026721 [details]
Bug 1506751 - Re-assign listeners for Observable instances that monitor TargetActor windows on navigate. r=pbro

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1506751

User impact if declined: Media queries in the Inspector Rules view do not get updated after the user refreshes the page. This is a serious issue because it makes users lose trust in the accuracy of the tool.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

If yes, steps to reproduce: Yes, see instructions in this comment: https://bugzilla.mozilla.org/show_bug.cgi?id=1506751#c2

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The fix ensures the listeners for media query updates are re-attached after page refresh. This should not impact other parts.

String changes made/needed: None

Comment 20

[Julien Cristau [:jcristau] (back on August 5)]

(In reply to Razvan Caliman [:rcaliman] from comment #19)
> Feature/Bug causing the regression: Bug 1506751

The intent here is to ask when the issue was introduced.  Not which bug this is.

And since I'm here... as we're late in the beta cycle, and this is already present in 63, is there a particular reason letting the fix ride the trains to 65 would be bad?

Comment 21

[nuwanda]

(In reply to Julien Cristau [:jcristau] from comment #20)
> (In reply to Razvan Caliman [:rcaliman] from comment #19)

> And since I'm here... as we're late in the beta cycle, and this is already
> present in 63, is there a particular reason letting the fix ride the trains
> to 65 would be bad?

"This is a serious issue because it makes users lose trust in the accuracy of the tool."

Isn't this enough to move it along ASAP? I filed the bug and have been using Chrome for my development due to the subsequent concerns about the accuracy of Firefox devtools. Some bugs are merely inconvenient or annoying but this goes directly to the viability of Firefox as a trusted tool for developers.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9026721 [details]
Bug 1506751 - Re-assign listeners for Observable instances that monitor TargetActor windows on navigate. r=pbro

Anyway, 64 is on release now, so moving the nomination flag accordingly.

Comment 23

[Razvan Caliman [:rcaliman]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #22)
> Comment on attachment 9026721 [details]
> Bug 1506751 - Re-assign listeners for Observable instances that monitor
> TargetActor windows on navigate. r=pbro
> 
> Anyway, 64 is on release now, so moving the nomination flag accordingly.

Sorry for dropping the ball on this. Given how late in the cycle it is, I think it's fine to just let the patch ride the trains for last 2-3 weeks before organically reaching Firefox release.

I'll pay more attention next time to pinpoint the regression bug in the uplift request form.

Comment 24

[Razvan Caliman [:rcaliman]]

Comment on attachment 9026721 [details]
Bug 1506751 - Re-assign listeners for Observable instances that monitor TargetActor windows on navigate. r=pbro

This patch will reach Firefox release soon anyway; withdrawing uplift request.