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

[css-pseudo-4] Identity of Element.pseudo() return value #3607

Closed
fantasai opened this issue Feb 2, 2019 · 9 comments
Closed

[css-pseudo-4] Identity of Element.pseudo() return value #3607

fantasai opened this issue Feb 2, 2019 · 9 comments

Comments

@fantasai
Copy link
Collaborator

fantasai commented Feb 2, 2019

@heycam wrote in #3541 (comment):


We need some wording about the identity of the object returned by pseudo(), for example whether it's required to be the same object each time you call it on an element with a given type.

Now that I think about it, if we do require that the same object is returned, what does it mean for the CSSPseudoElement object if we restyle the element and the pseudo goes away (and so null would be returned from another call to pseudo())? Is the CSSPseudoElement detached in some way? If we restyle again and the pseudo-element is recreated, does the old CSSPseudoElement remain functional? Might it be simpler to always return an object for a supported pseudo type, regardless of whether the pseudo currently exists?

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed Identity of Element.pseudo() return value.

The full IRC log of that discussion <heycam> Topic: Identity of Element.pseudo() return value
<heycam> github: https://github.com//issues/3607
<fantasai> ScribeNicK: fantasai
<fantasai> heycam: The issue here is that the spec doesn't say anything currently about when we need to return the same object that we used before on this function
<fantasai> heycam: The other confusing par tis, the function is defined ot return null if the pseudo doesn't currently exisst
<fantasai> heycam: Already we have a case where sometimes it'll return different values
<fantasai> heycam: But what is meant to happen if e.g. you restyle the element and its ::before content changes to something different
<fantasai> heycam: Do we keep track of whether we went ot null in the interim?
<fantasai> heycam: what is meant to happen?
<fantasai> florian: Would you prefer to return the same object in general to avoid garbage?
<fantasai> florian: or return different objects
<fantasai> heycam: given you want to use these as event targets, I thin it would be best to return the same object
<fantasai> heycam: Otherwise you grabe an object, attach an element, then pull it again to remove your event handler, and it's not there so you can't remove it
<fantasai> florian: You could maybe have an object that is a shallow copy?
<fantasai> florian: Then you'd have the same items on it
<fantasai> florian: that you could reach, although the object identity would be different
<fantasai> heycam: I think that'd be weird
<fantasai> Rossen: How is that different from having a reference to an element
<fantasai> Rossen: and referencing... asynchronously
<fantasai> Rossen: In terms of lifetime and identity, original reference will be held
<fantasai> Rossen: You'll have an element disconnected from the tree
<fantasai> Rossen: You will have some sort of state of whatever it is, and you may use it if it's useful to you
<fantasai> Rossen: but you have understanding that this element is no longer part of DOM
<fantasai> heycam: One difference is that it's a lot less obvious when the underlying thing goes away
<fantasai> heycam: It's determined just by the style values of the element, e.g. what 'content' property value is
<fantasai> heycam: whereas normal elements, it's more obvious when the thing has gone away
<fantasai> Rossen: To me what you're describing is the same as saying, if I have the pseudo element and I ask the containing element
<fantasai> Rossen: If I change the pseudo element to the extent that I need to generate a new pseudo elemen
<fantasai> Rossen: then I expect that everything gets disconnected
<fantasai> Rossen: This would be the same pattern as disconnecting a node from the DOM and still having a reference to it
<fantasai> heycam: I didn't considere that .parent would become null in that sense
<fantasai> heycam: could change the spec
<fantasai> Rossen: It's one thing to change state
<fantasai> Rossen: Different thing to completely change the element
<fantasai> Rossen: which in DOM case, this is removing the element and creating a new one
<fantasai> Rossen: for example, changing a div with a text node
<fantasai> Rossen: That would be equivalent transformation
<fantasai> Rossen: in either case if you get disconnected you still have a reference to the original source element
<fantasai> Rossen: or in this case pseudoelement
<fantasai> Rossen: But it's no longer useful to you, and is discoverable that it's no longer useful to you
<fantasai> florian: To concerns
<fantasai> florian: One, elements that are disconnected from the tree is a thing that exists already
<fantasai> florian: Pseudo-elements isn't
<fantasai> florian: Wondering how many odd entities or situations we'll create by making that exist
<fantasai> florian: Also, for ::before / ::after, that sort-of makes sense
<fantasai> florian: But with e.g. ::selection, the lifecycle is less clear
<fantasai> florian: If you select different things, have you collapsed the range or ... does it change when you change your selection???
<fantasai> florian: having an object that you always return regardless of styling
<fantasai> florian: is not so consistent with the element view of the world you were talking about
<fantasai> florian: But exposes a lot less details that are hard to understand for the author
<fantasai> Rossen: I don't have a ready answer, but I still think that mixing identies and lifetimes is going to be ..
<fantasai> Rossen: Keeping identity but changing type would be horrible
<fantasai> Rossen: Also, we're over time.
<fantasai> Rossen: I don't think we can resolve on this atm
<fantasai> heycam: I think it'd be good for more people to put opinions in the issues
<fantasai> heycam: I think in your model, we'd need to know what causes the identity of the object to chnage.
<fantasai> Rossen: Ok, let's end the call here. Thanks for calling in. We'll chat again next week
<fantasai> Meeting closed.
<heycam> github: end topic
<Rossen> trackbot, end meeting

