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

Fix handling of negative zero in number selectors #17127

Merged

Conversation

karwosts
Copy link
Contributor

@karwosts karwosts commented Jun 30, 2023

Proposed change

Fix some quirks around number selector and negative decimals.

It was difficult to input the value -0.5 in a number selector because if I clear the field and enter -0 it was immediately replaced with 0 by the ha-textfield. So typing - 0 . 5 was giving 0.5

The only way to enter it was to enter 0.5 and then move the cursor to the front and add in the -.

This PR fix that behavior so -0 is not modified.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue or discussion:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

@@ -64,7 +64,7 @@ export class HaNumberSelector extends LitElement {
class=${classMap({ single: this.selector.number?.mode === "box" })}
.min=${this.selector.number?.min}
.max=${this.selector.number?.max}
.value=${this.value ?? ""}
.value=${Object.is(this.value, -0) ? "-0" : this.value ?? ""}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too much of a hack IMO. We should fix this more generically in ha-text-field, unless it only occurs when using ha-selector-number?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if it is possible to fix in ha-textfield.

ha-textfield extends mwc-textfield, where value is a string property.

The issue is that when user types "-0", we get a event with value "-0" from mwc-textfield and convert it to Number, which is -0.

Then when we next render the ha-textfield, we set value to -0. But since value is a string, the number is implicitly converted to string. Unfortunately Number(-0).toString() is "0", not "-0".

Without changing ha-textfield to accept number types, it never sees the -0 input.

I'm not sure if there is such a thing as a "more generic" fix, since I believe 0/-0 are the only numbers that have this issue.

Copy link
Contributor Author

@karwosts karwosts Jul 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess if there was an alternative fix, it would be ha-selector-number storing the value from the textfield as a string instead of a number. Haven't thought through the implications of that though.

That might fix a minor issue where if you enter 0.001 and hit backspace, it switches to 0 and not 0.00

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I was assuming that the issue was caused by autocorrection in ha-text-field. If that's not the case and it's instead caused by string coercion, then we should better control that and get rid of the type incompatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I've uploaded an alternative fix that I think I like better, it fixes more cases of text field changing value unexpectedly.

selector now tracks the string in the textbox in addition to the number provided by the property, and only overwrites the string when converting that string to a number gives a different value than the number provided by the property.

Let me know if that looks better.

@karwosts karwosts marked this pull request as draft July 2, 2023 03:38
@karwosts karwosts marked this pull request as ready for review July 2, 2023 04:14
@bramkragten
Copy link
Member

I think this can be fixed by not calling value-changed in _handleInputChange if the value is -0? As that will get normalized to 0?

@bramkragten
Copy link
Member

What btw is also very annoying is that if you empty the input with backspace, it is immediately filled with the default value:

(at least when used in a blueprint)

CleanShot 2023-07-02 at 14 39 26

@karwosts
Copy link
Contributor Author

karwosts commented Jul 2, 2023

I think this can be fixed by not calling value-changed in _handleInputChange if the value is -0? As that will get normalized to 0?

I don't feel like that would work right. If for example the field is undefined, and you type in -0, and switch to yaml mode, it would still be undefined in the yaml, because you don't push an update?

Or if you have type -0.5 in the field, and hit backspace, then it will not update and the yaml will remain at -0.5?

@karwosts
Copy link
Contributor Author

karwosts commented Jul 2, 2023

What btw is also very annoying is that if you empty the input with backspace, it is immediately filled with the default value:

Huh I thought that was maybe something I fixed back in #16196. I think at the time I was only looking at text selectors though, maybe something unique about number causing this. Kind of feel like that's maybe an unrelated issue though?

edit: checked and looks like that fix only works for selectors that have string-type values.

@github-actions
Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@github-actions github-actions bot added the stale label Sep 30, 2023
@karwosts
Copy link
Contributor Author

I still like this fix and would like to see it merged.

@karwosts karwosts removed the stale label Sep 30, 2023
@bramkragten bramkragten merged commit ae35fd1 into home-assistant:dev Oct 9, 2023
@karwosts karwosts deleted the number-selector-negative-zero branch October 9, 2023 13:55
@github-actions github-actions bot locked and limited conversation to collaborators Oct 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants