Fix attribute order sensitivity for range input #33831
Conversation
Added an E2E test to verify the fix.
ghost
added
the
src/Components/test/testassets/BasicTestApp/FormsTest/InputRangeComponent.razor
Outdated
Show resolved
Hide resolved
src/Components/test/testassets/BasicTestApp/FormsTest/InputRangeComponent.razor
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| [Fact] | ||
| public void InputRangeAttributeOrderDoesNotAffectValue() |
There was a problem hiding this comment.
@SteveSandersonMS is it possible to test these sort of changes as vanilla JS tests? I'm not saying we wouldn't have this, but given our perennial struggles with browser testing (and I'm not super sure how much playwrite helps here), is it possible to have some of these additionally covered by JS based unit tests?
There was a problem hiding this comment.
If you mean "not involving a browser", then I'd say that's not really viable. We're specifically interacting with DOM APIs here so unless there's a real DOM, the best a test could do is something like "assert we called a set of methods on a mock" (which would prove nothing about correct behavior). Or, testing against something like JSDom would be a loose approximation to what would happen for real.
We have to be able to run browser-based tests. It can't be beyond our abilities as a team/organization! So many other front-end teams can manage this. If our existing infrastructure isn't fit for that purpose, we'll set up new infrastructure.
| } | ||
| case 'TEXTAREA': { | ||
| const value = attributeFrame ? frameReader.attributeValue(attributeFrame) : null; | ||
| (element as any).value = value; |
There was a problem hiding this comment.
Just trying to figure out if it's necessary/desirable to split up input/select/textarea into three separate code paths here. If we were willing to defer the assignment of value on textarea as well (and I don't know of any reason to treat it differently), could all this coalesce back into a single code path?
Rather than having selectValuePropname and inputValuePropname as distinct properties, could we have a single deferredValuePropname that's applied to all three element types?
I know there would still be a small special-case aspect for select (because of needing to call setSelectElementValue), but it might be able to share the rest of the logic with input/textarea, which wouldn't themselves need to be distinguished.
If you think it's already better as-is then that's fine. I'm just wondering if we can reduce the number of distinct cases because there's already a lot of branching in here that we have to mentally trace through each time we want to modify the behavior.
There was a problem hiding this comment.
I agree, unifying the code paths makes it a bit cleaner. I removed setSelectElementValue in favor of setDeferredElementValue, which sets the value property of an element whose value gets assigned after other attributes have been populated, and the special case for select elements happens in there (since the same check would happen in the initial assignment of the value as well as the deferred assignment). Let me know if you think this should be done differently.
|
@MackinnonBuck This is looking great! I had one small suggestion about possibly coalescing some of the code paths but besides that it's looking super. The E2E test is ideal. |
There was a problem hiding this comment.
Excellent! Thanks for the update.
It might not matter, but I noticed at Browser.ts line 372 (in the updated code in this PR) there's still one case where we assign an input's value directly in a non-deferred way. It's specifically for type='time'. I'm unsure whether min/max values apply here and might clamp the value. If you think this is a concern, would it be possible to include this in the deferral system? It may well not be a concern.
Anyway, this is looking great - please feel free to merge!
|
@SteveSandersonMS Thanks for the feedback! I don't think that will be a concern - the |
|
Thanks for checking. In that case this looks all done to me :) |
3 participants
Fix attribute order sensitivity for range input
When an
<input type="range" />element specified avalueattribute before itsminandmaxattributes, thevalueproperty for the input would always be clamped by the defaultminandmaxvalues for range inputs (0-100) instead of the specifiedminandmax. This PR ensures that thevalueattribute respects the specifiedminandmaxattribute values if they exist, regardless of attribute order.PR Description
This fix ensures the the
valueattribute for all<input>elements is applied after other attributes. I have added an E2E test to verify the correct behavior.Addresses #33499