1351275 - stylo: Restyle is not triggered when changing className of element

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: testcase

Steps to reproduce:
1. open this testcase in stylo build with e10s enabled
2. don't do anything, don't even move pointer inside the testcase page

Expected result:
There should a green block after one second

Actual result:
The green block doesn't appear until you move pointer into the document, or change the viewport size.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

(didn't mean to take it... not sure why it made me take this bug...)

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

It's bug 1351276 that the checkbox is checked by default for me :/

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

It doesn't seem to be e10s-only, actually. I can reproduce this issue with non-e10s stylo as well... Not sure why the test failure I'm seeing in bug 1345696 only happens with e10s.

Is it just the bug I filed long ago which never gets fixed?

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Hmm... We're not marking the PresShell as needing a style flush, so we never do the traversal that finds out we need to actually restyle.

I wonder if we should try to compute the hint ASAP as Gecko does, to avoid the whole traversal business. There are a few trade-offs involved there I guess (when the state actually changes style, it's a bit faster to just find it during the restyle process, but when it doesn't...).

Anyway, I'm building a patch that should fix this.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1351275: Cleanup nsRefreshDriver observer array handling to avoid walking them twice.

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

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8851976 [details]
Bug 1351275: Cleanup nsRefreshDriver observer array handling to avoid walking them twice.

https://reviewboard.mozilla.org/r/124238/#review127022

::: layout/base/ServoRestyleManager.cpp:516
(Diff revision 1)
> +
> +    // This ensures we get an actual flush so we can restyle the element if
> +    // needed.
> +    //
> +    // TODO(emilio): There are a bunch of element-specific stuff we need to do,
> +    // see GeckoRestyleManager::AttributeChanged.
> +    PostRestyleEventInternal(/* aForLazyConstruction = */ false);

Hm, why is this necessary? Shouldn't Servo_Element_GetSnapshot invoke maybe_restyle, which invokes Gecko_SetOwnerDocumentNeedsStyleFlush?

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #7)
> Comment on attachment 8851976 [details]
> Bug 1351275: Schedule a style flush when we create snapshots due to an
> attribute change.
> 
> https://reviewboard.mozilla.org/r/124238/#review127022
> 
> ::: layout/base/ServoRestyleManager.cpp:516
> (Diff revision 1)
> > +
> > +    // This ensures we get an actual flush so we can restyle the element if
> > +    // needed.
> > +    //
> > +    // TODO(emilio): There are a bunch of element-specific stuff we need to do,
> > +    // see GeckoRestyleManager::AttributeChanged.
> > +    PostRestyleEventInternal(/* aForLazyConstruction = */ false);
> 
> Hm, why is this necessary? Shouldn't Servo_Element_GetSnapshot invoke
> maybe_restyle, which invokes Gecko_SetOwnerDocumentNeedsStyleFlush?

You're right... Maybe it's due to the refresh driver observer stuff or something? I'll debug it a bit tomorrow I guess.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

It's due to that, without it, the next refresh driver tick doesn't really flush styles.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8851976 [details]
Bug 1351275: Cleanup nsRefreshDriver observer array handling to avoid walking them twice.

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

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 8851976 [details]
Bug 1351275: Cleanup nsRefreshDriver observer array handling to avoid walking them twice.

https://reviewboard.mozilla.org/r/124238/#review127038

So, in bug 1350441 (whose servo-side bits are currently running through homu), I had to centralize the dirty-descendant bit propagation so that we could call it properly from manual style resolution in resolve_style_internal. I _think_ we can get away without the Gecko_SetOwnerDocumentNeedsStyleFlush call there because any document with unstyled descendants should already have a style flush pending, but it seems less-than-robust, especially if we find ourselves having other occasions to call note_dirty_descendants from the servo side.

In general, I think a cleaner / more-robust model would be to:
(1) move the refresh driver observing logic to nsIPresShell::SetNeedsStyleFlush
(2) Teach note_dirty_descendants to call Gecko_SetOwnerDocumentNeedsStyleFlush if it gets all the way to the root without finding an ancestor that already has the bit set.
(3) Stop calling SetNeedsStyleFlush from PostRestyleEventInternal, letting the Servo code take care of it.

That seems to be the most robust and consistent ordering of this stuff from my perspective. WDYT?

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1351275: Move style flush observer logic to nsIPresShell, and align layout observing code.

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

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1351275: Make the pres shell observe style flushes if needed when calling from Servo.

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

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8851976 [details]
Bug 1351275: Cleanup nsRefreshDriver observer array handling to avoid walking them twice.

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

Comment 15

[Bobby Holley (:bholley)]

Comment on attachment 8851976 [details]
Bug 1351275: Cleanup nsRefreshDriver observer array handling to avoid walking them twice.

https://reviewboard.mozilla.org/r/124238/#review127266

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8852485 [details]
Bug 1351275: Move style flush observer logic to nsIPresShell, and align layout observing code.

https://reviewboard.mozilla.org/r/124694/#review127270

::: layout/base/nsIPresShell.h:643
(Diff revision 1)
>             mNeedThrottledAnimationFlush ||
>             mInFlush;
>    }
>  
>    inline void SetNeedStyleFlush();
> +  inline void NeedsToObserveRefreshDriver();

Whoops, this one should go away actually :)

Comment 17

[Bobby Holley (:bholley)]

Comment on attachment 8852485 [details]
Bug 1351275: Move style flush observer logic to nsIPresShell, and align layout observing code.

https://reviewboard.mozilla.org/r/124694/#review127278

Nit: Would have been easier to review if the mObservingLayoutFlushes rename were a separate patch, but I can't complain too much about such awesome work. Thanks for doing this!

::: layout/base/RestyleManager.cpp:497
(Diff revision 1)
>  void
>  RestyleManager::PostRestyleEventInternal(bool aForLazyConstruction)
>  {
>    // Make sure we're not in a style refresh; if we are, we still have
>    // a call to ProcessPendingRestyles coming and there's no need to
>    // add ourselves as a refresh observer until then.
>    bool inRefresh = !aForLazyConstruction && mInStyleRefresh;
>    nsIPresShell* presShell = PresContext()->PresShell();
> -  if (!ObservingRefreshDriver() && !inRefresh) {
> -    SetObservingRefreshDriver(PresContext()->RefreshDriver()->
> +  if (!inRefresh) {
> +    presShell->ObserveStyleFlushes();
> -        AddStyleFlushObserver(presShell));
>    }
>  
>    // Unconditionally flag our document as needing a flush.  The other
>    // option here would be a dedicated boolean to track whether we need
>    // to do so (set here and unset in ProcessPendingRestyles).
>    presShell->SetNeedStyleFlush();
>  }

In a 4th patch (since it's only correct after part 3), can you move PostRestyleEventInternal into GeckoRestyleManager? We shouldn't need it for Servo anymore.

(We should also remove the conditionality in that function for calling Servo_NoteExplicitHints while we're at it).

Comment 18

[Bobby Holley (:bholley)]

Comment on attachment 8852486 [details]
Bug 1351275: Make the pres shell observe style flushes if needed when calling from Servo.

https://reviewboard.mozilla.org/r/124696/#review127280

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8851976 [details]
Bug 1351275: Cleanup nsRefreshDriver observer array handling to avoid walking them twice.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124238/diff/3-4/

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8852485 [details]
Bug 1351275: Move style flush observer logic to nsIPresShell, and align layout observing code.

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

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8852486 [details]
Bug 1351275: Make the pres shell observe style flushes if needed when calling from Servo.

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

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8852485 [details]
Bug 1351275: Move style flush observer logic to nsIPresShell, and align layout observing code.

https://reviewboard.mozilla.org/r/124694/#review127278

> In a 4th patch (since it's only correct after part 3), can you move PostRestyleEventInternal into GeckoRestyleManager? We shouldn't need it for Servo anymore.
> 
> (We should also remove the conditionality in that function for calling Servo_NoteExplicitHints while we're at it).

As discussed, I'll fix this in a followup bug.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d958b42c79d0
Cleanup nsRefreshDriver observer array handling to avoid walking them twice. r=bholley
https://hg.mozilla.org/integration/autoland/rev/cae4319bfeab
Move style flush observer logic to nsIPresShell, and align layout observing code. r=bholley
https://hg.mozilla.org/integration/autoland/rev/53c4db985ef3
Make the pres shell observe style flushes if needed when calling from Servo. r=bholley

Comment 24

[Bobby Holley (:bholley)]

NI Emilio as a reminder to file the followup bug we discussed.

Comment 25

[Bobby Holley (:bholley)]

Whoops, bug 1351791 already on file. Sorry for missing that. :-)

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d958b42c79d0
https://hg.mozilla.org/mozilla-central/rev/cae4319bfeab
https://hg.mozilla.org/mozilla-central/rev/53c4db985ef3