1432850 - changing body class dynamically doesn't apply

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[ben.monro]

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.

Comment 1

[Emilio Cobos Álvarez (: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!

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: 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!

Comment 3

[Emilio Cobos Álvarez (: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.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

(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)

Comment 5

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 6

[ben.monro]

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 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1432850: Look at the snapshots when invalidating due to stylesheet changes.

This goes with a reftest, of course, and with another reftest that we'd fail
setting the rule's selectorText (but I want to check that into WPT since I
wanted to add a very similar invalidation set for that, should we implement it).

Review commit: https://reviewboard.mozilla.org/r/215498/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215498/

Comment 8

[Emilio Cobos Álvarez (:emilio)]

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!

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8945283 [details]
Bug 1432850: Look at the snapshots when invalidating due to stylesheet changes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215498/diff/1-2/

Comment 10

[Boris Zbarsky [:bzbarsky]]

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?

Comment 11

[Emilio Cobos Álvarez (:emilio)]

https://hg.mozilla.org/integration/autoland/rev/4cb489d6c353

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e5ab5177f799
Look at the snapshots when invalidating due to stylesheet changes. r=bz

Comment 13

[Emilio Cobos Álvarez (:emilio)]

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

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/e5ab5177f799

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Ben, does this now work in Nightly for you? I'd love to have your verification before we uplift the fix to 59.

Comment 16

[ben.monro]

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

Thanks for the quick turn around on this guys!

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Please provide a patch which applies cleanly. There are conflicts because bug 1428491 didn't get uplifted.

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Attachment: Beta patch.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9f9cfb102591