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

Replace attached/detached callbacks with insertedIntoDocument/removedFromDocument #362

Closed
rniwa opened this issue Jan 23, 2016 · 35 comments
Closed

Comments

@rniwa
Copy link
Collaborator

rniwa commented Jan 23, 2016

Elements that define things or get used by other elements should probably do their work when they’re inserted into a document. e.g. HTMLBaseElement needs to modify the base URL of a document when it gets inserted. To support this use case, we need callbacks when an element is inserted into a document/shadow-tree and removed from a document/shadow-tree.

Once we have added such callbacks, let us call them insertedIntoDocument and removedFromDocument callbacks, attached and detached callbacks seems rather arbitrary and unnecessary as the author can easily check the existence of the browsing context via document.defaultView.

@domenic
Copy link
Collaborator

domenic commented Jan 25, 2016

F2F agreement: let's do this. Delta from the current spec: attached -> insertedIntoDocument; detached -> removedFromDocument; remove the "and this document has a browsing context" clause.

Order of callbacks should be root of tree (document element) to leaves, to be consistent with other callbacks.

Shadow DOM does not change the answer. If you are inserted into a shadow tree whose shadow root is not in document, you do not get called. When the shadow root gets inserted, you are called (in order).

@domenic
Copy link
Collaborator

domenic commented Jan 25, 2016

F2F further discussion: nobody is as sure about the name anymore. If it's long it might need to be precise (insertedIntoDocumentDeeply or insertedIntoComposedDocument).

@smaug----
Copy link

I'd still prefer having callback for ancestor chain changes and not for document changes, but I guess I can live without that.

@annevk
Copy link
Collaborator

annevk commented Jan 27, 2016

During the meeting we claimed that "attach" and "detach" were unknowns, but note that we did introduce attachShadowRoot(). Of course, its semantics are quite a bit different so those might still be bad words to use here.

My preference here would be to use short names (e.g., inserted and removed), since if we suffix them with Callback they'll be long enough. Developers will find out soon enough when they fire or simply read it in the standard.

@hayatoito
Copy link
Contributor

Regarding with the naming,

We should definitely avoid using InsertedIntoDocument/removedFromDocument.
As we have decided not to extend the meaning of 'in a document' in #57, we should avoid re-using insertedIntoDocument in another meaning here.

It would be nice that the new name should be consistent with this issue #81, where Node.inDocumentDeeply is tentatively proposed. However, I do not like a term of 'in a document deeply' as a web-facing API name.

My naive idea: How about using a term of connected? A term of connected is a reserved word in Web Standard?

