Bug 115656 – Regression: Event#stopPropagation() does not halt bubbling for webkitTransitionEnd

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Matt Seeley]

Created attachment 200713 [details]
Repro case

I couldn't find bugs with 'stopPropagation()' using Quick Search. If this is a dupe please accept my apologies. I did not verify if the stopPropagation() behavior is broken for other event types.

Expected behavior:

A webkitTransitionEnd event handler on a parentNode should not be invoked by a descendants webkitTransitionEnd when the descendant has previously called stopPropagation().

Actual:

Calling stopPropagation() when the descendant is handling the event does not stop propagation. When the descendant is handling the event it is the first responder in the bubbling phase (target === currentTarget).

Notes and repro:

Nightly build r139829 [0] regressed support for stopping event propagation of transitionEnd events. Build r139728 [1] is the last good build. We spotted the bug in Chrome >= 26.0.1410.43 (25.0.1364.160 works as expected) and back tracked it to WebKit nightly builds.

The attached repro case assigns the same handler function to a parent and child nodes. Next an animation is triggered on the child node. The first responding node (child) attempts to stop event propagation. This test passes when the test executes one time and target === currentTarget. Or, technically when the last handling node is also target === currentTarget yet this doesn't occur during bubbling. You get the idea.

Downloading the DMG, mounting in Finder, then running WebKit executable directly is fine. I haven't needed to install/remove.

* Close all open WebKit instances
* Open the repro attachment in the last good build
* Notice that the event handler is only invoked once (pass)
* Now open the repro attachment in the first bad build
* Notice the event handler is invoked twice (fail)

[0] First bad: r139829.dmg">http://builds.nightly.webkit.org/files/trunk/mac/WebKit-SVN-r139829.dmg
[1] Last good: r139728.dmg">http://builds.nightly.webkit.org/files/trunk/mac/WebKit-SVN-r139728.dmg

[Matt Seeley]

Downstream Chromium affected too: http://code.google.com/p/chromium/issues/detail?id=238379

[Matt Seeley]

Updating the title to clearly indicate a regression.

Are there any blockers preventing confirmation and fix scheduling?

[Simon Fraser (smfr)]

Regressed in http://trac.webkit.org/changeset/139762

Matt, does this break some important content?

[Radar WebKit Bug Importer]

<rdar://problem/13876866>

[Matt Seeley]

Hi Simon, thanks for identifying the changeset.

The regression introduced several bugs into our HTML UIs which triggered work upon transition end. We worked around by comparing the event's target to currentTarget. These references are equal only when bubbling is at the originating element. The workaround is straightforward once identified.

[Alexis Menard (darktears)]

Created attachment 201728 [details]
Patch

[Darin Adler]

Comment on attachment 201728 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=201728&action=review

Looks good, needs more work on test coverage.

> Source/WebCore/ChangeLog:15
> +        Test: transitions/transition-end-event-prefixed-01.html

This test has insufficient coverage. We need to test coverage of all 9 of the things that are copied from the prefixed event to the original event. If I delete any one of the 9 from the “sync” function then the test should fail. If you can’t cover all 9 you should at least get as close as you can. The test will probably demonstrate that we don’t need to copy all 9, which is a good thing for us to know!

> Source/WebCore/ChangeLog:19
> +        (WebCore):

This bogus line should be removed.

> Source/WebCore/dom/EventTarget.cpp:165
> +static void syncEventAttributes(const Event* sourceEvent, Event* targetEvent)

The word “sync” sounds like a bidirectional operation with some kind of merging. I suggest a different name for this function that sounds directional. Maybe the word “propagate” or “copy” or “update”.

I’d maybe call this copyEventAttributesForPairedEvents, or something like that.

I don’t think sourceEvent and targetEvent are good names for the arguments because the word “target” has a specific name in the context of events.

It’s also non-obvious that this copies enough of the attributes. It seems this set is good enough for the transition end event, but is it good enough for all future events we might want to use this on?

> Source/WebCore/dom/EventTarget.cpp:211
> +    RefPtr<Event> prefixedEvent;

