fable-compiler / fable-browser

Fable bindings for Browser Web APIs
MIT License
67 stars 37 forks source link

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

Open davedawkins opened 3 years ago

davedawkins commented 3 years ago

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 commented 3 years ago

It's tricky, we had some discussion about this here: https://github.com/fable-compiler/fable-browser/pull/76#discussion_r698608040

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 commented 3 years ago

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 commented 3 years ago

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

davedawkins commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

That documentation says that it returns a Node:

Returns

An object inheriting from Node.

It then goes on to list the concrete types.

alfonsogarciacaro commented 3 years ago

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 :+1: