Bug 107986 - Implement pseudoElement attribute on transition DOM events.
: Implement pseudoElement attribute on transition DOM events.
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Event Handling
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Alexis Menard (darktears)
: WebExposed
Depends on:
Blocks: 93136
  Show dependency treegraph
 
Reported: 2013-01-25 15:29 PST by Alexis Menard (darktears)
Modified: 2013-01-29 08:57 PST (History)
11 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Alexis Menard (darktears) 2013-01-25 15:29:03 PST
Implement pseudoElement attribute on transition DOM events.
Comment 1 Alexis Menard (darktears) 2013-01-25 15:38:54 PST
Created attachment 184820 [details]
Patch
Comment 2 Elliott Sprehn 2013-01-25 15:57:47 PST
Comment on attachment 184820 [details]
Patch

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 Alexis Menard (darktears) 2013-01-26 03:41:46 PST
Created attachment 184859 [details]
Patch
Comment 4 Alexis Menard (darktears) 2013-01-26 03:44:19 PST
(In reply to comment #2)
> (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?

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 Elliott Sprehn 2013-01-26 08:18:30 PST
Comment on attachment 184859 [details]
Patch

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 Alexis Menard (darktears) 2013-01-28 04:22:56 PST
Created attachment 184970 [details]
Patch
Comment 7 Alexis Menard (darktears) 2013-01-28 06:42:59 PST
Created attachment 184978 [details]
Patch
Comment 8 Julien Chaffraix 2013-01-28 11:18:50 PST
Comment on attachment 184978 [details]
Patch

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 Alexis Menard (darktears) 2013-01-28 13:02:06 PST
Created attachment 185040 [details]
Patch for landing
Comment 10 WebKit Review Bot 2013-01-28 14:01:29 PST
Comment on attachment 185040 [details]
Patch for landing

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 Build Bot 2013-01-28 18:07:25 PST
Comment on attachment 185040 [details]
Patch for landing

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 Build Bot 2013-01-28 18:53:52 PST
Comment on attachment 185040 [details]
Patch for landing

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 Build Bot 2013-01-28 21:56:46 PST
Comment on attachment 185040 [details]
Patch for landing

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 Alexis Menard (darktears) 2013-01-29 08:37:02 PST
Created attachment 185251 [details]
Patch for landing
Comment 15 WebKit Review Bot 2013-01-29 08:57:05 PST
Comment on attachment 185251 [details]
Patch for landing

Clearing flags on attachment: 185251

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