Added semanticsIdentifier to Text Widgets
#163843
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
@chunhtai @Renzo-Olivares I wonder if you guys have thoughts on this semantics change for Text. |
| @@ -0,0 +1,165 @@ | |||
| // Copyright 2014 The Flutter Authors. All rights reserved. | |||
There was a problem hiding this comment.
Can you put this in examples/api/lib/widgets ?
| child: Text.rich( | ||
| TextSpan( | ||
| text: 'Please open the ', | ||
| // semanticsIdentifier: 'please_open', |
There was a problem hiding this comment.
why do we comment these out?
There was a problem hiding this comment.
I forgot to restore after creating Appium screenshots. Fixed it now.
| @@ -126,18 +132,21 @@ List<InlineSpanSemanticsInformation> combineSemanticsInfo( | |||
| final List<InlineSpanSemanticsInformation> combined = <InlineSpanSemanticsInformation>[]; | |||
| String workingText = ''; | |||
| String workingLabel = ''; | |||
| String? workingIdentifier; | |||
| List<ui.StringAttribute> workingAttributes = <ui.StringAttribute>[]; | |||
| for (final InlineSpanSemanticsInformation info in infoList) { | |||
| if (info.requiresOwnNode) { | |||
| combined.add( | |||
| InlineSpanSemanticsInformation( | |||
There was a problem hiding this comment.
I am not sure how this was working before, if the first one in the infoList, won't we add an empty info here?
There was a problem hiding this comment.
You're right, there is no need to make changes to this method. It is working fine without it.
| @@ -154,12 +163,14 @@ List<InlineSpanSemanticsInformation> combineSemanticsInfo( | |||
| ); | |||
| } | |||
| workingLabel += effectiveLabel; | |||
| workingIdentifier = info.semanticsIdentifier; | |||
There was a problem hiding this comment.
This should always be null according to the doc, right? can you use assert instead?
There was a problem hiding this comment.
You're right, there is no need to make changes to this method. It is working fine without it.
| @@ -154,12 +163,14 @@ List<InlineSpanSemanticsInformation> combineSemanticsInfo( | |||
| ); | |||
| } | |||
| workingLabel += effectiveLabel; | |||
| workingIdentifier = info.semanticsIdentifier; | |||
| } | |||
| } | |||
| combined.add( | |||
| InlineSpanSemanticsInformation( | |||
There was a problem hiding this comment.
This is also some weird code, wouldn't we added am empty one if last one requiresOwnNode
There was a problem hiding this comment.
You're right, there is no need to make changes to this method. It is working fine without it.
| @@ -289,9 +289,14 @@ void main() { | |||
|
|
|||
| test('TextSpan computeSemanticsInformation', () { | |||
| final List<InlineSpanSemanticsInformation> collector = <InlineSpanSemanticsInformation>[]; | |||
| const TextSpan(text: 'aaa', semanticsLabel: 'bbb').computeSemanticsInformation(collector); | |||
| const TextSpan( | |||
| text: 'aaa', | |||
There was a problem hiding this comment.
Can you add a test for text widget with textspan that has semantics identifier and assert a semantics node with that identifier is created?
There was a problem hiding this comment.
Added a test to test/widgets/text_semantics_test.dart
|
looks like there is a formatting issue |
| @override | ||
| void initState() { | ||
| super.initState(); | ||
| WidgetsBinding.instance.addPostFrameCallback((_) { |
There was a problem hiding this comment.
This seems to be debug code, can you remove this?
|
Hi @chunhtai, I have addressed the remaining issues. Please check. Thanks! |
| @@ -92,7 +96,7 @@ class InlineSpanSemanticsInformation { | |||
| /// True if this configuration should get its own semantics node. | |||
| /// | |||
| /// This will be the case of the [recognizer] is not null, of if | |||
There was a problem hiding this comment.
nit: this paragraph should be: This will be the case if the [recognizer] is not null, or if [isPlaceholder] is true, or if [semanticsIdentifier] has a value.
There was a problem hiding this comment.
Fixed it, thank you!
a7aedff to
9fe5e62
Compare
|
Hi @chunhtai | @Renzo-Olivares , I see the "Google testing" workflow is failing. I am not sure how to resolve it or proceed with the merge. This is my first time contributing to flutter so please advise on how to merge it. Thanks! |
|
This breaks some internal code where it implements the abstract interface InlineSpanSemanticsInformation, the internal test will need to be migrated |
|
waiting for cl/734306543 |
|
autosubmit label was removed for flutter/flutter/163843, because - The status or check suite Mac_arm64 build_tests_3_4 has failed. Please fix the issues identified (or deflake) before re-applying this label. |
This PR aims to add
semanticsIdentifiertoTextand some of its internal objects to pass the semantics information for adding identifier to the semantics nodesFrom the issue filed at #163842, the following is a description of the problem.
The semantics identifier helps in uniquely identifying elements using UI automation tools like Appium, UIAutomator and XCUITests by setting identifiers that the screen readers cannot see but the said tools can. This is especially useful when working with a multi-lingual or multi-tenant app, where the element IDs need to be unique but the content can be different. The
Semanticswidget already has support for declaring it. However, theTextandText.richvariants only support settingsemanticsLabelwithout explicitly setting the identifiers. The widgets themselves can be wrapped with aSemanticswidget but it still does not cater for a rich text that can have multiple text spans, each containing unique lables and identifiers, and optionally gesture detectors for handling links.Consider the following UI for two different tenants:
Here, both the tenants utilise different strings to convey the same message. The structure of the message stays the same so the identifiers help in unifying the element identification process across the tenant apps in the automation tools without having to write another script for every other tenant.
Without the identifiers, the automation scripts require a rewrite per tenant to be able to successfully locate the element and even tap on the hyperlink.
With PR Changes
Appium Views
For the given sample code,
Text.rich Sample
With Identifier
Without Identifier Changes
Semantics Tree Dump
The followings are the semantics tree dump for both the cases
With Identifier
Without Identifier Changes
fixes #163842