Closed Bug 1613830 Opened 5 years ago Closed 5 years ago

Add a helper method to nsINode which returns itself (if it's an Element) or its parent element (if it's not an element)

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla75
Tracking Flags:
Tracking Status
firefox74 --- wontfix
firefox75 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(1 file)

Bug 1613830 - Add `nsINode::GetAsElementOrParentElement()` r=smaug!
47 bytes, text/x-phabricator-request
Details | Review

In editor module, if there is a helper method in nsINode (or nsIContent) which returns itself when it's an Element or its parent element when it's not an Element, it's useful. For example, there are this kind of loops:

for (nsIContent* content = aContent; content; content = content->GetParent()) {
  if (HTMLEditUtils::IsList(content)) {
    DoSomething(*content->AsElement());
  }
}

If the helper method name is GetAsElementOrParentElement(), this can be:

for (Element* element = aContent->GetAsElementOrParentElement(); element; element->GetParentElement()) {
  if (HTMLEditUtils::IsList(element)) {
    DoSomething(*element);
  }
}

I think that GetAsElementOrParentElement() or GetClosestElement() can be its name. What do you think, smaug?

Flags: needinfo?(bugs)

I meant this is what I want:

  return IsElement() ? AsElement() : GetParentElement();

Do we need to care about ShadowDOM?
Say, if ShadowRoot has a text node as a child, should the method return ShadowRoot::Host() ?
Or perhaps we could explicitly limit to same DOM subtree, so that this would never cross shadow boundary (this is probably the case).

GetAsElementOrParentElement() is a bit long, but it is also very clear what it does, so perhaps I prefer that.
And Get* hints about possible null value.

Flags: needinfo?(bugs)

(In reply to Olli Pettay [:smaug] from comment #2)

Do we need to care about ShadowDOM?

In my understanding, internal contents in ShadowDOM shouldn't be editable when its parent tree's contenteditable has focus. Therefore, I don't need API for that.

Say, if ShadowRoot has a text node as a child, should the method return ShadowRoot::Host() ?

If there should be an API which can cross ShadowDOM boundaries, I think so because there are an existing API pair, nsINode::GetParentNode() vs. GetParentOrShadowHostNode().

This patch assumes that only element node can have content node. I.e., we
won't hit the following MOZ_ASSERT:

Element* element = nullptr;
nsIContent* content = aContent;
while (content) {
  if (!content->IsElement()) {
    element = content->AsElement();
    break;
  }
  content = content->GetParent();
}
MOZ_ASSERT(!content || content == element || content->GetParent() == element);

I think this is not a great idea... For once, the example in comment 0 seems cleaner to me without this. But ignoring Shadow DOM seems somewhat unfortunate.

Should we instead add something like an nsINode::InclusiveAncestors<T> iterator which would do something like:

nsINode* cur = this;
do {
  if (auto* t = T::FromNode(cur)) {
    return t;
  }
  cur = cur->GetParentNode();
} while (cur);

That way your example becomes:

for (Element* element : InclusiveAncestors<Element>(aContent)) {
  if (HTMLEditUtils::IsList(element)) {
    DoSomething(*element);
  }
}

wdyt?

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)

Should we instead add something like an nsINode::InclusiveAncestors<T> iterator which would do something like:

nsINode* cur = this;
do {
  if (auto* t = T::FromNode(cur)) {
    return t;
  }
  cur = cur->GetParentNode();
} while (cur);

I think that it's overwork for the original purpose because Element node must be the content itself or its parent because of validation at adding DOM node to a parent node.

That way your example becomes:

for (Element* element : InclusiveAncestors<Element>(aContent)) {
  if (HTMLEditUtils::IsList(element)) {
    DoSomething(*element);
  }
}

On the other hand, this example is really interesting if it can take limiter node such as editing host.

I think that it's enough to add GetAsElementOrParentElementOrShadowHost() and GetAsElementOrFlattenedTreeParentElement() for avoiding the confusion.

Flags: needinfo?(emilio)

Ah, but GetAsElementOrParentElementOrShadowHost() does not make sense since its result cannot be Element*. Perhaps, adding GetAsElementOrFlattenedTreeParentElement() must be enough for avoiding the confusion.

(Oops, I'm confused with shadow root. shadow host has to be an element.)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #7)

Ah, but GetAsElementOrParentElementOrShadowHost() does not make sense since its result cannot be Element*.

Hmm, how not? A ShadowRoot is not an element, but its shadow host should be. Err.. just saw your edit.

Perhaps, adding GetAsElementOrFlattenedTreeParentElement() must be enough for avoiding the confusion.

I don't feel very strongly, if there's a usecase for any of these sure, otherwise not worth adding dead code IMO.

Flags: needinfo?(emilio)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #8)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #7)

Ah, but GetAsElementOrParentElementOrShadowHost() does not make sense since its result cannot be Element*.

Hmm, how not? A ShadowRoot is not an element, but its shadow host should be. Err.. just saw your edit.

Yeah, sorry for the spam.

Perhaps, adding GetAsElementOrFlattenedTreeParentElement() must be enough for avoiding the confusion.

I don't feel very strongly, if there's a usecase for any of these sure, otherwise not worth adding dead code IMO.

Okay, then, I'll land the patch as-is.

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #6)

I think that it's overwork for the original purpose because Element node must be the content itself or its parent because of validation at adding DOM node to a parent node.

Out of curiosity, why do you think it's overwork? That is not true if the element is in a shadow tree, and GetParentElement does an IsElement check anyway.

On the other hand, this example is really interesting if it can take limiter node such as editing host.

What is the problem with if (element == editingHost) { break; }? all these iterators would be lazy.

I just gave it a shot and this doesn't seem complicated to implement... I'll post a patch soonish just for feedback.

See Also: → 1617084

(In reply to Emilio Cobos Álvarez (:emilio) from comment #10)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #6)

I think that it's overwork for the original purpose because Element node must be the content itself or its parent because of validation at adding DOM node to a parent node.

Out of curiosity, why do you think it's overwork? That is not true if the element is in a shadow tree, and GetParentElement does an IsElement check anyway.

Using loop for the purpose makes some developers think that non-element content may be able to contain another content node. I'd like to make such code simpler for easier to understand.

On the other hand, this example is really interesting if it can take limiter node such as editing host.

What is the problem with if (element == editingHost) { break; }? all these iterators would be lazy.

Of course, it's okay, but I like limiter is also checked by the iterator. Then, the loop block becomes clearer for what it does. And also it may avoid to forget checking the loop end because there are a lot of buggy loop in editor which do not check editing host.

I just gave it a shot and this doesn't seem complicated to implement... I'll post a patch soonish just for feedback.

Thank you! I'll check it.

Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/04dcc4d0d505 Add `nsINode::GetAsElementOrParentElement()` r=smaug

https://hg.mozilla.org/mozilla-central/rev/04dcc4d0d505

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox75: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla75
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: