Closed Bug 1430301 Opened 2 years ago Closed 2 years ago

Implement ShadowRoot.elementFromPoint/elementsFromPoint

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: smaug, Assigned: smaug)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files)

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.