Implement web components event retargeting.

RESOLVED FIXED in mozilla34

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: wchen, Assigned: wchen)

Tracking

(Blocks: 2 bugs, {dev-doc-needed})

unspecified
mozilla34
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

4 years ago
https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#event-retargeting

Comment 1

4 years ago
.relatedTarget handling probably needs something new, but hopefully .target can be dealt with
the current system.
http://mxr.mozilla.org/mozilla-central/source/content/base/src/FragmentOrElement.cpp?rev=1cb64629a1c9&mark=821-822,826-827#813
would probably need to deal web component event retargeting.
(Assignee)

Comment 2

4 years ago
Created attachment 820838 [details] [diff] [review]
Implement web components event path and event retargeting.
Attachment #820838 - Flags: review?(mrbkap)
Comment on attachment 820838 [details] [diff] [review]
Implement web components event path and event retargeting.

smaug should really be reviewing this one.
Attachment #820838 - Flags: review?(mrbkap) → review?(bugs)

Comment 4

4 years ago
Better to run some perf tests.
At least http://mozilla.pettay.fi/moztests/events/event_speed_2.1_nolistener.html

Comment 5

4 years ago
Comment on attachment 820838 [details] [diff] [review]
Implement web components event path and event retargeting.

First some coding style nits

>   virtual void SetShadowRoot(ShadowRoot* aBinding) MOZ_OVERRIDE;
>+  virtual nsTArray<nsIContent*> &DestInsertionPoints() MOZ_OVERRIDE;
>+  virtual nsTArray<nsIContent*> *GetExistingDestInsertionPoints() const MOZ_OVERRIDE;
Nit, & and * go with the type.
So nsTArray<nsIContent*>& and nsTArray<nsIContent*>*

>+  /**
>+   * Same as DestInsertionPoints except that this method will return
>+   * null if the array of destination insertion points does not already
>+   * exist.
>+   */
>+  virtual nsTArray<nsIContent*> *GetExistingDestInsertionPoints() const = 0;
ditto

