-
Notifications
You must be signed in to change notification settings - Fork 558
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
WebHost: Fix NamedRange values clamping to the range in player-options #3613
base: main
Are you sure you want to change the base?
Conversation
If a NamedRange has a `special_range_names` entry outside the `range_start` and `range_end`, the HTML5 range input will clamp the submitted value to the closest value in the range. These means that, for example, Pokemon RB's "HM Compatibility" option's "Vanilla (-1)" option would instead get posted as "0" rather than "-1". This change updates NamedRange to behave like TextChoice, where the select element has a `name` attribute matching the option, and there is an additional element to be able to provide an option other than the select element's choices. This uses a different suffix of `-range` rather than `-custom` that TextChoice uses. The reason is we need some way to decide whether to use the custom value or the select value, and that method needs to work without JavaScript. For TextChoice this is easy, if the custom field is empty use the select element. For NamedRange this is more difficult as the browser will always submit *something*. My choice was to only use the value from the range if the select box is set to "custom". Since this only happens with JS as "custom' is hidden, I made the range hidden under no-JS. If it's preferred, I could make the select box hidden instead. Let me know. This PR also makes the `js-required` class set `display: none` with `!important` as otherwise the class wouldn't work on any rule that had `display: flex` with more specificity than a single class.
@ThePhar I think you should have a look at this too, as you're working on another big WebHost rework. |
The original reason supporting values outside the normal range was implemented was, to my knowledge, to allow users to exclude certain options from being chosen if "random" is selected. Rather than add complexity to the WebHost to support this system, the proper solution is to change the system to more gracefully exclude specified options from "random." There was discussion about this in #ap-core-dev a while back. |
The games in core that create a NamedRange with options outside the range (and thus are affected by this bug) are:
In all cases, the reason for an option outside the range is to provide an experience that is completely different from what an option in the range would provide. Thus it seems less like the goal is 'allow users to exclude certain options from being chosen if "random" is selected' and more like "provide an option that provides a completely different experience than one you would select from the slider". Missing by a pixel and getting 51 instead of 50 for most of the above options would basically be the same rando. -2 vs -1 vs 0 in Moreover, this is a bug actively impacting users right now. We've seen multiple reports of users trying to select -2 for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't speak for the overall design choice, but I can say that this behavior should be considered a critical bug, and until someone actually PRs something better, this is infinitely better than doing nothing.
Code seems good enough to do what it wants to do.
Well, then your conclusion doesn't match the ones I found when asking world devs why they used this. The point is so that |
More than just "random doesn't pick it", my point was I think it also makes sense for the slider to not pick it either. |
If I had been asked, my response would have been closer to remyjete's. Now that you mention it, I guess random is also a concern, but that's the kind of thing that I only would have thought of later, once someone brings it up. My intent is definitely "Here is a range of normal values, and here is one or several special values that can be used instead of the range, affecting the same game mechanic but in a completely different way" |
What is this fixing or adding?
If a NamedRange has a
special_range_names
entry outside therange_start
andrange_end
, the HTML5 range input will clamp the submitted value to the closest value in the range.These means that, for example, Pokemon RB's "HM Compatibility" option's "Vanilla (-1)" option would instead get posted as "0" rather than "-1".
This change updates NamedRange to behave like TextChoice, where both the select dropdown and the additional input element for custom values get posted with the form.
This uses a different suffix of
-range
rather than-custom
that TextChoice uses. The reason for not just reusing-custom
is we need some way to decide whether to use the custom value or the select value, and that method needs to work without JavaScript. For TextChoice this is easy, if the custom field is empty use the select element. For NamedRange this is more difficult as the browser will never submit an empty string for a range input. My choice was to only use the value from the range if the select box is set to "custom". Since this only happens with JS as "custom' is hidden, I made the range hidden under no-JS, thus limiting the options under no-JS to those provided by the select dropdown. If it's preferred, I could make the select box hidden instead. Let me know. But I didn't want to remove the custom entry field for TextChoice, which is why I went with a different suffix.This PR also makes the
js-required
class setdisplay: none
with!important
as otherwise the class wouldn't work on any rule that haddisplay: flex
with more specificity than a single class.Fixes https://discord.com/channels/731205301247803413/1257712995593617449
How was this tested?
With both JS enabled and disabled, I created Pokemon RB YAMLs with the HM/TM compatibility options set to Vanilla (-1), None (0), Full (100), and for JS enabled only various custom options with the sliders.