Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upInclude animations and transitions in the cascade #26806
Conversation
highfive
commented
Jun 5, 2020
|
Heads up! This PR modifies the following files:
|
| RuleInclusion::All, | ||
| PseudoElementResolution::IfApplicable, | ||
| ); | ||
| let new_primary = resolver.resolve_style_with_default_parents(); |
This comment has been minimized.
This comment has been minimized.
emilio
Jun 5, 2020
Member
I'm a bit confused about this. Why does this need to resolve_*, then re-cascade?
This comment has been minimized.
This comment has been minimized.
mrobinson
Jun 8, 2020
Author
Member
You're right. Re-cascading here is bogus. I've fixed this in the latest version of the branch.
This comment has been minimized.
This comment has been minimized.
emilio
Jun 8, 2020
Member
So... I'm still a bit unclear on why this is the right thing to do. This is not just recascading but also re-selector-matching. I'd expect this to do basically just this.
And to be clear, I think this is fine as a stepping-stone if needed, but I don't understand why something like that wouldn't work.
This comment has been minimized.
This comment has been minimized.
mrobinson
Jun 8, 2020
Author
Member
You're right. I think I was trying this before, but not properly replacing rules. I'll need to do it a bit more carefully because replace_rules is written with the animations-only restyle in mind. Thanks for the feedback.
|
@emilio, thanks for taking a look at this. I've addressed your comment and also have made a few changes in order to avoid re-resolving the style when animations or transitions aren't changing. |
|
I've pushed a new commit to the branch that replaces the transition and animations rules directly and then does a re-cascade. |
Instead of applying animations and transitions to styled elements, include them in the cascade. This allows them to interact properly with things like font-size and !important rules.
|
I have a pushed a new version of the branch that uses |
|
Thank you, this looks pretty sensible! |
| } | ||
|
|
||
| fn cancel_active_transitions(&mut self) { | ||
| self.dirty = !self.transitions.is_empty(); |
This comment has been minimized.
This comment has been minimized.
emilio
Jun 10, 2020
Member
Does this dirty too much? Should it just dirty right before the transition.state = assignment?
This comment has been minimized.
This comment has been minimized.
mrobinson
Jun 10, 2020
Author
Member
Yep. This could be better. This now sets the dirty bit right before the change.
| } | ||
|
|
||
| // If we have modified animation or transitions, we recascade style for this node. | ||
| let mut rule_node = new_resolved_styles.primary.style.0.rules().clone(); |
This comment has been minimized.
This comment has been minimized.
emilio
Jun 10, 2020
Member
May be worth exposing a ResolvedElementStyles::primary_style() or something so that this reads a bit better, but no big deal.
This comment has been minimized.
This comment has been minimized.
mrobinson
Jun 10, 2020
Author
Member
Okay. I've exposed ResolvedElementStyles::primary_style() and ResolvedElementStyles::primary_style_mut(). It does read better now.
| .as_ref() | ||
| .map(|a| a.borrow_arc()), | ||
| &mut rule_node, | ||
| ); |
This comment has been minimized.
This comment has been minimized.
emilio
Jun 10, 2020
Member
May be worth either asserting that rule_node is not the primary style's rule node anymore, or check for that and bail out.
This comment has been minimized.
This comment has been minimized.
mrobinson
Jun 10, 2020
Author
Member
This makes sense. I've changed it to return early if the rule nodes are equal after replacing them.
mrobinson
force-pushed the
mrobinson:animations-cascade
branch
2 times, most recently
from
dfdfcc5
to
f6ca6cf
|
@bors-servo r=emilio |
|
|
|
Thanks for the review! |
Include animations and transitions in the cascade Instead of applying animations and transitions to styled elements, include them in the cascade. This allows them to interact properly with things like font-size and !important rules. <!-- Please describe your changes on the following line: --> --- <!-- 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] 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. -->
|
|
Yay! |
When animations and transitions change don't always re-resolve node style, just replace the animation and transition rules and re-cascade.
|
@bors-servo r=emilio I've updated the branch to reflect the newly passing tests. |
|
|
Include animations and transitions in the cascade Instead of applying animations and transitions to styled elements, include them in the cascade. This allows them to interact properly with things like font-size and !important rules. <!-- Please describe your changes on the following line: --> --- <!-- 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] 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 retry
|
Include animations and transitions in the cascade Instead of applying animations and transitions to styled elements, include them in the cascade. This allows them to interact properly with things like font-size and !important rules. <!-- Please describe your changes on the following line: --> --- <!-- 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] 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 retry |
|
@bors-servo retry mac1 was having a bad day, so I restarted it. |
|
|
mrobinson commentedJun 5, 2020
Instead of applying animations and transitions to styled elements,
include them in the cascade. This allows them to interact properly with
things like font-size and !important rules.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors