[css-pseudo] implement animation support for ::marker pseudos
Categories
(Core :: CSS Transitions and Animations, enhancement, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox68 | --- | fixed |
People
(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)
References
Details
(Keywords: dev-doc-complete)
Attachments
(1 file, 2 obsolete files)
We've added support for the ::marker pseudo-element in bug 205202.
We intentionally skipped animation support there and decided to
do that in this follow-up bug instead.
Generally, it should work as for ::before/::after.
Assignee | ||
Comment 1•5 years ago
|
||
Assignee | ||
Comment 2•5 years ago
|
||
Animations seems to work with those changes, except that animation events does not have the correct .pseudoElement name or something:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbed474b7c1233ff9d434cccedebbb943a8dd70b&selectedJob=235787679
Any ideas?
Comment 3•5 years ago
|
||
I suspect there are quite a few other places we'll need to update including at least:
- dom/animation/CSSPseudoElement.cpp
- dom/animation/EffectCompositor.cpp (lots here)
Also, we'll want tests for getting back the CSSPseudoElement object corresponding to a ::marker. You can see an example of how to get that object in the getPseudoElement
function.
I'm a little unsure sure about the CSS animations web-platform-tests. The existing tests need to be rewritten so I'm a little unsure how much we want to make that job bigger. At the same time I wonder if there is much value in testing that animation-delay applies to a ::marker pseudo element. (I can't imagine any reason why it would fail there but work elsewhere.) I haven't had a proper look but it might be worth focussing the tests on lifetime issues, interactions with other pseudos, and other areas where we might anticipate problems.
Updated•5 years ago
|
Assignee | ||
Comment 4•5 years ago
|
||
Thanks, I've updated all places I could find under dom/animation/
that handles pseudos but still no luck for that testcase:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e155d85f3da3891c472e04ede61eddc09e83b585&selectedJob=235966705
Assignee | ||
Comment 5•5 years ago
|
||
Also, we'll want tests for getting back the CSSPseudoElement object corresponding to a ::marker. You can see an example of how to get that object in the
getPseudoElement
function.
OK, I added this test and it fails.
Assignee | ||
Comment 6•5 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #3)
I'm a little unsure sure about the CSS animations web-platform-tests. The existing tests need to be rewritten so I'm a little unsure how much we want to make that job bigger.
OK, we can leave out the manual tests if you wish. I just wanted
to verify it with some basic tests and those tests seems to work
fine, fwiw.
Comment 7•5 years ago
|
||
I haven't done a thorough audit of all the places we handle pseudos but one other key places that sticks out is:
AnimationCollection<AnimationType>::GetPropertyAtomForPseudoType
inAnimationCollection.cpp
Comment 8•5 years ago
|
||
Oh, and mozilla::dom::Element::GetAnimations
and that vicinity.
Comment 9•5 years ago
|
||
In fact, if you just search for the mozilla::PseudoStyleType::before pseudo type, and ::after type you should find most of the occurrences.
Assignee | ||
Comment 10•5 years ago
|
||
Yup, I found Element::GetAnimations too, and fixed it, but still no luck.
I'll take a look at AnimationCollection.cpp...
(amazing how many places one has to touch to add an additional pseudo :-) )
Comment 11•5 years ago
|
||
You'll need to update the Pseudo Element spec too:
Assignee | ||
Comment 12•5 years ago
|
||
I got it to pass those failing tests locally, let's see what Try says:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bf7573d34f8b48bc773d3882dfc32290d1d59c4
Assignee | ||
Comment 13•5 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #11)
You'll need to update the Pseudo Element spec too
Filed a spec issue: https://github.com/w3c/csswg-drafts/issues/3763
Assignee | ||
Comment 14•5 years ago
|
||
Assignee | ||
Comment 15•5 years ago
|
||
Assignee | ||
Comment 16•5 years ago
|
||
Let me know if you want me to exclude the added -manual tests.
I included them just in case.
Sorry for all the copy-pasta btw. Ideally a change like this
should be a one-liner (excluding tests). Perhaps we could
add a CSS_PSEUDO_ELEMENT_ANIMATED bit in nsCSSPseudoElements.h
and then all these changes would follow from that. That would
be a lot less error prone. It's quite easy to introduce errors
while updating these things manually, or missing a place.
(I did both while adding this pseudo.)
I'll leave that as an exercise for the reader... ;-)
Assignee | ||
Updated•5 years ago
|
Comment 17•5 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #16)
Let me know if you want me to exclude the added -manual tests.
I included them just in case.
Yeah, I would drop them if you don't mind. When I finally get around to tidying up the css-animations WPT I plan to drop all the -manual tests, converting them to automated tests where appropriate.
Assignee | ||
Comment 18•5 years ago
|
||
Assignee | ||
Comment 19•5 years ago
|
||
Hmm, it seems phabricator didn't understand that the new patch
was just an amended version of the last one...
Assignee | ||
Updated•5 years ago
|
Comment 20•5 years ago
|
||
Pushed by mpalmgren@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/438983032b34 [css-pseudo] implement animation support for ::marker pseudos. r=emilio
Comment 21•5 years ago
|
||
bugherder |
Updated•5 years ago
|
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16149 for changes under testing/web-platform/tests
Can't merge web-platform-tests PR due to failing upstream checks: Github PR https://github.com/web-platform-tests/wpt/pull/16149 * Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/Rg6kO7UXT9G2EE8XoJ2n-Q)
Upstream PR merged
Updated•4 years ago
|
Comment 25•4 years ago
|
||
This has been added to the Firefox 68 release notes as part of the work on documenting ::marker.
Description
•