[A11y] Allow percentage strings like "50%" as SemanticsValue for ProgressIndicator#183670
Conversation
There was a problem hiding this comment.
Code Review
This pull request enhances accessibility by allowing descriptive strings for SemanticsValue in ProgressIndicator widgets. The implementation correctly modifies the validation logic to use double.tryParse, preventing crashes with non-numeric values. The added tests confirm this new behavior. My feedback includes suggestions to make these tests more comprehensive by asserting the minValue and maxValue properties, ensuring the semantics data is fully validated.
| // Validate that the value is within the min and max range if all values are | ||
| // valid numbers. If the value is a descriptive string (e.g., "50%"), skip | ||
| // numeric validation. | ||
| final double? currentValue = double.tryParse(data.value); |
There was a problem hiding this comment.
can you double check what would happen in web when the value contains non numerical string?
There was a problem hiding this comment.
on web if the value is set to "randomstring" it will announce "0% ,randomstring" , it's not ideal.
There was a problem hiding this comment.
I think we can do maybe:
[SemanticsValue] support a double or a percentage string like "50%", and if it's a percentage string we change it to a double for web.
There was a problem hiding this comment.
post processing like this may be confusing. I would rather throw and let customer figure out.
There was a problem hiding this comment.
do we only throw for web or throw for all platform?
There was a problem hiding this comment.
I was reading slider when trying to adding a slider role.
For slider,
we set semantics value to XX% for both material and cupertino, https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/material/slider.dart#L1938
And doing some post processing in web: https://github.com/flutter/flutter/blob/master/engine/src/flutter/lib/web_ui/lib/src/engine/semantics/incrementable.dart#L95-L100
I think we can do the same thing for progressbar to support XX%
SemanticsValue for ProgressIndicatorSemanticsValue for ProgressIndicator
fbf210f to
0d76183
Compare
|
An existing Git SHA, To re-trigger presubmits after closing or re-opeing a PR, or pushing a HEAD commit (i.e. with |
a7d0d03 to
ec93484
Compare
SemanticsValue for ProgressIndicatorSemanticsValue for ProgressIndicator
This comment was marked as resolved.
This comment was marked as resolved.
| final double? min = double.tryParse(semanticsObject.minValue ?? ''); | ||
| final double? max = double.tryParse(semanticsObject.maxValue ?? ''); | ||
| if (min != null && max != null) { | ||
| final double calculatedValue = min + (percentage / 100.0) * (max - min); |
There was a problem hiding this comment.
is this a use case backing this? the developer sets min and max and use % in value? I don't think we do this by default? If the this is set by user, would be better to just throw or ignore them and use which ever value store in the progress indicator, since we will be introducing a failure of value for them.
There was a problem hiding this comment.
for slider, we support setting min and max as double and use % in value.
for progress bar, we dont support setting min and max as double and use % in value.
I think we can make progress bar and slider consistent.
the use case was raised in #182491
There was a problem hiding this comment.
do we have safe guard if the value and semantics value doesn't match? for example the value is 60, but the semantics value is 45%. If not, whats should get priority over the other.
There was a problem hiding this comment.
we don't have a safe guard for that.
but we also don't have a safe guard for number values, if the value is 60 and but the developer set the semantic value to 70. we will announce 70.
| setAttribute('aria-valuenow', calculatedValue.toString()); | ||
| } | ||
| } | ||
| setAttribute('aria-valuetext', value); |
There was a problem hiding this comment.
if we set this, do we still need valuenow?
There was a problem hiding this comment.
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Attributes/aria-valuetext says valuetext is only need if it's some meaningful info on the top of valuenow. For example, a battery meter value might be conveyed as aria-valuetext="8% (34 minutes) remaining".
since it's only for custom text like that we can only set valuenow, not valuetext. I will remove the valuetext.
There was a problem hiding this comment.
it almost feels like we need another semantics property for this, it feels a bit troublsome for both customer and our code to make sense of custom value string and how they apply to these property. If almost feels like we need a separate semantics property for this.
just thowing out idea, what if we set the hint to valuetext?
…ue` for `ProgressIndicator` (flutter/flutter#183670)
…ue` for `ProgressIndicator` (flutter/flutter#183670)
…ue` for `ProgressIndicator` (flutter/flutter#183670)
…ue` for `ProgressIndicator` (flutter/flutter#183670)
…ue` for `ProgressIndicator` (flutter/flutter#183670)
…ue` for `ProgressIndicator` (flutter/flutter#183670)
…ue` for `ProgressIndicator` (flutter/flutter#183670)
…ue` for `ProgressIndicator` (flutter/flutter#183670)
flutter/flutter@9cd60b5...a0924c7 2026-04-07 dacoharkes@google.com Reland "[data_assets] Cleanup tests" (flutter/flutter#184714) 2026-04-07 matt.kosarek@canonical.com Use the WindowRegistry in the multiple_windows example app (flutter/flutter#184579) 2026-04-07 15619084+vashworth@users.noreply.github.com Introduce command to build a swift package for SwiftPM add to app integration (flutter/flutter#184660) 2026-04-07 sigurdm@google.com Have `flutter create` create a pubspec.lock to ensure pinned versions are being used. (flutter/flutter#175352) 2026-04-07 59215665+davidhicks980@users.noreply.github.com [widgets/raw_menu_anchor.dart] Always call onClose and onCloseRequested on descendants before parent. (flutter/flutter#182357) 2026-04-07 rmolivares@renzo-olivares.dev `WindowsPlugin` should not crash when ffiPlugin enabled (flutter/flutter#184695) 2026-04-06 97480502+b-luk@users.noreply.github.com Use full goto.google.com hostname for go/ links (flutter/flutter#184679) 2026-04-06 34871572+gmackall@users.noreply.github.com Apply rect clipping to surface views (flutter/flutter#184471) 2026-04-06 jhy03261997@gmail.com [A11y] Allow percentage strings like "50%" as `SemanticsValue` for `ProgressIndicator` (flutter/flutter#183670) 2026-04-06 louisehsu@google.com Fix invisible accessibility element before scroll view (flutter/flutter#184155) 2026-04-06 engine-flutter-autoroll@skia.org Roll Skia from 163dfdf500c7 to e264d870a380 (2 revisions) (flutter/flutter#184674) 2026-04-06 54688429+TrangLeQuynh@users.noreply.github.com Keep last character obscured when toggling obscureText (flutter/flutter#183488) 2026-04-06 15619084+vashworth@users.noreply.github.com Parse scheme file with XML parser for SwiftPM migrator (flutter/flutter#184525) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC bmparr@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…rogressIndicator` (flutter#183670) Fix: flutter#182491 So they can make the progress bar read "50%" instead of "0.5", "50" and provide a better user experience. ## Pre-launch Checklist - [ ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [ ] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [ ] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [ ] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [ ] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
…r#11463) flutter/flutter@9cd60b5...a0924c7 2026-04-07 dacoharkes@google.com Reland "[data_assets] Cleanup tests" (flutter/flutter#184714) 2026-04-07 matt.kosarek@canonical.com Use the WindowRegistry in the multiple_windows example app (flutter/flutter#184579) 2026-04-07 15619084+vashworth@users.noreply.github.com Introduce command to build a swift package for SwiftPM add to app integration (flutter/flutter#184660) 2026-04-07 sigurdm@google.com Have `flutter create` create a pubspec.lock to ensure pinned versions are being used. (flutter/flutter#175352) 2026-04-07 59215665+davidhicks980@users.noreply.github.com [widgets/raw_menu_anchor.dart] Always call onClose and onCloseRequested on descendants before parent. (flutter/flutter#182357) 2026-04-07 rmolivares@renzo-olivares.dev `WindowsPlugin` should not crash when ffiPlugin enabled (flutter/flutter#184695) 2026-04-06 97480502+b-luk@users.noreply.github.com Use full goto.google.com hostname for go/ links (flutter/flutter#184679) 2026-04-06 34871572+gmackall@users.noreply.github.com Apply rect clipping to surface views (flutter/flutter#184471) 2026-04-06 jhy03261997@gmail.com [A11y] Allow percentage strings like "50%" as `SemanticsValue` for `ProgressIndicator` (flutter/flutter#183670) 2026-04-06 louisehsu@google.com Fix invisible accessibility element before scroll view (flutter/flutter#184155) 2026-04-06 engine-flutter-autoroll@skia.org Roll Skia from 163dfdf500c7 to e264d870a380 (2 revisions) (flutter/flutter#184674) 2026-04-06 54688429+TrangLeQuynh@users.noreply.github.com Keep last character obscured when toggling obscureText (flutter/flutter#183488) 2026-04-06 15619084+vashworth@users.noreply.github.com Parse scheme file with XML parser for SwiftPM migrator (flutter/flutter#184525) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC bmparr@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Fix: #182491
So they can make the progress bar read "50%" instead of "0.5", "50" and provide a better user experience.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.