Skip to content

/ servo

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add animation and transition support for pseudo-elements #26923

Merged
merged 1 commit into from Jun 16, 2020
Merged

Add animation and transition support for pseudo-elements #26923

merged 1 commit into from Jun 16, 2020

Conversation

@mrobinson
Copy link
Member

mrobinson commented Jun 15, 2020 •
edited

This change extends the DocumentAnimationSet to hold animations for
pseudo-elements. Since pseudo-elements in Servo are not in the DOM like
in Gecko, they need to be handled a bit carefully in stylo. When a
pseudo-element has an animation, recascade the style. Finally, this
change passes the pseudo-element string properly to animation events.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #10316
  • There are tests for these changes
@highfive
Copy link

highfive commented Jun 15, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/animations.rs, components/script/dom/bindings/trace.rs
  • @KiChjang: components/script/animations.rs, components/script/dom/bindings/trace.rs
  • @emilio: components/style/servo/selector_parser.rs, components/style/matching.rs, components/style/dom.rs, components/style/gecko/wrapper.rs, components/style/animation.rs and 2 more
@mrobinson mrobinson mentioned this pull request Jun 15, 2020
Closed
20 of 21 tasks complete
@jdm
jdm reviewed Jun 15, 2020
View changes
components/style/animation.rs Outdated Show resolved Hide resolved
components/layout_thread_2020/dom_wrapper.rs Outdated Show resolved Hide resolved
tests/wpt/metadata/css/css-animations/animationevent-pseudoelement.html.ini Outdated Show resolved Hide resolved
tests/wpt/metadata/css/css-transitions/pseudo-elements-001.html.ini
@@ -2,12 +2,6 @@
[transition padding-left on :before / values]

This comment has been minimized.

Sign in to view
Copy link
@jdm

jdm Jun 15, 2020

Member

What does it mean that this subtest fails?

This comment has been minimized.

Sign in to view
Copy link
@mrobinson

mrobinson Jun 16, 2020 •
edited

Author Member

It looks like this test triggers a bug in Layout 2013, where pseudo content that has an empty content string is not represented properly in the flow tree. I've confirmed that the tests pass when these test cases use a non-empty content string.
components/style/matching.rs Outdated Show resolved Hide resolved
@emilio
emilio reviewed Jun 15, 2020
View changes
components/layout_2020/flow/root.rs Outdated Show resolved Hide resolved
components/style/animation.rs Outdated Show resolved Hide resolved
components/style/matching.rs Outdated Show resolved Hide resolved
components/style/style_resolver.rs Show resolved Hide resolved
components/style/style_resolver.rs Outdated Show resolved Hide resolved
@mrobinson mrobinson force-pushed the mrobinson:pseudo-animations-2 branch from f8397f0 to d53782b Jun 16, 2020
@mrobinson mrobinson changed the title [WIP] Add animation and transition support for pseudo-elements Add animation and transition support for pseudo-elements Jun 16, 2020
@mrobinson mrobinson marked this pull request as ready for review Jun 16, 2020
@mrobinson
Copy link
Member Author

mrobinson commented Jun 16, 2020

Thanks for the reviews. I've incorporated your suggested change and this PR should be ready to review and merge whenever.
@emilio
emilio approved these changes Jun 16, 2020
View changes
Copy link
Member

emilio left a comment

This looks pretty sensible to me, with a follow-up PR (or at least issue on file) to unify those structs. Thanks!
components/style/animation.rs Show resolved Hide resolved
components/style/animation.rs Outdated Show resolved Hide resolved
components/style/style_resolver.rs Show resolved Hide resolved
components/style/servo/selector_parser.rs Show resolved Hide resolved
@mrobinson
Add animation and transition support for pseudo-elements
Loading status checks…
f3e373b 
This change extends the DocumentAnimationSet to hold animations for
pseudo-elements. Since pseudo-elements in Servo are not in the DOM like
in Gecko, they need to be handled a bit carefully in stylo.  When a
pseudo-element has an animation, recascade the style. Finally, this
change passes the pseudo-element string properly to animation events.

Fixes: #10316
@mrobinson mrobinson force-pushed the mrobinson:pseudo-animations-2 branch from d53782b to f3e373b Jun 16, 2020
@mrobinson
Copy link
Member Author

mrobinson commented Jun 16, 2020

@bors-servo r=emilio

Thanks again for the reviews!
@bors-servo
Copy link
Contributor

bors-servo commented Jun 16, 2020

📌 Commit f3e373b has been approved by emilio
@highfive highfive assigned emilio and unassigned jdm Jun 16, 2020
bors-servo added a commit that referenced this pull request Jun 16, 2020
@bors-servo
Auto merge of #26923 - mrobinson:pseudo-animations-2, r=emilio
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
f67b75f 
Add animation and transition support for pseudo-elements

This change extends the DocumentAnimationSet to hold animations for
pseudo-elements. Since pseudo-elements in Servo are not in the DOM like
in Gecko, they need to be handled a bit carefully in stylo.  When a
pseudo-element has an animation, recascade the style. Finally, this
change passes the pseudo-element string properly to animation events.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #10316
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 16, 2020

⌛ Testing commit f3e373b with merge f67b75f...
@bors-servo
Copy link
Contributor

bors-servo commented Jun 16, 2020

💔 Test failed - status-taskcluster
@mrobinson
Copy link
Member Author

mrobinson commented Jun 16, 2020

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Jun 16, 2020

⌛ Testing commit f3e373b with merge a84b060...
@bors-servo
Copy link
Contributor

bors-servo commented Jun 16, 2020

☀️ Test successful - status-taskcluster
Approved by: emilio
Pushing a84b060 to master...
@bors-servo bors-servo merged commit a84b060 into servo:master Jun 16, 2020
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
@mrobinson mrobinson deleted the mrobinson:pseudo-animations-2 branch Jun 16, 2020
@mrobinson mrobinson mentioned this pull request Jun 16, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdm jdm

@emilio emilio

Assignees

@emilio emilio

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Support pseudo elements for transition and animations
5 participants
@mrobinson @highfive @bors-servo @jdm @emilio
You can’t perform that action at this time.