Closed Bug 1538618 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: CSS Transitions and Animations, enhancement, P3)

Product:
Core
Component:
CSS Transitions and Animations
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla68
Tracking Flags:
Tracking Status
firefox68 --- fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 2 obsolete files)

Bug 1538618 - [css-pseudo] implement animation support for ::marker pseudos. r=emilio
47 bytes, text/x-phabricator-request
Details | Review

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

Attached 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

https://hg.mozilla.org/mozilla-central/rev/438983032b34

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox68: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Assignee: nobody → mats
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16149 for changes under testing/web-platform/tests
Regressions: 1545707

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

See Also: → 1615469
Blocks: 1615473
You need to log in before you can comment on or make changes to this bug.