Implement media element pseudo-classes [chromium/src : main]
Philip Jägenstedt (Gerrit)
Message from Philip Jägenstedt![]( "Open in Gerrit")
Set Ready For Review
Related details
- Philip Jägenstedt
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Jägenstedt (Gerrit)
Philip Jägenstedt added 1 comment![]( "Open in Gerrit")
This implementation is fairly complete, but I've opened https://github.com/whatwg/html/issues/11958 about one detail. I'd like the spec to change and will follow up with another CL and test if so.
Related details
- Dale Curtis
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Jägenstedt (Gerrit)
Philip Jägenstedt added 1 comment![]( "Open in Gerrit")
This implementation is fairly complete, but I've opened https://github.com/whatwg/html/issues/11958 about one detail. I'd like the spec to change and will follow up with another CL and test if so.
Oops, I forgot to upload my latest changes, doing that now.
Rune Lillesveen (Gerrit)
Rune Lillesveen added 2 comments![]( "Open in Gerrit")
PseudoStateChanged(CSSSelector::kPseudoStalled);There are several PseudoStateChanged() calls not exercised by tests (and code coverage check complaining). Could maybe have some more wpts for pseudo state changes?
Related details
- Dale Curtis
- Philip Jägenstedt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Jägenstedt (Gerrit)
Philip Jägenstedt added 2 comments![]( "Open in Gerrit")
Philip JägenstedtPerhaps mention which pseudo-classes?
Done
PseudoStateChanged(CSSSelector::kPseudoStalled);There are several PseudoStateChanged() calls not exercised by tests (and code coverage check complaining). Could maybe have some more wpts for pseudo state changes?
Yes, I'll try removing them one by one and seeing which don't fail any tests. Also the tests are failing on the bots but passed for me locally, so I need to investigate that.
Related details
- Dale Curtis
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dale Curtis
| Code-Review | +1 |
1 comment
media code changes lgtm, but tests seem unhappy.
Related details
- Philip Jägenstedt
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Jägenstedt (Gerrit)
Philip Jägenstedt added 1 comment![]( "Open in Gerrit")
return MatchesBufferingPseudo() && sent_stalled_event_;Thanks to https://github.com/whatwg/html/issues/12145 I realized that I should check `MatchesBufferingPseudo()` here and not just `sent_stalled_event_`. That makes the invalidation more complicated, however.
futhark@ do you have any general guidance for how precise invalidation needs to be? Is it worth carefully ensuring that `PseudoStateChanged()` is only called when it's really changed, or can one take the approach of calling it whenever it might have changed to reduce bookkeeping?
(Note that the tests are still wrong, so this CL isn't ready for review.)
Related details
- Dale Curtis
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 1 comment![]( "Open in Gerrit")
return MatchesBufferingPseudo() && sent_stalled_event_;Thanks to https://github.com/whatwg/html/issues/12145 I realized that I should check `MatchesBufferingPseudo()` here and not just `sent_stalled_event_`. That makes the invalidation more complicated, however.
futhark@ do you have any general guidance for how precise invalidation needs to be? Is it worth carefully ensuring that `PseudoStateChanged()` is only called when it's really changed, or can one take the approach of calling it whenever it might have changed to reduce bookkeeping?
(Note that the tests are still wrong, so this CL isn't ready for review.)
The style machinery assumes something changed when PseudoStateChanged() is called, so it depends on what the selectors involving the given pseudo class look like, the size of your DOM, and how often you invalidate.
Worst case, if your media element comes early in the pre-order traversal of your document, there are thousands of elements in your document, and it contains a selector such as: `video:playing ~ * * { ... }`, you are basically recomputing styles for thousands of elements for each style update after you call PseudoStateChanged().
Related details
- Dale Curtis
- Philip Jägenstedt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Jägenstedt (Gerrit)
Philip Jägenstedt added 1 comment![]( "Open in Gerrit")
return MatchesBufferingPseudo() && sent_stalled_event_;Rune LillesveenThanks to https://github.com/whatwg/html/issues/12145 I realized that I should check `MatchesBufferingPseudo()` here and not just `sent_stalled_event_`. That makes the invalidation more complicated, however.
futhark@ do you have any general guidance for how precise invalidation needs to be? Is it worth carefully ensuring that `PseudoStateChanged()` is only called when it's really changed, or can one take the approach of calling it whenever it might have changed to reduce bookkeeping?
(Note that the tests are still wrong, so this CL isn't ready for review.)
The style machinery assumes something changed when PseudoStateChanged() is called, so it depends on what the selectors involving the given pseudo class look like, the size of your DOM, and how often you invalidate.
Worst case, if your media element comes early in the pre-order traversal of your document, there are thousands of elements in your document, and it contains a selector such as: `video:playing ~ * * { ... }`, you are basically recomputing styles for thousands of elements for each style update after you call PseudoStateChanged().
Thanks, so it sounds like I should make some effort to only call `PseudoStateChanged()` when it did actually change, and not just invalidate whenever paused/readyState/networkState change.
Related details
- Dale Curtis
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |