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.

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

Semantics framework updates #5601

Merged
merged 43 commits into from
Jul 20, 2018
Merged

Conversation

jonahwilliams
Copy link
Member

@jonahwilliams jonahwilliams commented Jun 23, 2018

  • Adds a semantic tag for images and support to iOS and Android.
  • Adds the ability to provide user labels to images with talkback on Android
  • Adds a semantic tag for live regions and support on Android.
  • Adds a semantic tag pair for hasToggledState/Toggled and support on android, map to checked on iOS
  • Adds dismissal semantic action on Android and iOS. (needed for snackbar)

Unfortunately the image semantics tag does not seem to effect iOS smart invert. I will need to more investigation into this.
Fixes flutter/flutter#18759

@jonahwilliams jonahwilliams changed the title [WIP] Add semantics flag for images [WIP] Semantics framework updates Jul 2, 2018
@@ -146,6 +147,14 @@ class SemanticsAction {
/// Accessibility focus and input focus can be held by two different nodes!
static const SemanticsAction didLoseAccessibilityFocus = const SemanticsAction._(_kDidLoseAccessibilityFocusIndex);

/// A request that the node should be dismissed.
///
/// A Snackbar, for example, may have a dismiss action to indicate to the user
Copy link
Contributor

Choose a reason for hiding this comment

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

[Snackbar]

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

/// A request that the node should be dismissed.
///
/// A Snackbar, for example, may have a dismiss action to indicate to the user
/// that it can removed after it is no longer relevant. on Android, TalkBack
Copy link
Contributor

Choose a reason for hiding this comment

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

on -> On

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

/// Whether the semantics node is a live region.
///
/// A live region indicates that updates to semantics node are important. Platforms
/// are free to use this information to make polite updates to the user to inform
Copy link
Contributor

Choose a reason for hiding this comment

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

remove "are free to", or maybe replace it with "typically" or some such. We're not giving them permission here.

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

@jonahwilliams
Copy link
Member Author

The change to the FlutterAppDelegate was necessary to build using the latest xcode beta

@jonahwilliams jonahwilliams changed the title [WIP] Semantics framework updates Semantics framework updates Jul 4, 2018
@jonahwilliams
Copy link
Member Author

Revert FltuterAppDelegate change, determined that it is not actually from iOS 8.0+

@jonahwilliams
Copy link
Member Author

WIP bot won't go away...

@jonahwilliams
Copy link
Member Author

Because of the issues that have come up with the implementation of live region, I am going to remove them from this PR and follow up with a better implementation later.

@jonahwilliams
Copy link
Member Author

I got everything working (on textfield, snackbar, and progress indicators).

Some explanation of the behavior and fixes needed:

When the semantics object is passed through the a11y bridge after being marked dirty, there is no corresponding accessibility node info created until 1) a TYPE_WINDOW_CONTENT_CHANGED event is dispatched with the id in question as the target or 2) touch exploration hits the parent semantics object.

When a TYPE_WINDOW_CONTENT_CHANGED is received where the target is already known to be a live region, a verbal announcement is made. But if the accessibility node hasn't been created yet, then no announcement.

So the fix is, I need to track Created, Dirty, and Clean states (done with an enum) so that I know when I need to send announcements. I've confirmed that this works as expected for text fields, progress indicators (where I imperatively send events), and snackbars *

  • snackbars require a further change where we disable animations when touch exploration is enabled. I will pipe that through a new window setting. The reason is when it is first created the text is not visible so the live region update does nothing

@jonahwilliams
Copy link
Member Author

PTAL @goderbauer

@jonahwilliams
Copy link
Member Author

OKAY, that is not whaat is happening. What is happening is a transparency effect on the snackbar means it has no text when it first appears

@jonahwilliams
Copy link
Member Author

Update text field handling - we will always mark it as a live region and trigger and update when the label (only) changes. On the framework side, we would need to ensure that 1) the semantics labels from the various affordances of the material text field are combined in a reasonable way and 2) the typing text doesn't trigger a secondary announcement (because this hides the hint).

Also during my testing of this I discovered two bugs in our current text field implementation - the dash in the phone number edit box is read out as a minus, and the password edit box is inexplicably doing math.

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.

The format changes make this really hard to review. Maybe do those in a separate PR if you're forced to do them?

