Skip to content

Add longPress/Tap event to SemanticService - #16945

Merged
jonahwilliams merged 20 commits into
flutter:masterfrom
jonahwilliams:checkboxValues
May 3, 2018
Merged

jonahwilliams merged 20 commits into
flutter:masterfrom
jonahwilliams:checkboxValues

Conversation

@jonahwilliams

@jonahwilliams jonahwilliams commented Apr 25, 2018 •

Copy link
Copy Markdown
Contributor

Engine implementation: flutter-team-archive/engine#5081
Partial fix of #14187

Creates a new a11y event sent along with a haptic feedback request on Android when a longPress or inkWell tap event is sent. Also sends the semantic tap event on the toggleable update.

@jonahwilliams jonahwilliams changed the title Add longPress event to SemanticService Add longPress/Tap event to SemanticService Apr 25, 2018
@Hixie

Hixie commented Apr 27, 2018

Copy link
Copy Markdown
Contributor

tests?

cc @goderbauer


/// An event which triggers long press semantic feedback.
///
/// Only used on Android.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe change to: "currently only honored on Android" and say a word about how android typically reacts to these events?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Update doc on the semantics event themselves


/// An event which triggers tap semantic feedback.
///
/// Only used on Android.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

same here and in the other file below.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

same as above

/// * [wrapForTap] to trigger platform-specific feedback before executing a
/// [GestureTapCallback].
static Future<Null> forTap(BuildContext context) async {
SemanticsService.tap();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

From the BuildContext you could figure out the renderObject and form there you could probably get to the ID of the semantics node that this tap event is for. If you include that ID in the SemanticsEvent, can we then use that ID on the engine side instead of the id of the currently focused node? The focus might have changed between triggering the tap and processing the tap event. This ID would be stable.

@jonahwilliams jonahwilliams Apr 30, 2018 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This only works if the RenderObject is a semantics boundary, otherwise the actual RenderObject with a SemanticsNode may be an arbitrary child or parent of the object in question.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Semantics information always flow up in the tree (with the exception of maybe traversal order) and they can't really "jump over" a semantics node. So, is it not always the first ancestor renderObject with a semantic node that you're looking for?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can try traversing the tree upwards in that case - will update with those results.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have a somewhat hacky solution - it requires accessing the private _semantics member of RenderObject though, I don't see a way around this.

@goderbauer

Copy link
Copy Markdown
Member

Also +1 to tests :)

@jonahwilliams

Copy link
Copy Markdown
Contributor Author

Added some tests

@jonahwilliams

Copy link
Copy Markdown
Contributor Author

Updated RenderObject with a sendSematicsEvent API. This looks up the render object tree for the first non-null semantics node and sends the semantics event from this node.

@jonahwilliams

Copy link
Copy Markdown
Contributor Author

@goderbauer friendly ping!

@goderbauer goderbauer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

///
/// See [SemanticsNode.sendEvent] for a full description of the behavior.
void sendSemanticsEvent(SemanticsEvent semanticsEvent) {
if (_semantics != null) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this return right away if semantics are disabled?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done


/// An event which triggers long press semantic feedback.
///
/// Currently only honored on Android. Triggers a long-press specific sound

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: remove extra white-space after .?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done


/// An event which triggers tap semantic feedback.
///
/// Currently only honored on Android. Triggers a tap specific sound when

@goderbauer goderbauer May 2, 2018 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: extra whitespace.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

'type': 'tap',
'nodeId': object.debugSemantics.id,
'data': <String, dynamic>{},
});

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Bonus points for asserting that the node with id object.debugSemantics.id was actually annotated with the click action for the checkbox in the semantics tree.

Same might be useful for some of the other tests.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done for all tests except tooltip

@goderbauer goderbauer left a comment

Copy link
Copy Markdown
Member

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 50bd39a into flutter:master May 3, 2018
@jonahwilliams
jonahwilliams deleted the checkboxValues branch May 3, 2018 18:04
cbracken added a commit to cbracken/flutter that referenced this pull request May 3, 2018
Also reverts attempted fix:
* fix names of demos in perf tests (flutter#17257)

Also reverts:
* Add longPress/Tap event to SemanticService (flutter#16945)

This reverts commit 22c12f9.
This reverts commit 40ddf01.
This reverts commit 50bd39a.
DaveShuckerow pushed a commit to DaveShuckerow/flutter that referenced this pull request May 14, 2018
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Aug 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants