1430301 - Implement ShadowRoot.elementFromPoint/elementsFromPoint

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

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

http://w3c.github.io/webcomponents/spec/shadow/#extensions-to-the-documentorshadowroot-mixin

Comment 1

[Hsin-Yi Tsai (she/her)[:hsinyi]]

My understanding is that this is one we want to ship in our 1st implementation, mark P2. Feel free to correct me if I got something wrong.

Comment 2

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

ok, wpt tests are really buggy here, as far as I see.
And there are spec bugs too https://github.com/w3c/webcomponents/issues/735 and
https://github.com/w3c/webcomponents/issues/736 and others.

So, I'm just going to get a behavior compatible with Chrome which also fails on most of the tests.

Comment 3

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

Attachment: shadow_element_from_point.diff

So I just moved stuff from nsDocument to DocumentOrShadowRoot
and added basically just
+      if (node && node->IsShadowRoot()) {
+        node = static_cast<ShadowRoot*>(node)->Host();
+      }

and
+    node = Retarget(node);

On wpt this passes the same tests as Chrome and fails others the same way.
Spec is unclear and many tests buggy.

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

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8951327 [details] [diff] [review]
shadow_element_from_point.diff

Review of attachment 8951327 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, thanks!

Feel free to ignore the pre-existing nits if you want.

::: dom/base/DocumentOrShadowRoot.cpp
@@ +191,5 @@
> +                          ((aIgnoreRootScrollFrame ? nsIDocument::IGNORE_ROOT_SCROLL_FRAME : 0) |
> +                           (aFlushLayout ? nsIDocument::FLUSH_LAYOUT : 0) |
> +                           nsIDocument::IS_ELEMENT_FROM_POINT),
> +                          elementArray);
> +  if (elementArray.IsEmpty()) {

nit: elementArray.SafeElementAt(0) instead?

@@ +219,5 @@
> +  if (aFlags & nsIDocument::FLUSH_LAYOUT) {
> +    doc->FlushPendingNotifications(FlushType::Layout);
> +  }
> +
> +  nsIPresShell* ps = doc->GetShell();

Thanks for fixing up the pointers here :)

@@ +238,5 @@
> +    nsLayoutUtils::IGNORE_PAINT_SUPPRESSION | nsLayoutUtils::IGNORE_CROSS_DOC |
> +    ((aFlags & nsIDocument::IGNORE_ROOT_SCROLL_FRAME) ? nsLayoutUtils::IGNORE_ROOT_SCROLL_FRAME : 0));
> +
> +  // Dunno when this would ever happen, as we should at least have a root frame under us?
> +  if (outFrames.IsEmpty()) {

I know this is pre-existing, but this condition may as well be removed, since the other meaningful thing after this is a loop over outFrames.

Also, that may as well be an AutoTArray. Fine to do it as followups or what not.

@@ +245,5 @@
> +
> +  // Used to filter out repeated elements in sequence.
> +  nsIContent* lastAdded = nullptr;
> +
> +  for (uint32_t i = 0; i < outFrames.Length(); i++) {

nit: again, I know this is pre-existing, so feel free to ignore, but instead of indexing multiple times into the array you can just use a range-based for loop here.

@@ +248,5 @@
> +
> +  for (uint32_t i = 0; i < outFrames.Length(); i++) {
> +    nsIContent* node = doc->GetContentInThisDocument(outFrames[i]);
> +
> +    if (!node || !node->IsElement()) {

This condition doesn't make sense to me. If there's no node, we ho ahead and call GetParent on the next line.

I suspect we could effectively ASSERT(node) here, given that... But to be safe you can just do node && !node->IsElement(), I guess.

@@ +260,5 @@
> +          !nsSVGUtils::IsInSVGTextSubtree(outFrames[i])) {
> +        continue;
> +      }
> +      node = node->GetParent();
> +      if (node && node->IsShadowRoot()) {

nit:

if (auto* shadow = ShadowRoot::FromNodeOrNull(node)) {
  node = shadow->Host();
}

May be nicer.

@@ +268,5 @@
> +
> +    //XXXsmaug There is plenty of unspec'ed behavior here
> +    //         https://github.com/w3c/webcomponents/issues/735
> +    //         https://github.com/w3c/webcomponents/issues/736
> +    node = Retarget(node);

Ok, agreed it should be ok to align with Chrome in the meantime, thanks for leaving the links in a comment.

::: dom/base/DocumentOrShadowRoot.h
@@ +135,5 @@
> +    FLUSH_LAYOUT = 2,
> +    IS_ELEMENT_FROM_POINT = 4
> +  };
> +
> +  virtual void ElementsFromPointHelper(float aX, float aY,

no need for virtual, right?

Comment 5

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

(In reply to Emilio Cobos Álvarez [:emilio] from comment #4)
> 
> Feel free to ignore the pre-existing nits if you want.
I will :)

> 
> nit: elementArray.SafeElementAt(0) instead?
ignoring

> 
> I know this is pre-existing, but this condition may as well be removed,
> since the other meaningful thing after this is a loop over outFrames.
> 
> Also, that may as well be an AutoTArray. Fine to do it as followups or what
> not.
ignoring

> 
> @@ +245,5 @@
> > +
> > +  // Used to filter out repeated elements in sequence.
> > +  nsIContent* lastAdded = nullptr;
> > +
> > +  for (uint32_t i = 0; i < outFrames.Length(); i++) {
> 
> nit: again, I know this is pre-existing, so feel free to ignore, but instead
> of indexing multiple times into the array you can just use a range-based for
> loop here.
Not sure it is any better.

> > +    if (!node || !node->IsElement()) {
> 
> This condition doesn't make sense to me. If there's no node, we ho ahead and
> call GetParent on the next line.
> 
> I suspect we could effectively ASSERT(node) here, given that... But to be
> safe you can just do node && !node->IsElement(), I guess.
Again, existing code. I tried to keep the changes minimum. Perhaps I shouldn't have changed * syntax

> @@ +260,5 @@
> > +          !nsSVGUtils::IsInSVGTextSubtree(outFrames[i])) {
> > +        continue;
> > +      }
> > +      node = node->GetParent();
> > +      if (node && node->IsShadowRoot()) {
> 
> nit:
> 
> if (auto* shadow = ShadowRoot::FromNodeOrNull(node)) {
>   node = shadow->Host();
> }
> 
> May be nicer.
yeah, it is.

> ::: dom/base/DocumentOrShadowRoot.h
> @@ +135,5 @@
> > +    FLUSH_LAYOUT = 2,
> > +    IS_ELEMENT_FROM_POINT = 4
> > +  };
> > +
> > +  virtual void ElementsFromPointHelper(float aX, float aY,
> 
> no need for virtual, right?
ah, copy paste error.

Comment 6

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

(In reply to Olli Pettay [:smaug] from comment #5)
> > if (auto* shadow = ShadowRoot::FromNodeOrNull(node)) {
> >   node = shadow->Host();
> > }
of course won't use auto, since it doesn't improve readability

Comment 7

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

And looks like there are some more unexpected wpt passes. fixing those too of course.

Comment 8

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

Attachment: shadow_element_from_point_2.diff

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/35987648d470
Implement ShadowRoot.elementFromPoint/elementsFromPoint, r=emilio

Comment 10

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/35987648d470

Comment 11

[Chris Mills [:cmills]]

I already documented these ones; see https://developer.mozilla.org/en-US/docs/Web/API/DocumentOrShadowRoot