@@ -248,7 +265,13 @@ class SemanticsFlag {

/// The semantics node has the quality of either being "checked" or "unchecked".
///
/// This flag is mutually exclusive with [hasToggledState].
Copy link
Member

Choose a reason for hiding this comment

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

We should enforce this with an assert on the framework side.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was planning to do this in the same place with handle the scopesRoute assert

private final FlutterView mOwner;
private boolean mAccessibilityEnabled = false;
private SemanticsObject mA11yFocusedObject;
private SemanticsObject mInputFocusedObject;
private SemanticsObject mHoveredObject;
private int previousRouteId = ROOT_NODE_ID;
private List<Integer> previousRoutes;
private String mPackageName;
Copy link
Member

Choose a reason for hiding this comment

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

What's this used for? It looks like it's only set in the constructor, but never 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.

I was using it to allow talkback to label images, but we actually need a stable id like the url or asset path also. Removed for now

@@ -118,22 +135,19 @@ public AccessibilityNodeInfo createAccessibilityNodeInfo(int virtualViewId) {
if (virtualViewId == View.NO_ID) {
AccessibilityNodeInfo result = AccessibilityNodeInfo.obtain(mOwner);
mOwner.onInitializeAccessibilityNodeInfo(result);
if (mObjects.containsKey(ROOT_NODE_ID))
result.addChild(mOwner, ROOT_NODE_ID);
if (mObjects.containsKey(ROOT_NODE_ID)) result.addChild(mOwner, ROOT_NODE_ID);
Copy link
Member

Choose a reason for hiding this comment

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

To be consistent with our other java code, I'd keep the line break after the if condition. Maybe add { } if it makes the formatter happy.

Here and all other ifs below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added back most of the {}, that seems to make clang format happy

@@ -146,6 +160,7 @@ public AccessibilityNodeInfo createAccessibilityNodeInfo(int virtualViewId) {
if (object.textSelectionBase != -1 && object.textSelectionExtent != -1) {
result.setTextSelection(object.textSelectionBase, object.textSelectionExtent);
}
result.setLiveRegion(View.ACCESSIBILITY_LIVE_REGION_POLITE);
Copy link
Member

Choose a reason for hiding this comment

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

This probably deserves an explaining comment for the next guy looking at this code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a comment

@jonahwilliams
Copy link
Member Author

Regarding formatting changes - I can't submit without them but in the future I will make sure it is done in a separate commit. I don't think it would be reasonable to make a separate initial PR just to reformat

@@ -602,6 +657,28 @@ void updateSemantics(ByteBuffer buffer, String[] strings) {
}
sendAccessibilityEvent(event);
}
if (object.hasFlag(Flag.IS_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.

Should we document that live region and text field are mutually exclusive?

@jonahwilliams
Copy link
Member Author

Made the updates as discussed offline - instead of tracking a Map of liveRegions and textFields I use has/had flag and a comparison of labels using a previousLabel field.

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

@@ -362,6 +368,7 @@ - (UIAccessibilityTraits)accessibilityTraits {
traits |= UIAccessibilityTraitAdjustable;
}
if ([self node].HasFlag(blink::SemanticsFlags::kIsSelected) ||
[self node].HasFlag(blink::SemanticsFlags::kIsToggled) ||
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a TODO that this doesn't give us the on/off announcements that we really want?

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

sendAccessibilityEvent(object.id, AccessibilityEvent.TYPE_WINDOW_CONTENT_CHANGED);
} else if (object.hasFlag(Flag.IS_TEXT_FIELD) && object.didChangeLabel()
&& mInputFocusedObject != null && mInputFocusedObject.id == object.id) {
sendAccessibilityEvent(object.id, AccessibilityEvent.TYPE_WINDOW_CONTENT_CHANGED);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a comment here explaining why this is here for textfield?

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

e.getText().add((String) data.get("message"));
sendAccessibilityEvent(e);
}
// Requires that the node id provided corresponds to a live region, or TalkBack will
// ignore the event.
Copy link
Member

Choose a reason for hiding this comment

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

Add: "The event will cause talkback to read out the new label even if node is not focused" or something like that?

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

@jonahwilliams jonahwilliams merged commit 96f5f5b into flutter:master Jul 20, 2018
@jonahwilliams jonahwilliams deleted the image_flag branch July 20, 2018 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants