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.

Sign up for GitHub

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

Jump to bottom

Not setting current-value for fluent-number-field #1424

Merged
merged 1 commit into from
Feb 2, 2024
Merged

Not setting current-value for fluent-number-field #1424

merged 1 commit into from
Feb 2, 2024

Conversation

cupsos
Copy link
Contributor

@cupsos cupsos commented Feb 2, 2024 •
edited
Loading

Pull Request

📖 Description

In Blazor Server, holding down Space, ArrowUp, or ArrowDown leads to a loop in value changes.
This issue has been reproduced in both the dev branch and v4.3.1.

switch-loop
number-field-loop

Both fluent-switch and fluent-number-field have value and current-value properties.

The difference between them is that current-value represents the real-time value displayed in the UI.
This means that current-value and value can differ, especially with rapid inputs.

Since setting either current-value or value triggers a change in value, under certain conditions, this can cause a loop in value changes.

This PR resolves the issue by not setting current-value.

flowchart TD
subgraph fluent-ui web-component
current-value
value
onchange
end

subgraph blazor-server
SetCurrentValueAsync
StateHasChanged
end

value
--> onchange
-->|fire on value changed| SetCurrentValueAsync
--> StateHasChanged
-->|set value if diffrent| value & current-value
value -->|change each ohther| current-value
current-value -->|change each ohther| value
Loading

🎫 Issues

👩‍💻 Reviewer Notes

There are other components that set current-value and value, but since the bug could not be reproduced with them, they were not included in the commit.

📑 Test Plan

✅ Checklist

General

  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

  • I have added a new component
  • I have added Unit Tests for my new compontent
  • I have modified an existing component
  • I have validate Unit Tests for an existing component

⏭ Next Steps

@cupsos
Copy link
Contributor Author

cupsos commented Feb 2, 2024

@microsoft-github-policy-service agree

@dvoituron
Copy link
Collaborator

I agree with you that this is a bug we need to fix.

But I don't think we need to remove current-value from the components. Because this attribute represents the value sent by the form. current-value must be set when the component is first rendered, to on for example, and must not change afterwards.

For example, using this simple HTML code using the Web Component (same for fluent-switch).

<html>
  <head>
    <script type="module" src="https://unpkg.com/@fluentui/web-components"></script>
  </head>
  <form action="" method="get">
    <input name="myOne" checked value="1" current-value="2" type="checkbox">One
    <fluent-checkbox name="myTwo" value="3" current-value="4">Two</fluent-checkbox>
    <button type="submit">Save</button>
  </form>
</html>

image

@cupsos
Copy link
Contributor Author

cupsos commented Feb 2, 2024

Oh, I didn't know that. Yes, you're right.

fluentui-blazor/src/Core/Components/Base/FluentInputBase.cs

Lines 166 to 173 in ed2a3c8

/// <summary>
/// Gets or sets the current value of the input.
/// </summary>
protected TValue? CurrentValue
{
get => Value;
set => _ = SetCurrentValueAsync(value);
}

Right now, can't make a fluent-switch with different value and current-value because FluentInputBase.CurrentValue is protected and the same as Value. Fixing the code that uses CurrentValue and changing the API is hard for me.

I think can remove current-value from fluent-number-field for now. Pressing up or down arrows in fluent-number-field is common, but it's unlikely people will keep pressing space in fluent-switch.

If it's okay, I'll undo the changes to fluent-switch and change the PR title.

@vnbaaij
Copy link
Collaborator

vnbaaij commented Feb 2, 2024

I've done some more check and looked at it with @dvoituron. I removed the code that put both value and current-value in the fluent-switch web component (in our FluentSwitch component) in a separate PR

@cupsos cupsos force-pushed the cupsos/not-setting-current-value branch from 41a87a5 to 97dbde9 Compare February 2, 2024 15:13
@cupsos cupsos changed the title Not setting current-value for fluent-switch and fluent-number-field Not setting current-value for fluent-number-field Feb 2, 2024
@vnbaaij vnbaaij self-requested a review February 2, 2024 15:34
@vnbaaij vnbaaij enabled auto-merge (squash) February 2, 2024 15:34
@dvoituron
Copy link
Collaborator

Could you update the Unit Tests?
image

@vnbaaij vnbaaij disabled auto-merge February 2, 2024 15:52
@vnbaaij vnbaaij merged commit dc45fab into microsoft:dev Feb 2, 2024
1 of 2 checks passed
vnbaaij pushed a commit that referenced this pull request Feb 6, 2024
@cupsos @vnbaaij
Not setting current-value for fluent-number-field (#1424)
9a76d59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vnbaaij vnbaaij vnbaaij approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cupsos @dvoituron @vnbaaij