Closed Bug 887541 Opened 11 years ago Closed 10 years ago

Implement web components event retargeting.

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla34

People

(Reporter: wchen, Assigned: wchen)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 3 obsolete files)

Implement web components event path and event retargeting. v3
47.34 KB, patch
smaug
: review+
Details | Diff | Splinter Review 
.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.

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

    
    

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

    
    

    
  
This is on my plate. I will have patches up for this soon.

    
    

    
  


  

        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.
Blocks: 1011601

    
    

    
  
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.

    
    

    
  
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)

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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.
Blocks: 1015297
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
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.

    
    

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

    
    

    
  


  
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)

    
    

    
  


  
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+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/fff4d503ad88
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Component: DOM → DOM: Core & HTML

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: