Bug 205694 – REGRESSION (r252724): Unable to tap on play button on google video 'See the top search trends of 2019'

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Simon Fraser (smfr)]

REGRESSION (r252724): Unable to tap on play button on google video 'See the top search trends of 2019'

[Simon Fraser (smfr)]

Created attachment 386630 [details]
Patch

[Simon Fraser (smfr)]

<rdar://problem/58062987>

[Simon Fraser (smfr)]

Created attachment 386645 [details]
Patch

[Antoine Quint]

You probably want to get rid of the scheme changes.

[Antti Koivisto]

Comment on attachment 386645 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=386645&action=review

> Source/WebCore/animation/KeyframeEffect.cpp:1099
> -    if (m_triggersStackingContext && targetStyle.hasAutoUsedZIndex())
> -        targetStyle.setUsedZIndex(0);
> +    Style::Adjuster::adjustAnimatedStyle(targetStyle, *m_target, m_triggersStackingContext);
>  }

A better approach might be to simply mark a bit that this adjustment is needed and do it from TreeResolver::createAnimatedElementUpdate where you have things like parent/parent box style available. Alternatively those things need to be passed through here.

> Source/WebCore/page/animation/CompositeAnimation.cpp:342
> -    if (forceStackingContext && animatedStyle) {
> -        if (animatedStyle->hasAutoUsedZIndex())
> -            animatedStyle->setUsedZIndex(0);
> -    }
> +    if (animatedStyle)
> +        Style::Adjuster::adjustAnimatedStyle(*animatedStyle, element, forceStackingContext);
>  

Why are we making code changes to the legacy animation engine? Don't we have a new one enabled by default? Are these changes tested?

> Source/WebCore/style/StyleAdjuster.cpp:538
> +static const RenderStyle* parentBoxStyle(const Element& element)
> +{
> +    auto* renderer = element.renderer();
> +    if (!renderer)
> +        return nullptr;
> +    
> +    if (is<RenderBox>(renderer) && is<RenderBox>(renderer->parent()))
> +        return &renderer->parent()->style();
> +
> +    return nullptr;
> +}

We can't look into renderers for style during style resolution (which is where animations are resolved too). All style changes are resolved in one go and applied to the render tree as a separate step. A renderer may have stale style, and it might not even have been created yet. If parent box style is needed, it needs to passed in, like with other Adjuster functions.

> WebKit.xcworkspace/xcshareddata/xcschemes/All Source.xcscheme:60
> -               BuildableName = "libANGLE.a"
> +               BuildableName = "libANGLE-shared.dylib"

Some random garbage here.

[Simon Fraser (smfr)]

(In reply to Antti Koivisto from comment #5)
> Comment on attachment 386645 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=386645&action=review
> 
> > Source/WebCore/animation/KeyframeEffect.cpp:1099
> > -    if (m_triggersStackingContext && targetStyle.hasAutoUsedZIndex())
> > -        targetStyle.setUsedZIndex(0);
> > +    Style::Adjuster::adjustAnimatedStyle(targetStyle, *m_target, m_triggersStackingContext);
> >  }
> 
> A better approach might be to simply mark a bit that this adjustment is
> needed and do it from TreeResolver::createAnimatedElementUpdate where you
> have things like parent/parent box style available. Alternatively those
> things need to be passed through here.

Fixed.

> > Source/WebCore/page/animation/CompositeAnimation.cpp:342
> > -    if (forceStackingContext && animatedStyle) {
> > -        if (animatedStyle->hasAutoUsedZIndex())
> > -            animatedStyle->setUsedZIndex(0);
> > -    }
> > +    if (animatedStyle)
> > +        Style::Adjuster::adjustAnimatedStyle(*animatedStyle, element, forceStackingContext);
> >  
> 
> Why are we making code changes to the legacy animation engine? Don't we have
> a new one enabled by default? Are these changes tested?

Yes, existing tests hit the new code paths (I saw assertions with legacy animation tests while working on the patch).

> > Source/WebCore/style/StyleAdjuster.cpp:538
> > +static const RenderStyle* parentBoxStyle(const Element& element)
> > +{
> > +    auto* renderer = element.renderer();
> > +    if (!renderer)
> > +        return nullptr;
> > +    
> > +    if (is<RenderBox>(renderer) && is<RenderBox>(renderer->parent()))
> > +        return &renderer->parent()->style();
> > +
> > +    return nullptr;
> > +}
> 
> We can't look into renderers for style during style resolution (which is
> where animations are resolved too). All style changes are resolved in one go
> and applied to the render tree as a separate step. A renderer may have stale
> style, and it might not even have been created yet. If parent box style is
> needed, it needs to passed in, like with other Adjuster functions.


