Closed Bug 1384120 Opened 2 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: barrdetwix, Assigned: hiro)

References

(Blocks 1 open bug, )

Details

Attachments

(2 files, 1 obsolete file)

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.
Confirmed in Nightly 56 x64 20170725144053 @ Debian Testing.
Status: UNCONFIRMED → NEW
Has STR: --- → yes
Ever confirmed: true
This is a great bug report - thanks!
Stylo also breaks this page's icon fonts. I filed bug 1384275 to track that issue.
Summary: Stylo: Flicker during mouseover triggered fade → Stylo: image flickers during mouseover triggered fade on fluentcpp.com
Priority: -- → P2
Attached file A reduced test case (obsolete) —
This is also related to animations with content property change.
Assignee: nobody → hikezoe
Priority: P2 → --
Priority: -- → P2
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.
Attachment #8891599 - Attachment is obsolete: true
(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.
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 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`.
Attachment #8894699 - Flags: review?(emilio+bugs) → review+
(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!
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)
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d79b9e6b0d03
Replace old pseudo style context with a new style context including animations. r=emilio
https://hg.mozilla.org/mozilla-central/rev/d79b9e6b0d03
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.