Bug 107986 - Implement pseudoElement attribute on transition DOM events.
: Implement pseudoElement attribute on transition DOM events.
Status: RESOLVED FIXED
: WebKit
Event Handling
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
: WebExposed
:
: 93136
  Show dependency treegraph
 
Reported: 2013-01-25 15:29 PST by
Modified: 2013-01-29 08:57 PST (History)


Attachments
Patch (37.02 KB, patch)
2013-01-25 15:38 PST, Alexis Menard (darktears)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (37.10 KB, patch)
2013-01-26 03:41 PST, Alexis Menard (darktears)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (37.15 KB, patch)
2013-01-28 04:22 PST, Alexis Menard (darktears)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (36.97 KB, patch)
2013-01-28 06:42 PST, Alexis Menard (darktears)
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (37.15 KB, patch)
2013-01-28 13:02 PST, Alexis Menard (darktears)
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (37.18 KB, patch)
2013-01-29 08:37 PST, Alexis Menard (darktears)
no flags Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2013-01-25 15:29:03 PST
Implement pseudoElement attribute on transition DOM events.
------- Comment #1 From 2013-01-25 15:38:54 PST -------
Created an attachment (id=184820) [details]
Patch
------- Comment #2 From 2013-01-25 15:57:47 PST -------
(From update of attachment 184820 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=184820&action=review

> Source/WebCore/dom/EventDispatcher.cpp:156
> +    if (referenceNode->isPseudoElement())

I think this needs to be above the SVG block. The check at the top: if (!referenceNode->isSVGElement() || !referenceNode->isInShadowTree()) is going to be true for PseudoElements.

> Source/WebCore/dom/PseudoElement.cpp:42
> +String PseudoElement::pseudoElementName(PseudoId pseudoId)

We might rename this since other places the pseudo name means the non-colon prefixed version in webkit. It's really only the animation stuff that needs this. Maybe pseudoElementNameForEvents() ?

> Source/WebCore/dom/TransitionEvent.cpp:39
> +    , pseudoElement()

It's really weird we put these in the list even though we're calling the default constructor. Other parts of webkit don't do this, but I see we do it here.

> Source/WebCore/page/animation/AnimationController.cpp:189
> +                element = element->parentElement();

You don't need this. Since PseudoElement::pseudoElementName is going to return empty string for other elements just do below:

TransitionEvent::create(it->eventType, it->name, it->elapsedTime, PseudoElement::pseudoElementName(it->element.get())).

You also don't need to change where you call dispatchEvent, since the EventDispatcher already retargets the event. Just remove the override of dispatchEvent in PseudoElement.h

> Source/WebCore/page/animation/AnimationController.cpp:197
> +            element->dispatchEvent(WebKitAnimationEvent::create(it->eventType, it->name, it->elapsedTime));

What about animation events?
------- Comment #3 From 2013-01-26 03:41:46 PST -------
Created an attachment (id=184859) [details]
Patch
------- Comment #4 From 2013-01-26 03:44:19 PST -------
(In reply to comment #2)
> (From update of attachment 184820 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184820&action=review
> 
> > Source/WebCore/dom/EventDispatcher.cpp:156
> > +    if (referenceNode->isPseudoElement())
> 
> I think this needs to be above the SVG block. The check at the top: if (!referenceNode->isSVGElement() || !referenceNode->isInShadowTree()) is going to be true for PseudoElements.
> 
> > Source/WebCore/dom/PseudoElement.cpp:42
> > +String PseudoElement::pseudoElementName(PseudoId pseudoId)
> 
> We might rename this since other places the pseudo name means the non-colon prefixed version in webkit. It's really only the animation stuff that needs this. Maybe pseudoElementNameForEvents() ?
> 
> > Source/WebCore/dom/TransitionEvent.cpp:39
> > +    , pseudoElement()
> 
> It's really weird we put these in the list even though we're calling the default constructor. Other parts of webkit don't do this, but I see we do it here.
> 
> > Source/WebCore/page/animation/AnimationController.cpp:189
> > +                element = element->parentElement();
> 
> You don't need this. Since PseudoElement::pseudoElementName is going to return empty string for other elements just do below:
> 
> TransitionEvent::create(it->eventType, it->name, it->elapsedTime, PseudoElement::pseudoElementName(it->element.get())).
> 
> You also don't need to change where you call dispatchEvent, since the EventDispatcher already retargets the event. Just remove the override of dispatchEvent in PseudoElement.h
> 
> > Source/WebCore/page/animation/AnimationController.cpp:197
> > +            element->dispatchEvent(WebKitAnimationEvent::create(it->eventType, it->name, it->elapsedTime));
> 
> What about animation events?

I'll do those when I work on unprefixing the animations. I'll create a bug to make sure I will not forget. In the other hand it's not on the IDL definition of http://dev.w3.org/csswg/css3-animations/#AnimationEvent-interface . I'll bring it to the CSS WG on Monday.
------- Comment #5 From 2013-01-26 08:18:30 PST -------
(From update of attachment 184859 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=184859&action=review

You need to use much shorter transition times in your test, ex. 1ms, right now your test really does take 1 second, which is too slow.

> Source/WebCore/dom/PseudoElement.cpp:44
> +    switch (pseudoId) {

I think you want to declare each of these as a static ASCIILiteral so every time we fire the event we don't need to allocate new strings. See: Document::readyState

> LayoutTests/fast/css-generated-content/pseudo-transition-event.html:83
> +        setTimeout(isSuccessfullyParsed, 3000);

There's no need for this setTimeout or window.internals check. Just call isSuccessfullyParsed();

> LayoutTests/fast/css-generated-content/pseudo-transition-event.html:85
> +    if (testRunner)

if (window.testRunner)

This fails with a variable doesn't exist right now.

> LayoutTests/fast/css-generated-content/pseudo-transition-event.html:93
> +    window.div = div;

You don't need to do window.div = div.

> LayoutTests/fast/css-generated-content/pseudo-transition-event.html:97
> +    document.addEventListener( 'transitionend', recordTransitionEvent, false);

Last argument to addEventListener is optional in all modern browsers.
------- Comment #6 From 2013-01-28 04:22:56 PST -------
Created an attachment (id=184970) [details]
Patch
------- Comment #7 From 2013-01-28 06:42:59 PST -------
Created an attachment (id=184978) [details]
Patch
------- Comment #8 From 2013-01-28 11:18:50 PST -------
(From update of attachment 184978 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=184978&action=review

The code change looks fine, some comment on the test.

> Source/WebCore/ChangeLog:18
> +

As discussed over IRC, we should also add it to the AnimationEvent interface for consistency:

http://dev.w3.org/csswg/css3-animations/#AnimationEvent-interface

It could be done in a follow-up patch.

> Source/WebCore/ChangeLog:54
> +        (WebCore::AnimationControllerPrivate::fireEventsAndUpdateStyle):

Please file the entries above, that would make the patch easier to read.

> LayoutTests/fast/css-generated-content/pseudo-transition-event.html:70
> +    recordedEvents.sort(compareEventInfo);
> +    expectedEvents.sort(compareEventInfo);

This is super weird for me to dispatch the events in parallel and then order them for the purpose of comparing to the expected results. You could drive the next transition from your event listener after having checked the event against the expected. Both ways are equivalent, except that your approach stresses animation in parallel vs sequential in what I proposed. All in all, this is a NIT.

Also, let's make debugging failures easier by dispatching individual failures instead of the generic "FAIL expectedEvents != recordedEvents".

> LayoutTests/fast/events/constructors/transition-event-constructor.html:57
> +shouldBeEqualToString("new TransitionEvent('eventType', { pseudoElement: '::before' }).pseudoElement", "::before");
> +shouldBeEqualToString("new TransitionEvent('eventType', { pseudoElement: '' }).pseudoElement", "");

How about testing bad values?

Like what should new TransitionEvent('eventType', { pseudoElement: 'cheese' }).pseudoElement return?
------- Comment #9 From 2013-01-28 13:02:06 PST -------
Created an attachment (id=185040) [details]
Patch for landing
------- Comment #10 From 2013-01-28 14:01:29 PST -------
(From update of attachment 185040 [details])
Attachment 185040 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16155746

New failing tests:
fast/css-generated-content/pseudo-transition-event.html
------- Comment #11 From 2013-01-28 18:07:25 PST -------
(From update of attachment 185040 [details])
Attachment 185040 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16152888

New failing tests:
fast/css-generated-content/pseudo-transition-event.html
fast/workers/worker-close-more.html
fast/workers/worker-lifecycle.html
------- Comment #12 From 2013-01-28 18:53:52 PST -------
(From update of attachment 185040 [details])
Attachment 185040 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16149904

New failing tests:
fast/css-generated-content/pseudo-transition-event.html
fast/workers/worker-lifecycle.html
------- Comment #13 From 2013-01-28 21:56:46 PST -------
(From update of attachment 185040 [details])
Attachment 185040 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16160897

New failing tests:
fast/css-generated-content/pseudo-transition-event.html
fast/workers/worker-lifecycle.html
------- Comment #14 From 2013-01-29 08:37:02 PST -------
Created an attachment (id=185251) [details]
Patch for landing
------- Comment #15 From 2013-01-29 08:57:05 PST -------
(From update of attachment 185251 [details])
Clearing flags on attachment: 185251

Committed r141119: <http://trac.webkit.org/changeset/141119>
------- Comment #16 From 2013-01-29 08:57:10 PST -------
All reviewed patches have been landed.  Closing bug.