w3c / csswg-drafts

CSS Working Group Editor Drafts
https://drafts.csswg.org/
Other
4.44k stars 657 forks source link

[web-animations-1] Clarification about `target` and `pseudoElement` relationship #4745

Open graouts opened 4 years ago

graouts commented 4 years ago

I'm not sure the spec is clear about what the animation target is if pseudoElement is not null.

Consider these two tests in web-animations/interfaces/Animatable/animate.html for instance:

test(t => {
  const div = createDiv(t);
  div.classList.add('pseudo');
  getComputedStyle(div,"::before").content; // Sync style
  const anim = div.animate(null, {pseudoElement: '::before'});
  assert_equals(anim.effect.target, div, 'The returned element has the correct target element');
  assert_equals(anim.effect.pseudoElement, '::before',
                'The returned Animation targets the correct selector');
}, 'animate() with pseudoElement an Animation object targeting ' +
   'the correct pseudo-element');

test(t => {
  const div = createDiv(t);
  const anim = div.animate(null, {pseudoElement: '::before'});
  assert_equals(anim.effect.target, div, 'The returned element has the correct target element');
  assert_equals(anim.effect.pseudoElement, '::before',
                'The returned Animation targets the correct selector');
}, 'animate() with pseudoElement without content creates an Animation object targeting ' +
   'the correct pseudo-element');

In the first example, I understand what's happening: the target element has a ::before pseudo-element with content and it's created before Element.animate() is called. It seems clear that the animation target is the ::before pseudo-element.

In the second example, I'm not so sure. When Element.animate() is called the target div has no ::before pseudo-element. So what is the effective target of that animation? Does it actually target any element? What if the animation starts and the div gets a ::before pseudo-element? Does the animation then affects that element?

I guess my question is whether we need a targeted pseudo-element to exist when the API tries to reference it, or if it's all resolved live.

Similarly, I'm confused by this test in web-animations/interfaces/Animation/commitStyles.html:

test(t => {
  const div = createDiv(t);
  div.classList.add('pseudo');
  const animation = div.animate(
    { opacity: 0 },
    { duration: 1, fill: 'forwards', pseudoElement: '::before' }
  );

  assert_throws_dom('NoModificationAllowedError', () => {
    animation.commitStyles();
  });
}, 'Throws if the target element is a pseudo element');

At the time Animation.animate() is called, style resolution has not happened yet and there is no ::before pseudo-element for div. However, there would be when Animation.commitStyles() is called.

Up until pseudoElement was added, it was clear whether an animation was targeting something, because you'd have to pass a value to KeyframeEffect.target. Now that the target is specified across a pair of properties without a direct reference to an Element, it's not as clear.

graouts commented 4 years ago

@george-steel Maybe you can help making sense of this?

Loirooriol commented 4 years ago

When Element.animate() is called the target div has no ::before pseudo-element.

AFAIK, the ::before pseudo-element will always exist (in the element tree). Then, it may not generate any box, but this should be no different than an element with display: none.

graouts commented 4 years ago

When Element.animate() is called the target div has no ::before pseudo-element.

AFAIK, the ::before pseudo-element will always exist (in the element tree). Then, it may not generate any box, but this should be no different than an element with display: none.

If that's true, could you point me to spec text backing this? It would make things easier, indeed.

But there are also tests that specifically test whether elements have or don't have ::before selectors set for elements, specifically web-animations/interfaces/KeyframeEffect/target.html.

george-steel commented 4 years ago

@Loirooriol is correct, a pseudo-element not having a content property is equivilent to display: none, the pseudo-element exists (and can be a target of getComputedStyle()) but does not generate a box. The two types of tests are there because many browsers use two different internal representations for elements with and without ::before selectors. They expect the same behavior.

graouts commented 4 years ago

Is there spec text to back up the notion of "a pseudo-element not having a content property is equivalent to display: none"? Or is it specifically that there is no text that says otherwise?

george-steel commented 4 years ago

@birtles @stephenmcgruer What do you think of this?

Loirooriol commented 4 years ago

@graouts In the definition of content: none,

On pseudo-elements it inhibits the creation of the pseudo-element as if it had display: none.

It's not crystal clear since the pseudo-element is still created in the element tree, only its boxes are inhibited (which is what display: none does), but it points out the equivalence.

I filed #1810 for explicitly defining how the element tree is constructed, but it was deferred.

graouts commented 4 years ago

Right, looking at the full definition of content: none (emphasis mine):

On elements, this inhibits the children of the element from being rendered as children of this element, as if the element was empty. On pseudo-elements it inhibits the creation of the pseudo-element as if it had display: none.

