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

Move adoptedStyleSheets from Element to Node, also add getRootNode()? #85

Open
davedawkins opened this issue Oct 19, 2021 · 7 comments
Open

Comments

@davedawkins
Copy link
Contributor

I think there's an argument for moving adoptedStyleSheets from Element to Node. According to the spec and examples, you only expect to see this on a root node such as HTMLDocument and ShadowRoot.

Neither of these appears to be an Element, but both inherit from Node:

https://developer.mozilla.org/en-US/docs/Web/API/HTMLDocument

image

and

https://dom.spec.whatwg.org/#documentfragment

In addition, it would be useful to add getRootNode() with the same extension (or elsewhere).

https://developer.mozilla.org/en-US/docs/Web/API/Node/getRootNode

If in agreement, I can submit a PR that changes this code to

type Node with 
    /// returns this DocumentOrShadow adopted stylesheets or sets them.
    /// https://wicg.github.io/construct-stylesheets/#using-constructed-stylesheets
    [<Emit("$0.adoptedStyleSheets{{=$1}}")>]
    member __.adoptedStyleSheets with get(): CSSStyleSheet array = jsNative and set(v: CSSStyleSheet array) = jsNative
   [<Emit("$0.getRootNode()")>]
    member __.getRootNode() : Node = jsNative    
@alfonsogarciacaro
Copy link
Member

It's tricky, we had some discussion about this here: #76 (comment)

The issue is these "interfaces" that appear in the spec (like DocumentOrShadowRoot) don't correspond to DOM classes and are more like TS unions: so like saying this class and this one can implement this method/property. As @AngelMunoz noted, it looks Element can implement adoptedStyleSheets even if it's not specified in the spec.

I'd prefer not to touch adoptedStyleSheets as it's already been released as "stable", but we can implement Node.getRootNode if it's missing.

@davedawkins
Copy link
Contributor Author

davedawkins commented Oct 20, 2021

The real problem I'm facing though, is the need to downcast a Node (which isn't an Element) to an Element in order to access adoptedStyleSheets. I'm getting around it for now with "?adoptedStyleSheets" or my own Node extension class (as seen above).

As @AngelMunoz noted, it looks Element can implement adoptedStyleSheets even if it's not specified in the spec.

If Node implements adoptedStyleSheets, then Elements will inherit it.

more like TS unions

Given that Element inherits from Node, I don't think we have the need for muiltiple definitions yet. We have a single inheritance chain for the observed real-world cases (HTMLDocument and ShadowRoot, both Nodes) and the potential future ones (Element)

It's not that I think Element shouldn't implement it, it's that the only use cases I'm seeing right now are specifically not Elements; their common type is Node. I'm proposing pushing the existing definition higher in the inheritance chain.

@alfonsogarciacaro
Copy link
Member

What's the situation where you need to access adoptedStyleSheets from a Node? Do you a code sample?

@davedawkins
Copy link
Contributor Author

Sure. Here I'm using getRootNode() and the result node is where the adoptedStyleSheets is located:

https://github.com/davedawkins/Sutil/blob/df509a31ee5b1030004064e9a5f76492cb8b63ed/src/Sutil/Styling.fs#L199

@alfonsogarciacaro
Copy link
Member

alfonsogarciacaro commented Oct 21, 2021

Ah, ok, I see your point. Tricky part is according to the documentation it doesn't really return Node but either HTMLDocument or ShadowRoot (a "union" again). Hmm...

@davedawkins
Copy link
Contributor Author

davedawkins commented Oct 21, 2021

That documentation says that it returns a Node:

Returns

An object inheriting from Node.

It then goes on to list the concrete types.

@alfonsogarciacaro
Copy link
Member

Hmm, still unsure if Node is the best place for this. But your arguments are sound and given that users should be checking whether adoptedStyleSheets is null or not anyways, I guess we can put it in Node at the end. Hopefully the change shouldn't break existing code. Please do send the PR 👍

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

No branches or pull requests

2 participants