@emilio
Copy link
Collaborator

emilio commented Feb 13, 2019

We discussed a bit about it in #3603. I think I'd prefer for the return value to not guarantee identity. I can imagine people poking at a bunch of pseudos on all elements on a webpage, and having to keep storage for them on the DOM nodes to guarantee identity sounds like a bit of a pain. But I can poke to some of the DOM / HTML folks for feedback on that, maybe I'm being a bit over-paranoid.

@tabatkins
Copy link
Member

My main argument for keeping identity stable (and thus caching objects upon first access) is that they're effectively DOM nodes (no children or attributes, but they have style, at least), and DOM nodes can be compared by object identity. I suspect it will be useful to be able to ask "is the pseudo in this variable also present in this array?", etc. Without stable object identity, this will be a much more annoying question to answer.

Having a stable identity doesn't prevent them from being collected; the DOM node should only have a weak reference to its pseudos. So if someone iterates the tree and pings every pseudo, it'll result in an explosion of pseudo nodes, but if they're not holding onto them they'll all die in the next collection.

This means they're observably not the same object in the presence of expandos, but I'm okay with that. I just don't want people to be able to ask for a pseudo, store it, then ask again and get a different object that's not === to the first.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed CSS Pseudo Elements.

The full IRC log of that discussion <fantasai> Topic: CSS Pseudo Elements
<fantasai> github: https://github.com//issues/3607
<fantasai> heycam: Last time we discussed this on a call, I was suggesting that the pseudo() function which returns a CSSPseudoElement object
<fantasai> heycam: should always return the same object regardless of what values content has
<fantasai> heycam: just so that we don't have to rely on what the current style state is to know when these objects should be dropped and recreated
<fantasai> heycam: It felt a little strange at the time
<fantasai> heycam: one thing that might argue against returning same object is if we have an API in the future that can create from script one of htese objects and insert it into the tree
<fantasai> heycam: Might be problematic to have stable objet identiy created for style, vs created from script
<fantasai> heycam: But that's similar to what we do for @font-face
<fantasai> TabAtkins: If you reparse the style sheet, we'll create new objects
<fantasai> myles: Actually, I spent a week of my time making that not true.
<fantasai> TabAtkins: Can you open an issue on not doing that?
<fantasai> myles: I originally wanted to do the thing you said
<fantasai> myles: was brought to my attention that it was very common in JS that authors would tack on properties to random objects
<fantasai> myles: so you can't just delete and recreate them
<fantasai> mstensho: For Blink implementation, we used to recreate the speudo-element internally when content changed, but we don't do that anymore
<fantasai> mstensho: when content property changes, we regenerate ... but the object stays the same
<mstensho> That's futhark speaking, not me
<fantasai> emilio: But you still regenerate when you switch content property to none and back again, right?
<fantasai> TabAtkins: If you turn content to none and then ask for the pseudo, you do return an object that you can return style on right?
<astearns> s/mstensho/futhark/
<fantasai> mstensho: Are you talking about the pseudo() method or gCS()
<fantasai> TabAtkins: pseudo
<fantasai> futhark: We don't implement it
<fantasai> emilio: Do we want to keep them lightweight objects or do we add .style?
<fantasai> emilio: If we add .style we need to keep the ? around anyway
<gregwhitworth> fantasai: currently we've dropped .style from the spec
<gregwhitworth> TabAtkins: wait, we dropped .style?
<fantasai> heycam: we kept .type and .element and it's an event target
<fantasai> TabAtkins: OK
<fantasai> emilio: If you add some stateful thing that we don't want to disappear randomly, then keeping object itself around is not a huge deal because you need to keep that info around
<fantasai> emilio: but if not, then recreating it would be better
<fantasai> TabAtkins: I think it should have .style
<fantasai> TabAtkins: later
<fantasai> TabAtkins: Because pseudos act like DOM elements, they fill a similar purpose
<fantasai> TabAtkins: Having object identity work it's nice
<fantasai> TabAtkins: Without that it's more annoying
<fantasai> TabAtkins: ...
<fantasai> TabAtkins: Myles's argument about FontFace objects suggests that they should stay around on these objects, too.
<fantasai> myles: I don't know how applicable that argument is to pseudos
<florian> q+
<fantasai> dbaron: I think one other comment about expandos is that CSSOM objects are historically one of the objects where expandos don't stay around
<fantasai> dbaron: They stay around in lots of places, but not in CSSOM objects
<fantasai> TabAtkins: Is it just font face objects that are conneted, or ??
<fantasai> q+
<fantasai> myles: Font face rules will change... I don't remember.
<florian> q-
<fantasai> florian: Another argument in favor of long-lived is if they're not, we need to specify in detail what their lifecycles are
<fantasai> florian: And if we don't we'll expose a lot of implementation details
<AmeliaBR> https://developer.mozilla.org/en-US/docs/Glossary/Expando "Expando properties are properties added to DOM nodes with JavaScript, where those properties are not part of the object's DOM specification"
<fantasai> florian: Do you regenerate on content property changing form on estring to another? flipt o none and back? n a display : none subtree?
<fantasai> florian: etc.
<fantasai> TabAtkins: [soething about garbage collector]
<fantasai> TabAtkins: Need to be either long-lived or regenerated on every call
<fantasai> myles: Most important part is that right now it's undefiend when the CSS engine decides to reparse rules or recreate derived objects
<fantasai> myles: That's pretty important fo roptimization
<fantasai> myles: so tying JS-visible semantics to that schedule would be scary
<fantasai> myles: Instead we should define something more rigorous
<fantasai> dbaron: Why is your GC idea not viable?
<fantasai> TabAtkins: It allows you to detect when GC happens by seeing how long the expando survives
<fantasai> dbaron: traditional way around that is that the expando makes it not collectible
<fantasai> TabAtkins: OK. My idea was to just hold it as a weak ref
<fantasai> dbaron: I think your weak ref idea is workable if we make expandos make it not collectible.
<fantasai> myles: It's true in our engine also
<fantasai> TabAtkins: If that's the case, then all the exposed possibibitlities for GC are handled if expando force a strong reference
<florian> ScribeScribe: Florian
<xfq> ack fantasai
<florian> fantasai: one concern was about keeping around a lot of memory if you iterate the entire tree
<florian> fantasai: if the pseudo doesn't generate a box, you return null, but as soon as you generate a box you generate the object and keep it around
<fantasai> emilio: that's also incompatible with making that API work with stuff like selection
<fantasai> TabAtkins^: incompatible with .style
<fantasai> emilio: For ::first-line ::first-letter, fine, but won't work for some other stuff
<fantasai> TabAtkins: I think returning null is OK only for things that don't exist, like the name is invalid
<TabAtkins> s/[soething about garbage collector]/If we just make the element have a weak ref to its pseudo, then expandos let you observe GC.
<fantasai> fantasai: Shouldn't that throw an error?
<fantasai> TabAtkins: No
<fantasai> astearns: We already talked about that
<fantasai> florian: How long-lived is it?
<fantasai> heycam: So we always return the same object, and it's kinda like it's connected to the element you call it on
<fantasai> TabAtkins: You're allowed to drop the object if doing so is unobservable
<fantasai> TabAtkins: e.e.g if no expando, no references, can drop it and recreate it
<fantasai> TabAtkins: I guess you can't observe object identity without a reference
<fantasai> smfr: Same object in IDL?
<fantasai> TabAtkins: That's advisory in the IDL. Have to say in algorithm what happens
<fantasai> myles: Can we make another IDL attribute that means "for real"? :)
<smfr> [SameObject]
<fantasai> heycam: What it means for a particular API is not particularly clear, so need to sayin the prose what type of object and stuff.
<fantasai> heycam: It's a hint to the JS engine, that's all.
<fantasai> myles^: so it means nothing
<fantasai> florian: There's a hint to the spec reader that there's some prose somewhere that matters?
<fantasai> TabAtkins: yes
<fantasai> astearns: So afaict, the proposed resolution is that pseudo() will return the same object always
<TabAtkins> Proposed Resolution: pseudo() must always return the same object (up to observability)
<fantasai> astearns: and we will have text suggesting that htis can be GC if that is unobservable
<fantasai> astearns: Objections/concerns?
<TabAtkins> "same object" for a given element/pseudo pair
<fantasai> RESOLVE: pseudo() must always return the same object. Note that object can be GC'd if this is unobservable.

