Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Semantics framework updates #18758

Merged
merged 31 commits into from
Jul 24, 2018
Merged

Semantics framework updates #18758

merged 31 commits into from
Jul 24, 2018

Conversation

jonahwilliams
Copy link
Member

@jonahwilliams jonahwilliams commented Jun 23, 2018 •
edited
Loading

Requires flutter/engine#5601

Fixes #14468
Fixes #3815
Fixes #11140

Partial fix of #16673
Android fix of #16395
Android fix of #14378

Changes:

  • Move the SemanticsConfiguration update from RenderToggleable to each subclass, so that Switch can use toggled.
  • Add image, liveRegion, toggled properties to Semantics, SemanticsConfiguration, SemanticsNode
  • Added semanticsLabel and excludeFromSemantics to Image (the latter so that we avoid creating a semantics node)
  • Added onDismiss semantics action which maps to the modal escape on iOS and dismiss action on Android.
  • Added dismiss and liveRegion to snackbar widget
  • Updated custom painter semantics to handle image, liveRegion, toggle
  • Updated relevant tests to use correct flag/action

@jonahwilliams jonahwilliams changed the title [WIP] add semantics label, excludeFromSemantics to Image [WIP] Semantics framework updates Jul 2, 2018
@jonahwilliams jonahwilliams mentioned this pull request Jul 2, 2018
Closed
@jonahwilliams jonahwilliams changed the title [WIP] Semantics framework updates Semantics framework updates Jul 21, 2018
@goderbauer goderbauer self-requested a review July 24, 2018 00:56
goderbauer
goderbauer approved these changes Jul 24, 2018
View reviewed changes
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

packages/flutter/lib/src/semantics/semantics.dart
/// * [SemanticsFlag.image], for the flag this setting controls.
final bool image;

/// If non-null, whether the node should be considered a live region.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great doc comment!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌮

packages/flutter/lib/src/semantics/semantics.dart Outdated
@@ -693,7 +733,7 @@ class SemanticsProperties extends DiagnosticableTree {
/// vertically scrollable.
///
/// VoiceOver users on iOS can trigger this action by swiping down with three
/// fingers. TalkBack users on Android can trigger this action by swiping
/// fingers. TalkBack users on Android can trigger this action by swiping
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove extra white space?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

packages/flutter/lib/src/semantics/semantics_event.dart Outdated
@@ -133,3 +133,21 @@ class TapSemanticEvent extends SemanticsEvent {
@override
Map<String, dynamic> getDataMap() => const <String, dynamic>{};
}

/// An event which triggers an announcement of a live region.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... a polite announcement ... ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it didn't come through?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind. I see it's there now. Was looking at an old diff :(

packages/flutter/lib/src/semantics/semantics.dart

/// If non-null, whether the node should be considered a live region.
///
/// On Android, when a live region semantics node is first created TalkBack
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you document what iOS does when this is set?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

packages/flutter/lib/src/semantics/semantics_event.dart Outdated
/// An event which triggers an announcement of a live region.
///
/// This requires that the semantics node has already been marked as a live
/// region. Only Android (TalkBack) will make a verbal announcement, as long as
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does iOS do?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing :(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably spell that out in the doc comment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind. I see it's there now. Was looking at an old diff :(

goderbauer
goderbauer approved these changes Jul 24, 2018
View reviewed changes
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jonahwilliams jonahwilliams merged commit 810a29d into flutter:master Jul 24, 2018
@jonahwilliams jonahwilliams deleted the image_flag branch July 24, 2018 20:10
Hixie
Hixie reviewed Aug 18, 2018
View reviewed changes
packages/flutter/lib/src/rendering/proxy_box.dart
@@ -3136,6 +3137,9 @@ class RenderSemanticsAnnotations extends RenderProxyBox {
bool scopesRoute,
bool namesRoute,
bool hidden,
bool image,
bool liveRegion,
bool isSwitch,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this argument is ignored. was that a mistake?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not breaking anything - but it is entirely redundant. Will remove

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have a PR out to remove it

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Hixie Hixie Hixie left review comments

@goderbauer goderbauer goderbauer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@jonahwilliams @Hixie @goderbauer @googlebot