Fixed by doing the work in TreeResolver::createAnimatedElementUpdate() which has access to the parent style.

[Simon Fraser (smfr)]

Created attachment 386720 [details]
Patch

[alan]

Comment on attachment 386720 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=386720&action=review

> Source/WebCore/style/StyleTreeResolver.cpp:322
> +                animationMayHaveChangedStyle = true;

animationMayHaveChangedComputedStyle

> LayoutTests/ChangeLog:22
> [phone]  Simon Fraser  <[e-mail]>
> +
> +        REGRESSION (r252724): Unable to tap on play button on google video 'See the top search trends of 2019'
> +        https://bugs.webkit.org/show_bug.cgi?id=205694
> +        <rdar://problem/58062987>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * animations/z-index-in-keyframe-expected.html: Added.
> +        * animations/z-index-in-keyframe.html: Added.
> +

oops

[Antti Koivisto]

Comment on attachment 386720 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=386720&action=review

> Source/WebCore/style/StyleAdjuster.cpp:536
> +void Adjuster::adjustAnimatedStyle(RenderStyle& style, const RenderStyle& parentStyle, bool forceStackingContext)
> +{
> +    // Set an explicit used z-index in two cases:
> +    // 1. When the element respects z-index, and the style has an explicit z-index set (for example, the animation
> +    //    itself may animate z-index).
> +    // 2. When we want the stacking context side-effets of explicit z-index, via forceStackingContext.
> +    // It's important to not clobber an existing used z-index, since an earlier animation may have set it, but we
> +    // may still need to update the used z-index value from the specified value.
> +    bool elementRespectsZIndex = style.position() != PositionType::Static || parentStyle.isDisplayFlexibleOrGridBox();

I think you want parentBoxStyle instead of parentStyle here since the test appears to be for the parent box type.

> Source/WebCore/style/StyleTreeResolver.cpp:352
> +        Adjuster::adjustAnimatedStyle(*newStyle, parent().style, forceStackingContext);

I think you want to pass parentBoxStyle() here.

They are different when parent().style doesn't generate a box (because it has display:contents).

[Antti Koivisto]

Comment on attachment 386720 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=386720&action=review

> Source/WebCore/dom/Element.h:76
> +struct AnimationImpact {

Alternatively OptionSet

[Antti Koivisto]

Comment on attachment 386720 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=386720&action=review

> Source/WebCore/style/StyleAdjuster.cpp:528
> +void Adjuster::adjustAnimatedStyle(RenderStyle& style, const RenderStyle& parentStyle, bool forceStackingContext)

It would be nicer if this took AnimationImpact instead of a bool.

[Simon Fraser (smfr)]

Created attachment 386794 [details]
Patch

[Antti Koivisto]

Comment on attachment 386794 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=386794&action=review

> Source/WebCore/style/StyleAdjuster.cpp:528
> +void Adjuster::adjustAnimatedStyle(RenderStyle& style, const RenderStyle* parentStyle, OptionSet<AnimationImpact> impact)

Please also rename the variable to parentBoxStyle

[Antti Koivisto]

Comment on attachment 386794 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=386794&action=review

> Source/WebCore/style/StyleTreeResolver.cpp:308
> +    OptionSet<AnimationImpact> animationImpact;
> +    bool animationMayHaveChangedStyle = false;

Shouldn't it be sufficient to test for non-empty animationImpact?

[Simon Fraser (smfr)]

(In reply to Antti Koivisto from comment #14)
> Comment on attachment 386794 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=386794&action=review
> 
> > Source/WebCore/style/StyleTreeResolver.cpp:308
> > +    OptionSet<AnimationImpact> animationImpact;
> > +    bool animationMayHaveChangedStyle = false;
> 
> Shouldn't it be sufficient to test for non-empty animationImpact?

True! First I have to figure out what I broke. The double negatives in Element::applyKeyframeEffects were very confusing and I tried to fix, but I think that broke stuff.

[Simon Fraser (smfr)]

Created attachment 386808 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 386808 [details]
Patch

Rejecting attachment 386808 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 386808, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/13300067

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/254054/webkit