REGRESSION (r275227): Check boxes on V-Safe site flicker when selected

• Component: Animations
• Status: RESOLVED FIXED

[Antoine Quint]

REGRESSION (r275227): Check boxes on V-Safe site flicker when selected

[Antoine Quint]

Created attachment 430389 [details]
Patch

[Antoine Quint]

<rdar://problem/77722651>

[Simon Fraser (smfr)]

Comment on attachment 430389 [details]
Patch

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

> Source/WebCore/rendering/style/StyleCachedImage.cpp:60
> +    if (m_cssValue.ptr() == otherCached.m_cssValue.ptr() || m_cssValue->equals(otherCached.m_cssValue.get()))

Can this use arePointingToEqualData()?

> LayoutTests/webanimations/background-image-css-variable-no-transition.html:18
> +        /* The bug fails to reproduce if the icon is not stored in a variable. */
> +        background-image: var(--checkbox-icon);
> +        /* The bug fails to reproduce without a transition. */

Comments can be removed.

> LayoutTests/webanimations/background-image-css-variable-no-transition.html:32
> +    setTimeout(() => {
> +        document.body.classList.add('changed');
> +        shouldBe("document.getAnimations().length", "0");
> +    });

Did you intend to use the default timeout? Doesn't this need waitUntilDone/notifyDone?

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 430389 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430389&action=review
> 
> > Source/WebCore/rendering/style/StyleCachedImage.cpp:60
> > +    if (m_cssValue.ptr() == otherCached.m_cssValue.ptr() || m_cssValue->equals(otherCached.m_cssValue.get()))
> 
> Can this use arePointingToEqualData()?

To do this I think we'd need to add an operator== for CSSImageValue.

> > LayoutTests/webanimations/background-image-css-variable-no-transition.html:18
> > +        /* The bug fails to reproduce if the icon is not stored in a variable. */
> > +        background-image: var(--checkbox-icon);
> > +        /* The bug fails to reproduce without a transition. */
> 
> Comments can be removed.

Will do.

> > LayoutTests/webanimations/background-image-css-variable-no-transition.html:32
> > +    setTimeout(() => {
> > +        document.body.classList.add('changed');
> > +        shouldBe("document.getAnimations().length", "0");
> > +    });
> 
> Did you intend to use the default timeout? Doesn't this need
> waitUntilDone/notifyDone?

It does, as the stress bot found out.

[Antoine Quint]

(In reply to Antoine Quint from comment #4)
> (In reply to Simon Fraser (smfr) from comment #3)
> > Comment on attachment 430389 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=430389&action=review
> > 
> > > Source/WebCore/rendering/style/StyleCachedImage.cpp:60
> > > +    if (m_cssValue.ptr() == otherCached.m_cssValue.ptr() || m_cssValue->equals(otherCached.m_cssValue.get()))
> > 
> > Can this use arePointingToEqualData()?
> 
> To do this I think we'd need to add an operator== for CSSImageValue.
> 
> > > LayoutTests/webanimations/background-image-css-variable-no-transition.html:18
> > > +        /* The bug fails to reproduce if the icon is not stored in a variable. */
> > > +        background-image: var(--checkbox-icon);
> > > +        /* The bug fails to reproduce without a transition. */
> > 
> > Comments can be removed.
> 
> Will do.
> 
> > > LayoutTests/webanimations/background-image-css-variable-no-transition.html:32
> > > +    setTimeout(() => {
> > > +        document.body.classList.add('changed');
> > > +        shouldBe("document.getAnimations().length", "0");
> > > +    });
> > 
> > Did you intend to use the default timeout? Doesn't this need
> > waitUntilDone/notifyDone?
> 
> It does, as the stress bot found out.

It can, however, be written in a way where setTimeout() is not required at all.

[Antoine Quint]

Created attachment 430398 [details]
Patch

[Antoine Quint]

So a test (webanimations/empty-keyframes-crash.html, added in r233903) regresses because it relied on the buggy behavior fixed here. I'm not sure how to rewrite that test though…

[Antoine Quint]

Created attachment 430400 [details]
Patch

[EWS]

Committed r278376 (238403@main): <https://commits.webkit.org/238403@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430400 [details].