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
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
eb006ca
initial implementation of a live region for Android and addition of s…
jonahwilliams Jun 12, 2018
5329a88
Update semantics.dart
jonahwilliams Jun 13, 2018
446621b
Add image flag for semantics
jonahwilliams Jun 23, 2018
87b2b01
Merge branch 'master' of github.com:flutter/engine into live_region
jonahwilliams Jun 27, 2018
f53d5e3
update doc comment
jonahwilliams Jun 27, 2018
e3a3c58
merge with origin
jonahwilliams Jun 27, 2018
fa7b778
add dismiss action
jonahwilliams Jul 1, 2018
65d55f1
merge with live region change
jonahwilliams Jul 1, 2018
cd9fa47
wip
jonahwilliams Jul 2, 2018
a82dd2f
Address comments
jonahwilliams Jul 2, 2018
38c62ae
remove unused live region code from ios
jonahwilliams Jul 2, 2018
91ea4f3
remove for real
jonahwilliams Jul 2, 2018
fca700a
Address review comments and fix compile error in xcode beta
jonahwilliams Jul 4, 2018
e5cec1c
Update FlutterAppDelegate.mm
jonahwilliams Jul 10, 2018
8915945
Merge branch 'master' of github.com:flutter/engine into image_flag
jonahwilliams Jul 10, 2018
61e4617
add toggleable flags
jonahwilliams Jul 10, 2018
7e060d2
Add toggle semantics flags and support in android, mapping to checked…
jonahwilliams Jul 10, 2018
37380f7
fix typos in semantics
jonahwilliams Jul 10, 2018
e21a2ae
move live region experiments
jonahwilliams Jul 11, 2018
d7bc1f6
remove automatic live region update
jonahwilliams Jul 11, 2018
fc98ca0
Merge branch 'master' of github.com:flutter/engine into image_flag
jonahwilliams Jul 11, 2018
4cc928a
clean up comments
jonahwilliams Jul 11, 2018
c83e15a
merge with master
jonahwilliams Jul 11, 2018
b75b257
address some comments
jonahwilliams Jul 11, 2018
8114b03
remove initial live region update, all updates handled by framework
jonahwilliams Jul 11, 2018
9169bf2
WIP
jonahwilliams Jul 11, 2018
2ed58bd
never mind they work for real
jonahwilliams Jul 12, 2018
4dbca6c
add comments
jonahwilliams Jul 12, 2018
fb13c0f
clean up comments
jonahwilliams Jul 12, 2018
1778dde
doc comment clean up
jonahwilliams Jul 12, 2018
99fef79
Update semantics.dart
jonahwilliams Jul 12, 2018
788e239
fix live regions once and for all
jonahwilliams Jul 13, 2018
c0d6d30
clang format
jonahwilliams Jul 14, 2018
f25ae65
Update semantics.dart
jonahwilliams Jul 14, 2018
17e60b3
Update semantics.dart
jonahwilliams Jul 14, 2018
20134b4
Merge branch 'master' of github.com:flutter/engine into image_flag
jonahwilliams Jul 17, 2018
89bfcc1
special case text field
jonahwilliams Jul 17, 2018
9efe1ac
address comments
jonahwilliams Jul 19, 2018
f32c555
remove unused import
jonahwilliams Jul 19, 2018
4249d62
make adjusted changes
jonahwilliams Jul 20, 2018
8f2df6a
Merge branch 'image_flag' of github.com:jonahwilliams/engine into ima…
jonahwilliams Jul 20, 2018
a1e0882
remember that equals and == are different in Java
jonahwilliams Jul 20, 2018
11ea454
Address comments
jonahwilliams Jul 20, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions lib/ui/semantics.dart
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ class SemanticsAction {
static const int _kPasteIndex = 1 << 14;
static const int _kDidGainAccessibilityFocusIndex = 1 << 15;
static const int _kDidLoseAccessibilityFocusIndex = 1 << 16;
static const int _kDismissIndex = 1 << 17;

/// The numerical value for this action.
///
Expand Down Expand Up @@ -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

/// 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

/// announces this after reading the label. On iOS, VoiceOver users can
/// perform a standard gesture to dismiss it.
static const SemanticsAction dismiss = const SemanticsAction._(_kDismissIndex);
Copy link
Member

Choose a reason for hiding this comment

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

How does TalkBack expose the dismiss action? Is this not just a custom action of the 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.

Copy link
Member

Choose a reason for hiding this comment

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

That link is for VoiceOver :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

this is what the ios MDC use for snackbar, for example


/// The possible semantics actions.
///
/// The map's key is the [index] of the action and the value is the action
Expand All @@ -168,6 +177,7 @@ class SemanticsAction {
_kPasteIndex: paste,
_kDidGainAccessibilityFocusIndex: didGainAccessibilityFocus,
_kDidLoseAccessibilityFocusIndex: didLoseAccessibilityFocus,
_kDismissIndex: dismiss,
};

@override
Expand Down Expand Up @@ -207,6 +217,8 @@ class SemanticsAction {
return 'SemanticsAction.didGainAccessibilityFocus';
case _kDidLoseAccessibilityFocusIndex:
return 'SemanticsAction.didLoseAccessibilityFocus';
case _kDismissIndex:
return 'SemanticsAction.dismiss';
}
return null;
}
Expand All @@ -228,6 +240,8 @@ class SemanticsFlag {
static const int _kScopesRouteIndex= 1 << 11;
static const int _kNamesRouteIndex = 1 << 12;
static const int _kIsHiddenIndex = 1 << 13;
static const int _kIsImageIndex = 1 << 14;
static const int _kIsLiveRegionIndex = 1 << 15;

const SemanticsFlag._(this.index);

Expand Down Expand Up @@ -366,6 +380,28 @@ class SemanticsFlag {
/// used to implement accessibility scrolling on iOS.
static const SemanticsFlag isHidden = const SemanticsFlag._(_kIsHiddenIndex);

/// Whether the semantics node represents an image.
///
/// Platforms have special behavior for images. TalkBack will inform the user
/// the labeled node is an image. iOS may use the image flag to avoid
Copy link
Member

Choose a reason for hiding this comment

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

Is the smart invert bit still correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, removed

/// inverting their color when using smart invert.
static const SemanticsFlag isImage = const SemanticsFlag._(_kIsImageIndex);

/// 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

/// them of this.
///
/// On Android, TalkBack will make a polite announcement of the first and
/// subsequent updates to the label of this node. This flag is not currently
Copy link
Member

Choose a reason for hiding this comment

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

The iOS a11y bridge does something with this flag. What is not supported about it?

/// supported on iOS.
///
/// An example of a live region is a [SnackBar] widget. When it appears
/// on the screen it may be difficult to focus to read the value. A live
/// region causes a polite announcement to be generated automatically.
static const SemanticsFlag isLiveRegion = const SemanticsFlag._(_kIsLiveRegionIndex);

/// The possible semantics flags.
///
/// The map's key is the [index] of the flag and the value is the flag itself.
Expand All @@ -384,6 +420,8 @@ class SemanticsFlag {
_kScopesRouteIndex: scopesRoute,
_kNamesRouteIndex: namesRoute,
_kIsHiddenIndex: isHidden,
_kIsImageIndex: isImage,
_kIsLiveRegionIndex: isLiveRegion,
};

@override
Expand Down Expand Up @@ -417,6 +455,10 @@ class SemanticsFlag {
return 'SemanticsFlag.namesRoute';
case _kIsHiddenIndex:
return 'SemanticsFlag.isHidden';
case _kIsImageIndex:
return 'SemanticsFlag.isImage';
case _kIsLiveRegionIndex:
return 'SemanticsFlag.isLiveRegion';
}
return null;
}
Expand Down
3 changes: 3 additions & 0 deletions lib/ui/semantics/semantics_node.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ enum class SemanticsAction : int32_t {
kPaste = 1 << 14,
kDidGainAccessibilityFocus = 1 << 15,
kDidLoseAccessibilityFocus = 1 << 16,
kDismiss = 1 << 18,
};

const int kScrollableSemanticsActions =
Expand All @@ -59,6 +60,8 @@ enum class SemanticsFlags : int32_t {
kScopesRoute = 1 << 11,
kNamesRoute = 1 << 12,
kIsHidden = 1 << 13,
kIsImage = 1 << 14,
kIsLiveRegion = 1 << 15,
};

struct SemanticsNode {
Expand Down
56 changes: 53 additions & 3 deletions shell/platform/android/io/flutter/view/AccessibilityBridge.java
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import android.opengl.Matrix;
import android.os.Build;
import android.os.Bundle;
import android.content.Context;
import android.util.Log;
import android.view.View;
import android.view.accessibility.AccessibilityEvent;
Expand Down Expand Up @@ -41,13 +42,15 @@ class AccessibilityBridge extends AccessibilityNodeProvider implements BasicMess
private static final int ROOT_NODE_ID = 0;

private Map<Integer, SemanticsObject> mObjects;
private Map<Integer, String> mLiveRegions;
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


private final BasicMessageChannel<Object> mFlutterAccessibilityChannel;

Expand All @@ -68,7 +71,8 @@ enum Action {
CUT(1 << 13),
PASTE(1 << 14),
DID_GAIN_ACCESSIBILITY_FOCUS(1 << 15),
DID_LOSE_ACCESSIBILITY_FOCUS(1 << 16);
DID_LOSE_ACCESSIBILITY_FOCUS(1 << 16),
DISMISS(1 << 17);

Action(int value) {
this.value = value;
Expand All @@ -91,7 +95,9 @@ enum Flag {
IS_OBSCURED(1 << 10),
SCOPES_ROUTE(1 << 11),
NAMES_ROUTE(1 << 12),
IS_HIDDEN(1 << 13);
IS_HIDDEN(1 << 13),
IS_IMAGE(1 << 14),
IS_LIVE_REGION(1 << 15);

Flag(int value) {
this.value = value;
Expand All @@ -104,9 +110,11 @@ enum Flag {
assert owner != null;
mOwner = owner;
mObjects = new HashMap<Integer, SemanticsObject>();
mLiveRegions = new HashMap<Integer, String>();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe document what this maps to?

previousRoutes = new ArrayList<>();
mFlutterAccessibilityChannel = new BasicMessageChannel<>(owner, "flutter/accessibility",
StandardMessageCodec.INSTANCE);
mPackageName = owner.getContext().getPackageName();
}

void setAccessibilityEnabled(boolean accessibilityEnabled) {
Expand Down Expand Up @@ -182,6 +190,16 @@ public AccessibilityNodeInfo createAccessibilityNodeInfo(int virtualViewId) {
if (object.hasFlag(Flag.IS_BUTTON)) {
result.setClassName("android.widget.Button");
}
if (object.hasFlag(Flag.IS_IMAGE)) {
result.setClassName("android.widget.ImageView");
// conform to the expected id from TalkBack's CustomLabelManager.
// talkback/src/main/java/labeling/CustomLabelManager.java#L525
result.setViewIdResourceName(mPackageName + ":id/" + Integer.toString(virtualViewId));
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 setting the ID give us?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed for now. If we could find a stable id per image, then a user could provide a custom label to an unlabeled image. Not the most important feature in the world but nice to support

}
if (object.hasAction(Action.DISMISS)) {
result.setDismissable(true);
result.addAction(AccessibilityNodeInfo.ACTION_DISMISS);
}

if (object.parent != null) {
assert object.id > ROOT_NODE_ID;
Expand Down Expand Up @@ -230,6 +248,7 @@ public AccessibilityNodeInfo createAccessibilityNodeInfo(int virtualViewId) {
}
}
if (object.hasAction(Action.INCREASE) || object.hasAction(Action.DECREASE)) {
// TODO(jonahwilliams): support AccessibilityAction.ACTION_SET_PROGRESS once SDK is updated.
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 just copy the Action's ID like we did for ACTION_SHOW_ON_SCREEN to make this work now? Also, how is that action exposed to the user?

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 is exposed as a special action in the local context menu - but since it is a newer action it doesn't have a hardcoded resource id, just a static field

result.setClassName("android.widget.SeekBar");
if (object.hasAction(Action.INCREASE)) {
result.addAction(AccessibilityNodeInfo.ACTION_SCROLL_FORWARD);
Expand All @@ -238,7 +257,12 @@ public AccessibilityNodeInfo createAccessibilityNodeInfo(int virtualViewId) {
result.addAction(AccessibilityNodeInfo.ACTION_SCROLL_BACKWARD);
}
}

if (object.hasFlag(Flag.IS_LIVE_REGION)) {
result.setLiveRegion(View.ACCESSIBILITY_LIVE_REGION_POLITE);
} else if (object.hadFlag(Flag.IS_LIVE_REGION)) {
mLiveRegions.remove(object.id);
Copy link
Member

Choose a reason for hiding this comment

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

Why is it not added to mLiveRegions in the if branch?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately I had a bug and it was covering up why this wasn't working. We can only get a live region update after Talkback is aware of the fact that the node in question is a live region. Because I was mistakenly not marking it sent after the first pass, a later build would trigger the update correctly.

}

boolean hasCheckedState = object.hasFlag(Flag.HAS_CHECKED_STATE);
result.setCheckable(hasCheckedState);
if (hasCheckedState) {
Expand Down Expand Up @@ -390,6 +414,9 @@ public boolean performAction(int virtualViewId, int action, Bundle arguments) {
mOwner.dispatchSemanticsAction(virtualViewId, Action.PASTE);
return true;
}
case AccessibilityNodeInfo.ACTION_DISMISS: {
mOwner.dispatchSemanticsAction(virtualViewId, Action.DISMISS);
Copy link
Member

Choose a reason for hiding this comment

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

return true?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

}
}
return false;
}
Expand Down Expand Up @@ -598,6 +625,14 @@ void updateSemantics(ByteBuffer buffer, String[] strings) {
sendAccessibilityEvent(selectionEvent);
}
}
if (object.hasFlag(Flag.IS_LIVE_REGION)) {
String priorLabelHint = mLiveRegions.get(object.id);
String labelHint = object.getLabelHint();
mLiveRegions.put(object.id, labelHint);
if (priorLabelHint == null || !priorLabelHint.equals(labelHint)) {
sendAccessibilityEvent(object.id, AccessibilityEvent.TYPE_WINDOW_CONTENT_CHANGED);
}
}
}
}

Expand Down Expand Up @@ -705,6 +740,7 @@ private void createWindowChangeEvent(SemanticsObject route) {
private void willRemoveSemanticsObject(SemanticsObject object) {
assert mObjects.containsKey(object.id);
assert mObjects.get(object.id) == object;
mLiveRegions.remove(object.id);
object.parent = null;
if (mA11yFocusedObject == object) {
sendAccessibilityEvent(mA11yFocusedObject.id, AccessibilityEvent.TYPE_VIEW_ACCESSIBILITY_FOCUS_CLEARED);
Expand Down Expand Up @@ -1061,6 +1097,20 @@ private float max(float a, float b, float c, float d) {
return Math.max(a, Math.max(b, Math.max(c, d)));
}

private String getLabelHint() {
StringBuilder sb = new StringBuilder();
if (label != null) {
sb.append(label);
}
if (sb.length() > 0) {
sb.append(", ");
}
if (hint != null) {
sb.append(hint);
}
return sb.length() > 0 ? sb.toString() : null;
}

private String getValueLabelHint() {
StringBuilder sb = new StringBuilder();
String[] array = { value, label, hint };
Expand Down
13 changes: 13 additions & 0 deletions shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,13 @@ - (BOOL)accessibilityScroll:(UIAccessibilityScrollDirection)direction {
return YES;
}

- (BOOL)accessibilityPerformEscape {
if (![self node].HasAction(blink::SemanticsAction::kDismiss))
return NO;
[self bridge] -> DispatchSemanticsAction([self uid], blink::SemanticsAction::kDismiss);
return YES;
}

#pragma mark UIAccessibilityFocus overrides

- (void)accessibilityElementDidBecomeFocused {
Expand Down Expand Up @@ -356,6 +363,12 @@ - (UIAccessibilityTraits)accessibilityTraits {
if ([self node].HasFlag(blink::SemanticsFlags::kIsHeader)) {
traits |= UIAccessibilityTraitHeader;
}
if ([self node].HasFlag(blink::SemanticsFlags::kIsImage)) {
traits |= UIAccessibilityTraitImage;
}
if ([self node].HasFlag(blink::SemanticsFlags::kIsLiveRegion)) {
traits |= UIAccessibilityTraitUpdatesFrequently;
}
return traits;
}

Expand Down