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

<input type="range"> is sensitive to the order of attribute setting #2427

Open
wycats opened this issue Mar 9, 2017 · 18 comments
Open

<input type="range"> is sensitive to the order of attribute setting #2427

wycats opened this issue Mar 9, 2017 · 18 comments

Comments

@wycats
Copy link

wycats commented Mar 9, 2017

These two pieces of code behave observably differently in Chrome and Firefox (for reasons described inline):

s = document.createElement('input');
s.setAttribute('type', 'range'); // defaults min to 0 and max to 100; value is 50, the middle
s.setAttribute('min', '0'); // no change
s.setAttribute('max', '10'); // max is now < 50, so value is truncated to max, 10
s = document.createElement('input');
s.setAttribute('min', '0'); // min is 0
s.setAttribute('max', '10'); // max is 10
s.setAttribute('type', 'range'); // value is 5, mid-way between 0 and 10

I encountered this in Glimmer. We translate HTML syntax into DOM calls according to the HTML tree construction spec, but the spec assumes that an implementation can set all attributes simultaneously, which is not possible with today's APIs. We could special-case the type attribute for input and always make sure to set it last, but that's unfortunate (and who knows, there may be other issues, today or in the future, with another order).

It doesn't matter whether the attributes are set with the element with or without an owner document; the behavior is a consequence of the "suffering from an overflow" constraint.

@domenic
Copy link
Member

domenic commented Mar 9, 2017

It's not clear what the spec bug is here. There are many attributes on the platform that are order-sensitive in what effect they have due to interdependencies between them. <input> elements have a lot of interdependent attributes in the same element, but for example so do <audio> and <video>. Your two pieces of code do different things, and should indeed behave differently per spec.

@wycats
Copy link
Author

wycats commented Mar 9, 2017

@annevk
Copy link
Member

annevk commented Mar 9, 2017

I think what @wycats didn't highlight is that you get different results through the HTML parser which sets these all at once. I think that is illustrative of needing the hook browsers have to set multiple attributes at once, which I thought we all agreed was a bug and something we did not want to expose to custom elements (and hope to remove any dependencies of if we found them).

@wycats
Copy link
Author

wycats commented Mar 9, 2017

@annevk Right. Glimmer attempts to translate HTML syntax into the exact set of DOM calls that will give us the correct semantics. Most of the time, this works well.

For example, we fixed a bug in Glimmer a while ago where we were inserting elements in the wrong order (appending into DOM when seeing close-element, instead of open-element) that created problems for <select>. We fixed the bug by making our DOM calls compliant with the tree construction spec order.

In this case, the web's APIs simply don't offer us a way to do what the tree construction spec wants (set a bunch of attributes at once), so we'll have to special-case input[type] and hope another problem doesn't appear in the other direction.

@domenic
Copy link
Member

domenic commented Mar 9, 2017

I see. Brainstorming a solution: we add some kind of dirty value flag for input type=range (or reuse the one that's already present for all inputs?), and if that's not set, then suffering from underflow/overflow/step mismatch tries to set to the default value (or some version of the default value which also respects step="").

@wycats
Copy link
Author

wycats commented Mar 9, 2017

@domenic I think that works. I also think it makes sense to add setAttributes() eventually.

@chancancode
Copy link

FWIW, Safari does more or less what @domenic proposes (as far as we can tell), whereas chrome and Firefox both have the "problem". Unsure about IE, haven't had a chance to test it yet.

@annevk
Copy link
Member

annevk commented Mar 10, 2017

(FWIW, I think not adding setAttributes() and fixing the places where we rely on such functionality is better, as it will eventually make for a more robust system. setAttributes() would also lead to custom elements starting to rely on all attributes being set at once for instance.)

@wycats
Copy link
Author

wycats commented Mar 10, 2017

@chancancode this makes this issue an interop problem, but that also means we may have more leeway to fix it.

@zcorpan
Copy link
Member

zcorpan commented Mar 10, 2017

In some places (at least img, video) we have deliberately tried to make attribute order not matter. I think that is generally a good thing, input specifically seems like it should follow that principle.

@snuggs
Copy link
Member

snuggs commented Mar 10, 2017

Great in theory @wycats however i'm wondering what the side effects to NamedNodeMap would be by setting all attributes. The spec states attributes are in no particular order. Also this is what is returned from Element.attributes.

  1. How would this effect:
    • NamedNodeMap.setNamedItem
    • NamedNodeMap.setNamedItemNS (Added in Level 2 Core specification)

And should those be pluralized as well?

  1. How would this effect the attribute list parser since there was a recent move to Attr instead of Node?

Of course the same questions are asked of removals i.e.(removeNamedItemNS etc)

I ran into this situation recently myself with custom elements. Got by with placing a Proxy on the state and a MutationObserver between the state and the element. Do keep in mind that this feels like a "polyfill" of sorts. Custom elements

@zcorpan thanks for img, video :-)

@annevk I agree 100% re. setAttributes & custom elements. I'm certain this would effect attributeChangedCallback() and observedAttributes /cc @mrbernnz

https://dom.spec.whatwg.org/#attr
https://www.w3.org/TR/DOM-Level-2-Core/core.html#ID-1780488922
https://github.com/snuggs/snuggsi/blob/master/elements/attributes.es

Serabe added a commit to Serabe/glimmer-vm that referenced this issue Mar 15, 2017
If a template had an `input` `range` whose type was set before a max or a min
attribute was added, the initial value would be wrong.

In plain HTML, the initial value for `<input max="10" type="range" />` would be
`5` (`(min + max) / 2 = (0 + 10) / 2 = 5`).

In Glimmer, this would be 10 because the code would be equivalent to:

```js
let input = document.createElement('input');
input.type = "range";
input.max = "10";
```

Setting `type` to `range` would make the value sanitization algorihtm to kick
in, setting the value to 50. When setting the `max` to `10`, the algorithm
would sanitize the value (50 by now) to the maximum (10).

More info in whatwg/html#2427
Fixes emberjs/ember.js#14958
@domenic domenic self-assigned this May 10, 2017
domenic added a commit that referenced this issue May 10, 2017
This fixes #2427, by only clamping to the minimum/maximum when the
element's dirty value flag is set. This allows attributes to be set
without regard for their order, which is much better than being
order-dependent (and thus does not require magic in the HTML parser to
set multiple attributes at once). It also better matches the informative
definition of the dirty value flag as tracking the relationship between
the element's value and its default value.
@domenic
Copy link
Member

domenic commented May 10, 2017

I tried fixing this in #2661, but it turns out my fix matches no browsers. I was hoping we could at least match Safari.

Safari seems to give a value of 0 in the case

<input type="range" id="value_smaller_than_min" min=0 max=5 value=-10 />

(from the web-platform-test range.html) whereas #2661 would give that a value of... 3, I think? (Not 2.5.)

One way of fixing this is to say that suffering from an overflow/underflow uses the default value if there is no value="" content attribute, and otherwise clamps the value. This seems unusual, but I am not sure what the right solution is.

I would love to hear from @cdumez what WebKit does here in the three cases:

<input type="range" min=0 max=5 value=-10>
const i1 = document.createElement("input");
i1.setAttribute("type", "range");
i1.setAttribute("max", "10");
const i2 = document.createElement("input");
i2.setAttribute("max", "10");
i2.setAttribute("type", "range");

and how it differs from the spec's model. Then maybe we can change the spec to match WebKit's behavior.

@domenic
Copy link
Member

domenic commented Aug 3, 2017

I'm unassigning myself as I haven't been able to find a good model here and don't believe I'll have time to work one out. I'd welcome community help (or Safari engineer help) in determining what model Safari uses and whether it suits our purposes of avoiding order-dependent attribute setting. If we can find that model I can re-commit to taking time to spec it.

One way of starting this is to propose a reasonable model that matches Safari's results for all of the tests in https://github.com/w3c/web-platform-tests/blob/master/html/semantics/forms/the-input-element/range.html . #2661 is not such a model, as discussed above.

@annevk
Copy link
Member

annevk commented Oct 4, 2017

A model that would explain the above (value ends up being 0 for the first and 5 for the subsequent examples) is that value is observed, rather than set. That's how <progress> works (now).

It's not entirely clear to me how to make that work within <input> though.

@annevk
Copy link
Member

annevk commented Oct 4, 2017

