Restyle should reflect animations and transitions #26331

mrobinson · 2020-04-27T09:57:24Z

When doing a restyle, we should apply animations and transitions to the
new style so that it is reflected in getComputedStyle() and the new
style information properly cascades. This is the first part of properly
ticking animations and transitions.

This causes a couple new animations tests failures (along with many new
passes), but we currently don't have support for properly handling
animations after they have completed, so this isn't totally unexpected.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #20116

There are tests for these changes OR
These changes do not require tests because ___

highfive · 2020-04-27T09:57:30Z

Heads up! This PR modifies the following files:

@emilio: components/layout/animation.rs, components/style/animation.rs, components/style/matching.rs

nox · 2020-04-27T10:04:54Z

r? @jdm

I don't know anything about animations yet and I'm taking the week off starting tomorrow.

emilio

This looks fine but it'd be nice to avoid cloning the style unconditionally. Maybe just wrapping the relevant blocks in an if !animation_state.is_empty() or such would work.

emilio · 2020-04-27T11:19:40Z

components/style/matching.rs

@@ -445,7 +445,10 @@ trait PrivateMatchMethods: TElement {
        let mut animation_state = animation_states.remove(&this_opaque).unwrap_or_default();

        if let Some(ref mut old_values) = *old_values {
-            animation_state.compute_before_change_style::<Self>(
+            // We convert old values into `before-change-style` here.
+            let old_values = Arc::make_mut(old_values);


So, this will copy the style almost surely, right? It'd be nice to not do that if there are no animations running.

Oh, nice catch! I've modified this so that the methods now take an Arc which will ensure that callers don't accidentally clone the style like I did in this version of the change.

emilio · 2020-04-27T11:19:40Z

components/style/matching.rs

@@ -475,6 +478,20 @@ trait PrivateMatchMethods: TElement {
                .cancel_transitions_with_nontransitioning_properties(&transitioning_properties);
        }

+        {
+            let new_values = Arc::make_mut(new_values);


Same, it seems this will disable all sorts of style sharing optimizations.

Same here. I originally went with an is_empty() check, but ended up doing this in the methods themselves to enable more fine-grained control over when the style is cloned.

mrobinson · 2020-04-27T13:35:29Z

Thanks for the reviews! I'm about to push a new version of this change which switches back to using Arc<ComputedValues> everywhere so that callers of new methods don't accidentally clone styles. It will also eliminate one more use of eager Arc::make_mut that existed prior to this change.

mrobinson · 2020-04-28T17:31:48Z

@bors-servo r=jdm,emilio

bors-servo · 2020-04-28T17:31:49Z

📌 Commit c89f448 has been approved by jdm,emilio

bors-servo · 2020-04-28T17:31:55Z

⌛ Testing commit c89f448 with merge 1274980...


        Auto merge of #26331 - mrobinson:animation-update-on-restyle, r=jdm,e…

…milio Restyle should reflect animations and transitions When doing a restyle, we should apply animations and transitions to the new style so that it is reflected in `getComputedStyle()` and the new style information properly cascades. This is the first part of properly ticking animations and transitions. This causes a couple new animations tests failures (along with many new passes), but we currently don't have support for properly handling animations after they have completed, so this isn't totally unexpected.  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #20116  - [x] There are tests for these changes OR - [ ] These changes do not require tests because ___

bors-servo · 2020-04-28T18:03:33Z

💔 Test failed - status-taskcluster

jdm · 2020-04-28T18:12:59Z

So many new passing tests!

mrobinson · 2020-04-29T11:28:01Z

@bors-servo r=jdm,emilio

Trying this again with updated expectations, but this might take a couple rounds.

jdm · 2020-04-29T16:56:40Z

There's an ini file with a problem:

ParseError: Token 'group_start' doesn't equal expected type 'group_end':  line 28
  File "/repo/python/servo/testing_commands.py", line 342, in test_tidy
    tidy_failed = tidy.scan(not all_files, not no_progress, stylo=stylo, no_wpt=no_wpt)
  File "/repo/python/tidy/servo_tidy/tidy.py", line 1141, in scan
    for error in errors:
  File "/repo/python/tidy/servo_tidy/tidy.py", line 517, in check_manifest_dirs
    p = parser.parse(lines)
  File "/repo/tests/wpt/web-platform-tests/tools/wptrunner/wptrunner/wptmanifest/parser.py", line 789, in parse
    return p.parse(stream)
  File "/repo/tests/wpt/web-platform-tests/tools/wptrunner/wptrunner/wptmanifest/parser.py", line 540, in parse
    self.manifest()
  File "/repo/tests/wpt/web-platform-tests/tools/wptrunner/wptrunner/wptmanifest/parser.py", line 564, in manifest
    self.data_block()
  File "/repo/tests/wpt/web-platform-tests/tools/wptrunner/wptrunner/wptmanifest/parser.py", line 585, in data_block
    self.data_block()
  File "/repo/tests/wpt/web-platform-tests/tools/wptrunner/wptrunner/wptmanifest/parser.py", line 586, in data_block
    self.eof_or_end_group()
  File "/repo/tests/wpt/web-platform-tests/tools/wptrunner/wptrunner/wptmanifest/parser.py", line 591, in eof_or_end_group
    self.expect(token_types.group_end)
  File "/repo/tests/wpt/web-platform-tests/tools/wptrunner/wptrunner/wptmanifest/parser.py", line 555, in expect
    "Token '{}' doesn't equal expected type '{}'".format(self.token[0], type))


        Skip various CSS animation tests directories

These are all going to be flaky until we get further along with the transition and animation improvements. The flakiness is due to the fact that it is more likely that these tests pass now.

jdm · 2020-04-29T16:57:46Z

tests/wpt/include.ini

@@ -19,20 +19,34 @@ skip: true
    skip: false
  [CSS2]
    skip: false
+    [linebox]
+      skip:false


Possibly the missing space here?

Looks like I had an extra level of indentation in the css-align and css-color sections. Should be fixed now.

mrobinson · 2020-04-29T17:21:00Z

@bors-servo try=wpt-mac

It looked like the tidy precheck passed without issues, so I feel a bit safer about triggering this now.

bors-servo · 2020-04-29T17:21:06Z

⌛ Trying commit fb9c352 with merge 258654e...


        Auto merge of #26331 - mrobinson:animation-update-on-restyle, r=<try>

Restyle should reflect animations and transitions When doing a restyle, we should apply animations and transitions to the new style so that it is reflected in `getComputedStyle()` and the new style information properly cascades. This is the first part of properly ticking animations and transitions. This causes a couple new animations tests failures (along with many new passes), but we currently don't have support for properly handling animations after they have completed, so this isn't totally unexpected.  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #20116  - [x] There are tests for these changes OR - [ ] These changes do not require tests because ___

bors-servo · 2020-04-29T18:45:31Z

💔 Test failed - status-taskcluster

mrobinson · 2020-04-29T18:51:30Z

@bors-servo r=jdm,emilio

The WPT failure is:

UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 58: ordinal not in range(128)

bors-servo · 2020-04-29T18:51:30Z

📌 Commit fb9c352 has been approved by jdm,emilio

jdm · 2020-04-29T18:57:39Z

@mrobinson For future reference, look at the filtered-wpt-errorsummary.log file instead of the live log.

mrobinson · 2020-04-29T19:01:53Z

@jdm Thanks for the pointer. Looks like the real failure during the try was #25513.

bors-servo · 2020-04-29T19:57:54Z

⌛ Testing commit fb9c352 with merge 5925d42...


        Auto merge of #26331 - mrobinson:animation-update-on-restyle, r=jdm,e…

…milio Restyle should reflect animations and transitions When doing a restyle, we should apply animations and transitions to the new style so that it is reflected in `getComputedStyle()` and the new style information properly cascades. This is the first part of properly ticking animations and transitions. This causes a couple new animations tests failures (along with many new passes), but we currently don't have support for properly handling animations after they have completed, so this isn't totally unexpected.  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #20116  - [x] There are tests for these changes OR - [ ] These changes do not require tests because ___

bors-servo · 2020-04-29T20:39:34Z

💔 Test failed - status-taskcluster

jdm · 2020-04-29T21:52:28Z

@bors-servo retry

bors-servo · 2020-04-29T22:06:26Z

⌛ Testing commit fb9c352 with merge d1279eb...

bors-servo · 2020-04-29T22:26:28Z

☀️ Test successful - status-taskcluster
Approved by: jdm,emilio
Pushing d1279eb to master...

mrobinson requested review from jdm and emilio Apr 27, 2020

highfive assigned nox Apr 27, 2020

highfive added the S-awaiting-review label Apr 27, 2020

mrobinson added the A-layout/animations label Apr 27, 2020

mrobinson force-pushed the mrobinson:animation-update-on-restyle branch from 73462ea to 3321960 Apr 27, 2020

highfive assigned jdm and unassigned nox Apr 27, 2020

emilio approved these changes Apr 27, 2020

View changes

mrobinson force-pushed the mrobinson:animation-update-on-restyle branch from 3321960 to 08fd1af Apr 27, 2020

jdm approved these changes Apr 27, 2020

View changes

mrobinson force-pushed the mrobinson:animation-update-on-restyle branch from 08fd1af to c89f448 Apr 27, 2020

mrobinson mentioned this pull request Apr 27, 2020

Improvements to the CSS Transitions and Animations implementation #26328

Closed

20 of 21 tasks complete

highfive added S-awaiting-merge and removed S-awaiting-review labels Apr 28, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels Apr 28, 2020

mrobinson force-pushed the mrobinson:animation-update-on-restyle branch from c89f448 to 95adeee Apr 29, 2020

highfive added S-awaiting-review and removed S-tests-failed labels Apr 29, 2020

Skip various CSS animation tests directories

Loading status checks…

fb9c352

These are all going to be flaky until we get further along with the transition and animation improvements. The flakiness is due to the fact that it is more likely that these tests pass now.

jdm reviewed Apr 29, 2020

View changes

mrobinson force-pushed the mrobinson:animation-update-on-restyle branch from 44f306e to fb9c352 Apr 29, 2020

highfive added the S-tests-failed label Apr 29, 2020

highfive added S-awaiting-merge and removed S-awaiting-review S-tests-failed labels Apr 29, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels Apr 29, 2020

highfive added S-awaiting-merge and removed S-tests-failed labels Apr 29, 2020

bors-servo merged commit d1279eb into servo:master Apr 29, 2020
2 checks passed

2 checks passed

Community-TC (pull_request) TaskGroup: success
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Apr 29, 2020

mrobinson mentioned this pull request May 14, 2020

Is CSS transition on font-size property implemented? #12745

Closed

servo / servo

Restyle should reflect animations and transitions #26331

Restyle should reflect animations and transitions #26331

mrobinson commented Apr 27, 2020 •

edited

highfive commented Apr 27, 2020

nox commented Apr 27, 2020

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mrobinson commented Apr 27, 2020

mrobinson commented Apr 28, 2020

bors-servo commented Apr 28, 2020

bors-servo commented Apr 28, 2020

bors-servo commented Apr 28, 2020

jdm commented Apr 28, 2020

mrobinson commented Apr 29, 2020

jdm commented Apr 29, 2020

This comment has been minimized.

This comment has been minimized.

mrobinson commented Apr 29, 2020

bors-servo commented Apr 29, 2020

bors-servo commented Apr 29, 2020

mrobinson commented Apr 29, 2020

bors-servo commented Apr 29, 2020

jdm commented Apr 29, 2020

mrobinson commented Apr 29, 2020

bors-servo commented Apr 29, 2020

bors-servo commented Apr 29, 2020

jdm commented Apr 29, 2020

bors-servo commented Apr 29, 2020

bors-servo commented Apr 29, 2020

servo / servo

Join GitHub today

Restyle should reflect animations and transitions #26331

Restyle should reflect animations and transitions #26331

Conversation

mrobinson commented Apr 27, 2020 • edited

highfive commented Apr 27, 2020

nox commented Apr 27, 2020

emilio left a comment

This comment has been minimized.

emilio Apr 27, 2020

This comment has been minimized.

mrobinson Apr 27, 2020

This comment has been minimized.

emilio Apr 27, 2020

This comment has been minimized.

mrobinson Apr 27, 2020

mrobinson commented Apr 27, 2020

mrobinson commented Apr 28, 2020

bors-servo commented Apr 28, 2020

bors-servo commented Apr 28, 2020

bors-servo commented Apr 28, 2020

jdm commented Apr 28, 2020

mrobinson commented Apr 29, 2020

jdm commented Apr 29, 2020

This comment has been minimized.

jdm Apr 29, 2020

This comment has been minimized.

mrobinson Apr 29, 2020

mrobinson commented Apr 29, 2020

bors-servo commented Apr 29, 2020

bors-servo commented Apr 29, 2020

mrobinson commented Apr 29, 2020

bors-servo commented Apr 29, 2020

jdm commented Apr 29, 2020

mrobinson commented Apr 29, 2020

bors-servo commented Apr 29, 2020

bors-servo commented Apr 29, 2020

jdm commented Apr 29, 2020

bors-servo commented Apr 29, 2020

bors-servo commented Apr 29, 2020

mrobinson commented Apr 27, 2020 •

edited