1925324 - SVG/SMIL animation of 'display' with to='none' (leaving 'from' unspecified/implied) causes the element to fail-to-reappear during repeats of the animation

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

STR:

1. Load attached testcase.

EXPECTED RESULTS:
All lines should blink in the same way.

ACTUAL RESULTS:
When the animations start, the line for Blinking by animating 'display' with 'to' remains hidden through the whole animation.

Regression pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5207b1392b11db534550a5eb801302e6dbb58f95&tochange=9a2eac450781b026b42c44ca8f0f92bb0846b6e2

--> Regression from bug 1458814 (which is mildly unexpected since I think that bug was meant to be an optimization with no behavior impact).

This is is the remaining piece of bug 536454 (which is otherwise mostly-fixed). (I spun off a new bug for this since it was a new regression long after bug 536454 was filed.)

Other browsers seem to do worse than we do on this testcase, FWIW (leaving more lines than us hidden during the animation).

Comment 1

[Daniel Holbert [:dholbert]]

(Clarifying summary -- the bug here is that, when the animation repeats, we should put display back at its initial value for the first half of each repeat iteration. But instead, it looks like we're leaving it at none for the whole time.)

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

:emilio, since you are the author of the regressor, bug 1458814, could you take a look?

For more information, please visit BugBot documentation.

Comment 3

[Daniel Holbert [:dholbert]]

(Treat the bugbot needinfo is low-priority, given that other browsers do worse than us here. But if you've got ideas about what might be going on, that's great too. Looking at the commits in the regressor, it's not yet clear to me how/why this would have broken.)

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1925324 - Properly regenerate the underlying 'from' value, for SVG/SMIL to-animation of CSS properties

We need to remove the property in that case. Any chance I could convince
you to write a test, since you're more familiar than me with the SMIL
testing infrastructure (and SMIL in general)?

Otherwise I can give it a shot tomorrow.

Comment 5

[Robert Longson [:longsonr]]

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

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: Bug 1925324: Add test for a SVG to-animation (to "display:none") being sampled at underlying value. r?longsonr

Thanks to Robert Longson for writing an initial prototype-WPT that this WPT is
based on.

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Robert Longson [:longsonr] from comment #5)

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

The test in this^ Try run had some trouble and needed a somewhat-flaky (and lint-rejected) setTimeout in order to work, so I rewrote things a bit to make it more robust -- see just-attached phab patch.

Comment 8

[Daniel Holbert [:dholbert]]

Try run with the updated test (comment 6):
https://treeherder.mozilla.org/jobs?repo=try&revision=4d0cdf76177b12870df32746d1097eb4d50f8577

Locally, the test seems quite reliable, and it fails without the actual bugfix vs. passes with the bugfix (as you would hope).

Comment 9

[Daniel Holbert [:dholbert]]

Here's a try run with the test (from comment 6) but without the bugfix, where hopefully the test should fail across the board:
https://treeherder.mozilla.org/jobs?repo=try&revision=3aca230def8c765bc179c211431b0ba834dcfab0

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/693f59f0b6cf
Properly regenerate the underlying 'from' value, for SVG/SMIL to-animation of CSS properties r=dholbert

Comment 11

[Daniel Holbert [:dholbert]]

Try runs for the simplified reftest-style WPT in latest phab revision:
With bugfix (expected green):
https://treeherder.mozilla.org/jobs?repo=try&revision=369955660670dd463d453809669eb21bcf9999cf

Without bugfix (expected orange):
https://treeherder.mozilla.org/jobs?repo=try&revision=d98f1493f27dfbbfc7089a9c48b677383c174b37

Comment 12

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/693f59f0b6cf

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b378432dac5f
Add test for a SVG to-animation (to "display:none") being sampled at underlying value. r=longsonr

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/48711 for changes under testing/web-platform/tests

Comment 15

[Sandor Molnar[:smolnar]]

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

Comment 16

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

Upstream PR merged by moz-wptsync-bot