Implement ShadowRoot.elementFromPoint/elementsFromPoint

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

(Depends on 1 bug, {dev-doc-complete})

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(2 attachments)

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.
Priority: -- → P2
Depends on: 1436230
Assignee: nobody → bugs
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.
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
Attachment #8951327 - Flags: review?(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?
Attachment #8951327 - Flags: review?(emilio) → review+
(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.
(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
And looks like there are some more unexpected wpt passes. fixing those too of course.
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/35987648d470
Implement ShadowRoot.elementFromPoint/elementsFromPoint, r=emilio
https://hg.mozilla.org/mozilla-central/rev/35987648d470
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.