As a start, I think it would help if we moved underflow/overflow checks to the value sanitization algorithm rather than have them apply all the time.

The other thing that seems to be needed here however is that whenever max or min is set, the value is sanitized as well. That's needed because they influence the initial value. Without that happening the initial value would always be dependent on when min and max are set, as far as I can tell.

@jessi3py would you agree with that analysis? (For context, we basically don't want the order in which you set attributes to end up mattering.)

@annevk
Copy link
Member

annevk commented Oct 4, 2017

(I wonder if the step attribute also contributes to this somehow, but I haven't found a suitable example that demonstrates it.)

@jessi3py-zz
Copy link

In Firefox's implementation, we do sanitize the value when min/max is set. But in our sanitization algorithm, we set it to the middle value only if there is no value set (including content attribute and initial value), otherwise we clamp the value based on min/max.

So I think the question is still whether we should force set it to the middle value when value has not changed by the user?

Let me know if I'm missing anything.

@annevk
Copy link
Member

annevk commented Oct 9, 2017

That's a good point. We need to check for the "dirty value flag" too and only reset value when the dirty value flag is false. Otherwise we'd still have the problem that value is set as soon as it gets sanitized.

The main reason to "force set it" is to make it not matter in what order the attributes get set. Making the behavior of the element dependent on that is not a great developer experience.

Serabe added a commit to Serabe/glimmer-vm that referenced this issue Oct 27, 2017
If a template had an `input` `range` whose type was set before a max or a min
attribute was added, the initial value would be wrong.

In plain HTML, the initial value for `<input max="10" type="range" />` would be
`5` (`(min + max) / 2 = (0 + 10) / 2 = 5`).

In Glimmer, this would be 10 because the code would be equivalent to:

```js
let input = document.createElement('input');
input.type = "range";
input.max = "10";
```

Setting `type` to `range` would make the value sanitization algorihtm to kick
in, setting the value to 50. When setting the `max` to `10`, the algorithm
would sanitize the value (50 by now) to the maximum (10).

This was originally discussed in glimmerjs#425, but an alternative solution was
asked for. While this PR keeps part of the original implementation
(could not find a simple way to fix the issue when a template is being
rendered), this PR goes further and fixes it for Emberish components and
`...attributes` in Glimmer components.

More info in whatwg/html#2427
Fixes emberjs/ember.js#14958
Serabe added a commit to Serabe/glimmer-vm that referenced this issue Nov 17, 2017
If a template had an `input` `range` whose type was set before a max or a min
attribute was added, the initial value would be wrong.

In plain HTML, the initial value for `<input max="10" type="range" />` would be
`5` (`(min + max) / 2 = (0 + 10) / 2 = 5`).

In Glimmer, this would be 10 because the code would be equivalent to:

```js
let input = document.createElement('input');
input.type = "range";
input.max = "10";
```

Setting `type` to `range` would make the value sanitization algorihtm to kick
in, setting the value to 50. When setting the `max` to `10`, the algorithm
would sanitize the value (50 by now) to the maximum (10).

This was originally discussed in glimmerjs#425, but an alternative solution was
asked for. While this PR keeps part of the original implementation
(could not find a simple way to fix the issue when a template is being
rendered), this PR goes further and fixes it for Emberish components and
`...attributes` in Glimmer components.

More info in whatwg/html#2427
Fixes emberjs/ember.js#14958
wycats added a commit to glimmerjs/glimmer-vm that referenced this issue Jun 6, 2023
The current tree builder uses an HTML buffer to create elements, which
means that all attributes get set atomically and before properties.

This resolves a category of bugs exemplified by
whatwg/html#2427 *if* `type`, `min`, `max`
(and other similar attributes) are set as attributes. This means that
the element will get fully and properly set up before the `value`
property is set.

The tests still rely on the fact that `disabled="anything"` gets set as
a property, coerced into a boolean, and then reflected back to the
`disabled` attribute as `true` (resulting in `disabled=''`).

This commit attempts to resolve those issues by using properties on
input fields for:

- value
- any boolean attributes like `checked`, `disabled`, `multiple`

This means that things like `pattern`, `min` and `max` are set as
attributes, which causes the semantics of form constraints to perfectly
match HTML's semantics (because we're literally creating an element
from HTML).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

7 participants