-
Notifications
You must be signed in to change notification settings - Fork 121
[POS Settings] Accessibility improvements #16068
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
[POS Settings] Accessibility improvements #16068
Conversation
When the header contains longer text that fits on screen, is cut to max 1 line only, we can remove the limit for larger sizes so all text is visible
|
|
…ents # Conflicts: # WooCommerce/Classes/POS/Presentation/Settings/PointOfSaleSettingsHardwareDetailView.swift
using the .plain style is unnecessary here, and causes an issue with visual feedback when these are tapped
|
One review is enough! |
joshheald
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.
Generally works well, thanks for the attention to detail.
I noticed a couple of things while testing:
POS modals don't block views underneath from VoiceOver
POS modals don't take over the accessibility context when presented here – I think they do when presented in the main view. You shouldn't be able to select views under the modal with VoiceOver, as it's confusing.
voiceover.behind.modals.mp4
This should be handled by POSModal already, so it's a bit strange: POSRootModalViewModifier has this code:
content
.blur(radius: modalManager.isPresented ? 8 : 0)
.disabled(modalManager.isPresented)
.accessibilityElement(children: modalManager.isPresented ? .ignore : .contain)
Perhaps it's something like .posRootModal() being set in a different part of the heirarchy than it is for the dashboard?
Store settings content can scroll under status bar
I've not checked why, but at larger sizes when you start being able to scroll the store details, they show up above the title, behind the status bar, which is a bit strange. Interestingly, it doesn't happen with Hardware. Perhaps it's a safe area issue?
store.info.scrolling.mp4
| }, | ||
| buttonIcon: "xmark")) | ||
| .foregroundColor(.posSurface) | ||
| .accessibilityAddTraits(.isHeader) |
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.
This means the x button is announced as a heading as well. Not a huge problem but a bit odd. Perhaps it would be better to update the internals of the POSPageHeaderView to have this trait just on the text – it saves us adding it elsewhere.
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.
Yes, let's update the POSPageHeaderView better, I've logged it on WOOMOB-1254 so can be tested separately
|
Thanks for the review Josh!
Good catch, I logged this one as WOOMOB-1253
Thanks, this is fixed here #16070 👍 |

Closes WOOMOB-1221
Closes WOOMOB-1229
Closes WOOMOB-1230
Description
This PR adds general accessibility improvements to POS Settings by updating
PointOfSaleSettingsViewand its child views by:.dynamicTypeSizefor Text.isHeaderand.isButtonPointOfSaleModalHeaderto not force .lineLimit(1) when using larger sizesUntitled-20250902.mov
Testing information