1817641 - Change computed/animated/resolved CSS colors to use AbsoluteColor

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, task)

Comment 1

[Tiaan Louw [:tlouw]]

Attachment: Bug 1817641 - Convert RGBA to AbsoluteColor for computed/animated/resolved CSS colors r=emilio,#layout-reviewers

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Ok, so Tiaan asked me to look at a couple of the remaining failures, and I think they are not a bug in his patch. It seems a general issue that happens to be uncovered by it.

Try run: https://treeherder.mozilla.org/jobs?repo=try&resultStatus=testfailed%2Cbusted%2Cexception%2Csuccess%2Cusercancel%2Crunning%2Cpending%2Crunnable&revision=a3d15d35dc536424e2007f974c68084e07fa47b6

I've looked at the WPT failures, not at dom/animation/test/chrome/test_animation_observers_sync.html (that still needs some debugging). But the WPT failure failure / timeouts seem to be because the reftest harness takes too long to take the screenshot. Explanation of what's going on below:

• Before Tiaan's patch, colors in the computed style are stored as (r: u8, g: u8, b: u8, a: f32). Tiaan is making them all floats.
• The WPT tests follow the same pattern where they have a background-color animation, then perform some action, then eventually explicitly calls takeScreenshot().
• The test harness seems to wait for a stable paint.
• Before the patch, the rounding to u8 for the color components caused some frames where the effective computed style didn't change, so we didn't trigger a main-thread paint / MozAfterPaintPending, and the reftest finished. But after the patch the f32 values change constantly (as expected), and thus the reftest harness waits too long or forever to take an screenshot.

Example hackaround/incorrect patch that "fixes" the issue:

diff --git a/layout/style/nsStyleStruct.cpp b/layout/style/nsStyleStruct.cpp
index f66ec79a9dfc4..f3ef3d46b528a 100644
--- a/layout/style/nsStyleStruct.cpp
+++ b/layout/style/nsStyleStruct.cpp
@@ -2108,7 +2108,8 @@ void nsStyleBackground::TriggerImageLoads(Document& aDocument,
 nsChangeHint nsStyleBackground::CalcDifference(
     const nsStyleBackground& aNewData) const {
   nsChangeHint hint = nsChangeHint(0);
-  if (mBackgroundColor != aNewData.mBackgroundColor) {
+  if (mBackgroundColor.CalcColor(NS_TRANSPARENT) !=
+      aNewData.mBackgroundColor.CalcColor(NS_TRANSPARENT)) {
     hint |= nsChangeHint_RepaintFrame;
   }

So, questions:

• For James: Should the reftest harness special-case "takeScreenshot() was called" as a signal to take the screenshot unconditionally after the next paint? That'd fix those failures and seems like it'd be more in line with test author's expectations.
• For Hiro: Why isn't the animation on the main thread throttled anyways (which would also paper over this)? Shouldn't we animate background-color in the compositor? As I write this I seem to vaguely recall us not throttling finite transitions, is that what's going on? If so that's a bit unfortunate.

cc'ing Timothy and Daniel as well since IIRC they are also familiar with how the Gecko reftests deal with this, in case anything like this rings a bell.

Comment 3

[Emilio Cobos Álvarez [:emilio]]

The chrome test failure seems similar, insofar it expects this not to trigger a transition but it does because of the subtle floating point differences...

Comment 4

[Hiroyuki Ikezoe (:hiro)]

I would say the tests are not so well sophisticated.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)

• For Hiro: Why isn't the animation on the main thread throttled anyways (which would also paper over this)? Shouldn't we animate background-color in the compositor? As I write this I seem to vaguely recall us not throttling finite transitions, is that what's going on? If so that's a bit unfortunate.

It should be running on the compositor, and I confirmed at least the background-color animation in background-color-animation-fallback-additive-keyframe.html is running on the compositor.

The test author seemed to expect that using a relatively long duration, 1000000ms, would result an unchanged rendering result. That's true, but from the perspective of the reftest harness, that's not considered as an unchanged result. There are repeatedly fired MozAfterPaint events. In the era of our old layer backend, we have layer invalidation stufff so that we didn't keep posting MozAfterPaint events in such cases, but WebRender does.

If I needed to write these tests, I'd use step-end to avoid this issue.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

The chrome test failure seems similar, insofar it expects this not to trigger a transition but it does because of the subtle floating point differences...

oof. Can we somehow infer the exact value in question? Looks like the answer is no..

Comment 6

[Emilio Cobos Álvarez [:emilio]]

Not really because for legacy reasons those colors serialize clamped :'(

Comment 7

[Timothy Nikkel (:tnikkel)]

If the tests can't be written so that they don't need to take a screenshot during an active animation, then perhaps we could have a variant of takeScreenshot that ignores pending paints and just takes a screenshot immediately? Changing takeScreenshot to never wait seems like it would cause more problems.

Comment 8

[Tiaan Louw [:tlouw]]

So using a step(2, end) in the timing function fixed most of the failures.

This is the new try run:

https://treeherder.mozilla.org/jobs?repo=try&revision=04a647470c26e32d4732da436895386b9f7a406c

As for test_animation_observers_sync.html what other property would be a good candidate for testing the animation behaviour?

Comment 9

[James Graham [:jgraham]]

So from the point of view of the harness, the whole "wait for a stable state" thing is a bit of a gecko implemenation detail which we assume will usually produce more correct results than taking the screenshot when waiting for a paint, but which we don't have a good cross-browser way to talk about. So we could make e.g. takeScreenshot do something to signal that we shouldn't wait (maybe with a parameter to not affect existing tests, but one could experiment with that), but I don't know to what extent that would generalise to other browsers.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

(In reply to Tiaan Louw from comment #8)

So using a step(2, end) in the timing function fixed most of the failures.

This is the new try run:

https://treeherder.mozilla.org/jobs?repo=try&revision=04a647470c26e32d4732da436895386b9f7a406c

As for test_animation_observers_sync.html what other property would be a good candidate for testing the animation behaviour?

Opacity?

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Maybe opacity has the same comparison issue, so z-index would be one of the candidates?

Comment 12

[Emilio Cobos Álvarez [:emilio]]

Yeah, z-index or any integer property would do.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cafdc2468b86
Convert RGBA to AbsoluteColor for computed/animated/resolved CSS colors r=emilio

Comment 14

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

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

Comment 15

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/cafdc2468b86

Comment 16

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

Upstream PR merged by moz-wptsync-bot