1384120 - Stylo: image flickers during mouseover triggered fade on fluentcpp.com

Status: RESOLVED FIXED

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

Description

[John Smith]

Screen recording (the unfortunately low framerate hides part of the effect) : https://youtu.be/bDskTWzezcA
This can be reproduced consistently on the source page: http://www.fluentcpp.com/2017/07/25/understandable-statements-run-slower/

Observed behavior: On mouseover and mouseout, during the fade in or fade out, the overlayed element flickers briefly.
Expected behavior: Same as without layout.css.servo.enable, that is, no flickering.

To be clear since I'm not familiar with this Bugzilla, I'm running nightly (build 20170725100346), I've only tested on Linux, and this only happens with layout.css.servo.enabled set to true, and I can reproduce 100% of the time.

Comment 1

[Darkspirit]

Confirmed in Nightly 56 x64 20170725144053 @ Debian Testing.

Comment 2

[Bobby Holley (:bholley)]

This is a great bug report - thanks!

Comment 3

[Chris Peterson [:cpeterson]]

Stylo also breaks this page's icon fonts. I filed bug 1384275 to track that issue.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: A reduced test case

This is also related to animations with content property change.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: More noticeable test case

A problem here is that when a transition starts along with content property change on a pseudo element, the content change causes a reframe of the parent element of the pseudo, it throws the initial transition damage away, thus the flicker happens.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> Created attachment 8893605 [details]
> More noticeable test case
> 
> A problem here is that when a transition starts along with content property
> change on a pseudo element, the content change causes a reframe of the
> parent element of the pseudo, it throws the initial transition damage away,
> thus the flicker happens.

It (the restyle damage thing) may be not a problem.  The flicker does not happen for CSS animations.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb0871ad6bc86eabd39672469dbb9d834cff1e0b

Comment 8

[Hiroyuki Ikezoe (:hiro)]

The try looks good.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> It (the restyle damage thing) may be not a problem.  The flicker does not
> happen for CSS animations.

The flicker also happens for CSS animations. I did write a reftest for it.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1384120 - Replace old pseudo style context with a new style context including animations.

When a reframe happens on the parent of a pseudo element which has animations,
we need to resolve style for the pseudo element that includes the animations'
style and also *replace* old style context (that does not include animations'
style) with it. Otherwise, we will use the old style context that has *no*
animations style, as a result, we will see a flicker right after the reframe.

Two reftests in this patch fail without this fix.  One is for CSS transitions,
the other one is for CSS animations.

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

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8894699 [details]
Bug 1384120 - Replace old pseudo style context with a new style context including animations.

https://reviewboard.mozilla.org/r/165862/#review170984

::: layout/base/nsCSSFrameConstructor.cpp:1989
(Diff revision 1)
>      if (hasServoAnimations) {
>        // If animations are involved, we avoid the SetExplicitStyle optimization
> -      // above.
> +      // above. We need to resolve style with animations and replace old one.
>        mPresShell->StyleSet()->AsServo()->StyleNewSubtree(container);
> +      pseudoStyleContext =
> +        styleSet->ResolvePseudoElementStyle(aParentContent->AsElement(),

nit: This can just be `ResolveServoStyle(container->AsElement())`, if you prefer, I think it'd be slightly clearer.

Also, perhaps the comment should say "grab the style with animations from the pseudo" instead of `resolve style with animations`, since the main styling work is done in `StyleNewSubtree`, and the ResolvePseudoElementStyle really just looks up in `container`.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #10)
> Comment on attachment 8894699 [details]
> Bug 1384120 - Replace old pseudo style context with a new style context
> including animations.
> 
> https://reviewboard.mozilla.org/r/165862/#review170984
> 
> ::: layout/base/nsCSSFrameConstructor.cpp:1989
> (Diff revision 1)
> >      if (hasServoAnimations) {
> >        // If animations are involved, we avoid the SetExplicitStyle optimization
> > -      // above.
> > +      // above. We need to resolve style with animations and replace old one.
> >        mPresShell->StyleSet()->AsServo()->StyleNewSubtree(container);
> > +      pseudoStyleContext =
> > +        styleSet->ResolvePseudoElementStyle(aParentContent->AsElement(),
> 
> nit: This can just be `ResolveServoStyle(container->AsElement())`, if you
> prefer, I think it'd be slightly clearer.
> 
> Also, perhaps the comment should say "grab the style with animations from
> the pseudo" instead of `resolve style with animations`, since the main
> styling work is done in `StyleNewSubtree`, and the ResolvePseudoElementStyle
> really just looks up in `container`.

Oh, 'grab'! Great! It gets clearer! Thanks as usual!

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8894699 [details]
Bug 1384120 - Replace old pseudo style context with a new style context including animations.

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

Comment 13

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 81484920975d -d e85bed50a2ac: rebasing 412027:81484920975d "Bug 1384120 - Replace old pseudo style context with a new style context including animations. r=emilio" (tip)
merging layout/reftests/css-transitions/reftest.list
warning: conflicts while merging layout/reftests/css-transitions/reftest.list! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8894699 [details]
Bug 1384120 - Replace old pseudo style context with a new style context including animations.

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d79b9e6b0d03
Replace old pseudo style context with a new style context including animations. r=emilio

Comment 16

[Carsten Book [:Tomcat]]

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