>+FragmentOrElement::DestInsertionPoints()
>+{
>+  nsDOMSlots *slots = DOMSlots();
and here

>+  return slots->mDestInsertionPoints;
>+}
>+
>+nsTArray<nsIContent*>*
>+FragmentOrElement::GetExistingDestInsertionPoints() const
>+{
>+  nsDOMSlots *slots = GetExistingDOMSlots();
and here

>+nsGenericDOMDataNode::DestInsertionPoints()
>+{
>+  nsDataSlots *slots = DataSlots();
ditto

>+nsGenericDOMDataNode::GetExistingDestInsertionPoints() const 
>+{
>+  nsDataSlots *slots = GetExistingDataSlots();
ditto

>+  virtual nsTArray<nsIContent*> &DestInsertionPoints() MOZ_OVERRIDE;
>+  virtual nsTArray<nsIContent*> *GetExistingDestInsertionPoints() const MOZ_OVERRIDE;
And here

And maybe elsewhere too

Comment 6

4 years ago
Comment on attachment 820838 [details] [diff] [review]
Implement web components event path and event retargeting.

>   bool isAnonForEvents = IsRootOfChromeAccessOnlySubtree();
>   if ((aVisitor.mEvent->message == NS_MOUSE_ENTER_SYNTH ||
>        aVisitor.mEvent->message == NS_MOUSE_EXIT_SYNTH) &&
>       // Check if we should stop event propagation when event has just been
>       // dispatched or when we're about to propagate from
>       // chrome access only subtree.
>       ((this == aVisitor.mEvent->originalTarget &&
>-        !ChromeOnlyAccess()) || isAnonForEvents)) {
>+        !ChromeOnlyAccess()) || isAnonForEvents || GetShadowRoot())) {
Should we check whether event is trusted? Spec doesn't talk about trusted only, so I guess no.

> 
>   nsIContent* parent = GetParent();
>+  if (HasFlag(NODE_IS_IN_SHADOW_TREE)) {
>+    // Events propagate from ShadowRoot to the ShadowRoot host.
>+    ShadowRoot* thisShadowRoot = ShadowRoot::FromNode(this);
>+    if (thisShadowRoot) {
>+      parent = thisShadowRoot->GetHost();
>+    }
>+  }
Why to call GetParent() if you may set parent to thisShadowRoot->GetHost(); in the next if.

Perhaps 
ShadowRoot* shadowRoot;
nsIContent* parent =
  HasFlag(NODE_IS_IN_SHADOW_TREE) && shadowRoot = ShadowRoot::FromNode(this)) ?
    shadowRoot : GetParent();

or some such. 


>+  // Web components have a special event chain.
>+  nsTArray<nsIContent*>* destPoints = GetExistingDestInsertionPoints();
We really don't want an extra virtual call if just possible.
Can we call GetExistingDestInsertionPoints only if HasFlag(NODE_IS_IN_SHADOW_TREE) is true?


I'll need to read the spec few times and continue reviewing this then.
(Assignee)

Comment 7

4 years ago
(In reply to Olli Pettay [:smaug] from comment #4)
> Better to run some perf tests.
> At least
> http://mozilla.pettay.fi/moztests/events/event_speed_2.1_nolistener.html

The numbers are around the same with and without the changes for this microbenchmark. It's at around 106ms in both cases. I'm getting a bit of variance between runs of about 1-2ms, but the numbers do not clearly indicate that the changes have made it slower.

Comment 8

4 years ago
Comment on attachment 820838 [details] [diff] [review]
Implement web components event path and event retargeting.

>+  // Web components have a special event chain.
>+  nsTArray<nsIContent*>* destPoints = GetExistingDestInsertionPoints();
>+  if (destPoints) {
>+    // There is a sequence of insertion points that need to go
>+    // in the event chain.
This is the crazy part of shadow dom's event dispatch.
I hope the spec will change, but since it is currently on crack, our
implementation needs to be too.


> 
>-  if (parent) {
>+  if (!aVisitor.mDestInsertionPoints.IsEmpty()) {
>+    // For events involving distributed nodes in web components,
>+    // propagate the event to according to the destination insertion
>+    // points until there are none remaining, then follow the normal
>+    // path.
>+    aVisitor.mParentTarget = aVisitor.mDestInsertionPoints[0];
>+    aVisitor.mDestInsertionPoints.RemoveElementAt(0);
Could we please keep destination insertion points in the same order as in the spec.
So mParentTarget should be set to the last insertion point, not to the first.
"Let CURRENT be the final destination of CURRENT"

>@@ -247,16 +249,43 @@ nsDOMMouseEvent::GetRelatedTarget()
>         do_QueryInterface(mEvent->AsMouseEventBase()->relatedTarget);
>       break;
>     default:
>       break;
>   }
> 
>   if (relatedTarget) {
>     nsCOMPtr<nsIContent> content = do_QueryInterface(relatedTarget);
>+    nsCOMPtr<nsIContent> currentTarget = do_QueryInterface(mEvent->currentTarget);
>+
>+    // Return an adjusted related target in order to preserve encapsulation
>+    // in web components.
>+    if (content && currentTarget &&
>+        content->GetRootShadow() != currentTarget->GetRootShadow()) {
>+      // Walk up the ancestor node trees of the related target until
>+      // we encounter the node tree of the current target in order
>+      // to find the adjusted related target. Walking up the tree may
>+      // not find a common ancestor node tree if the related target is in
>+      // an ancestor tree, but in that case it does not need to be adjusted.
>+      nsIContent* adjustedTarget = content;
>+      while (adjustedTarget) {
>+        ShadowRoot* ancestorNodeTree = adjustedTarget->GetRootShadow();
>+        if (currentTarget->GetRootShadow() == ancestorNodeTree) {
>+          relatedTarget = adjustedTarget;
>+          break;
>+        }
>+
>+        if (!ancestorNodeTree) {
>+          break;
>+        }
>+
>+        adjustedTarget = ancestorNodeTree->GetHost();
>+      }
>+    }
>+
Could you move this stuff to nsDOMEvent so that other events can use it.
(and if it was static, Touch could use it too.)



Could I see a new patch with all the comments fixed.
This is complicated stuff.
Attachment #820838 - Flags: review?(bugs) → review-
Is this bug on anyone's plate? This has a few repercussions to accessibility, and would give us a clearer path forward in B2G.

Updated

3 years ago
Blocks: 1014592
(Assignee)

Comment 10

3 years ago
This is on my plate. I will have patches up for this soon.
Blocks: 1005830
(Assignee)

Comment 11

3 years ago
Created attachment 8477243 [details] [diff] [review]
Implement web components event path and event retargeting. v2
Attachment #820838 - Attachment is obsolete: true
Attachment #8477243 - Flags: review?(bugs)
swilkes: cc'ing you in so you are aware that this is blocking gaia-header being fully accessible.

Updated

3 years ago
Blocks: 1011601
(Assignee)

Comment 13

3 years ago
This is updated event path algorithm that I put together from all the notes and examples with some slight modifications to better handle shadow insertion points.

Input
    NODE, a node
    EVENT, an event
Output
    PATH, an event path, a ordered list of an event target

1. Let PATH be the empty ordered list of nodes
2. Let CURRENT be NODE
3. Let INSERTION-POINTS be an empty stack of nodes.
4. Add CURRENT to PATH
5. Repeat while CURRENT exists:
    1. If the destination insertion points of CURRENT is not empty:
        1. Push the destination insertion points into INSERTION-POINTS in order of first destination to final destination, excluding shadow insertion points.
        2. Let LAST-PUSHED-INSERTION-POINT be the last node in the destination insertion points of CURRENT to be pushed into INSERTION-POINTS
        2. If LAST-PUSHED-INSERTION-POINT exists:
            1. Pop INSERTION-POINTS and set CURRENT to be the popped node.
    2. If CURRENT is a shadow root:
        1. If INSERTION-POINTS is not empty:
            1. Pop INSERTION-POINTS and set CURRENT to be the popped node.
        2. Otherwise:
            1. Let SHADOW-HOST be the shadow host which hosts CURRENT
            2. If SHADOW-HOST hosts the node tree which NODE participates in and EVENT is one of the events which must be stopped:
                1. Stop this algorithm
            3. If CURRENT is not the youngest shadow root hosted by SHADOW-HOST:
                1. Let SHADOW-INSERTION-POINT be the shadow insertion point into which CURRENT shadow root is projected.
                2. Set CURRENT to SHADOW-INSERTION-POINT.
            4. Otherwise set CURRENT to SHADOW-HOST.
    3. Otherwise:
        1. Let CURRENT be the parent node of CURRENT.
    4. If CURRENT exists:
        1. Add CURRENT to PATH.
Blocks: 1011604
Hi,

we landed the gaia part in contacts as the creator of the patch asked us to give it a try and have it QA even before we branch 2.1.

Eitan, if you are not ok, I'll backout the gaia part for contacts. 

In any case, we need to know if we are on track with this, since the gaia-headers webcomponent already landed in several apps.
Flags: needinfo?(eitan)

Comment 15

3 years ago
Please let me know if we can land other patches in the meantime. Web components needs maximum QA time before FL so (as I'm sure we all know right now) every day counts right now. Thanks and let me know.
I think the question is if wchen has any confidence that this could land during this cycle. I tried out Gaia headers with the latest patch on this bug, and accessibility works as expected.
Flags: needinfo?(eitan) → needinfo?(wchen)
I'm just reviewing this. But note, the event propagation path isn't sorted out in the spec level.
The current spec has something odd, and we probably end up to something close to, or exactly to
comment 13.
Though, it is possible, or even probable, that the details in the path don't matter in most cases.
(In reply to Stephany Wilkes from comment #15)
> Please let me know if we can land other patches in the meantime. Web
> components needs maximum QA time before FL so (as I'm sure we all know right
> now) every day counts right now. Thanks and let me know.
I believe we should not land other patches before this bug has landed. To my knowledge (correct me if I'm wrong), there is no user benefit to have <gaia-header> in 2.1. But there is an accessibility impact if we land <gaia-header> changes without this patch.

Comment 19

3 years ago
There is user benefit and impact in the sense that we cannot enable many other user-facing improvements until 2.1 header has landed; cannot easily make things work and look better in other formats, like tablet; cannot enable theme-ing support for contributors; etc. Not landing header blocks all of the other web components work (which will include things like RTL support) for 2.2, and so on. There is user impact to many user communities that may not be obvious.
I don't understand how not landing this in 2.1 blocks 2.2 work. We can land it the first day after we branch 2.1.

Comment 21

3 years ago
I suggest discussing that with Kevin and Vivien and others. Also speak to Wilson about landing, risk and why he wants maximal stabilization time for these, please. I'm only on the UX side of this and the devs have a lot of this story.
The gaia-header component has already landed in many apps. The only risk to 2.2 we have is that developers may be blocked from landing code until then when they can get an early start now. 

Since web components are only enabled for certified apps, I think it makes sense to land this once reviewed even if there are spec questions. Since there is already a patch here, even if it does't make the branch date it's something that we could potentially uplift to aurora.

Updated

3 years ago
Blocks: 1015297

Updated

3 years ago
Blocks: 1011602
(In reply to Anthony Ricaud (:rik) from comment #20)
> I don't understand how not landing this in 2.1 blocks 2.2 work. We can land
> it the first day after we branch 2.1.

This would require the backing-out of ~18 patches that landed over the course of 2.1 feature sprints. Eitan has confirmed that this patch does address the accessibility issues with shadow-dom, and by the looks of things it will be landing shortly.

It would be advantageous if we could land the outstanding gaia-header patches now so that any regressions can be addressed before feature-lock. But I understand if people are uncomfortable with this.

IMO we should relax and focus our energy on more important things as there seems to be little risk here.
No longer blocks: 1015297, 1011602

Updated

3 years ago
Blocks: 1016814
Discussing with Eitan, apps that can regress in terms of accessibility are Contacts, Dialer, SMS, System and Settings. I've marked all the bugs for those apps as depending on this.
(Assignee)

Comment 25

3 years ago
(In reply to Eitan Isaacson [:eeejay] from comment #16)
> I think the question is if wchen has any confidence that this could land
> during this cycle. I tried out Gaia headers with the latest patch on this
> bug, and accessibility works as expected.

I think that a fix can make it in this cycle as I don't think the patch isn't doing anything particularly objectionable. Given the time sensitive nature of the bug I'll address review comments with top priority.
Flags: needinfo?(wchen)
Of course I meant "apps that *can't* regress".

(In reply to Wilson Page [:wilsonpage] from comment #23)
> This would require the backing-out of ~18 patches that landed over the
> course of 2.1 feature sprints. Eitan has confirmed that this patch does
> address the accessibility issues with shadow-dom, and by the looks of things
> it will be landing shortly.
I don't think that requires backing out 18 patches but just the Contacts one and System one per comment 24.

Updated

3 years ago
Blocks: 1015297, 1011602
Comment on attachment 8477243 [details] [diff] [review]
Implement web components event path and event retargeting. v2

>+      // In the web components case, we may need to stop propagation of events.
>+      nsIContent* adjustedTarget =
>+        Event::GetShadowRelatedTarget(this, relatedTarget);
>+      if (this == adjustedTarget) {
>+        aVisitor.mParentTarget = nullptr;
I think we need to propagate the event to chrome in any case.
So, find if we're in a window and get nsPIDOMWindow::GetParentTarget()


The patch makes event propagation go out from the shadow dom even if we should stop
(and only propagate to chrome)
http://w3c.github.io/webcomponents/spec/shadow/#events-that-are-always-stopped
Well, in case of 'load', we shouldn't propagate to chrome. ('load' is very special. It never propagate from document to window)

>+++ b/content/base/src/nsContentUtils.cpp
>@@ -2125,16 +2125,23 @@ nsContentUtils::ContentIsCrossDocDescend
>                                               nsINode* aPossibleAncestor)
> {
>   NS_PRECONDITION(aPossibleDescendant, "The possible descendant is null!");
>   NS_PRECONDITION(aPossibleAncestor, "The possible ancestor is null!");
> 
>   do {
>     if (aPossibleDescendant == aPossibleAncestor)
>       return true;
>+
>+    // Step over shadow root to the host node.
>+    ShadowRoot* shadowRoot = ShadowRoot::FromNode(aPossibleDescendant);
>+    if (shadowRoot) {
>+      aPossibleDescendant = shadowRoot->GetHost();
>+    }
>+
Looks like this is the right thing to do for all the callers, but please document in nsContentUtils.h

>@@ -274,16 +275,23 @@ MouseEvent::GetRelatedTarget()
>         do_QueryInterface(mEvent->AsMouseEventBase()->relatedTarget);
>       break;
>     default:
>       break;
>   }
> 
>   if (relatedTarget) {
>     nsCOMPtr<nsIContent> content = do_QueryInterface(relatedTarget);
>+    nsCOMPtr<nsIContent> currentTarget = do_QueryInterface(mEvent->currentTarget);
>+
>+    nsIContent* shadowRelatedTarget = GetShadowRelatedTarget(currentTarget, content);
>+    if (shadowRelatedTarget) {
>+      relatedTarget = shadowRelatedTarget;
>+    }
Could you file a followup to fix FocusEvent too.
That doesn't do the magic MouseEvent does right now, but we haven't also really mapped that to
all the focus handling properly yet.


>+function eventListener(e) {
>+  eventChain.push(this);
>+}
>+
>+function isEventChain(actual, expected, msg) {
>+  is(actual.length, expected.length, msg);
>+  for (var i = 0; i < expected.length; i++) {
>+    is(actual[i], expected[i], msg + " at " + i);
>+  }
>+}
This is actually good to test the chain this way, but could you add also a check that
EventListenerService.getEventTargetChainFor() returns the same stuff.
getEventTargetChainFor() returns also the chrome event targets, so only the first expected.length items map to expected.length

>+/*
>+ * Test 2: Test of event dispatch through a nested ShadowRoots with content insertion points.
>+ *
>+ * <div elemFive> --- <shadow-root shadowTwo>
>+ *       |                       |
>+ * <div elemOne>          <div elemFour> ----- <shadow-root shadowOne>
>+ *                               |                        |
>+ *                       <content elemTwo>       <content elemThree>
>+ */

Where is 'test' in this hierarchy? Shouldn't it be under shadowOne


This is wonderfully simple.
Could take another look with those chrome dispatch fixed.
(I think we want the chrome dispatch so that this all works closer to xbl)
Attachment #8477243 - Flags: review?(bugs) → review-
(Assignee)

Comment 28

3 years ago
Created attachment 8479452 [details] [diff] [review]
Implement web components event path and event retargeting. v3

Addressed the review comments. The only big change I've made is adding code to stop events of certain types.
Attachment #8477243 - Attachment is obsolete: true
Attachment #8479452 - Flags: review?(bugs)
(Assignee)

Comment 29

3 years ago
Created attachment 8479456 [details] [diff] [review]
Implement web components event path and event retargeting. v3

Forgot to hg add a test file.
Attachment #8479452 - Attachment is obsolete: true
Attachment #8479452 - Flags: review?(bugs)
Attachment #8479456 - Flags: review?(bugs)
Comment on attachment 8479456 [details] [diff] [review]
Implement web components event path and event retargeting. v3

>+      // In the web components case, we may need to stop propagation of events.
>+      if (GetShadowRoot()) {
>+        nsIContent* adjustedTarget =
>+          Event::GetShadowRelatedTarget(this, relatedTarget);
>+        if (this == adjustedTarget) {
>+          // If we do stop propgation, we still want to propgate
propagate 

>+          // the event to chrome (nsPIDOMWindow::GetParentTarget()).
>+          nsCOMPtr<nsPIDOMWindow> win = OwnerDoc()->GetWindow();
>+          EventTarget* parentTarget = win ? win->GetParentTarget() : nullptr;
>+
>+          aVisitor.mParentTarget = parentTarget;
Shouldn't we still have mCanHandle = false here like in the previous patch.

And sorry, just realized we have mAutomaticChromeDispatch true by default.
So if mCanHandle is set false, we try to propagate to chrome.
So no need for  aVisitor.mParentTarget = parentTarget; here.

>+  ShadowRoot* thisShadowRoot = ShadowRoot::FromNode(this);
>+  if (thisShadowRoot) {
>+    // The following events must always be stopped at the root node of the node tree:
>+    //   abort
>+    //   error
>+    //   select
>+    //   change
>+    //   load
>+    //   reset
>+    //   resize
>+    //   scroll
>+    //   selectstart
>+    if (aVisitor.mDOMEvent) {
You don't want this 'if' here
(mDOMEvent can be null even if mEvent is non-null)

>+      bool stopEvent = false;
>+      switch (aVisitor.mEvent->message) {
>+        case NS_IMAGE_ABORT:
>+        case NS_LOAD_ERROR:
>+        case NS_FORM_SELECTED:
>+        case NS_FORM_CHANGE:
>+        case NS_LOAD:
>+        case NS_FORM_RESET:
>+        case NS_RESIZE_EVENT:
>+        case NS_SCROLL_EVENT:
>+          stopEvent = true;
>+          break;
>+        case NS_USER_DEFINED_EVENT:
.. but here before accessing mDOMEvent

>+      if (stopEvent) {
>+        // If we do stop propgation, we still want to propgate
propagate
Attachment #8479456 - Flags: review?(bugs) → review+
(Assignee)

Comment 31

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/fff4d503ad88
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/fff4d503ad88
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Keywords: dev-doc-needed
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1014592
You need to log in before you can comment on or make changes to this bug.