[css-pseudo] implement animation support for ::marker pseudos

RESOLVED FIXED in Firefox 68

Status

()

enhancement
P3
normal
RESOLVED FIXED
4 months ago
Last month

People

(Reporter: mats, Assigned: mats)

Tracking

({dev-doc-complete})

Trunk
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.

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?

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.

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

Posted patch A getPseudoElement() test (obsolete) — Splinter Review

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.

(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.

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 in AnimationCollection.cpp

Oh, and mozilla::dom::Element::GetAnimations and that vicinity.

In fact, if you just search for the mozilla::PseudoStyleType::before pseudo type, and ::after type you should find most of the occurrences.

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 :-) )

You'll need to update the Pseudo Element spec too:

https://drafts.csswg.org/css-pseudo-4/#csspseudoelement

I got it to pass those failing tests locally, let's see what Try says:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bf7573d34f8b48bc773d3882dfc32290d1d59c4

(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

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... ;-)

Attachment #9053455 - Attachment is obsolete: true

(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.

Hmm, it seems phabricator didn't understand that the new patch
was just an amended version of the last one...

Attachment #9053496 - Attachment is obsolete: true
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/438983032b34
[css-pseudo] implement animation support for ::marker pseudos.  r=emilio
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
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)
Regressions: 1545707
Upstream PR merged

This has been added to the Firefox 68 release notes as part of the work on documenting ::marker.

You need to log in before you can comment on or make changes to this bug.