So my preference

  1. connectedIntoDocument/disconnectedFromDocument (If we prefer explicit long name)
  2. inserted/removed (If we prefer a short name, however, we still need a good name for node.isConnected #81)

@rniwa
Copy link
Collaborator Author

rniwa commented Jan 27, 2016

I like connectedToDocument / disconnectedFromDocument better than inserted / removed because it's more explicit especially if we wanted to add the latter two whenever any ancestor changes in the future.

@domenic
Copy link
Collaborator

domenic commented Feb 3, 2016

How do people feel about just connected / disconnected?

@treshugart
Copy link

Looking at the history, and recalling from memory, seems there have been several changes here:

  • insertedCallback / removedCallback
  • enteredViewCallback / leftViewCallback
  • enteredDocumentCallback / leftDocumentCallback
  • attachedCallback / detachedCallback
  • connected / disconnected?

I asked around at work and someone said:

imo, connected seems to mean that the 2 connected objects are more freely independent of each other. Whereas attached has a less flexible context.

like you'd say a leech was attached to your body, not connected to your body.

Others seemed to agree that attached and detached are the best fit.

In terms of consistency:

  • inserted -> Node.insertBefore()
  • attached -> HTMLElement.attachShadowRoot()
  • disconnected -> MutationObserver.disconnect()

The closest in terms of semantics seems to be inserted and Node.insertBefore(). IIRC insertedCallback and removedCallback were the original variations and were also used by Chrome's first implementation.

I think the most important thing here is that names are descriptive, consistent with each other and don't change in the future. I'd vote for attachedToDocument and detachedFromDocument which leaves you open to use attachedToNode and detachedFromNode if that behaviour is to be re-implemented in the future.

Whatever happened to using symbols?

@rniwa
Copy link
Collaborator Author

rniwa commented Feb 4, 2016

How do people feel about just connected / disconnected?

That doesn't tell us to what it's connected or from what it's disconnected.

@domenic
Copy link
Collaborator

domenic commented Feb 4, 2016

That doesn't tell us to what it's connected or from what it's disconnected.

The idea would be that we would define in the spec new terms "connected" and "disconnected" which inherently have to do with being in a document.

If you're saying it doesn't tell readers of the code that, I think that's kind of true, but that will be true of any short and easy to type name. With my developer hat on, I'd personally rather pay the cost once of looking up "connected" if I get confused, than the cost of typing "connectedIntoDocumentCallback" every time I do this.

@rniwa
Copy link
Collaborator Author

rniwa commented Feb 4, 2016

If you're saying it doesn't tell readers of the code that, I think that's kind of true, but that will be true of any short and easy to type name. With my developer hat on, I'd personally rather pay the cost once of looking up "connected" if I get confused, than the cost of typing "connectedIntoDocumentCallback" every time I do this.

I disagree. In practice, people would just copy & paste a template / name. I'd much rather have a specific name that communicates the meaning than an obnoxiously short name that doesn't tell us any semantics.

@JanMiksovsky
Copy link

@domenic's and @rniwa’s points suggest that a good criteria by which to evaluate proposed callback names/semantics would be to consider an actual pair of concise, complete, and natural definitions for when those callbacks happen.

Such definitions are the sort of thing that are typically waved off for someone at MDN, etc., to work out when they write the high-level documentation aimed at a typical web developer (instead of someone who reads specs). But in cases like this, I think it's likely that the documentation will struggle to explain the concept accurately, and in a way that actually uses the callback names naturally in the explanation. If we could agree on a plausible natural-language definitions up front — words we ourselves might use to describe these callbacks to a developer friend, or expect to see on MDN — perhaps the callback names would fall out more easily.

I'll admit that it's really, really hard for me to reconstruct the current thinking by reading #57 and put these callbacks in that context. But based on my understanding, I'll put on my documentation writer hat and take a shot. Here's an initial draft for the "connected" callback, using that term provisionally to see how it feels:

connected: This callback is invoked when the custom element becomes part of the composed document. An element is in the composed document if it's directly in the document's light DOM, or in the Shadow DOM of a host element which is in the composed document. Generally speaking, an element in the composed document could be rendered. (Although there are reasons it might not be — the element might be styled to be hidden, for example.)

Is this definition in the right ballpark? I didn't see the term "composed document" in the linked discussion, but was wondering whether that term could serve as a proxy for "in a composed tree whose root is a Document".

Playing with natural-language definitions might lead to more confidence in the terms we pick. If we liked the above definition, for example, we might note that the definition doesn't use the term "connected", suggesting perhaps that the term is weaker than we'd like. In contrast, the word "composed" is used a lot, suggesting that a callback term including that word might be an interesting direction.

@hayatoito
Copy link
Contributor

FYI, in #377, we are discussing how we should call the "in a document deeply". "in a composed document" is just proposed there.

We might want to have a consistent name with that if possible.

@annevk
Copy link
Collaborator

annevk commented Feb 4, 2016

@rniwa as a data point, whenever we pick a long name, developers complain. I have yet to see complaints about short names such as fetch. I very much agree with @domenic we should have simple names here. The semantics will be confusing no matter what if you're new to this.

@rniwa
Copy link
Collaborator Author

rniwa commented Mar 3, 2016

It looks like the winer here is either inserted / removed or connected / disconnected. Of the two options, I'd prefer connected and disconnected in the case we wanted to add new callback that gets called whenever ancestor chain changes in the future.

@annevk
Copy link
Collaborator

annevk commented Mar 3, 2016

So inserted/connected means inserted and your shadow-host-including inclusive ancestor is document?

@rniwa
Copy link
Collaborator Author

rniwa commented Mar 3, 2016

Right.

@annevk
Copy link
Collaborator

annevk commented Mar 3, 2016

I think "connected" makes the most sense then, since that's fairly specific. Do we also require that document to have a Window object or will this work regardless of that?

@rniwa
Copy link
Collaborator Author

rniwa commented Mar 3, 2016

Regardless (or at least that's what the consensus was at Jan F2F). I don't think the whole thing about window is useful given authors can just check !!this.ownerDocument.defaultView.

@annevk
Copy link
Collaborator

annevk commented Mar 3, 2016

ShadowRoot does not have defaultView, afaik.

@rniwa
Copy link
Collaborator Author

rniwa commented Mar 5, 2016

If we're disallowing custom elements inside a document without a browsing content in v1 in issue #369, this might not be relevant now. We can revisit that question when we tackle v2 API.

@smaug----
Copy link

Hmm, what if you move an existing custom element from a document which does have defaultView to a document which doesn't have a defaultView?
But anyhow, I don't think we should require document to have defaultView in this case.

@rniwa
Copy link
Collaborator Author

rniwa commented Mar 6, 2016

Well, in that case, the custom element will get upgraded (i.e. the constructor will be called) since the custom element in a document without a browsing context would not have been instantiated per issue #369 unless the author had manually instantiated a custom element and inserted into a document without a browsing context.

Having said that, we can have the document into which the custom element was inserted or from which it was removed as the first argument. That way, author can check document.defaultView on that argument very easily as follows:

class MyElement {
...
    connectedCallback(document) {
        if (document.defaultView)
            doSomething();
    }
...
}

Here's a more concrete proposal that summarizes the discussion thus far:

@annevk
Copy link
Collaborator

annevk commented Mar 7, 2016

@rniwa connectedCallback should also be invoked if custom element's parent got "connected", right? So I think we should enqueue for each inclusiveDescendant mentioned in step 6.2. And we should expand 6.2 to include hosted shadow trees, since it'll be callbacks all the way down. connectedCallback is basically the "insertion steps" callback, limited to "composed documents". (And I think we should therefore pass both document (indeed useful and relevant) and newAncestor.)

(Similar considerations for disconnectedCallback.)

@rniwa
Copy link
Collaborator Author

rniwa commented Mar 7, 2016

Oh oops, yes, we do need to recursively find all custom elements that got "connected".

@rniwa
Copy link
Collaborator Author

rniwa commented Mar 7, 2016

For newAncestor, are you suggesting that connectedCallback(document, newAncestor)? Or just for the purpose of passing arguments within the spec?

@annevk
Copy link
Collaborator

annevk commented Mar 7, 2016

That is what I'm suggesting, but maybe it is not necessary for a custom element to get that much context? I believe that is what browsers have internally, but I don't know offhand the nodes that require that information. There's a potential problem with that too, e.g., if newAncestor is inside a closed shadow tree we should probably not pass it…

@annevk
Copy link
Collaborator

annevk commented Mar 7, 2016

@smaug---- pointed out document does not need to be passed, you can simply use this.ownerDocument.defaultView. He also pointed out that in Gecko newAncestor/oldAncestor are only passed for the root (where they'd mean the same as newParent/oldParent) and not the descendants.

domenic added a commit that referenced this issue Mar 7, 2016
This redefines the custom element callback queue to be a custom element
action queue, to allow upgrades (which are no longer callback-based) to
take part. It also replaces attachedCallback/detachedCallback with
connectedCallback/disconnectedCallback, fixing #362. And it updates all
the places that enqueue these callbacks to be phrased in terms of
precise patches to DOM, except for upgrades.
@domenic
Copy link
Collaborator

domenic commented Mar 7, 2016

I think I did this in 2d07c73. I ended up passing the document since to me it doesn't seem at all clear that the element's node document is equal to the parent node's containing document. Happy to fix if wrong.

See https://w3c.github.io/webcomponents/spec/custom/#custom-element-lifecycle for rendered output

@domenic domenic closed this as completed Mar 7, 2016
@annevk
Copy link
Collaborator

annevk commented Mar 8, 2016

You should not pass document. When a node is in a document or one of that document's hosted shadow trees, it's node document is that document. This will also be true for a node that was just removed.

Although I wonder how the timing works out if you appendChild a node from a different document. Did you work that through?

@annevk annevk reopened this Mar 8, 2016
@domenic
Copy link
Collaborator

domenic commented Mar 8, 2016

@annevk I don't understand. The document seems 100% necessary. Consider this case:

const p = document1.createElement("p");
const c = document1.createElement("custom-foo");
p.appendChild(c);

document2.body.appendChild(p);

In this case p is being inserted into document2.body. inclusiveDescendant is c. And c's node document (i.e. the value of this.ownerDocument inside connectedCallback) is document1. But we are connecting it to document2. So we should be passing document2.

@annevk
Copy link
Collaborator

annevk commented Mar 8, 2016

There's no callback for the first appendChild(), due to lack of a shadow-host-including root that is a document.

For the second call to appendChild(), by the time the callbacks are called, this.ownerDocument will be document2. This follows from the operations appendChild() does.

@domenic
Copy link
Collaborator

domenic commented Mar 8, 2016

I see, I missed the pre-insertion steps. OK, we can remove the argument.

@trusktr
Copy link
Contributor

trusktr commented Aug 21, 2016

A little late, but why is Callback needed in the name?

The term "callback" is understood to be a function passed into another function, to be called in the future by some other code. In the years I've been using JavaScript, I never heard of class methods being called "callbacks" until I started using Custom Elements within the past year, and as user of the API I am never passing these methods anywhere, so they don't seem like "callbacks".

Maybe names like "whenConnected" and "whenDisconnected" would read better, and there's no misnomer there.

@WebReflection
Copy link

WebReflection commented Aug 22, 2016

the whenWhatever would be misleading since there's already a whenDefined which returns a Promise like object which is incompatible with the semantics of these methods ( connectedCallback / disconnectedCallback ).

I think these methods are just fine, and give room to libraries authors for different, enhanced connect, disconnect or onConnect/Disconnect methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants