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] Should Element.pseudo("unknown") be an error or return null? #3603

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

Comments

Projects
None yet
7 participants
@fantasai
Copy link
Collaborator

commented Feb 2, 2019

We resolved to have an Element.pseudo(type) interface that returns a CSSPseudoElement. What happens if someone passes a type that isn't known to the implementation? Do we treat it as non-existent or do we throw an error?

@heycam

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2019

Treating it as non-existent sounds good. So that would mean returning null per the current spec. But if my second paragraph last sentence question in #3541 (comment) is reasonable, we would could return a (mostly useless) CSSPseudoElement object here too.

@FremyCompany

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

+1 null (because qSA throwing errors is very very very annoying)


Compare:

if(document.body.pseudo("backdrop")) {
    // ... 
}

vs

var isBackdropSupported = true; try { document.body.pseudo("backdrop") } catch (ex) { isBackdropSupported = false; }
if(isBackdropSupported) {
    // ...
}
@css-meeting-bot

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

The CSS Working Group just discussed Should Element.pseudo("unknown") be an error or return null?.

The full IRC log of that discussion <dael> Topic: Should Element.pseudo("unknown") be an error or return null?
<dael> github: https://github.com//issues/3603
<dael> fantasai: Somewhat related to #3607about identity of the pseudo
<dael> leaverou: Does it present null in any other case? If it's not defined at all does it return null?
<dael> fantasai: That's an open question
<dael> leaverou: In general errors meandev have to handle. But we needto be able to distinguish doesn't exist and not defined. Feature detection needs to be possible
<fantasai> Questions:
<dael> fantasai: Open questions are should element.pseudo always return same object even if you removet he style that generated it? Other is if it doesn't exist in box tree do we get an object back? When we request something that doesn't exist b/c not supported what do we return?
<dael> TabAtkins: Addressed first question last week. Keeping objected identity stable is useful. Question of what to we return when before doesn't have a property and can you fiddle with an object. Second question, we do need to distinguish between a pseudo that doesn't exist and one that isn't valid. I don't think it's useful to return null if a pseduo element doesn't exist on an element. Perhaps a bool to say if it exists or not
<dael> TabAtkins: The for the unknown thing where you put ::foo it should throw an error. Even thought CSS style sheets can be forgiving, JS APIs shoudl throw in clear error cases.
<florian> q+
<dael> fremy: I think it makes sense to return an object all the time. On the error part I'm less sure b/c we'll have compat issues and errors can cause entire thing not to work. Less sure but understand arguement
<dael> leaverou: It's impossible in JS to tell if it's that the element doesn't exist or something else went wrong. You can sort of guess but not be sure
<dael> TabAtkins: We can return different types of errors. We don't throw that many errors and you can tell from type. Message will usually let you know what's going on
<dael> leaverou: From console, but can't programatically detect
<dael> TabAtkins: But there'sone error it can cause
<dael> leaverou: What about the future
<dael> florian: With this if you start nesting and I don't know if that's same error as asking for a pseudo that doesn't exist
<florian> q-
<dael> TabAtkins: Can't ask for a pseudo on a pseudo
<dael> fantasai: I think if we're deciding element returns null when it doesn't exist it makes sense, but if we're not we should return an error
<dael> astearns: Sounded like 3 parts to TabAtkins summary. 1) always return
<dael> TabAtkins: the same object for a give element/pseudo element pair
<TabAtkins> 1. Always return the same object for a given (element, pseudo-element) pair.
<dael> astearns: Always return same object. Return an object for when a element exists
<leaverou> Btw a historical case that may help: In old IE, element.style[foo] would error if the value was invalid. This was very widely considered as annoying by developers, until eventually IE changed and stopped throwing.
<dael> fremy: That means you need ot keep object for lifetime of element. You could have to store a gazillion objects which you don't need. I would have to check
<dael> florian: Garbage detected?
<dbaron> s/detected/collected/
<dael> s/fremy/emilio
<leaverou> s/element.style[foo]/setting element.style.foo/
<dael> emilio: I guess it's not observable. Any other that does the same?
<dbaron> (discussion about the element keeping a weak reference)
<dael> fremy: [missed] if you drop the reference it's garbage collected and you get the new one
<dael> fremy: Not possible to notice because you don't have anywhere to join
<fremy> s/join/compare to/
<dael> TabAtkins: If you ask for a ready promise those are cached and you're not calling because ready state has not changed. That sort of retention of objects is not uncommon
<dael> emilio: font face it's one object and this could be many objects. If it's a problem we can face it
<dael> TabAtkins: If you're iterating the entire tree, that's weird
<dael> emilio: I've seen people do it
<dael> florian: Pseudo with a certain style, you look at all
<dael> fantasai: One thing we could do is return null if doesn't exist on element. If at any point it does exist browser has to maintain the reference. Function might return null or that object, but never another. SO if pseudo element at some point exists you keep that reference.
<dael> TabAtkins: psueod element does exist- if there isn't css setting the before would we return null when asking for the before pseudo
<dael> emilio: Also what happens when display on sub tree? I don't know
<fremy> +1 to what TabAtkins just said
<dael> TabAtkins: I'm unhappy with it because it means you can't use this API to toggle a pseudo element on. You have to go through normal css which is a more complicated redirection. Sounds good but messes up too much
<dael> plinss[m]: Isn't that a feature? SHould we be able to create pseudo not backed by css?
<dael> fantasai: Currently has a .style that allows you to set and have it exist.
<Rossen> q+
<dael> fantasai: Interesting thing is to plinss[m] point you can't serialize that back out. WE have style that will serialize out for .style, but not for a pseudo element.
<dael> TabAtkins: If we accept nesting proposal style auto upgrades to be able to support that. Have to define, but you can embed a nested style in the style attribute. There's a route to make it serializable
<astearns> ack Rossen
<emilio> Rossen++
<dbaron> Another maybe-silly option is a null/undefined distinction, if you want to think of .pseudo() as sort of like a shorthand for a long list of DOM properties (where undefined means "not implemented" or "the browser doesn't know about it" and null means "known but not present")
<dael> Rossen: Question- Is anyone working with any DOM or HTML folks on this? Curious to their PoV. Sounds like a pretty overarching API that we should be working with at least DOM folks. I'd hate to see something like this worked on for so long and then go back to square one.
<dael> Rossen: Perhaps with an envoy it would be good to get their PoV
<dael> emilio: We can ask for feedback
<TabAtkins> dbaron, I don't see a good reason to return undefined vs throwing, tho.
<dael> fantasai: Would like to try and resolve and we can reopen if they give feedback and get a publication out to request a review
<AmeliaBR> If we accept Tab's "route to serialization" by allowing pseudo-element styles in the inline style of the main element, doesn't that also open up a "route to dynamically generating a pseudo-element" by declaring a `style="::before{content:"text"}` on the parent element's style object?
<dael> astearns: Would be nice, but not sure I'm hearing consensus
<dael> astearns: I agree figuring this stuff out does make this issue about a non-existent pseudo...how we answer all these questions does make a difference on how we address this particular issue
<fantasai> AmeliaBR, only if you escape those quotes properly :)
<TabAtkins> `style="&::before{content:'text'}"`, but yes
<dael> Rossen: I'm hearing a lot more questions then suggested answers. Doesn't suggest you're ready to resolve. If we're looking to push an updated version of spec I don't thinkw e need to rush a decision
<dael> astearns: Issue on the agenda is just unknown pseudo elements. Are there other issues for psuedos that can hop on and off of existance?
<fantasai> https://github.com//issues/3607
<dael> fantasai: #3607
<dael> astearns: Gotcha
<fantasai> That transcript misses some of Tab's follow-up comments
<dael> astearns: Not happy to not resolve, but I don't think we have a plan. What about we come up with a proposal for both issues, discuss at F2F, come to a decision there. We use time up to F2F to reach out to DOM people and anyone else that would have real input on what to decide
@FremyCompany

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

(In issue #3540 we marked support for non-before/after pseudos at-risk of being removed from the spec; if we do so, this function won't be useful as a detection mechanism anyway, and we need to decide what to do for first-line etc; should they really throw?)

@astearns astearns added Agenda+ F2F and removed Agenda+ labels Feb 13, 2019

@tabatkins

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

I'm cool with returning null here. As François says, there's value in doing simple tests to see if a pseudo is supported at all. If you actually try to use it, null will quickly throw an error anyway. (null.style throws immediately)

@css-meeting-bot

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

The CSS Working Group just discussed unknown pseudo() arg, and agreed to the following:

  • RESOLVED: Return null provisionally and open a TAG review issue.
The full IRC log of that discussion <fantasai> Topic: unknown pseudo() arg
<fantasai> github: https://github.com//issues/3603
<fantasai> TabAtkins: We can either go with JS likes to signal errors for error conditions
<fantasai> TabAtkins: or DOM likes to return nulls
<fantasai> TabAtkins: I believe we should return a null for this object.
<fantasai> TabAtkins: fremy made a good use case for being able to easily test whether a pseudo is supported in an element
<fantasai> TabAtkins: throwing is more verbose in JS
<fantasai> astearns: There's also question of does this element support this pseudo
<fantasai> TabAtkins: https://github.com//issues/3603#issuecomment-463287002
<fantasai> TabAtkins: I think this is fine.
<fantasai> TabAtkins: null will immediately throw an error if you try to access properties on it, so you'll gt an error if you don't check anyway
<fantasai> TabAtkins: Gives you same behavior in practice, but null gives you a more useful boolean result in practice
<fantasai> myles_: ...
<fantasai> TabAtkins: querySelector throws and it's really annoying
<fantasai> myles_: Just pointing out the fact that it throws
<fantasai> fantasai: If you and fremy agree, seems OK to me
<fantasai> fantasai: We can also ask the TAG if they have an opinion on this
<fantasai> astearns: Not sure it's that important
<bkardell_> +1
<fantasai> fantasai: It's about consistency across the platform. TAG is good for that.
<fantasai> RESOLVED: Return null provisionally and open a TAG review issue.
@annevk

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

I'd expect an exception if it doesn't meet the syntactic requirements for a pseudo-element. However, I guess that only really works if pseudo-elements won't gain arguments. Because at that point it's ambiguous whether before(...) meets the constraints or not.

It's somewhat confusing though that this issue uses "unknown" and the TAG review issue uses ":unknown". Does "before" work? Does ":before"? Does "::before"? Is there a formal definition of this method somewhere?

@astearns astearns removed the Agenda+ F2F label Mar 1, 2019

fantasai added a commit that referenced this issue Mar 26, 2019

[css-pseudo-4] Clarify return value of element.pseudo() - identity is…
… stable per element/type pair, returns null for unknown types. #3607 #3603

@fantasai fantasai added the Agenda+ label Mar 26, 2019

@fantasai

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 26, 2019

@annevk I think the simplest thing would be if the value isn't a string in the valid types list, it's an error. (Atm the spec just returns null for all type errors.) Agenda+ in case anyone thinks we should be doing something else.

@annevk

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

@fantasai so :before and ::before are errors? And a pseudo-element that takes an argument would also be an error if you pass in pseudo-with-argument(some-valid-argument)?

The latter seems problematic for the ::part pseudo-element, where you probably want to return different objects depending on the argument passed to the pseudo-element.

@fantasai

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 27, 2019

@annevk ::before would be valid. :before would be invalid. If we later allow ::part() then we'll have to define what gets returned for the various arguments to it; currently it's invalid because not supported.

@annevk

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

So if you use pseudo-element syntax and not pseudo-element identifiers, would ::\42 efore work too? It seems a lot simpler to only accept identifiers, though with pseudo-elements accepting arguments it might have to get more involved somehow (likely the method would have to take multiple arguments too).

@tabatkins

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

Accepting "identifiers" would mean that \42 efore would work too. ^_^ Presume you mean literal matches against a defined list of strings, tho.

Having to later map pseudo-element arguments into JS arguments really makes it feel like this is trying to draw a distinction that's not worth it? Like, I'm mostly for not invoking the CSS parser when not necessary, but it seems like it'll actually get some use here.

@annevk

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

Right, sorry. I see some kind of rough analogy to Houdini. That is, do you want to program with CSS/Selectors-like strings, or do you want to program against the underlying data model? It seems to me that per https://extensiblewebmanifesto.org/ but also some other principles (separation of concerns comes to mind) you want to provide an API for the underlying model.

@tabatkins

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

Sure, but don't apply that too naively. We're not exposing a reified "pseudo-element query" object that, when resolved against an Element, returns a PseudoElement. We def don't intend to require you to construct CSSKeywordValue/etc objects to pass over to it. It's not a fully TypedOM-ified API, and isn't intended to be, as that would just be busywork for no benefit. (The object forms exist to make it easier to read/alter things from JS; they're harder to write from JS than strings are. This is a write-only API.)

Turning .pseudo("::foo(bar, baz)") into .pseudo("foo", "bar", "baz") is similarly just busywork making it harder to read/write and connect to the stylesheet syntax

@annevk

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

Well, at some point someone will have to make that translation and define what it means. (E.g., whether argument-accepting pseudo-elements can return different PSeudoElement objects or whether that depends.)

@tabatkins

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

They'll return different objects if the arguments cause the calls to refer to different pseudo-elements, sure. If they resolve to the same pseudo they'll return the same object. "Same" here means whether styles applied with the two selectors cascade together or not.

(Moot right now due to the limited list, of course.)

Not sure how that eventual clarification is relevant to the syntax argument, tho. That question needs answering regardless of how you write the JS method.

@tabatkins

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

So, to answer the overall question:

We should do a [=CSS/parse=] on the string, matching it against the grammars of the allowed pseudo-elements.

@heycam

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

Although that makes me wonder about the pseudo argument of getComputedStyle, and whether it should change to do a [=CSS/parse=]...

@css-meeting-bot

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

The CSS Working Group just discussed Should Element.pseudo("unknown") be an error or return null?, and agreed to the following:

  • RESOLVED: Do a css parse on parameter for element.pseudo()
The full IRC log of that discussion <dael> Topic: Should Element.pseudo("unknown") be an error or return null?
<dael> github: https://github.com//issues/3603#issuecomment-467363388
<dael> astearns: There was additional conversation after we resolved. Is fantasai or TabAtkins on?
<dael> TabAtkins: Yes
<dael> TabAtkins: Question came up from Anna. Import is exactly how argument should be parsed. Direct string match or full css parse so you get escapes and that business?
<dael> TabAtkins: I usually prefer direct string but I believe this function will be extended to handle other pseudo elements with arguments. AS soon as you get past a single name string compare falls down. When we have css parsing there we shouldn't do custom
<dael> TabAtkins: I propose we do a css parse on the string and match against grammar of supported elements
<dael> astearns: Any discussion? Objections?
<dael> RESOLVED: Do a css parse on parameter for element.pseudo()
<dbaron> ok, I'm on now -- the problem seems to be that WebEx doesn't work in Nightly Firefox any more (across 2 machines) -- but it works in Chrome
@annevk

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

It seems the CSS WG discussion didn't touch upon @heycam's point, which is quite relevant.

@tabatkins

This comment has been minimized.

Copy link
Member

commented Apr 19, 2019

gCS() should indeed switch to [=CSS/parse=]; its current definition isn't well-founded, as there's no notion of a string "matching" a CSS construct by itself. (You need to parse it.)

@heycam

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

And that issue got filed as #3875.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.