Accessibility: Add text labels to toolbar buttons #1224
Conversation
- Adds text labels and tooltips to the toolbar buttons in the graph editor (macOS)
- Adds text labels and tooltips to the toolbar buttons in the projects view (macOS)
- Adds text labels to the toolbar buttons in the projects view and in the graph editor (iPadOS) - Removes code for rotating the “Restart Prototype” button’s icon (replace with a symbol effect in the future)
- Updates the label for the “Go to Node” toolbar button on iPadOS for consistency with macOS
|
@JohJakob thanks a ton for putting something together here! I just synced with the team, I think we can proceed forward with these changes if we do tooltips instead of permanent text labels. Hopefully this is still good accessible design, or at least an improvement. We see other apps doing this, like so: 
Otherwise my bet is you're running into some of the many Catalyst roadblocks we've hit. Let us know your thoughts here and if we can help in any way. HUGE thank you this PR ❤️ |
| iPadNavBarButton(title: "Go Up", iconName: .sfSymbol(.GO_UP_ONE_TRAVERSAL_LEVEL_SF_SYMBOL_NAME), tooltip: "") { dispatch(GoUpOneTraversalLevel()) | ||
| } | ||
| .disabled(!hasActiveGroupFocused) |
There was a problem hiding this comment.
I think the preference here is to actually hide the group traversal button if at the root hierarchy—I forget if .disabled just disables a visible button.
There was a problem hiding this comment.
Yes, .disabled() keeps the button visible. I prefer that to hiding the button, but that’s a personal preference. I think it is generally better to display all potential actions and not hide and show interactive elements conditionally.
I’m open to changing it back to the previous behaviour (with the addition of making it invisible to assistive technology as well since setting the opacity to 0 still keeps it in the accessibility tree).
There was a problem hiding this comment.
Let's leave your changes in for now! That'll allow us to get a feel for the change here. I'm definitely not against it.
| } primaryAction: { | ||
| action() | ||
| } | ||
| .rotation3DEffect(Angle(degrees: rotationZ), |
There was a problem hiding this comment.
I believe this rotation call impacts prototype restart icon on iPad—doesn't work on Catalyst. However let me know if removing this functionality enables better accessible design here.
There was a problem hiding this comment.
This removal doesn’t affect accessibility so perhaps it’s a bit out of scope here. I guess the wonky rotation just bothered me because it doesn’t rotate around the centre of the circle due to the arrowhead. In any case, using the rotation symbol effect would be a better solution.
There was a problem hiding this comment.
Haha yes totally agree it's a little wonky. I'd rather have some rotation than none, let's leave for now but definitely not against better animations here.
There was a problem hiding this comment.
Unfortunately, the whole animation stops working with the addition of the text label. What do you suggest?
There was a problem hiding this comment.
Got it. In that case let's leave it commented out and add a new comment explaining why it's disabled.
|
@ellbosch The text labels are not permanent. The user can choose between three toolbar display modes by doing a secondary click on the toolbar: Icon and Text, Icon Only, or Text Only. This is also the case in other apps like Mail. So this change adds the missing text labels for the actions that didn’t have a label before. Oh, and I forgot to mention that I also added tooltips (optional; if not provided, the label title will be shown as a tooltip). |
Oooooh nice!! This makes a lot of sense. We'll take a closer look at this PR and get back here with more feedback. |
|
I just pulled the branch and explored things more. It's honestly great. Some things to sort out first and then we can merge:
|
- Replaces the deprecated `.navigationBarLeading` toolbar item placement with `.topBarLeading`
- Replaces the project title text view with the attached `.onTapGesture()` modifier with a button for improved accessibility - Replaces the two document events `CatalystProjectTitleModalOpened` and `CatalystProjectTitleModalClosed` with a single `CatalystProjectTitleModalToggled` event
Oh, really? It compiles just fine over here. What errors do you get?
Unfortunately, that is impossible with the current implementation. I tried all sorts of tricks, but to no avail. I think the whole toolbar should be rewritten using
I shortened the labels except “Project Settings” because the distinction between the app’s settings and the project settings is pretty important. I have also replaced the project title text view (with an attached |
|
Alrighty @JohJakob we're real close to merging here. I just made a quick commit at Please give us a ping once the conflicts are resolved!! Thanks so much for your help 😍 |
- Adds the previously removed code for rotating icons in the toolbar - Disables the icon rotation modifier
- Updates view calls
|
I fixed the merge conflicts. Hopefully, everything is there now. The toolbars look complete to me at least. Could you please check that I didn’t miss anything important, @ellbosch? That’s how it looks on Mac in the “Icon and Text” display mode (the Sidebar button gets pushed into the overflow menu if I’m not in fullscreen for some reason):
And on iPad:
|
2 participants
Changes
Closes #1221
Discussion
As you can see, the text labels are not really optimal. On macOS, they are way too subtle (looks like the disabled state), but I was unable to change that. Creating the buttons using the hacky
Menutrick is also probably not sustainable. A properNSToolbarwould be a better solution for the future, I suppose. This would also allow for getting rid of the text labels beneath the title in the projects view and the document title in the graph editor.I was also unable to make the text labels change based on state (but it works for the icons?) so I chose state-independent labels like “Toggle Preview.”