node.isConnected #81
Comments
|
If the only use case of That meets the requirements, I guess. One bonus is that we do not have to think about how The definition of "in a document deeply" is here: http://w3c.github.io/webcomponents/spec/shadow/#dfn-in-a-document-deeply |
|
If we use |
hayatoito
changed the title
hayatoito
changed the title
|
So |
|
Looks ok to me. |
|
Can we just call it |
|
Node.connected wouldn't say anything to me. Connected to where? parent? document? document in browsing context? |
|
"connected", as a term, means "node's shadow-including root is a document". The time to object to that decision was in #362 when we decided on |
|
Well, I'm not too happy having document specific callbacks, when the generic callback should be about ancestor chain change (crossing shadow dom boundaries). |
|
This was ... also decided previously? I am not sure if you are intentionally reopening old issues? |
|
What issue am I reopening? |
|
(The issue of document-specific callbacks.) I guess I just disagree with that, since then it raises the question of how this differs from (dis)connectedCallback. That makes it harder to understand. |
|
well, if you map connected to the callback name, I would then expect all the nodes to get a callback call when .connected value changes, but that isn't something to happen. The new property is a property of all the nodes, not only custom elements. |
|
Right, only custom elements have custom callbacks. |
|
But the naming here isn't too big issue to me. It is just that in general we should focus on readability. And also .connected is more likely to cause compat issues. |
|
How about |
|
Seems OK. We haven't settled on a platform convention for whether booleans get prefixed with is (e.g. bubbles and cancelable vs. isTrusted). |
|
Yeah, it largely varies. Also @smaug---- if you're concerned about clashes, should we mark it [Unscopeable] too? |
|
IDL: partial interface Node {
readonly attribute boolean isConnected;
};Can I get some LGTMs from @rniwa @esprehn @smaug---- @hayatoito? (Without [Unscopeable] since |
|
LGTM. Regarding |
|
I guess this is ok. "connected" is silly, but given that it is being used elsewhere, fine. But, without [Unscopeable] is worrisome. And it is equally worrisome with rootNode, now that I think about it. |
|
Sure, seems fine. I'm okay with |
|
@rniwa the proposal is still |
|
@annevk : I'm saying I'm fine with either option. |
|
Great, thank you! |
|
@smaug---- I will leave out [Unscopeable] for now, but please let me know if that worry turns into something where we need to add it. It's not a big deal either way I think. |
|
fwiw WebKit and Blink call this concept "inDocument". |
|
@esprehn "inDocument" seems very similar to https://dom.spec.whatwg.org/#in-a-document which is quite a bit different. Though that does explain (and I think we knew that already) why Google engineers didn't think a whole lot of HTML would need to be updated to account for Shadow DOM. |
|
FYI. Blink is renaming "inDocument", so it becomes consistent with the DOM Standard. e.g. We have to get used to new terms. |
|
Blink might want to use "isConnected" too, instead of "inShadowIncludingDocument", because it's short. Let me work on that later. |
|
Yeah, @domenic suggested we should maybe rename in shadow-including document to "is connected" in the DOM Standard too, but then it becomes inconsistent with the other terms. Or we could use "connected" rather than "shadow-including" as our word throughout? Hmm... |
|
I do not have a strong preference. I guess "shadow-including" is too long and scary? Thus, we will use "a connected descendant" / "a connected inclusive ancestor" and so on? I am fine with it. "Node.isConnected" would be just the short name for "Node.isConnected(ToDocument)". That's fine too. I am afraid that someone does not like it because "connected" is a general term. However, we have already decided to use "isConnected" to specify the meaning of "shadow-including". Thus, this change would not make things worse, I guess. |
|
Yeah, I'll leave shadow-including in for now, but I think such a change would make sense in the future and not be super disruptive. |
Title: [Shadow]: Need mechanism to tell if an element in a ShadowRoot has been inserted into the Document (bugzilla: 22141)
Migrated from: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22141
comment: 0
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22141#c0
Daniel Freedman wrote on 2013-05-22 18:23:02 +0000.
Because of necessity of encapsulation, it is very hard to tell if an arbitrary element has been inserted into the document.
Blink current hides the insertion state from compareDocumentPosition,but document.contains will return the actual insertion state.
This seems fine to me, because contains would not leak the state of the surrounding elements like compareDocumentPosition would.
I can also imagine cases where user code may want to gate on the insertion state, such as updating a canvas.
comment: 1
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22141#c1
Dominic Cooney wrote on 2013-06-21 04:27:34 +0000.
Because the author has to have the node in question in hand, it seems like document.contains returning an accurate value for elements in Shadow DOM violates the letter of encapsulation but not the spirit.
I think document.contains should return a value reflecting whether the argument is in the document (including shadow trees.)
comment: 2
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22141#c2
Dimitri Glazkov wrote on 2013-06-21 15:00:58 +0000.
(In reply to comment #1)
That's a neat way to look at the problem. I am convinced.
comment: 3
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22141#c3
Erik Arvidsson wrote on 2013-08-01 20:05:42 +0000.
I think this is the wrong solution.
containshas always meant that I can walk from the node's parentNode to the ancestor. This changes the semantics of contains.What is the scenario we are trying to solve? Figuring that out, we can then come up with a reasonable API without changing the semantics of existing methods.
Let me flip the coin a bit. I've been using contains as a signal that the node is not in the main document (in a shadow tree or a disconnected tree).
comment: 4
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22141#c4
Jan Miksovsky wrote on 2013-10-24 23:14:53 +0000.
I ran across this issue this week, and Daniel pointed me at this bug. See the thread at https://groups.google.com/forum/#!topic/polymer-dev/mGs_zkA_7-4.
Erik was asking for a scenario, and this thread presents one. An overlay (popup) element wants to track and/or absorb all clicks on the document. Clicks outside the overlay should dismiss the overlay, while clicks inside the element should have no effect. The overlay author wires up a click handler on the document, capturing events so that it gets first crack at them (and stop them before they might inadvertently triggering other handlers). The overlay's click handler looks at the event target to decide whether the target is in the overlay or not. For this purpose, the developer decides to use contains(). The dev tests the overlay's click tracking behavior with light DOM content inside the overlay, and all works well.
Later, someone else (me) tries to include the overlay element in the shadow of another element, and distribute content into the overlay. When the user looks at the overlay, they see content presented inside the overlay. From the user's point of view, the overlay contains the content. However, if the user clicks on the (light DOM) content, the overlay's contains() check fails, and the overlay fails to behave properly.
One could argue the overlay element should be written differently. Maybe it could track clicks outside using capturing and track clicks inside using bubbling, or find some other solution. But I think the dev made a reasonable call to use contains() for this purpose, and it seemed to work for any tests they cared to make. It was only when the overlay element was subsumed into the shadow of another element that things broke.
I'll hazard that similar issues are likely to come up wherever a dev tries to use contains() within a custom element. Subsuming that element into the shadow of another element will cause problems. I believe custom elements will have a general need to know whether they (visually) contain a given element, regardless of whether the element is in their shadow or distributed to them.
How this is done, whether through modifying contains() or something new, I'll leave to people wiser than me to figure out.
comment: 5
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22141#c5
Hayato Ito wrote on 2013-10-25 04:09:07 +0000.
I think we can simulate the proposed contains() in JavaScript by using InsertionPoint.getDistributedNodes() and Node.getDestinationInsertionPoints().
I am aware that this will be burden for users, but it could be possible though I've not tried.
I don't think it's good idea to change the semantics of existing contains(). It should consider only a node tree and should ignore the status of the composed tree.
I am not against adding something new in native Shadow DOM if we need it absolutely.
comment: 6
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22141#c6
Daniel Freedman wrote on 2014-03-11 18:30:42 +0000.
Unfortunately, I think this is a very bad user experience decision.
Yes, we would be breaking the written semantics of document.contains, but we would be keeping the developer expectation that document.contains(node) means "this node is in the document".
Otherwise we've broken the developer assumptions about this method, and introduced a whole new variety of bugs.
For example, the jQuery flot charting library draws a helpful chart hover target that follows the mouse. It uses document.contains to determine if it should add page scroll offsets to the mouse x/y position to draw the the target correctly.
In the Polymer ShadowDOM polyfill, we followed the spec, and it broke this library: Polymer/polymer#162.
Ember is another library that uses document.contains to determine if an element should instantiate its controller: https://github.com/Polymer/ShadowDOM/issues/365
Even if we made a new function that does what we want, like Node.prototype.isInDocumentForReals, we still have to knock on all the library authors doors to get them to use the new method, or they say "ShadowDOM broke my stuff, it sucks" and everybody loses.
Web developers need their tools to work, and making document.contains continue to follow developer expectations seems like winning all around.
WDYT?
comment: 7
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22141#c7
Hayato Ito wrote on 2014-03-12 04:55:06 +0000.
To interpret the current spec literally, I think:
I understand that this might cause a very bad user experience.
How do we proceed? Let me propose some ideas.
A) Change the semantics of node.contains so that it should consider the tree of trees.
That means we are extending ancestor/descendant relationships beyond one node tree. I am afraid that we have to do monkey-patch to
node.containsin the Shadow DOM spec.B) Leave node.contains as is. That means if the context object and argument node are in the different node trees, it always returns false.
As alternative, we should provide a new API, such as node.inComposedTree(). node.inComposedTree return true if and only if:
The node participates in the composed tree whose root node is document.
C) Better ideas are welcome.
comment: 8
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22141#c8
Hayato Ito wrote on 2014-03-12 06:02:35 +0000.
FYI.
In blink, looks like the semantics of
node.containschanged inhttps://chromiumcodereview.appspot.com/21123005
However, we don't have any spec for that. I'm not convinced with decision.
I prefer Idea B. WDYT?
comment: 9
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22141#c9
Hayato Ito wrote on 2014-03-13 05:07:45 +0000.
If we go for idea B, we need API to satisfy the original requirement.
Because node.inComposedTree() is just an tentative idea, I appreciate opinions for better API to satisfy the requirement.
Other ideas:
Any ideas are welcome!
comment: 10
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22141#c10
Elliott Sprehn wrote on 2014-03-15 02:12:08 +0000.
(In reply to Hayato Ito from comment #9)
I think there's two separate questions here, Document.contains and Node.contains seem like totally separate questions.
"Am I in this Document"
"Am I a child of this Node"
Now that we have ShadowRoot.host you can actually walk up to the root and get the answer you want though, so perhaps reverting this is okay. We should probably add an inDocument() method though.
comment: 11
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22141#c11
Hayato Ito wrote on 2014-03-17 04:44:16 +0000.
FYI,
In blink, I've reverted the change of "node.contains": https://codereview.chromium.org/197283016/
(In reply to Elliott Sprehn from comment #10)
Agreed. IMO, we should avoid such a double meaning.
We should have a more explicit API instead of re-using 'document.contains'.
Yeah, we should discuss further to pursuit a good API.
I'd like to hear opinions from developers to address their use cases.
comment: 12
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22141#c12
Hayato Ito wrote on 2014-08-06 08:23:58 +0000.
Because we already have /deep/ combinator, http://drafts.csswg.org/css-scoping/#selectordef-deep, I think
document.deepContains(node)might be a good name.WDYT? If it's okay, let me spec that.
comment: 13
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22141#c13
Hayato Ito wrote on 2014-08-06 08:37:35 +0000.
It seems that Document doesn't have
contains.http://dom.spec.whatwg.org/#interface-document
We should add
deepContainsto Node, instead of Document.http://dom.spec.whatwg.org/#dom-node-contains
comment: 14
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22141#c14
Hayato Ito wrote on 2014-08-07 05:37:19 +0000.
I've added 'Node.deepContains(Node? other)' in
14322e5
I need a feedback.
Could someone verify whether this definition satisfies our requirements or not?
comment: 15
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22141#c15
Olli Pettay wrote on 2014-08-07 09:56:50 +0000.
I don't really like deepContains (partly because I think CSS is wrong by exposing
shadow dom in that way, and also because 'deep' doesn't really say anything.)
Couldn't we change contains() to take second param, which could be
some dictionary.
bool contains(Node? other, optional ContainsFilter filter)
dictionary ContainsFilter
{
bool includeShadowDOM = false; // includeShadowDOM is a bit bad name.
}
comment: 16
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22141#c16
Hayato Ito wrote on 2014-08-11 03:08:26 +0000.
Thank you for the feedback!
(In reply to Olli Pettay from comment #15)
It looks better to me than deepContains. I like it.
If no one have better idea than this, I'l change the spec.
In this case, should I make a patch for DOM spec?
comment: 17
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22141#c17
Anne wrote on 2014-08-25 08:12:30 +0000.
Well, includeShadow or some such could work. Or includeShadowNodes. Let's not use the term "DOM" in any API. Ideally, the terms we use for shadow nodes are consistent in some way.
comment: 18
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22141#c18
Hayato Ito wrote on 2014-08-25 08:35:46 +0000.
Well, actually, the Shadow DOM spec doesn't define any technical definition of 'Shadow DOM'. That isn't well defined term. We should avoid using the term of 'Shadow DOM' in APIs.
includeNodesInShadowTrees might be the most unambiguous, however it looks too redundant.
My preferences are:
A. includeShadow
B. includeShadowTrees
C. includeNodesInShadowTrees
D. includeShadowNodes
I don't have a strong opinion either. I am aware this might be bike-shed discussion, but the naming of API is important.
comment: 19
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22141#c19
Anne wrote on 2014-08-25 08:40:45 +0000.
includeShadow seems fine. Shortest and to the point. So I guess we'll have shadow trees and the one
<textarea>and friends use are isolated shadow trees? (And won't be exposed here of course.)comment: 20
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22141#c20
Hayato Ito wrote on 2014-08-25 09:02:57 +0000.
Good question.
IMO, document.contains(nodeInShadowTreesHostedByTextAreaElementInDocument, { includeShadow: treu}) should return
true. However, I'm not confident.I am not sure whether there is a use case for that, assuming external developers can't get an access for such a node usually.
AFAIK, only use cases I've heard so far is for nodes which are in user-created shadow trees, rather than nodes in UA shadow trees for built-in elements.
If I misunderstand your question, please correct me.
comment: 21
comment_url: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22141#c21
Pedram Emrouznejad wrote on 2015-02-15 17:33:11 +0000.
For what's it worth, I managed to solve my need for a document.contains that cuts across shadow boundaries by doing:
element.matches(':host-context(body) *')
(essentially, I needed to know if a shadow-nested element was in the main document)
+1 to a more versatile document.contains though. I'm not sure what other possible options a dictionary would be good for, so I think just a simple true/false as the second parameter would work well. If there is need for further config in the future, then I guess you can still keep the true/false parameter and overload it with a dictionary.
The text was updated successfully, but these errors were encountered: