1732211 - Restructure scroll-animations folder and fix the assertions in css/

Status: RESOLVED FIXED

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

Description

[Boris Chiou [:boris]]

It seems we add implementation-status: backlog for the entire wpt of scroll-animations because we have some timeouts and assertions in this folder.

In order to remove the implementation backlog, we have to fix the assertions and timeouts first.

Comment 1

[Boris Chiou [:boris]]

Attachment: Bug 1732211 - Remove implementation-status: backlog for scroll-animations.

1. We may get timeouts because these tests don't call takeSnapshot().
   We have to call it even if ScrollTimeline interface is not
   supported.
2. Some tests in css/ use infinite animation-duration, and this may
   cause an assertion when we multiply infinity(INT64_MAX) and zero.
   (The assertion is in StickyTimeDurationValueCalculator::Multiply()).

So we fix thest two issues in this patch.

Comment 2

[Boris Chiou [:boris]]

Attachment: Bug 1732211 - Restructure the tests in scroll-animations.

Move all tests which use ScrollTimeline interface into scroll-timelines
folder. The tests in this folder are belonging to
https://drafts.csswg.org/scroll-animations-1/#scroll-timelines section.

Gecko doesn't support ScrollTimeline interface, and some reftests which
use this interface may cause timeout because they have js errors.
We would like to skip the entire folder in Gecko until we start to
implement this interface. Besides, moving them into scroll-timelines
folder make more sense because the spec splits the scroll-driven animations
into two parts:

1. scroll timeline DOM APIs. (In scroll-timelines folder)
2. @scroll-timeline at rule. (In css folder, because they also use
   animation-timeline property)

Comment 3

[Boris Chiou [:boris]]

Attachment: Bug 1732211 - Use a large animation-duration instead of 1e10s.

In Gecko debug build, Using the infinite duration (larger than INT64_MAX)
together with requestAnimationFrame may cause an assertion because we may
multiply infinity and zero. This is an undefined behavior, so we intent
to not fix this in Gecko now. Just do a workaround to avoid false alarms
because these assertions are not related to scroll-animations.

Comment 4

[Boris Chiou [:boris]]

Hi Majid,

I would like to update the wpt folder structure of scroll-animations. Now we would like to support its CSS syntax first, i.e. animation-timeline property and @scroll-timline at rule. However, we are not planning to implement ScrollTimeline interface now, so the tests which use this DOM API may cause javescript errors, and so the reftests in scrol-animations/ may timeout.

In Gecko, we skip the entire folder because of these timeouts, and so we also skip the tests in the css folder now. In order to let Gecko CI still run tests in the css folder, I would like to put the tests which use ScrollTimeline interface into a separate folder, scroll-timelines (see https://phabricator.services.mozilla.com/D126727). Just wondering do you have any other concerns about this? Thanks.

Comment 5

[Majid Valipour]

Thanks for the heads up Boris. I no longer work on ScrollTimeline but I checked with kevers@ (https://github.com/kevers-google). He agrees that the restructuring is fine if it makes it easier for your to control which tests run. Blink would need to update their test expectations once this lans in wpt and sync occurs but they are now aware and will do that.

I personally am very excited that this feature is coming to Gecko \o/.

Comment 6

[Boris Chiou [:boris]]

(In reply to Majid Valipour from comment #5)

Thanks for the heads up Boris. I no longer work on ScrollTimeline but I checked with kevers@ (https://github.com/kevers-google). He agrees that the restructuring is fine if it makes it easier for your to control which tests run. Blink would need to update their test expectations once this lans in wpt and sync occurs but they are now aware and will do that.

I personally am very excited that this feature is coming to Gecko \o/.

Thanks, Majid and kevers. I will land these patches soon. :)

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/41911701ed42
Use a large animation-duration instead of 1e10s. r=birtles
https://hg.mozilla.org/integration/autoland/rev/3b86aaae6008
Restructure the tests in scroll-animations. r=hiro

Comment 8

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/41911701ed42
https://hg.mozilla.org/mozilla-central/rev/3b86aaae6008

Comment 9

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Failed to create upstream wpt PR due to merge conflicts. This requires fixup from a wpt sync admin.

Comment 10

[Boris Chiou [:boris]]

Hi James, would you mind checking the wpt sync issue? Thanks.

Comment 11

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/31188 for changes under testing/web-platform/tests

Comment 12

[James Graham [:jgraham]]

I updated it so the PR matches what we ended up with in m-c when fixing the conflict in the other direction. If that didn't end up right, let me know and we can create a new bug with the fix.

Comment 13

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot