changing body class dynamically doesn't apply

VERIFIED FIXED in Firefox 59

Status

()

defect
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: ben.monro, Assigned: emilio)

Tracking

(Blocks 2 bugs)

58 Branch
mozilla60
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox58 wontfix, firefox59 fixed, firefox60 fixed)

Details

Attachments

(3 attachments)

Reporter

Description

a year ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.132 Safari/537.36

Steps to reproduce:

I believe I've found a bug in Firefox.  I have a react app which dynamically changes the body class (adds 'className=red').  When navigating to another page, this className is removed from the body element, but the style is still applied and visible.  

Steps to reproduce:
Clone this repository: https://github.com/benmonro/firefox-css-issue
start the app using `yarn start` 
open http://localhost:3000 in Firefox 
in the inspector, notice the body will have class=red
next click the button labeled 'go to B'
in the inspector notice that body no longer has class=red, but the background is still red.


Actual results:

body keeps the red background red, even though the body element in inspector clearly shows that the class is not red.


Expected results:

body background should not be red.

Updated

a year ago
Component: Untriaged → Layout
Product: Firefox → Core
Assignee

Updated

a year ago
Component: Layout → CSS Parsing and Computation
Flags: needinfo?(emilio)
Thanks, I can repro this consistently, and it's a stylo regression it seems.

However, I haven't been able to repro in 57, which is weird.

Anyway, thanks for the report, will look into it!
Assignee

Updated

a year ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Posted file Testcase
Bummer, found it...

So the key to know why this bug happens is because we try to coalesce style-affecting changes, that is: we record all the changes that happened to the element, and then act consequently when we need to recompute its style.

Since we run style invalidation during the traversal, and thus after recomputing the styles that apply to the page, we don't find the .red rule anymore, and we think that the style can't have changed.

This is a stylo regression of course. Now about how to fix this, we have multiple options...

 (1) If stylesheets have changed, we can run invalidation _before_ updating the stylist, on the main thread. This sounds doable, but probably we can't just do it from ProcessPendingRestyles, since that's not the only thing that can flush the stylist. That's kind of annoying... But flushing the stylist can already generate invalidations for stylesheet additions anyway.

 (2) Collect stylesheet invalidations for stylesheet removals too. This sounds also easy, and probably cleaner.

 (3) The alternative to those would be to give up on coalescing style changes o always run invalidation from the AttributeChanged stuff, etc... which sounds unfortunate.

Boris, Bobby, any thought? I'm going to go with (2) I think, unless you think (1) or (3) are better. Will fix this tomorrow (already late here).

Thanks a lot for the report!
Assignee: nobody → emilio
Status: NEW → ASSIGNED
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bobbyholley)
Assignee

Updated

a year ago
Attachment #8945276 - Attachment mime type: text/plain → text/html
Flags: needinfo?(emilio)
Assignee

Updated

a year ago
Flags: needinfo?(emilio)
Huh, but as I look at the code, we already run invalidation for stylesheet removals... Why would that not be working properly?

Anyway, will investigate tomorrow.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #3)
> Huh, but as I look at the code, we already run invalidation for stylesheet
> removals... Why would that not be working properly?
> 
> Anyway, will investigate tomorrow.

(That's https://searchfox.org/mozilla-central/rev/4611b9541894e90a421debb57ddbbcff55c2f369/servo/components/style/stylesheet_set.rs#478)
Oh well, of course it's obvious.

The element no longer matches the sheet invalidation anymore (it no longer has the "red" class), so we don't invalidate it.

That probably means that the proper way to fix this is to make stylesheet invalidations look at the snapshots of the elements. Which is kinda gross because we don't have that handy right now, and requires a bit of poking at the restyle manager. But nothing unsolvable.
Reporter

Comment 6

a year ago
Whew.  I'm glad you guys are able to reproduce it.  To be clear the issue I had is actually with an image bg.  I presume it's the same problem.
Comment hidden (mozreview-request)
I'm very bad at leaving stuff for tomorrow... :(

(In reply to ben.monro from comment #6)
> Whew.  I'm glad you guys are able to reproduce it.  To be clear the issue I
> had is actually with an image bg.  I presume it's the same problem.

Yeah, it's the same issue, thanks for the report, it's a nice catch!

You can work around it adding a selector in the stylesheet that isn't neither a class, a tagname, or an id selector into the stylesheet you remove, and doesn't have any of those as an ancestor combinator.

Thanks a lot again for reporting it!
Flags: needinfo?(emilio)
Flags: needinfo?(bobbyholley)
Comment hidden (mozreview-request)
Assignee

Updated

a year ago
Flags: needinfo?(bzbarsky)

Comment 10

a year ago
mozreview-review
Comment on attachment 8945283 [details]
Bug 1432850: Look at the snapshots when invalidating due to stylesheet changes.

https://reviewboard.mozilla.org/r/215498/#review221828

r=me.  Nice catch.

::: servo/components/style/invalidation/stylesheets.rs:55
(Diff revision 1)
> -        match *self {
> -            Invalidation::Class(ref class) => {
> -                // FIXME This should look at the quirks mode of the document to
> +        // FIXME This should look at the quirks mode of the document to
> -                // determine case sensitivity.
> +        // determine case sensitivity.
> -                element.has_class(class, CaseSensitivity::CaseSensitive)
> +        //
> +        // FIXME(emilio): Actually write a test and fix this.

Needs a followup bug?
Attachment #8945283 - Flags: review?(bzbarsky) → review+
Assignee

Updated

a year ago
Blocks: 1433588
Assignee

Updated

a year ago
Blocks: 1433589

Comment 12

a year ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e5ab5177f799
Look at the snapshots when invalidating due to stylesheet changes. r=bz
Comment on attachment 8945283 [details]
Bug 1432850: Look at the snapshots when invalidating due to stylesheet changes.

I think it'd be nice to get this in beta, given it's early in the cycle.

Approval Request Comment
[Feature/Bug causing the regression]: Stylo
[User impact if declined]: broken webpages on combination of some dynamic changes
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]:  no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not risky
[Why is the change risky/not risky?]: invalidates a bit more often to avoid being unsound when handling stylesheet removals.
[String changes made/needed]: none
Attachment #8945283 - Flags: approval-mozilla-beta?

Comment 14

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e5ab5177f799
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Ben, does this now work in Nightly for you? I'd love to have your verification before we uplift the fix to 59.
Flags: needinfo?(ben.monro)
Reporter

Comment 16

a year ago
Hi Liz, yeah just tried in the nightly and it works!  :shipit:

Thanks for the quick turn around on this guys!
Flags: needinfo?(ben.monro)
Reporter

Updated

a year ago
Status: RESOLVED → VERIFIED
Comment on attachment 8945283 [details]
Bug 1432850: Look at the snapshots when invalidating due to stylesheet changes.

OK, let's uplift this for 59 beta 6.
Attachment #8945283 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Please provide a patch which applies cleanly. There are conflicts because bug 1428491 didn't get uplifted.
Flags: needinfo?(emilio)
Posted patch Beta patch.Splinter Review
Flags: needinfo?(emilio)
You need to log in before you can comment on or make changes to this bug.