Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding .closest() to more nodes #161

Open
caub opened this issue Feb 9, 2016 · 10 comments · May be fixed by #883
Open

Adding .closest() to more nodes #161

caub opened this issue Feb 9, 2016 · 10 comments · May be fixed by #883
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest

Comments

@caub
Copy link

caub commented Feb 9, 2016

Text nodes have no .closest method, when working with Range, I find useful to do Text.prototype.closest = function(s) {return this.parentNode.closest(s); } //with this.parentNode&& if we want to be strict

For me it makes sense to be able to search from a textnode, .matches wouldn't make sense (edit: it could, by returning null), but .closest would

@annevk
Copy link
Member

annevk commented Feb 10, 2016

This sounds reasonable. @hzr, @domenic? (Anyone else from Google that would be better to ask for DOM questions?)

@domenic
Copy link
Member

domenic commented Feb 10, 2016

Maybe @coonsta?

@hzr
Copy link
Contributor

hzr commented Feb 10, 2016

Sounds reasonable to me too.

@annevk
Copy link
Member

annevk commented Feb 10, 2016

If I were to fix this I'd probably include all possible children of Element by the way, not just Text.

@caub
Copy link
Author

caub commented Mar 3, 2016

ok, tend also to do the one below (since I'm using .closest on all sort of nodes that comes with range.commonAncestorContainer)

Document.prototype.closest = function() { 
    return null;
};

@annevk annevk changed the title Text nodes .closest Adding .closest() to more nodes Apr 5, 2016
@dominiccooney
Copy link

All possible children of Element + Document—does it make sense to put this on Node? I suppose Document, DocumentFragment, and ShadowRoot .closest return null? (/cc @hayatoito for ShadowRoot.)

@annevk
Copy link
Member

annevk commented Apr 6, 2016

So, it does not make much sense for Document, DocumentType, DocumentFragment, ShadowRoot, and Attr, if it comes back as a node, but it seems there's at least a use case for Document. And if Document makes sense, even if it only ever returns null, so does ShadowRoot.

If we put it on node we should probably use [Unscopable].

@hayatoito
Copy link
Member

Yeah, regarding ShadowRoot, it returns null because it is not an element and it is always the root.

@dominiccooney
Copy link

I think if you include Document and ShadowRoot you may as well include DocumentFragment? I agree leaving this off Attr makes sense. Are there any other properties like this, can the principle of consistency guide the decision?

@annevk annevk added the needs implementer interest Moving the issue forward requires implementers to express interest label Aug 23, 2016
@annevk
Copy link
Member

annevk commented Mar 13, 2018

I think there are many properties like this, but for historical reasons they have ended up on Attr too. And for historical reasons DocumentType is a thing as well.

I guess we have two options here:

  1. Just use Node.
  2. Define a new TreeNode mixin that excludes Attr. (It seems we include DocumentType for other new mixins, such as ChildNode.)

I'm inclined to go with 2 if we still want to do this.

@annevk annevk added the addition/proposal New features or enhancements label Apr 11, 2019
@shvaikalesh shvaikalesh linked a pull request Jul 31, 2020 that will close this issue
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest
Development

Successfully merging a pull request may close this issue.

6 participants