-
Notifications
You must be signed in to change notification settings - Fork 421
Add “Quantity Spin Button” pattern example #3328
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
Conversation
|
Heya @mcking65, just putting this preliminary draft PR on your radar. I still have some linting errors to investigate and tests to write, but I think we could start doing some peer reviews on the premise, design, and approach of the Quantity Spin Button example itself. |
|
The ARIA Authoring Practices (APG) Task Force just discussed The full IRC log of that discussion<jugglinmike> Topic: PR 3328: Add Quantity Spin Button<jugglinmike> github: https://github.com//pull/3328 <jugglinmike> Adam_Page: So this kicked off a couple weeks ago when someone filed an issue. They challenged us on a WCAG-non-conforming target size with our spin button date picker example. And they also pointed out that the ARIA text suggests the use of text input <jugglinmike> Adam_Page: We decided to replace the example IT's a very traditional spin button that is a numeric field which captures an integer and includes "plus" and "minus" buttons to increment and decrement. It satisfies target size. A keyboard can tab into it and type in a value directly I took care to make sure that voice control works well. The "plus" and "minus" buttons are omitted from the document tab flow, but they are still present in t <jugglinmike> he accessibility tree <jugglinmike> Adam_Page: Also a little bit of high-contrast mode finesse <jugglinmike> Adam_Page: I think that covers it! I also updated the "accessibility features" <jugglinmike> Matt_King: So you also replaced the date-picker spin button on the "patterns" page <jugglinmike> Matt_King: With collapsible listbox, we deprecated an example. So we have an editorial precedent for deprecating examples. <jugglinmike> Matt_King: We could deprecate this or delete it entirely <jugglinmike> Matt_King: We deprecated that listbox example because we thought people might be using it <jugglinmike> Matt_King: In this case, though, I don't know if there's value in deprecating versus just deleting <jugglinmike> Matt_King: The only way to get to that deprecated listbox is via the index <jugglinmike> Matt_King: We changed the title to say "deprecated" so that the deprecation is apparent even when it appears in the index <jugglinmike> Matt_King: But the other option is just to remove <jugglinmike> Matt_King: I don't know which is better in this case <jugglinmike> Adam_Page: Me neither. I'm just learning about this deprecation pattern <jugglinmike> Matt_King: I kind of like having a record of deprecation even if we don't think people are using it <jugglinmike> Matt_King: It's not like we have a lot of deprecated content like this which is acting like baggage... <jugglinmike> Adam_Page: Yeah, that sounds good to me. I will restore it and add the deprecation message <jugglinmike> Matt_King: We'll review informally while you make that change, and assign formal reviews next time <jugglinmike> Adam_Page: Sounds good <jugglinmike> Matt_King: This is awesome. You delivered quickly and very comprehensively Thank you, Adam_Page! |
This reverts commit 0b150bf.
|
The ARIA Authoring Practices (APG) Task Force just discussed The full IRC log of that discussion<jugglinmike> Topic: PR 3328: Add Quantity Spin Button<jugglinmike> github: https://github.com//pull/3328 <jugglinmike> Adam_Page: After we chatted about this last week, I restored the "date picker" example and put a deprecation warning on it (and ensured that the depcration warning appears on the index page) <Jem> https://deploy-preview-424--aria-practices.netlify.app/aria/apg/patterns/spinbutton/examples/quantity-spinbutton/#example <jugglinmike> Adam_Page: I also wrote a whole bunch of tests. I would be very grateful for a review from someone who knows what they are doing because I haven't written many of this kind of Selenium test <jugglinmike> Matt_King: Did you write tests on roles and states and properties? <jugglinmike> Adam_Page: Yes <jugglinmike> jongund: I can review the tests <jugglinmike> Adam_Page: Awesome, thank you! <jugglinmike> howard-e: I can review, as well <jugglinmike> Adam_Page: What about content? Should I do an editorial review? <jugglinmike> Adam_Page: Yes, please. I haven't made any changes to copy since last week, but my initial push included a lot of copy <jugglinmike> Matt_King: Do you want to take it out of the "draft" state? <jugglinmike> Adam_Page: Sure <jugglinmike> Jem: How do I get the focus on the minus and plus? <jugglinmike> Adam_Page: You don't <jugglinmike> Adam_Page: It's because the inputs themselves are focusable. A sighted user would bring focus to the inputs and know that they could either type numbers or use their arrow keys <jugglinmike> Adam_Page: The minus and plus buttons are present more for mouse users and especially for touch users <jugglinmike> jongund: That's consistent with the previous example <jugglinmike> Matt_King: Doesn't the spec say something about not focusing the plus and minus buttons? <jugglinmike> Adam_Page: It does! I can't remember if it's phrased as "authors MUST" or "authors SHOULD" |
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.
Error Correction Feature
I think that we should only allow one digit for the 1-8 and 0-8 text inputs, and 2 digits for the 0-12 input. It seems that would be more realistic error suppression, when a non-number is typed and the number in the textbox is selected I think the character should be ignored, instead of clearing the box with no value. It seems there should always be a number in the textboxes, even if it is invalid. If you type more only the last one or two allowed numbers would show. I am worried that if we don't do this, we will have comments about the example being insufficient in some simple error correcting features.
- In the 1-8 input 0 and 9 should be ignored and only one digit allowed
- In the 0-8 input, the 9 should be ignored and only one digit allowed
- In the 0-12 input, only two digits allowed
Output Element Labeling and Visibility to Screen Readers
I know status role does not require a label, but they can have a label. Since there are 3 output elements should we have labels on them so when screen reader users are just reviewing the page to know what the numbers are related to? Since the output element have zero dimensions (e.g. invisible visually) do any screen readers even navigate to them? We may want to include some information about that in the accessibility features.
Otherwise example looks great, thanks Adam for all your work.
|
This is definitely not a blocker, but I noticed that if I back tab into the example, the first time, the add animal button is focused. It only happens the first time after page load and will not happen if you first forward tab through the spin buttons. To repro:
|
mcking65
left a comment
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 made some editorial suggestions.
In addition, I recommend we drop the aria-describedby relationship because it creates redundant speech when screen readers correctly support min, max, and errormessage.
For example, here is JAWS output when the adult spinner is set to 4:
Adults edit spin box
4
1 to 8 min 1 max 8
Note the above output includes min, max, and description.
Here is the output when it is set to 44:
invalid entry Adults edit spin box Has Error
44
Must be between 1 and 8 1 to 8
Note that the above output includes both the description and the error message.
Interesting that JAWS does not convey min and max when the value is set to 44. Is that a JAWS bug, a spin button bug, or a spin button "feature"?
In a sense, when the value is out of range, min and max are redundant given the fact that the error message conveys min and max. So, it would be kind of nice for screen reader users if we were to intentionally omit min and max when the error message is present, but we shouldn't omit min and max if they are required by the spec.
|
@jongund wrote:
We had it this way originally, and decided it is more user friendly to allow for any value to be typed in and then provide an error. It is quite difficult to edit fields that are so tightly restricted.
The accessibility features documents that the output elements are visually hidden and have content that persist for 2 seconds, which makes them essentially invisible to screen readers as well. This would be a good ARIA notify use case in the future. |
Co-authored-by: Matt King <[email protected]>
Co-authored-by: Matt King <[email protected]>
Co-authored-by: Matt King <[email protected]>
Co-authored-by: Matt King <[email protected]>
Co-authored-by: Matt King <[email protected]>
shirsha
left a comment
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.
Review completed with comments
|
The ARIA Authoring Practices (APG) Task Force just discussed The full IRC log of that discussion<jugglinmike> Topic: PR 3328: Add Quantity Spin Button<jugglinmike> github: https://github.com//pull/3328 <jugglinmike> Matt_King: jongund wrote a comment about limiting the input <jugglinmike> Matt_King: That's interesting because it had previously been limited <jugglinmike> jongund: I'm satisfied with the current solution--if you've discussed it, then that's fine with me <jugglinmike> Matt_King: I added several suggestions for editorial changes, and I think they're very straightforward <jugglinmike> Adam_Page: Yes, I reviewed them all and accepted them all <jugglinmike> Matt_King: Excellent <jugglinmike> Matt_King: One little thing I noticed: if you back-tab into it, then the plus sign gets focused <jugglinmike> Adam_Page: I couldn't reproduce that, but I'm on macOS without a screen reader running. I'm going to spend more time with it after this call <jugglinmike> Matt_King: It's a minor thing, though; I think we can merge without addressing it <jugglinmike> Matt_King: Because we have aria-valuemin and aria-valuemax specified, and the aria-describedby is giving the same information, they create redundant speech for screen readers that automatically announce descriptions <jugglinmike> Matt_King: So my suggestion is to remove the relationship <jugglinmike> Matt_King: I shared some examples of the speech that is generated as a result of having description, min, and max all specified <jugglinmike> Adam_Page: I asked a coworker of mine to kick the tires on this thing several days ago, and he gave me the same feedback <jugglinmike> Adam_Page: The one potential downside I see is that not all screen readers announce min and max (in particular, macOS VoiceOver does not) <jugglinmike> Adam_Page: For those cases, aria-describedby is the best way to deliver that information <jugglinmike> Matt_King: We test this in ARIA-AT, but I thought that VoiceOver was not announcing descriptions automatically... <jugglinmike> Adam_Page: They are, now--the last time I checked, aria-describedby is getting immediately announced <jugglinmike> Matt_King: I know Vispero has done a lot of work to get proper support of min and max on sliders, and that they would want all controls to have equivalent support <jugglinmike> Matt_King: I guess we should just press this issue with Apple <jugglinmike> Matt_King: If Apple wanted to push back on Vispero's position for some reason, then maybe we would want the APG to espouse the use of descriptions <jugglinmike> Adam_Page: I'm good with it. It feels quite comparable to aria-errormessage <jugglinmike> Matt_King: Okay. Do we want to give audiences guidance that is more durable? Or do we want to degrade the experience for some screen reader users in order to avoid the potential bug in some screen readers? <jugglinmike> Siri: Even for description, when I check, it says that we have to enable a setting--otherwise, it may be skipped <jugglinmike> Matt_King: Well, ARIA-AT definitely tests with defaults. So if that behavior isn't enabled by default, then it would be reported as a bug in that project <jugglinmike> Matt_King: This conversation is really similar to the "abbr" conversation. However, in this particular case, because interoperability is ongoing (and the conversations with screen reader vendors are active), I feel giving more feedback on the current behavior <jugglinmike> Adam_Page: As an APG reference, it does feel a bit provocative to have an input with visible help text that is plainly help text and to have them not related with aria-describedby. <jugglinmike> Adam_Page: I would like to explicitly explain that choice <jugglinmike> Matt_King: Yeah, you can do that within the existing bullet point that your patch is including <jugglinmike> Adam_Page: I can complete this by tomorrow morning <jugglinmike> Matt_King: We'll put in a "publication" pull request tomorrow, but if it doesn't happen until Thursday, then that doesn't matter <jugglinmike> Daniel: Sure, a few days is not a problem <jugglinmike> Matt_King: I'm okay even if we submit the pull request on Thursday <jugglinmike> howard-e: Tomorrow or Thursday will both work for me <jugglinmike> Matt_King: Okay, so Adam_Page will make the changes, then I will review it, then once everything is good, I will merge and let howard-e know that everything is ready for publication <jugglinmike> Daniel: It's reading the 1 to 8 hint. I'm in macOS Tahoe. Since you're mapping, this behavior doesn't surprise me. <jugglinmike> Matt_King: Daniel, you clearly have that setting for speaking descriptions enabled. Do you know if you proactively turned it on? <jugglinmike> Daniel: I did. I don't think it's the default <jugglinmike> Matt_King: Okay! This is going to be a really good discussion to have with Apple. Hopefully we'll be aligned! |
|
Heya @mcking65, I’ve just removed the Here’s the updated prose I wrote for the Accessibility Features section — please let me know what you think. As soon as the cspell fix in #3365 is merged, I’ll pull it into this PR to prepare for publishing. |
howard-e
left a comment
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.
@adampage 2 minor issues with the test and left as inline suggestions. Other than that, all the updated code and test changes look good to me!
Co-authored-by: Howard Edwards <[email protected]>
Co-authored-by: Howard Edwards <[email protected]>
Co-authored-by: Howard Edwards <[email protected]>
Co-authored-by: Howard Edwards <[email protected]>
|
Got em — thanks very much, @howard-e! |
mcking65
left a comment
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.
@adampage
Looks awesome! Let's ship it!
|
Thank you, thank you @adampage!!!!!!!!!! :) |
Resolves #3261.
Also resolves #2241, #2238, #1837, #1655, and #1188.
WAI Preview Link (Last built on Thu, 18 Sep 2025 01:20:58 GMT).