Bug 211613 – Add a helper that returns a Node if it is an Element and otherwise returns its parent Element

• Component: WebCore Misc.
• Status: NEW

[Wenson Hsieh]

Inspired by <https://bugs.webkit.org/show_bug.cgi?id=211151#c8>

[Wenson Hsieh]

Created attachment 398838 [details]
Patch

[Chris Dumez]

lineageOfType<Element>(node).first()

[Chris Dumez]

(In reply to Chris Dumez from comment #2)
> lineageOfType<Element>(node).first()

Actually it looks like lineageOfType() only takes an Element in parameter for some reason.

[Chris Dumez]

Comment on attachment 398838 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398838&action=review

> Source/WebCore/dom/Node.h:125
> +    Element* inclusiveParentElement();

I find this name to be very confusing. It is unclear what an inclusive parent is supposed to be.

[Chris Dumez]

Comment on attachment 398838 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398838&action=review

>> Source/WebCore/dom/Node.h:125
>> +    Element* inclusiveParentElement();
> 
> I find this name to be very confusing. It is unclear what an inclusive parent is supposed to be.

My proposal: firstElementInLineage()

Naming would be consistent with our Element iterators (i.e. linerageOfType<>() / ancestorsOfType<>()).

[Wenson Hsieh]

(In reply to Chris Dumez from comment #5)
> Comment on attachment 398838 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=398838&action=review
> 
> >> Source/WebCore/dom/Node.h:125
> >> +    Element* inclusiveParentElement();
> > 
> > I find this name to be very confusing. It is unclear what an inclusive parent is supposed to be.

Darin offered this name, with the reasoning that the DOM specification uses the term "inclusive ancestor" to mean "self or ancestor", similarly "inclusive descendant" and "inclusive sibling”, so "inclusive parent" means self or parent element.

> 
> My proposal: firstElementInLineage()
> 
> Naming would be consistent with our Element iterators (i.e.
> linerageOfType<>() / ancestorsOfType<>()).

Nice — I find this name clear and consistent.

[Darin Adler]

(In reply to Chris Dumez from comment #5)
> My proposal: firstElementInLineage()

Great, love that name!

[Darin Adler]

So, should we have this as a Node member function, or a non-member function?

- I think that member functions are tidier and easier to read at call sites.

- On the other hand, an advantage of a non-member function is that we can have one that takes a Node& and another that takes a Node*.

Also, would be nice to either optimize calling this on Element& or Element* to just return the passed-in object, or to make it a compile time error to call it on one. If it’s a member function we'd do that with an override with "= delete" in the Element class. If it’s a non-member function we’d do it with an overload; not sure exactly how to make it a compile time error but I’m sure there’s a way.

[Darin Adler]

Comment on attachment 398838 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398838&action=review

There are a few other call sites that could use this. I found Range::createContextualFragment, HitTestResult::targetNode, and HitTestResult::innerNonSharedElement.

I don’t think we should land it with this name, but the direction does seem good.

> Source/WebCore/accessibility/AXObjectCache.cpp:2954
> +    auto* element = node->inclusiveParentElement();

This could be using lineageOfType<Element>, after getting this start, so same as what I say at the next call site below. Seems like almost half of these are just trying to get a lineageOfType started!

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1047
> +    for (auto& element : lineageOfType<Element>(*node->inclusiveParentElement())) {

For a case like this, it seems the real solution would be to upgrade lineageOfType so it can take a Node&.

This leads to the idea of just using lineageOfType<Element>().first() always for this as Chris suggested (could even find a way to optimize that so it doesn’t unnecessarily loop if the parent is a non-Element ContainerNode). Love to hear what Antti thinks — I believe he created lineageOfType so he might remembers why it doesn’t have an overload for Node. I think it was part of our ambition to refactor in the direction of having an Element-centric DOM where text nodes would not have to be allocated in advance.

Might also want to make it take a Node*/Element* and not iterate anything in that case; could really be handy to keep code tidy and not have a lot of explicit null checks. I would like to hear Antti’s opinion on that, too.

Note also that we intend to rename lineageOfType to lineage.

Given how many of these are just getting an argument to pass to lineageOfType, seems like we should fix that first. And even some of the ones not using it seem like they could be using it.

>>>> Source/WebCore/dom/Node.h:125
>>>> +    Element* inclusiveParentElement();
>>> 
>>> I find this name to be very confusing. It is unclear what an inclusive parent is supposed to be.
>> 
>> My proposal: firstElementInLineage()
>> 
>> Naming would be consistent with our Element iterators (i.e. linerageOfType<>() / ancestorsOfType<>()).
> 
> Darin offered this name, with the reasoning that the DOM specification uses the term "inclusive ancestor" to mean "self or ancestor", similarly "inclusive descendant" and "inclusive sibling”, so "inclusive parent" means self or parent element.

Love the firstElementInLineage name!

We should consider returning RefPtr instead of a raw pointer, as in any new function, better for helping us use smart pointers more consistently in the future. For a new function with no existing call sites, worth starting off right.

[Darin Adler]

(In reply to Darin Adler from comment #9)
> There are a few other call sites that could use this. I found
> Range::createContextualFragment, HitTestResult::targetNode, and
> HitTestResult::innerNonSharedElement.

Also Position::element.