1292063 - Support Event.composed

Status: RESOLVED FIXED

(Core :: DOM: Events, defect, P3)

Description

[Ming-Chou Shih [:stone]]

Add the composed attribute for pointer events

References
https://github.com/w3c/uievents/pull/91
https://github.com/w3c/pointerevents/pull/92

Comment 1

[Ming-Chou Shih [:stone]]

This bug is created for adding attribute 'composed' to DOM event interface, which is defined in [1]. More detailed discussions are in [2]. For events which are composed are defined in following links
-touch events (https://w3c.github.io/touch-events/#list-of-touchevent-types)
-pointer events (https://w3c.github.io/pointerevents/#pointer-event-types)
-wheel events (https://www.w3.org/TR/uievents/#events-wheel-types)
-focus events (https://www.w3.org/TR/uievents/#events-focus-types)
-mouse events (https://www.w3.org/TR/uievents/#events-mouse-types)
-input events (https://www.w3.org/TR/uievents/#events-input-types)
-keyboard events (https://www.w3.org/TR/uievents/#events-keyboard-types)
-composition events (https://www.w3.org/TR/uievents/#events-composition-types)
-drag&drop events (https://html.spec.whatwg.org/#dndevents)
-legacy events (https://w3c.github.io/uievents/#legacy-event-types)

[1] https://dom.spec.whatwg.org/#dom-event-composed
[2] https://github.com/w3c/webcomponents/issues/513

Comment 2

[Ming-Chou Shih [:stone]]

I found there are some events should also be clarified if they are also composed, such as
1) clipboard events (eCopy, ePaste, eCut)
2) scroll event (eScroll)
   I'm not sure whether other scroll related events should be composed, such as
     eLegacyMouseLineOrPageScroll,
     eLegacyMousePixelScroll,
     eScrolledAreaChanged,
     eScrollPortOverflow,
     eScrollPortUnderflow

I also found that some existed test cases depend on the event fired by anonymous element, such as
layout/reftests/ogg-video/poster-4.html and poster-7.html. In these two test case, they want to be notified the when image poster is loaded.

Comment 3

[Ming-Chou Shih [:stone]]

Attachment: input_test.html

About scroll behavior, tested the attached test case on different browsers. The test procedures are press mouse left button and hold inside the input element, then move mouse down or left.

Edge 25.10586.0.0
scroll events are fired to listener(capture) of window and input

Chrome 52.0.2743.116(Windows10) and Canary 55.0.2853.0 (Windows10)
scroll events aren't fire to listeners of window and input

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

<textarea> is more important to test.

Comment 5

[Ming-Chou Shih [:stone]]

Attachment: Part1 Add Event.composed (V2)

Comment 6

[Ming-Chou Shih [:stone]]

Attachment: Part2 Add composed argument to getEventTargetChainFor and refine test cases (V2)

Some test cases need to check whether the event target chain is correct. The event target chain depends on whether the event is composed. So I add one more argument to getEventTargetChainFor

Comment 7

[Ming-Chou Shih [:stone]]

Attachment: Part3 Add mComposedInNativeAnonymousContent to determine which events can cross the boundary of native anonymous content (V2)

Comment 8

[Ming-Chou Shih [:stone]]

Attachment: Part4 Refine test case to listen body load event instead of video load event (V2)

Comment 9

[Ming-Chou Shih [:stone]]

Attachment: Part5: Update the meta file of shadow-dom/event-composed.html (V2)

Some tests in shadow-dom/event-composed.html are passed when we support Event.composed.

Other failures need the interface 'Element.attachShadow'

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8792454 [details] [diff] [review]
Part1 Add Event.composed (V2)



>+    switch (mClass) {
>+      case eMouseEventClass:
>+        mFlags.mComposed = mMessage == eMouseClick ||
>+                           mMessage == eMouseDoubleClick ||
>+                           mMessage == eMouseDown || mMessage == eMouseDown ||
eMouseDown twice here


>+                           mMessage == eMouseEnter || mMessage == eMouseEnter ||
Same with eMouseEnter


>+  void SetComposed(const nsAString& aEventTypeArg)
>+  {
>+    mFlags.mComposed = // mouse events
>+                       aEventTypeArg.EqualsLiteral("click") ||
>+                       aEventTypeArg.EqualsLiteral("dblclick") ||
>+                       aEventTypeArg.EqualsLiteral("mousedown") ||
>+                       aEventTypeArg.EqualsLiteral("mouseenter") ||
>+                       aEventTypeArg.EqualsLiteral("mouseleave") ||
>+                       aEventTypeArg.EqualsLiteral("mousemove") ||
>+                       aEventTypeArg.EqualsLiteral("mouseout") ||
>+                       aEventTypeArg.EqualsLiteral("mouseover") ||
>+                       aEventTypeArg.EqualsLiteral("mouseup") ||
You're missing contextmenu here, but you have it as eContextMenu


Those fixed, r+

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8792455 [details] [diff] [review]
Part2 Add composed argument to getEventTargetChainFor and refine test cases (V2)


>+++ b/dom/events/nsIEventListenerService.idl
>@@ -73,16 +73,17 @@ interface nsIEventListenerService : nsIS
>    * Returns an array of event targets.
>    * aEventTarget will be at index 0.
>    * The objects are the ones that would be used as DOMEvent.currentTarget while
>    * dispatching an event to aEventTarget
>    * @note Some events, especially 'load', may actually have a shorter
>    *       event target chain than what this methods returns.
>    */
>   void getEventTargetChainFor(in nsIDOMEventTarget aEventTarget,
>+                              in boolean composed,
I wonder if composed should be optional. Maybe not. Maybe better to be explicit here.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8792456 [details] [diff] [review]
Part3 Add mComposedInNativeAnonymousContent to determine which events can cross the boundary of native anonymous content (V2)




>+  void SetDefaultComposedInNativeAnonymousContent()
>+  {
>+    // For compatibility concerns, we explicitly set all events (except those
>+    // events we want to stop propagation) currently we have to true and default
>+    // value to false in case we forget to set the flag to false for new events.
>+    switch (mClass) {
>+      case eAnimationEventClass:
>+      case eBeforeAfterKeyboardEventClass:
>+      case eClipboardEventClass:
>+      case eCommandEventClass:
>+      case eCompositionEventClass:
>+      case eDragEventClass:
>+      case eEditorInputEventClass:
>+      case eFocusEventClass:
>+      case eFormEventClass:
>+      case eGestureNotifyEventClass:
>+      case eGUIEventClass:
>+      case eInputEventClass:
>+      case eKeyboardEventClass:
>+      case eTouchEventClass:
>+      case eMouseScrollEventClass:
>+      case eMutationEventClass:
>+      case ePluginEventClass:
>+      case ePointerEventClass:
>+      case eQueryContentEventClass:
>+      case eSelectionEventClass:
>+      case eScrollAreaEventClass:
>+      case eScrollPortEventClass:
>+      case eSimpleGestureEventClass:
>+      case eSMILTimeEventClass:
>+      case eTransitionEventClass:
>+      case eUIEventClass:
>+      case eWheelEventClass:
>+      case eSVGZoomEventClass:
>+      case eMouseEventClass:
>+        mFlags.mComposedInNativeAnonymousContent = true;
>+        break;
>+      case eBasicEventClass:
>+        // nsVideoFrame may create anonymous image element which fires eLoad,
>+        // eLoadStart, eLoadEnd, eLoadError. We don't want these events cross
>+        // the boundary of NAC
>+        mFlags.mComposedInNativeAnonymousContent = mMessage != eLoad &&
>+                                                   mMessage != eLoadStart &&
>+                                                   mMessage != eLoadEnd &&
>+                                                   mMessage != eLoadError;
>+        break;
>+      default:
>+        mFlags.mComposedInNativeAnonymousContent = false;
>+        break;
>+    }
>+  }
I don't quite understand this. Why list all the e*EventClasses here, and have different behavior for
default case?
I would expect this to just have check for eLoad/eLoadStart/eLoadEnd/eLoadError and otherwise have 
mComposedInNativeAnonymousContent true.
That fixed, r+

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8792457 [details] [diff] [review]
Part4 Refine test case to listen body load event instead of video load event (V2)

I assume you tested other browsers that they don't fire load either for poster.
If not, please test.
(and if they do, we'll need to file some spec bugs)

Comment 14

[Ming-Chou Shih [:stone]]

(In reply to Olli Pettay [:smaug] (reviewing overload) from comment #13)
> Comment on attachment 8792457 [details] [diff] [review]
> I assume you tested other browsers that they don't fire load either for
> poster.
> If not, please test.
> (and if they do, we'll need to file some spec bugs)

Yes, tested on Edge (windows10, 25.10586.0.0) and Chrome Canary (55.0.2875.0 canary (64-bit)) and they don't fire load event for poster.

Comment 15

[Ming-Chou Shih [:stone]]

Attachment: Bug 1292063-Part1: Add Event.composed. r=smaug. (V3)

Followed review comments to refine patch and updated the patch summary

Comment 16

[Ming-Chou Shih [:stone]]

Attachment: Bug 1292063-Part2: Add composed argument to getEventTargetChainFor and refine test cases. r=smaug. (V3)

Updated the patch summary

Comment 17

[Ming-Chou Shih [:stone]]

Attachment: Bug 1292063-Part3: Add mComposedInNativeAnonymousContent to determine which events can cross the boundary of native anonymous content. r=smaug. (V3)

Followed review comments to refine patch and updated the patch summary

Comment 18

[Ming-Chou Shih [:stone]]

Attachment: Bug 1292063-Part4: Refine test case to listen body load event instead of video load event. r=smaug.

Updated the patch summary

Comment 19

[Ming-Chou Shih [:stone]]

Attachment: Bug 1292063-Part5: Update the meta file of shadow-dom/event-composed.html. r=smaug.

Updated the patch summary

Comment 20

[Ming-Chou Shih [:stone]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea6ceb597c3beaa54c39ce1aa3fe900459c03e73

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/22cf15bb84d4
Part 1: Add Event.composed. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/79c5fa0ce1c6
Part 2: Add composed argument to getEventTargetChainFor and refine test cases. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/a36f68c2886b
Part 3: Add mComposedInNativeAnonymousContent to determine which events can cross the boundary of native anonymous content. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c3aadf10c99
Part 4: Refine test case to listen body load event instead of video load event. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/06f519962c12
Part 5: Update the meta file of shadow-dom/event-composed.html. r=smaug

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/22cf15bb84d4
https://hg.mozilla.org/mozilla-central/rev/79c5fa0ce1c6
https://hg.mozilla.org/mozilla-central/rev/a36f68c2886b
https://hg.mozilla.org/mozilla-central/rev/2c3aadf10c99
https://hg.mozilla.org/mozilla-central/rev/06f519962c12

Comment 23

[Eric Shepherd [:sheppy]]

Because the spec isn’t very detailed on this issue, and finding other information was difficult, I had to piece this together a bit. I believe I have documented this correctly but I’d appreciate it if someone would please verify. If I’m mistaken, please put dev-doc-needed back on this bug and let me know what I got wrong. Thanks!

We had an existing article on Event.scoped, which I’ve renamed to Event.composed, with the appropriate redirect in place. I then updated the article, which didn’t make much sense, to match what appears to be the function of this property.

From what I can tell reading the spec and the code, this basically reports whether or not an event can bubble across the shadow root into the standard DOM.

If that’s untrue, please let me know what I’ve got wrong so I can fix it.

https://developer.mozilla.org/en-US/docs/Web/API/Event/composed

https://developer.mozilla.org/en-US/docs/Web/API/Event and Firefox 52 for developers have been similarly updated.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Not bubble but propagate. Bubble is just one of the event propagation phases.

Comment 25

[Ming-Chou Shih [:stone]]

Looks good to me, thanks.