Add more video attributes

[danieltroger]

Added some more of these attributes that were lacking https://developer.mozilla.org/en-US/docs/Web/HTML/Element/video

I added them in all lower case but as a user I'd prefer the camel cased version.

I can see some attributes are both all lower case and camelCase like here: https://github.com/danieltroger/dom-expressions/blob/0d5c46b24c9fba2d62dee8bc1acbf98c733fa633/packages/dom-expressions/src/jsx.d.ts#L1173-L1174

What's solid-js' convention?

[titoBouzout]

Great question! I have been thinking and dealing with this for the last couple of weeks.

What you want is to clone the whole template. That's for attributes/properties to be inlined. Think of the following:

const template = document.createElement('template')
template.innerHTML = '<div class="something"/>'

that's faster than

const template = document.createElement('template')
template.innerHTML = '<div/>'
template.firstChild.className = 'something'

The templates, are usually bigger than this, which makes it even more desirable. But the idea has been illustrated. (much faster to clone a chunk of html, than to clone it and then having to set some properties on it)

So this means you would want the lowercase and dashes spaced version whenever possible(attributes), so the template can be inlined and cloned faster. This is desired because of two reasons: template cloning and ssr.

The only reason why you may want a camel Case version is when the value its dynamic (it cannot be inlined because the value is not known), and if the user used a camel case then it's a property.

Solid has so many optimizations, that It's hard to follow. Basically it uses assignProps when it's spread ... or it inlines the assignment when its dynamic, such effect(()=>node.value = value). I have been thinking to fix this issue solidjs/solid#2312 but I can't make my mind if that should be fixed or not.

Glad to see you here!

[titoBouzout]

LGTM your PR!

[danieltroger]

Thank you @titoBouzout! It makes sense wanting to inline as much as possible into the template HTML string. And according to MDN the HTML attributes are indeed all lowercase.

But as far as the types goes, it seems like allowing the properties to be added with camelCase (which I think is slightly easier to read) still yields the same output, so it seems like the compiler treats it all the same in the end? See the output tab here: https://playground.solidjs.com/anonymous/aea47fbf-a24d-4260-adad-8a5bbc342b22

Somewhat related… why is loop for example not inlined in my example?

[titoBouzout]

But as far as the types goes, it seems like allowing the properties to be added with camelCase (which I think is slightly easier to read) still yields the same output, so it seems like the compiler treats it all the same in the end? See the output tab here: https://playground.solidjs.com/anonymous/aea47fbf-a24d-4260-adad-8a5bbc342b22

I am trying to get rid of any attribute/property transformation, its just problematic. For example on this case you are just lowercasing disablePictureInPicture, but what's the expectation with ariaTitle then? ariatitle won't work, it should be aria-title. What would be nice is opt-in to properties when the attribute has an uppercase letter, but just that, no transformations involved. The intention is more clear that way in my opinion. For example on this case, (disablePictureInPicture="false") it won't do what you are expecting, as string "false" is true(you can test this in plain html without solid, same output). https://playground.solidjs.com/anonymous/d3909ae8-2274-49f9-bd33-9512987d893b

Somewhat related… why is loop for example not inlined in my example?

There's a Map that holds a couple of attributes that are locked to be handled as properties, for different reasons I suppose. I also would like to get rid of this hardcoded properties and lookup for setters instead. https://github.com/ryansolid/dom-expressions/blob/main/packages/dom-expressions/src/constants.js

[ryansolid]

Wait does disableRemotePlayback not have a corresponding attribute? And is controlslist even writable?

@titoBouzout I don't think it is possible realistically to get rid of all the looks up. But for the most part I think that JSX should reflect the attributes has been my thinking. Although stuff like this is questionable. As now it looks like we have properties that aren't attributes.