Bug 226549 - REGRESSION (r275227): Check boxes on V-Safe site flicker when selected
Summary: REGRESSION (r275227): Check boxes on V-Safe site flicker when selected
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-06-02 12:25 PDT by Antoine Quint
Modified: 2021-06-02 15:33 PDT (History)
11 users (show)

See Also:


Attachments
Patch (4.58 KB, patch)
2021-06-02 12:27 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (4.70 KB, patch)
2021-06-02 13:46 PDT, Antoine Quint
simon.fraser: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (5.71 KB, patch)
2021-06-02 13:56 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2021-06-02 12:25:48 PDT
REGRESSION (r275227): Check boxes on V-Safe site flicker when selected
Comment 1 Antoine Quint 2021-06-02 12:27:41 PDT
Created attachment 430389 [details]
Patch
Comment 2 Antoine Quint 2021-06-02 12:27:46 PDT
<rdar://problem/77722651>
Comment 3 Simon Fraser (smfr) 2021-06-02 12:32:12 PDT
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?
Comment 4 Antoine Quint 2021-06-02 13:33:24 PDT
(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.
Comment 5 Antoine Quint 2021-06-02 13:45:24 PDT
(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.
Comment 6 Antoine Quint 2021-06-02 13:46:03 PDT
Created attachment 430398 [details]
Patch
Comment 7 Antoine Quint 2021-06-02 13:50:00 PDT
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…
Comment 8 Antoine Quint 2021-06-02 13:56:59 PDT
Created attachment 430400 [details]
Patch
Comment 9 EWS 2021-06-02 14:51:24 PDT
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].