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

[Rating] Unexpected component behavior after setting value as user #5312

Closed
macandcheese opened this issue Sep 13, 2022 · 14 comments
Closed
Assignees
Labels
4 - verified Issues that have been released and confirmed resolved. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. p - high Issue should be addressed in the current milestone, impacts component or core functionality

Comments

@macandcheese
Copy link
Contributor

macandcheese commented Sep 13, 2022

Actual Behavior

As an end-user, after interacting with the rating, there is unexpected behavior that occurs after setting (or clearing) a user-set value.

Screen.Recording.2022-09-13.at.10.00.41.AM.mov
  • If I've set, or set and cleared, the component will reset on a subsequent keyboard-invoked focus.
  • I cannot immediately use space or enter to set the value again, I need to navigate off and back to the desired rating.

Expected Behavior

  • I would not expect subsequent component focuses to reset the value, once I as a user have set it.
  • I would expect to be able to toggle between set / clear value while remaining focused on the same rating.

Reproduction Sample

https://codepen.io/mac_and_cheese/pen/ZEoBWGj?editors=100

Reproduction Steps

For clear issue:

  • Open codepen
  • Set a value on a rating
  • Use keyboard to leave / return to component focus
  • Rating will be cleared

For not being able to "re select" issue:

  • Open codepen
  • Set a value on a rating
  • Use keyboard to clear value
  • Attempt to use same key to set value again
  • Value will not set
  • Navigate to an adjacent star element, and return
  • Value will be set-able with the same key.

Reproduction Version

91

Relevant Info

No response

Regression?

No response

@macandcheese macandcheese added bug Bug reports for broken functionality. Issues should include a reproduction of the bug. 0 - new New issues that need assignment. needs triage Planning workflow - pending design/dev review. labels Sep 13, 2022
@driskull
Copy link
Member

@macandcheese the codepen seems to be incorrect for the issue.

@macandcheese
Copy link
Contributor Author

@macandcheese the codepen seems to be incorrect for the issue.

Wrong link - updated. It's just a single instance of a rating component, but it illustrates the issue.

@jcfranco jcfranco added the p - high Issue should be addressed in the current milestone, impacts component or core functionality label Oct 12, 2022
@geospatialem geospatialem removed the needs triage Planning workflow - pending design/dev review. label Oct 12, 2022
@driskull driskull self-assigned this Oct 31, 2022
@driskull driskull added 2 - in development Issues that are actively being worked on. and removed 0 - new New issues that need assignment. labels Oct 31, 2022
@driskull
Copy link
Member

I'm not sure we should allow toggling between a set value and no value when hitting enter or clicking. Radio buttons don't usually work like that.

If we want, maybe we could support the escape or delete key to reset to no value?

@driskull
Copy link
Member

driskull commented Oct 31, 2022

I'm also not quite sure while hitting enter or space needs to emit an event. It seems like a user already clicked or navigated to the value with the arrow keys and the emit has occurred. Any idea on why we need to emit again on enter or space? I think we also emit on tab out of the component. These extra events seem unnecessary to me.

@macandcheese
Copy link
Contributor Author

Agree on not needing the extra events while navigating. I do think enter / space / click again could be expected to clear the rating, but will defer to @geospatialem there for a11y.

@driskull
Copy link
Member

driskull commented Nov 1, 2022

I do think enter / space / click again could be expected to clear the rating

Can you elaborate on why? These are radio buttons and I don't think there is a way to clear radio buttons

@macandcheese
Copy link
Contributor Author

Because as a user I may want to clear the rating I’ve just set. This was the enhancement issue where it was added: #3043

I know there’s the internal hidden input / analog to the radio control but I’m not sure the UX is 1:1. Without the app building their own reset control, as a user I can’t clear my rating.

@driskull
Copy link
Member

driskull commented Nov 1, 2022

hmmm because the way its currently set up, it doesn't really clear the value it just sets it to zero. Assuming zero is a valid value to submit.

Maybe another issue is that the component has a default value of 0. So when the form is submitted that value of 0 will be submitted. Maybe it should be null?

@macandcheese
Copy link
Contributor Author

macandcheese commented Nov 1, 2022

Yeah, if we want to remove that current "unset my rating" functionality, an option could be to have "no stars" as a navigable item alongside 1-5, but to your point there is a difference between 0 and no value (both of which could be valid or invalid in a variety of scenarios). I've seen both in the wild, components that let a user "re-select to clear", and components that have a "0 step".

@driskull driskull removed their assignment Nov 1, 2022
@driskull driskull added 0 - new New issues that need assignment. and removed 2 - in development Issues that are actively being worked on. labels Nov 1, 2022
@driskull
Copy link
Member

driskull commented Nov 1, 2022

Yeah, not sure how to proceed with this one so putting it back in the box.

Issues to discuss

  • Should we allow unsetting value?
  • If we allow unsetting value, should it set the value to null instead of 0?
  • Currently, all inputs are focusable. Ideally, only one input should be focusable. You can notice this by shift-tab back to a rating.
  • Enter/Space always emit event even if the value is already set to 0.
  • tabbing out of component always emits value even if no change.

@geospatialem
Copy link
Member

I do think enter / space / click again could be expected to clear the rating, but will defer to @geospatialem there for a11y.

For a11y I'd expect clearing of the component from a separate element from the stars. It could make it a bit more clear, both visually and for AT users to distinguish the value and what is set in the component.

Perhaps something similar to W3C's star rating example?

@alisonailea alisonailea self-assigned this Dec 2, 2022
@alisonailea alisonailea added 2 - in development Issues that are actively being worked on. and removed 0 - new New issues that need assignment. labels Dec 7, 2022
@alisonailea alisonailea linked a pull request Dec 9, 2022 that will close this issue
alisonailea added a commit that referenced this issue Dec 16, 2022
**Related Issue:** #5312

## Summary

Resolves bugs keyboard interaction and focus-in/focus-out
Cleans up component events, state, methods, and props
Refactors the component to make the value prop the source of truth and
driver of the component state
Refactor tests to remove duplicates and improve scan-ability.

### Before

https://user-images.githubusercontent.com/4733155/189963194-da8ee607-f4a8-46bf-8e21-9b36d2aeef64.mov

### After

https://user-images.githubusercontent.com/3362490/208055841-20e6630a-c2ab-43b6-815d-18f14b0cd911.mp4
@alisonailea alisonailea added 3 - installed Issues that have been merged to master branch and are ready for final confirmation. and removed 2 - in development Issues that are actively being worked on. labels Jan 17, 2023
@github-actions github-actions bot assigned geospatialem and unassigned alisonailea Jan 17, 2023
@github-actions
Copy link
Contributor

Installed and assigned for verification.

@geospatialem
Copy link
Member

Accessibility works incredibly great, nice work! 👍🏻

@alisonailea Noticed using the Down arrow key moves focus to the the next star and Up arrow to the previous star when testing via next.727 with the following Codepen, is that the expected behavior? I'd expect the opposite behavior, but can update the docs if that's the expectation. cc @macandcheese

@geospatialem geospatialem added 4 - verified Issues that have been released and confirmed resolved. and removed 3 - installed Issues that have been merged to master branch and are ready for final confirmation. labels Jan 20, 2023
@geospatialem
Copy link
Member

Opened a new issue to tackle in a future upcoming release, the above issue has been verified.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - verified Issues that have been released and confirmed resolved. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. p - high Issue should be addressed in the current milestone, impacts component or core functionality
Projects
None yet
Development

No branches or pull requests

5 participants