Bug 231906 - REGRESSION(r284313): ::marker accelerated animations are broken
Summary: REGRESSION(r284313): ::marker accelerated animations are broken
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: 230008
  Show dependency treegraph
 
Reported: 2021-10-18 12:45 PDT by Tim Nguyen (:ntim)
Modified: 2021-10-22 14:02 PDT (History)
6 users (show)

See Also:


Attachments
Patch (32.42 KB, patch)
2021-10-19 06:37 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch for landing (32.90 KB, patch)
2021-10-19 09:17 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 Tim Nguyen (:ntim) 2021-10-18 12:45:46 PDT
<style>
@keyframes animate-marker {
  from {
    color: red;
    opacity: 1;
  }
  50% {
    color: green;
    opacity: 0;
  }
  to {
    color: blue;
    opacity: 1;
  }
}

::marker {
  animation: animate-marker 2s infinite; 
}
</style>

<li>Hello</li>

---

In the testcase, the marker disappears completely. Per spec, opacity should not apply at all, so the marker should be visible. But even if we did allow applying opacity, the opacity should animate properly.

regression of the `Styleable::fromRenderer` changes in bug 230008 for ::marker
Comment 1 Radar WebKit Bug Importer 2021-10-18 12:45:57 PDT
<rdar://problem/84383279>
Comment 2 Tim Nguyen (:ntim) 2021-10-18 12:47:28 PDT
This is the spec that says opacity should not apply on ::marker: https://www.w3.org/TR/css-lists-3/#marker-properties
Comment 3 Antoine Quint 2021-10-19 06:37:01 PDT
Created attachment 441718 [details]
Patch
Comment 4 Antti Koivisto 2021-10-19 08:38:51 PDT
Comment on attachment 441718 [details]
Patch

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

> Source/WebCore/style/PropertyAllowlist.cpp:41
> +// https://www.w3.org/TR/css-pseudo-4/#marker-pseudo (Editor's Draft, 25 July 2017)
> +// FIXME: this is outdated, see https://bugs.webkit.org/show_bug.cgi?id=218791.
> +bool isValidMarkerStyleProperty(CSSPropertyID id)

It sounded from spec reading in slack that this allow list is a wrong approach. All properties should be visible in the computed style and the rendering code should be ignoring whatever is disallowed.

This refactoring ends up being bit useless if we just delete all code here soon but I suppose it doesn't hurt.
Comment 5 Antoine Quint 2021-10-19 09:17:06 PDT
Created attachment 441737 [details]
Patch for landing
Comment 6 EWS 2021-10-19 10:18:13 PDT
Committed r284466 (243226@main): <https://commits.webkit.org/243226@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 441737 [details].
Comment 7 Simon Fraser (smfr) 2021-10-22 11:44:02 PDT
The test is flakey: bug 232040.
Comment 8 Simon Fraser (smfr) 2021-10-22 14:02:06 PDT
The testcase shows an opacity animation triggering paints on the ::marker, which seems wrong.