In neither case does it prevent any pseudo-elements which have this element or pseudo-element as an originating element from being generated.

That last bit seems to be what defines that pseudo-elements generation are not bound by content being set to something other than none.

birtles commented 4 years ago

When it comes to pseudo elements I gladly defer to @emilio.

stephenmcgruer commented 4 years ago

My understanding was that pseudo-elements conceptually always exist for every selector (even ones that aren't supported by the browser or spec'd anywhere). So the idea with the web-animation spec changes was that targeting, say, ::blargh would target the conceptual pseudo-element with that selector (even if no browser would ever implement or create that).

In effect, of course, such animations do nothing, and so browsers would be welcome to optimize them (as long they still stayed around* in case, e.g. the user changed the target).

But yeah, like birtles I defer to others with pseudo-element knowledge :D.

lpd-au commented 4 years ago

My understanding was that pseudo-elements conceptually always exist for every selector (even ones that aren't supported by the browser or spec'd anywhere). So the idea with the web-animation spec changes was that targeting, say, ::blargh would target the conceptual pseudo-element with that selector (even if no browser would ever implement or create that).

In effect, of course, such animations do nothing, and so browsers would be welcome to optimize them (as long they still stayed around* in case, e.g. the user changed the target).

I have a question related to this. If I've followed all the rabbit holes of the spec correctly, it currently says that when an animation with a syntactically invalid <pseudo-element-selector> is created, a DOMException with error name SyntaxError is thrown. When I pass the pseudo-selector ::blargh to Element.animate(), I receive back a valid animation object in Canary and "TypeError: Element.animate: Unsupported pseudo-selector '::blargh'" in Nightly. Which implementation is correct?

birtles commented 4 years ago

I have a question related to this. If I've followed all the rabbit holes of the spec correctly, it currently says that when an animation with a syntactically invalid <pseudo-element-selector> is created, a DOMException with error name SyntaxError is thrown. When I pass the pseudo-selector ::blargh to Element.animate(), I receive back a valid animation object in Canary and "TypeError: Element.animate: Unsupported pseudo-selector '::blargh'" in Nightly. Which implementation is correct?

The change to use a SyntaxError instead of a TypeError is a very recent one (yesterday?) so I suspect neither browser returns that yet.

However, ::blargh is not syntactically invalid so I think Nightly is wrong here.

@BorisChiou I think bug 1610981 might have added some incorrect tests here. Specifically here:

https://hg.mozilla.org/mozilla-central/rev/ebfed5eb1869#l30.40

It looks like the spec says we should accept unsupported (but syntactically valid) pseudos.

BorisChiou commented 4 years ago

The current spec:

If the provided value is not null or a syntactically valid the user agent must throw a DOMException with error name SyntaxError and leave the target pseudo-selector of this animation effect unchanged.

I didn't see Syntax Error when working on this for Gecko, so we should fix this soon, based on the up-to-date spec.

It looks like the spec says we should accept unsupported (but syntactically valid) pseudos.

Yes, I just filed a bug for this (https://bugzilla.mozilla.org/show_bug.cgi?id=1621174).

emilio commented 4 years ago

@birtles It seems a bit odd not to throw for unknown pseudos? That's what e.g. querySelector does.

flackr commented 4 years ago

That sounds like a good precedent for doing the same for the pseudoElement property.

birtles commented 4 years ago

@birtles It seems a bit odd not to throw for unknown pseudos? That's what e.g. querySelector does.

I didn't write the spec text here. @george-steel probably remembers better the logic here. Looking into querySelector I see it has:

Consistent with CSS’s forwards-compatible parsing principle, UAs must treat as invalid any pseudo-classes, pseudo-elements, combinators, or other syntactic constructs for which they have no usable level of support.

https://drafts.csswg.org/selectors-4/#invalid-selector

lpd-au commented 4 years ago

As a thought exercise, if the spec isn't changed to match querySelector from this discussion, what would be the correct way to detect whether an animation fired or not on the pseudo-element? (Side note: it's kind of unfortunate that current stable browsers now fire events on the originating element when they don't support KeyframeEffectOptions.pseudoElement.)

george-steel commented 4 years ago

@birtles The main reason for allowing unsupported pseudo-elements is to allow target and pseudoElement to be set in either order (since supported pseudo-elements can vary between elements). It is also consistent with getComputedStyle().

@lpd-au the correct way is to listen for an animation event (which should always fire on the originating element, like all DOM events) and check its pseudoElement property (of the event).

flackr commented 4 years ago

It seems to me we have two differing precedents for how to treat unknown pseudo-elements.

@george-steel I don't think there's any reason either option wouldn't work. We would allow parsing any pseudo-element even if it wasn't valid on the current element to allow pseudoElement to be set independent of target.

@george-steel @lpd-au Doesn't checking the event's pseudoElement imply that the animation in the animation event is different than the one which was constructed (such that the pseudoElement may vary)? I would have expected it to be the same WebAnimation object.

george-steel commented 4 years ago

@flackr from the MDN page on querySelectorAll()

If the specified selectors include a CSS pseudo-element, the returned list is always empty.

It doesn't sound like we have two conventions for pseudo-elements. We only have the getComputedStyle convention as querySelector doesn't support pseudo-elements at all. Even the section linked above on unsupported pseudo-element selectors being treated as invalid is only used in a context where an invalid selector means a no-op.

lpd-au commented 4 years ago

The main reason for allowing unsupported pseudo-elements is to allow target and pseudoElement to be set in either order (since supported pseudo-elements can vary between elements). It is also consistent with getComputedStyle().

This makes sense to me, I guess what I really want is something like a form of CSS.supports() that accepts an element and a pseudo-selector, which can be called prior to creating/updating an animation.

Doesn't checking the event's pseudoElement imply that the animation in the animation event is different than the one which was constructed (such that the pseudoElement may vary)? I would have expected it to be the same WebAnimation object.

Yes as far as I can tell by looking at Canary's implementation, the events fired are identical for ::before and ::blargh; that is, no animation events are fired and the finish animation playback events are identical.

george-steel commented 4 years ago

That might be a problem with Canary's implementation regarding events. If you have found the culprit, can you link it here? It might be using target() instead of EventTarget() internally by mistake.

flackr commented 4 years ago

@flackr from the MDN page on querySelectorAll()

If the specified selectors include a CSS pseudo-element, the returned list is always empty.

It doesn't sound like we have two conventions for pseudo-elements. We only have the getComputedStyle convention as querySelector doesn't support pseudo-elements at all. Even the section linked above on unsupported pseudo-element selectors being treated as invalid is only used in a context where an invalid selector means a no-op.

The context in which querySelector runs scope-match a selectors string which uses the parse a selector from selectors-4 which states that an unsupported psuedo-element selector is treated as invalid throws an exception as a result of the invalid selector. It seems odd to me that we wouldn't have consistent pseudo element parsing.

birtles commented 4 years ago

Yes, although I understand some pseudo-elements only have an effect on certain elements, I think the wording in selectors-4 refers to support independent of the target element,

UAs must treat as invalid any pseudo-classes, pseudo-elements, combinators, or other syntactic constructs for which they have no usable level of support.

That is, we should throw here when the UA doesn't have any support for the pseudo-element, whatsoever, but not in the case when the UA does not support the specific pseudo-element and element combination (for that presumably there will simply be no effect).

That text also treats both parsing errors and unsupported pseudo-elements as the same class, "invalid", so I think we should throw for both cases rather than making a distinction here.

querySelector throws a "SyntaxError" for invalid selectors so I think we should do that here. (And checking in Firefox and Chrome, both throw for document.querySelector('yer::yer') but return null for document.querySelector('yer::before'))

SelenIT commented 4 years ago

Does the quirk with ::-webkit-anything pseudo-element apply here as well?

flackr commented 4 years ago

I imagine it would, given we are deferring to selectors-4 for the notion of a valid parse.

lpd-au commented 4 years ago

no usable level of support

What does this mean in this context? If a browser supports a pseudo-element but doesn't support web animations on it, does it have a usable level of support? What if for some reason it supported web animations on one type of originating element (eg li) but not another (eg div), for the reasons @george-steel provided will that suffice as a usable level of support?

birtles commented 4 years ago

If a browser supports a pseudo-element but doesn't support web animations on it, does it have a usable level of support?

~Yes.~ Edit: Actually, maybe not. I think it would be fine to throw for this case (and it's probably more useful to do so).

What if for some reason it supported web animations on one type of originating element (eg li) but not another (eg div), for the reasons @george-steel provided will that suffice as a usable level of support?

Yes.

lpd-au commented 4 years ago

Edit: Actually, maybe not. I think it would be fine to throw for this case (and it's probably more useful to do so).

Is that something worth noting in the spec? My initial assumption was the opposite.

birtles commented 4 years ago

Edit: Actually, maybe not. I think it would be fine to throw for this case (and it's probably more useful to do so).

Is that something worth noting in the spec? My initial assumption was the opposite.

Yes, I think that would be worth noting.