Add Semantics labels to loading indicators and update related tests #2025
Conversation
|
Thanks for the Pull Request, I can provide some insights on this PR as a contributor just like you. |
lib/widgets/action_sheet.dart
Outdated
| ? CircularProgressIndicator() | ||
| : Text( | ||
| ? Semantics( | ||
| label: 'Loading…', |
There was a problem hiding this comment.
Please make sure not to insert hardcoded strings and instead use the zulip localization to fetch those from app_en.arb, this helps zulip to make the app available to user across the globe with different languages.
lib/widgets/message_list.dart
Outdated
| child: const CircularProgressIndicator(), | ||
| ), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
nit:
| child: const CircularProgressIndicator(), | |
| ), | |
| ), | |
| ); | |
| child: const CircularProgressIndicator()))); |
See in this file (and other examples across the codebase).
lib/widgets/topic_list.dart
Outdated
| child: CircularProgressIndicator(), | ||
| ), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
nit:
| child: CircularProgressIndicator(), | |
| ), | |
| ), | |
| ); | |
| child: CircularProgressIndicator()))); |
lib/widgets/home.dart
Outdated
| children: [ | ||
| const CircularProgressIndicator(), | ||
| Semantics( | ||
| label: 'Loading…', |
lib/widgets/topic_list.dart
Outdated
| padding: const EdgeInsets.symmetric(vertical: 16.0), | ||
| child: Semantics( | ||
| textDirection: Directionality.of(context), | ||
| label: 'Loading…', // plain string (not localized) |
lib/widgets/message_list.dart
Outdated
| child: Padding( | ||
| padding: const EdgeInsets.symmetric(vertical: 16.0), | ||
| child: Semantics( | ||
| label: 'Loading more messages', |
There was a problem hiding this comment.
Use ZulipLocalization.
lib/widgets/message_list.dart
Outdated
| child: CircularProgressIndicator(), | ||
| ))); // TODO perhaps a different indicator |
There was a problem hiding this comment.
nit:
| child: CircularProgressIndicator(), | |
| ))); // TODO perhaps a different indicator | |
| child: CircularProgressIndicator()))); // TODO perhaps a different indicator |
|
Thanks for taking the time to review this! @MritunjayTiwari14 .I’ll implement the changes you recommended and update the branch. |
Add a localized 'loading' label in multiple widgets and apply it consistently across the UI. Update ARB files and regenerate localization outputs. Update widget tests to use the localized semantics label instead of hardcoded strings.
chrisbobbe
left a comment
chrisbobbe
left a comment
There was a problem hiding this comment.
A few comments below from a quick skim. I suggest doing another round of self-review (see the project README), checking your code style against existing code to polish it as best you can.
| focusable: true, | ||
| child: CircularProgressIndicator(), | ||
| ) | ||
| : Text( |
There was a problem hiding this comment.
The formatting of this code looks really messy; please follow the project's style in the use of spaces, indentation, newlines, etc. (here and throughout the PR).
lib/widgets/action_sheet.dart
Outdated
| Widget build(BuildContext context) { | ||
| final designVariables = DesignVariables.of(context); | ||
|
|
||
| final loc = ZulipLocalizations.of(context); |
There was a problem hiding this comment.
| final loc = ZulipLocalizations.of(context); | |
| final zulipLocalizations = ZulipLocalizations.of(context); |
(Like everywhere else we do this.)
lib/widgets/message_list.dart
Outdated
| if (!model.fetched) { | ||
| return Center( | ||
| child: Padding( | ||
| padding: const EdgeInsets.symmetric(vertical: 16.0), |
There was a problem hiding this comment.
Why add extra padding? That's not part of the issue.
lib/widgets/message_list.dart
Outdated
| padding: EdgeInsets.symmetric(vertical: 16.0), | ||
| child: CircularProgressIndicator())); // TODO perhaps a different indicator | ||
| child: Semantics( | ||
| textDirection: Directionality.of(context), |
There was a problem hiding this comment.
Why is textDirection needed? I'm surprised to see this.
| }, | ||
| "loading": "Loading…", | ||
| "@loading": { | ||
| "description": "Label shown for loading indicators" | ||
| } |
There was a problem hiding this comment.
nit: follow indentation style
assets/l10n/app_en.arb
Outdated
| }, | ||
| "loading": "Loading…", | ||
| "@loading": { | ||
| "description": "Label shown for loading indicators" |
There was a problem hiding this comment.
nit: "Label shown for loading indicators" is misleading; this isn't shown in the app's visual UI. Rather, it's information presented to screen-reader software, to help create an interface for users with vision loss etc.
How about: "Semantic label for a loading indicator."
|
I noticed you've pushed changes, but I don't know if you intend the PR to be ready for another review. When it's ready, please post a comment here in the discussion. Thanks! |
cybernova2
force-pushed
the
fix-semantics-accessibility
branch
from
ae634b1 to
0360488
Compare
|
Hi @chrisbobbe — I’ve resolved all merge conflicts, updated the branch, and verified the Semantics changes are applied correctly. |
3 participants
Fixes: #1962
This PR improves accessibility by adding
Semanticswidgets around loadingindicators so screen readers (TalkBack / VoiceOver) announce that the app is
loading content.
What’s changed
CircularProgressIndicator(and similar loading states) in:Semantics(label: 'Loading…', liveRegion: true)Why
Screen readers were not able to focus on or announce the loading state,
making the app inaccessible while fetching messages or topics.
Testing
-Verified that find.bySemanticsLabel(...) correctly locates the loading indicators.
-Ensured no regressions in navigation or layout.
-flutter analyze reports no new warnings.
Screenshot (TalkBack focus on loading indicator)
This shows the loading indicator now exposes an accessible label via
Semantics().Notes
this new PR contains only the intended accessibility changes.
Related
Replaces the earlier PR (#2017) with a clean version.