microsoft / microsoft-ui-xaml

Windows UI Library: the latest Windows 10 native controls and Fluent styles for your applications
MIT License
6.31k stars 676 forks source link

[ColorPicker] R/G/B or H/S/V fields should not accept alphabetical characters - move to NumberBox? #3420

Open niels9001 opened 3 years ago

niels9001 commented 3 years ago

Describe the bug The input fields in the ColorPicker control should only accept numerical values, not alphabetical values. When entering an alphabetical value and tabbing to the next inputfield will remove the incorrect character.

This is fine. However, Windows Narrator will still announce the alphabetical character and will not let the (visually impaired) user know that the value was incorrect.

We are running into this issue with PowerToys Settings: https://github.com/microsoft/PowerToys/issues/7085

Steps to reproduce the bug

  1. Turn on Narrator
  2. Navigate (with tab) to the Red field.
  3. Enter character (e.g. 'a')
  4. Narrator says A
  5. Tab to the next field.

Expected behavior Since these are numberfields, why not use the NumberBox control? This would be more approriate (we can set the min and max as well!) and improves accesbility.

Screenshots image

marcelwgn commented 3 years ago

~@StephenLPeters Mind if I work on this issue?~

Edit: NVM, guess Felix already looked into this.

Felix-Dev commented 3 years ago

@niels9001 The NumberBox control is currently designed to validate entered input on focus lost (or enter key pressed). The reason behind this has been given by @SavoySchuler here:

I do understand that masking could be a desirable default, but let me share what my team discussed last week - we landed on the direction that validation (error indicator/message) is preferred in Fluent controls as the alternative, masking, can be frustrating and confusing for end users when they experience no output from their keyboard entry.

As such, even if we use a NumberBox, it will still currently render invalid characters when entered a string like "25a" in the R/G/B fields as shown in your post. Narrator will also announce the entered invalid character just fine. Furthermore, switching to the NumberBox control in its current form will mean that more than three characters can be entered by users in the R/G/B input fields: Typing in a string like "123456789" into the fields will render all entered digits, with the input being validated to "123" on focus lost. In the current ColorPicker UI, all entered characters after "123" will be masked so this would be a change in behavior (and to the worse in my opinion).

In addition, the NumberBox control is currently not set up to mask input characters. It has no TextChanging event, for example, which could be used to mask input characters. If we were to keep using the TextBox control, while we could apply an input mask then, Narrator still announces the entered character, with no additional information that this character has been invalid. As such, the user could enter 'a', Narrator announces that character, yet we do not actually render it, leaving the visually impaired user with the issue as reported here.

Given @SavoySchuler's and the team's thinking about masking vs validating, would masking alphabetical characters even be desired here for the ColorPicker's input fields?

I do think that limiting the user input for the R/G/B/... fields to three digits is reasonable. Being able to type just as many digits as one likes into these fields sounds quite strange to me. However, Narrator output should be changed then to reflect that limit. For example, given the typed input "2345", it would be nice to have Narrator announce something like "5 - invalid - max digit amount reached". This would be an ask for the TextBox control though.

Similar, if entering an invalid character like 'a' into the R/G/B/... fields, Narrator could announce something like "a - invalid - illegal character". Either way, Narrator should announce something for the visually impaired user to work with - no matter if we mask that character or not.

The ColorPicker's hexadecimal input field also has similar issue as reported here for the R/G/B .... fields (it renders invalid craracters). We should thus expand this issue to cover that input field as well. A switch to the NumberBox control for the hexadecimal field could simplify the ColorPicker implementation a bit though that will be dependent on the roadmap for hexadecimal support for the NumberBox (see issue https://github.com/microsoft/microsoft-ui-xaml/issues/2757 for that).

Overall speaking, I think changing the input fields of the ColorPicker from TextBox to NumberBox makes sense and will help simplify the ColorPicker implementation a bit. The current NumberBox control, however, might not yet be fully suited for that. We will likely still face the same Narrator issues as reported here which will impact the UX for the visually impaired user.

We could, in addition, think about adding visual feedback to the input fields. While this won't help the visually impaired user, it could improve the UX for the rest of the users. An example visual feedback could look like this (image by @mdtauk originally posted here: image

Thus, if the user enters the character 'a' into the input fields, we will show them visually that their input is incorrect and do not actually mask the incorrect input 'a').

@StephenLPeters @niels9001 Your thoughts?

niels9001 commented 3 years ago

@Felix-Dev Agreed :)! Thanks for the elaborate explanation.

I guess we can use the same validation message for Narrator to call out when selected?

mdtauk commented 3 years ago

Validation would have to be linked to the current value mode of the box.

Hex would need 6/8 digits allowed, with 0-9 A-F all being supported. (ARGB / RGBA as Hex)

But if the value were numerical digits only, A-F would not be valid - and then for R G B numberboxes, 255 would be the max value, and anything above that would be invalid.

crutkas commented 3 years ago

Note in PowerToys, we got flagged for accessibility on this.

robloo commented 3 years ago

Changing these input boxes to NumberBox was always the plan:

https://github.com/microsoft/microsoft-ui-xaml/issues/483#issuecomment-500140203 https://github.com/microsoft/microsoft-ui-xaml/issues/483#issuecomment-500618794

crutkas commented 3 years ago

hi team, any eta for getting this accessibility bug fixed?

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 5 days.

crutkas commented 1 year ago

Ping