Bug 107986

Created attachment 184820 [details] Patch

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?

Created attachment 184859 [details] Patch

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

Created attachment 184970 [details] Patch

Created attachment 184978 [details] Patch

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?

Created attachment 185040 [details] Patch for landing

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

Created attachment 185251 [details] Patch for landing

Comment on attachment 185251 [details] Patch for landing Clearing flags on attachment: 185251 Committed r141119: <http://trac.webkit.org/changeset/141119>

All reviewed patches have been landed. Closing bug.