Why does this need to be defined outside the if statement? Is there some reason we need to keep it alive after the block it’s allocated and used in?

[Alexis Menard (darktears)]

(In reply to comment #7)
> (From update of attachment 201728 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=201728&action=review
> 
> Looks good, needs more work on test coverage.
> 
> > Source/WebCore/ChangeLog:15
> > +        Test: transitions/transition-end-event-prefixed-01.html
> 
> This test has insufficient coverage. We need to test coverage of all 9 of the things that are copied from the prefixed event to the original event. If I delete any one of the 9 from the “sync” function then the test should fail. If you can’t cover all 9 you should at least get as close as you can. The test will probably demonstrate that we don’t need to copy all 9, which is a good thing for us to know!

Sure.

> 
> > Source/WebCore/ChangeLog:19
> > +        (WebCore):
> 
> This bogus line should be removed.

Will do.

> 
> > Source/WebCore/dom/EventTarget.cpp:165
> > +static void syncEventAttributes(const Event* sourceEvent, Event* targetEvent)
> 
> The word “sync” sounds like a bidirectional operation with some kind of merging. I suggest a different name for this function that sounds directional. Maybe the word “propagate” or “copy” or “update”.
> 
> I’d maybe call this copyEventAttributesForPairedEvents, or something like that.
> 
> I don’t think sourceEvent and targetEvent are good names for the arguments because the word “target” has a specific name in the context of events.
> 
> It’s also non-obvious that this copies enough of the attributes. It seems this set is good enough for the transition end event, but is it good enough for all future events we might want to use this on?

I can think of the AnimationEvents coming. But hopefully we'll avoid doing this prefixed mistake in the future. Let see if I can do something better.

> 
> > Source/WebCore/dom/EventTarget.cpp:211
> > +    RefPtr<Event> prefixedEvent;
> 
> Why does this need to be defined outside the if statement? Is there some reason we need to keep it alive after the block it’s allocated and used in?

No leftover from a previous approach.

[Julien Chaffraix]

Comment on attachment 201728 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=201728&action=review

>>> Source/WebCore/dom/EventTarget.cpp:165
>>> +static void syncEventAttributes(const Event* sourceEvent, Event* targetEvent)
>> 
>> The word “sync” sounds like a bidirectional operation with some kind of merging. I suggest a different name for this function that sounds directional. Maybe the word “propagate” or “copy” or “update”.
>> 
>> I’d maybe call this copyEventAttributesForPairedEvents, or something like that.
>> 
>> I don’t think sourceEvent and targetEvent are good names for the arguments because the word “target” has a specific name in the context of events.
>> 
>> It’s also non-obvious that this copies enough of the attributes. It seems this set is good enough for the transition end event, but is it good enough for all future events we might want to use this on?
> 
> I can think of the AnimationEvents coming. But hopefully we'll avoid doing this prefixed mistake in the future. Let see if I can do something better.

I really think this should be a method on Event so that it is kept in sync with Event if it changes (this code will be around for a while as we phase out prefixed event support so it's probably good to get it right).

[Alexis Menard (darktears)]

(In reply to comment #9)
> (From update of attachment 201728 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=201728&action=review
> 
> >>> Source/WebCore/dom/EventTarget.cpp:165
> >>> +static void syncEventAttributes(const Event* sourceEvent, Event* targetEvent)
> >> 
> >> The word “sync” sounds like a bidirectional operation with some kind of merging. I suggest a different name for this function that sounds directional. Maybe the word “propagate” or “copy” or “update”.
> >> 
> >> I’d maybe call this copyEventAttributesForPairedEvents, or something like that.
> >> 
> >> I don’t think sourceEvent and targetEvent are good names for the arguments because the word “target” has a specific name in the context of events.
> >> 
> >> It’s also non-obvious that this copies enough of the attributes. It seems this set is good enough for the transition end event, but is it good enough for all future events we might want to use this on?
> > 
> > I can think of the AnimationEvents coming. But hopefully we'll avoid doing this prefixed mistake in the future. Let see if I can do something better.
> 

In fact the syncEventAttributes only takes care of the Event base class attributes. The specific TransitionEndEvent attributes are handled with WebKitTransitionEvent::create(eventNames().webkitTransitionEndEvent, transitionEvent->propertyName(), transitionEvent->elapsedTime(), transitionEvent->pseudoElement()); when we create the new event.

> I really think this should be a method on Event so that it is kept in sync with Event if it changes (this code will be around for a while as we phase out prefixed event support so it's probably good to get it right).

[Alexis Menard (darktears)]

Created attachment 201742 [details]
Patch

[Darin Adler]

Comment on attachment 201742 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=201742&action=review

> Source/WebCore/dom/Event.cpp:169
> +void Event::copyEventAttributes(Event* event) const
> +{

Need a comment explaining why it’s OK to copy only the attributes in the Event base class, and not have this be a virtual function that works properly for classes derived from Event.

> Source/WebCore/dom/Event.h:166
> +    void copyEventAttributes(Event*) const;

It’s not obvious which direction this goes, even though there is const involved. Name should be different.

Perhaps copyEventAttributesTo. Or copyAttributesTo. No reason to repeat the class name. But more serious is the issue that this copies only base Event class attributes. You’d need something different if you wanted to copy all attributes.

> Source/WebCore/dom/EventTarget.cpp:204
> +        RefPtr<Event> prefixedEvent = createMatchingPrefixedEvent(event);

Maybe we need new design. A single event that can be dispatched twice with different event name rather than two events that we copy attributes between. Seems easier to get that design right.

> LayoutTests/transitions/transition-end-event-prefixed-01.html:63
> +        shouldBe("transitionEventContainer.type", "transitionEventBox.type");
> +        shouldBe("transitionEventContainer.bubbles", "transitionEventBox.bubbles");
> +        shouldBe("transitionEventContainer.timeStamp", "transitionEventBox.timeStamp");
> +        shouldBe("transitionEventContainer.cancelable", "transitionEventBox.cancelable");
> +        shouldBe("transitionEventContainer.srcElement", "transitionEventBox.srcElement");
> +        shouldBe("transitionEventContainer.returnValue", "transitionEventBox.returnValue");
> +        shouldBe("transitionEventContainer.cancelBubble", "transitionEventBox.cancelBubble");
> +        shouldBe("transitionEventContainer.defaultPrevented", "transitionEventBox.defaultPrevented");
> +        shouldBe("transitionEventContainer.target", "transitionEventBox.target");
> +        shouldBe("transitionEventContainer.currentTarget", "testContainer");
> +        // TransitionEnd event specific properties.
> +        shouldBe("transitionEventContainer.pseudoElement", "transitionEventBox.pseudoElement");
> +        shouldBe("transitionEventContainer.elapsedTime", "transitionEventBox.elapsedTime");
> +        shouldBe("transitionEventContainer.propertyName", "transitionEventBox.propertyName");

The fact that these are equal doesn’t prove anything, unless we change them all. They could just be equal by chance?

Another way of putting it is that we need 13 different FAIL when you run this test without first patching WebKit, otherwise the test doesn’t actually test the fix.

[Alexis Menard (darktears)]

(In reply to comment #12)
> (From update of attachment 201742 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=201742&action=review
> 
> > Source/WebCore/dom/Event.cpp:169
> > +void Event::copyEventAttributes(Event* event) const
> > +{
> 
> Need a comment explaining why it’s OK to copy only the attributes in the Event base class, and not have this be a virtual function that works properly for classes derived from Event.
> 
> > Source/WebCore/dom/Event.h:166
> > +    void copyEventAttributes(Event*) const;
> 
> It’s not obvious which direction this goes, even though there is const involved. Name should be different.
> 
> Perhaps copyEventAttributesTo. Or copyAttributesTo. No reason to repeat the class name. But more serious is the issue that this copies only base Event class attributes. You’d need something different if you wanted to copy all attributes.
> 
> > Source/WebCore/dom/EventTarget.cpp:204
> > +        RefPtr<Event> prefixedEvent = createMatchingPrefixedEvent(event);
> 
> Maybe we need new design. A single event that can be dispatched twice with different event name rather than two events that we copy attributes between. Seems easier to get that design right.

Yes I believe it's time to change.

> 
> > LayoutTests/transitions/transition-end-event-prefixed-01.html:63
> > +        shouldBe("transitionEventContainer.type", "transitionEventBox.type");
> > +        shouldBe("transitionEventContainer.bubbles", "transitionEventBox.bubbles");
> > +        shouldBe("transitionEventContainer.timeStamp", "transitionEventBox.timeStamp");
> > +        shouldBe("transitionEventContainer.cancelable", "transitionEventBox.cancelable");
> > +        shouldBe("transitionEventContainer.srcElement", "transitionEventBox.srcElement");
> > +        shouldBe("transitionEventContainer.returnValue", "transitionEventBox.returnValue");
> > +        shouldBe("transitionEventContainer.cancelBubble", "transitionEventBox.cancelBubble");
> > +        shouldBe("transitionEventContainer.defaultPrevented", "transitionEventBox.defaultPrevented");
> > +        shouldBe("transitionEventContainer.target", "transitionEventBox.target");
> > +        shouldBe("transitionEventContainer.currentTarget", "testContainer");
> > +        // TransitionEnd event specific properties.
> > +        shouldBe("transitionEventContainer.pseudoElement", "transitionEventBox.pseudoElement");
> > +        shouldBe("transitionEventContainer.elapsedTime", "transitionEventBox.elapsedTime");
> > +        shouldBe("transitionEventContainer.propertyName", "transitionEventBox.propertyName");
> 
> The fact that these are equal doesn’t prove anything, unless we change them all. They could just be equal by chance?

But most of them are read-only attribute from JS point of view.  The test 02 and 03 actually test the ones you can change. Maybe some of these attributes in 01 are equals by chance but with their read only property, I am unable to simulate changing them from the layout tests to cover the out-of-sync problem. I see your point about having them to fail individually but when the transition event is dispatched to client's code there is little the client code can do to modify these values, I added them for sanity check.

> 
> Another way of putting it is that we need 13 different FAIL when you run this test without first patching WebKit, otherwise the test doesn’t actually test the fix.

[Alexis Menard (darktears)]

Created attachment 201945 [details]
Patch

[Darin Adler]

Comment on attachment 201945 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=201945&action=review

I still think there are deficiencies in the test, but this is OK.

> Source/WebCore/dom/EventTarget.cpp:183
> +    const AtomicString prefixedTypeName = prefixedType(event);

Why the const?

> Source/WebCore/dom/EventTarget.cpp:192
> +        const AtomicString unprefixedTypeName = event->type();

Why the const?

[Alexis Menard (darktears)]

(In reply to comment #15)
> (From update of attachment 201945 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=201945&action=review
> 
> I still think there are deficiencies in the test, but this is OK.
> 

What you would change?

> > Source/WebCore/dom/EventTarget.cpp:183
> > +    const AtomicString prefixedTypeName = prefixedType(event);
> 
> Why the const?
> 
> > Source/WebCore/dom/EventTarget.cpp:192
> > +        const AtomicString unprefixedTypeName = event->type();
> 
> Why the const?

I'll remove them

[Alexis Menard (darktears)]

(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 201945 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=201945&action=review
> > 
> > I still think there are deficiencies in the test, but this is OK.
> > 
> 
> What you would change?

Maybe you meant that I should try to modify the event with for example initEvent so I could change the default values but unfortunately initEvent bail out early if the event is currently being dispatched (the target !0) and I can't modify the target from JS.

> 
> > > Source/WebCore/dom/EventTarget.cpp:183
> > > +    const AtomicString prefixedTypeName = prefixedType(event);
> > 
> > Why the const?
> > 
> > > Source/WebCore/dom/EventTarget.cpp:192
> > > +        const AtomicString unprefixedTypeName = event->type();
> > 
> > Why the const?
> 
> I'll remove them

[Alexis Menard (darktears)]

Created attachment 201987 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 201987 [details]
Patch for landing

Clearing flags on attachment: 201987

Committed r150239: <http://trac.webkit.org/changeset/150239>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.