@annevk
Copy link
Member

annevk commented Feb 26, 2019

I think you either need to preserve a 1:1 relationship, always return a new object, or perhaps return a new object per task on the event loop (though this gets complicated). Some kind of weak references isn't the way to go. Note that with many objects their associated style objects would also be many (that relationship has to be 1:1 presumably) and they'd all end up being proxies for the actual style or some such. That seems somewhat complicated to define. Unless you invalidate the objects per task or some such.

@tabatkins
Copy link
Member

It's "always the same object, up to observability constraints". So if you don't hold a reference and don't set any expandos, the object can be freed and a fresh one given later, but otherwise it must be cached.

@annevk
Copy link
Member

annevk commented Feb 26, 2019

I missed you had already resolved this to a satisfactory outcome. I mainly meant to outline constraints on the solution space. Making [SameObject] useful is tracked by whatwg/webidl#212 by the way.

@Loirooriol
Copy link
Contributor

if you don't hold a reference and don't set any expandos, the object can be freed

This includes weak references, right?

var s = new WeakSet();
s.add(element.pseudo(type)); // Store weak reference
// later
s.has(element.pseudo(type)); // Should be true because GC shouldn't be observable

fantasai added a commit that referenced this issue Mar 26, 2019
… stable per element/type pair, returns null for unknown types. #3607 #3603
@fantasai
Copy link
Collaborator Author

Edited. @tabatkins mind reviewing?

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

8 participants