solidjs / solid

A declarative, efficient, and flexible JavaScript library for building user interfaces.
https://solidjs.com
MIT License
32.21k stars 917 forks source link

HTML Progress Doesn't work #1835

Open mattbdc opened 1 year ago

mattbdc commented 1 year ago

Describe the bug

Works with DOM API: https://playground.solidjs.com/anonymous/05d9ad70-852d-40ca-8134-544ed1a6c21a

Doesn't work with Solid: https://playground.solidjs.com/anonymous/4f293905-4892-478a-b6ba-ad4dd5912df7

Click Make Indeterminate a few times, should be able to toggle on and off, doesnt work on Solid

Your Example Website or App

Above

Steps to Reproduce the Bug or Issue

Click button

Expected behavior

Works like DOM API

Screenshots or Videos

No response

Platform

Additional context

No response

leaysgur commented 1 year ago

FYI: As someone who has encountered similar case in the past... 👀

The current implementation of Solid(or rather dom-expressions) transforms

<progress value={1} />

to

progressEl["value"] = 1;

and if that key is removed on update

progressEl["value"] = null;

https://github.com/ryansolid/dom-expressions/blob/b63993dea6ff4d715763fc1f3f5d23980fafbe1c/packages/dom-expressions/src/client.js#L328-L341

And in this case, value: null will be reflected as value: 0, so the value attribute will still remain and not go back to :indeterminate state.

For that purpose, we need to delete the entire attribute, we want to use

progressEl.removeAttribute("value");

to have it, we can use

<progress attr:value={1} />

instead of value={1}.

This is the current situation and workaround, but I don't know if this is a bug or as expected... 😅

mattbdc commented 1 year ago

Thank you. Shouldn't all attributes be treated as attr on non components? such that all HTML native elements <tag attr="x" /> should be handled as <tag attr:attr="x" /> under what scenario would a native HTML element not prefer this scenario?

mattbdc commented 1 year ago

Further to the above, I guess it relates to the top answer on this https://stackoverflow.com/questions/6003819/what-is-the-difference-between-properties-and-attributes-in-html

ryansolid commented 1 year ago

Thank you. Shouldn't all attributes be treated as attr on non components? such that all HTML native elements <tag attr="x" /> should be handled as <tag attr:attr="x" /> under what scenario would a native HTML element not prefer this scenario?

Not necessarily.. value we handle as props for a reason.. related to input fields I believe. It wasn't always that way but we needed to change it for another one of the DOMs interesting inconsistencies. I think maybe we can be restrictive to only handling it as a prop on some elements perhaps. I maybe should look to see if there is a difference in null handling in some other frameworks because value as a property is pretty common.

fabiospampinato commented 1 year ago

@ryansolid I think this has to be special-cased because there's no .value that causes the progress bar to be indeterminate. See this playground: https://playground.solidjs.com/anonymous/4fe75699-3c5b-429f-b9bd-c9c087f51cb6

And MDN says:

If there is no value attribute, the progress bar is indeterminate;

Hence no possible value causes it to be indeterminate, so there must be some values (presumably null and undefined) that cause the "value" attribute to be removed.

gbj commented 1 year ago

Not necessarily.. value we handle as props for a reason.. related to input fields I believe. It wasn't always that way but we needed to change it for another one of the DOMs interesting inconsistencies. I think maybe we can be restrictive to only handling it as a prop on some elements perhaps. I maybe should look to see if there is a difference in null handling in some other frameworks because value as a property is pretty common.

Yeah input value and checked attributes in particular only set the initial/default value, whereas the properties set the current value, so most frameworks override this DOM behavior by default. (Maybe the same for option/select too, I forget.)

ryansolid commented 1 year ago

Yeah I just ckecked Inferno and it sets null to empty string on value property. So I guess it has the same problem.

Also Preact too: https://preactjs.com/repl?code=aW1wb3J0IHugcmVuZGVyIH0gZnJvbSAncHJlYWN0JzsKaW1wb3J0IHsgdXNlU3RhdGUgfSBmcm9tICdwcmVhY3QvaG9va3MnOwoKZnVuY3Rpb24gQ291bnRlcigpIHsKICAKICBjb25zdCBbcHJvZ3Jlc3NWYWx1ZSwgc2V0UHJvZ3Jlc3NWYWx1ZV0gPSB1c2VTdGF0ZSh1bmRlZmluZWQpCgogIGNvbnN0IHRvZ2dsZUluZGV0ZXJtaW5hdGUgPSAoKSA9PiB7CiAgICAgIGlmKHByb2dyZXNzVmFsdWUpIHsKICAgICAgICBzZXRQcm9ncmVzc1ZhbHVlKHVuZGVmaW5lZCkKICAgICAgfSBlbHNlIHsKICAgICAgICAgc2V0UHJvZ3Jlc3NWYWx1ZSg1MCkKICAgICAgfQogIH0KCiAgcmV0dXJuICg8ZGl2IHN0eWxlPSJiYWNrZ3JvdW5kOiBibHVlIj4KCiAgICAgIDxwcm9ncmVzcyB7Li4uKHByb2dyZXNzVmFsdWUgPyB7IHZhbHVlOiBwcm9ncmVzc1ZhbHVlIH0gOiB7fSl9IG1heD0iMTAwIiBzdHlsZT0id2lkdGg6IDEwMCUiLz4KICAgICAgPGJ1dHRvbiBvbmNsaWNrPXt0b2dnbGVJbmRldGVybWluYXRlfT5NYWtlIGluZGV0ZXJtaW5hdGU8L2J1dHRvbj4KICAgPC9kaXY%2BCiAgKTsKfQoKcmVuZGVyKDxDb3VudGVyIC8%2BLCBkb2N1bWVudC5nZXRFbGVtZW50QnlJZCgnYXBwJykpOwo%3D

Yeah input value and checked attributes in particular only set the initial/default value, whereas the properties set the current value, so most frameworks override this DOM behavior by default. (Maybe the same for option/select too, I forget.)

This sounds right. Hmm.. it is pretty common issue. We can try to find some sort of fix perhaps that isn't too complicated I hope. But this may end up being a won't fix.

fabiospampinato commented 1 year ago

The fix I implemented on my end was basically checking if a value of null/undefined was being set on a progress element and removing the attribute (technically I'm always removing the attribute when setting to null/undefined, I'm just converting the undefined to null because the undefined throws). Maybe there are other ways to fix it if that's not good enough for Solid.

trusktr commented 1 year ago

For reference here's the example working with attr::

https://playground.solidjs.com/anonymous/53995944-2a7c-42ed-8ebd-e21ea4bd7ce5