Add link support in web accessibility #46117

chunhtai · 2023-09-20T19:33:56Z

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

chunhtai

Hi @yjbanov and @mdebbar can you take a look at this draft pr to see if this is the direction we want to go before I add more tests?

chunhtai · 2023-09-20T19:36:05Z

lib/web_ui/lib/src/engine/semantics/link.dart

+  }
+
+  /// The element used for link, i.e. `<a>`.
+  late DomHTMLElement anchorElement;


There are still some more work needed to wire up the semantics action to this element, but I would like to know whether this is the way we want to pursuit.

An alternative is to Make semanticsObject.element to be a directly, that way I can reuse all the RoleManager like Tappable or Focusable.

yjbanov · 2023-09-20T22:48:46Z

lib/web_ui/lib/src/engine/semantics/link.dart

+      ..height = '${semanticsObject.rect!.height}px';
+    semanticsObject.element.append(anchorElement);
+    anchorElement.setAttribute('href', '#');
+    anchorElement.addEventListener('focus',


Can you reuse the Focusable role manager instead of implementing custom focus management?

The focusable role manager will only work on SemanticsObject.element I think? If I want to do that, I will need to add a way to create an <a> as SemanticsObject.element instead of the <flt-semantics-node> that it hardcoded to create. I am fine with that, but it seems to divert from what we were doing with text_field.dart

Ah, in that case you might want AccessibilityFocusManager.

I updated the code to use RoleManager instead, PTAL

yjbanov · 2023-09-20T22:49:54Z

lib/web_ui/lib/src/engine/semantics/link.dart

+          EnginePlatformDispatcher.instance.invokeOnSemanticsAction(
+              semanticsObject.id, ui.SemanticsAction.didLoseAccessibilityFocus, null);
+        }));
+    anchorElement.addEventListener('click',


Similarly here, can we reuse the Tappable role manager?

yjbanov · 2023-09-20T22:50:48Z

lib/web_ui/lib/src/engine/semantics/semantics.dart

@@ -1501,6 +1510,7 @@ class SemanticsObject {
      PrimaryRole.image => ImageRoleManager(this),
      PrimaryRole.platformView => PlatformViewRoleManager(this),
      PrimaryRole.generic => GenericRole(this),
+      PrimaryRole.link => Link(this),


Let's keep the generic role last to underscore that it's the fallback role that we only use as a last resort.

chunhtai · 2023-09-27T20:31:39Z

a friendly bump

yjbanov · 2023-09-27T20:34:45Z

lib/web_ui/lib/src/engine/semantics/semantics.dart

+  void _updateElement(PrimaryRole role) {
+    switch (role) {
+      case PrimaryRole.link:
+        element = domDocument.createElement('a');


The element field is designed to be final, as it participates in the parent's DOM structure. Simply swapping the element field will result in a dangling element not attached to anywhere. Let's chat about what you are trying to do and find a better solution.

yjbanov · 2023-10-11T16:56:47Z

lib/web_ui/lib/src/engine/semantics/semantics.dart

@@ -437,6 +441,8 @@ abstract class PrimaryRoleManager {
  /// management intereferes with the widget's functionality.
  PrimaryRoleManager.blank(this.role, this.semanticsObject);

+  late final DomElement element = _createAndInitElement();


Does it need to be late if it is initialized immediately? late has runtime cost (it uses a null check behind-the-scenes) and complexity cost (it is initialized upon first access, which is less predictable than constructor initialization).

non-late final can only accept static function as its initializer. In this case, there isn't a way for sub class to inject their logic

Oh I see, you need to call this.createElement which not static. In that case late make sense. However, I'd still initialize it in the constructor. Otherwise, it gets initialized upon first read of the field, and since it's hard to predict when exactly that field is accessed we can get into race conditions, such as _createAndInitElement reading from semanticsObject that has changed its state after the constructor is finished. Can you move element = _createAndInitElement(); into the constructor body?

lib/web_ui/lib/src/engine/semantics/semantics.dart

lib/web_ui/test/engine/semantics/semantics_test.dart

yjbanov · 2023-10-19T19:28:46Z

lib/web_ui/lib/src/engine/semantics/link.dart

+  @override
+  DomElement createElement() {
+    final DomElement element = domDocument.createElement('a');
+    // TODO(chunhtai): Fill in the real link once the framework sends entire uri.


Is there a Github issue this can point to?

yjbanov · 2023-10-19T19:31:02Z

lib/web_ui/lib/src/engine/semantics/semantics.dart

+  @protected
+  DomElement createElement() => domDocument.createElement('flt-semantics');
+
+  DomElement _createAndInitElement() {


nit: actually, I'd still keep this static and pass all the information it needs to initialize the element (I think all it needs is the element created by createElement and semanticsObject). This way this method will not need access to this.

yjbanov

…136982) flutter/engine@d46933e...b27e1b3 2023-10-20 [email protected] Add link support in web accessibility (flutter/engine#46117) 2023-10-20 [email protected] [web] Support `flutterViewId` in platform view messages (flutter/engine#46891) 2023-10-20 [email protected] Fix async image loading issues in skwasm. (flutter/engine#47117) 2023-10-20 [email protected] Add option to save Impeller failure images in rendertests (flutter/engine#47142) 2023-10-20 [email protected] Roll Skia from b960e9140f56 to 9ffd5ef9a9ed (3 revisions) (flutter/engine#47167) 2023-10-20 [email protected] [macOS] Eliminate extraneous loadView calls (flutter/engine#47166) 2023-10-20 [email protected] Roll Dart SDK from ba96a157a8eb to 53fee35b299f (1 revision) (flutter/engine#47165) 2023-10-20 [email protected] [Impeller] GPU Tracer for GLES. (flutter/engine#47080) 2023-10-20 [email protected] Roll Skia from de628929015d to b960e9140f56 (2 revisions) (flutter/engine#47164) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

fixes flutter/flutter#134795 ## Pre-launch Checklist - [ ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [ ] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [ ] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests. - [ ] I updated/added relevant documentation (doc comments with `///`). - [ ] I signed the [CLA]. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat

github-actions bot added the platform-web Code specifically for the web engine label Sep 20, 2023

chunhtai commented Sep 20, 2023

View reviewed changes

chunhtai requested review from yjbanov and mdebbar September 20, 2023 19:36

yjbanov reviewed Sep 20, 2023

View reviewed changes

chunhtai force-pushed the issues/134795 branch from d346d35 to 9922887 Compare September 21, 2023 21:05

chunhtai requested a review from yjbanov September 21, 2023 21:43

chunhtai marked this pull request as ready for review September 21, 2023 21:43

chunhtai force-pushed the issues/134795 branch from 6675306 to 58c57b7 Compare September 27, 2023 20:31

yjbanov reviewed Sep 27, 2023

View reviewed changes

chunhtai force-pushed the issues/134795 branch 2 times, most recently from e1589f3 to 69fd141 Compare October 2, 2023 18:11

chunhtai requested a review from yjbanov October 2, 2023 18:27

yjbanov reviewed Oct 12, 2023

View reviewed changes

chunhtai force-pushed the issues/134795 branch from bc197b9 to 37126c3 Compare October 18, 2023 21:49

chunhtai requested a review from yjbanov October 18, 2023 21:54

yjbanov reviewed Oct 19, 2023

View reviewed changes

chunhtai added 5 commits October 20, 2023 10:35

Add link support

c3ae256

licenes

2969e65

format

bdac95d

update

ea8b7a5

update

1b71298

chunhtai force-pushed the issues/134795 branch from 37126c3 to 1b71298 Compare October 20, 2023 17:36

fix ci

7788013

chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 20, 2023

yjbanov approved these changes Oct 20, 2023

View reviewed changes

chunhtai merged commit b27e1b3 into flutter:main Oct 20, 2023

engine-flutter-autoroll mentioned this pull request Oct 20, 2023

Roll Flutter Engine from d46933e6772e to b27e1b38375b (9 revisions) flutter/flutter#136982

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 20, 2023

b27e1b383 Add link support in web accessibility (flutter/engine#46117)

b5a91c8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add link support in web accessibility #46117

Add link support in web accessibility #46117

chunhtai commented Sep 20, 2023

chunhtai left a comment •

edited

Loading

chunhtai Sep 20, 2023

yjbanov Sep 20, 2023

chunhtai Sep 20, 2023 •

edited by mdebbar

Loading

yjbanov Sep 22, 2023

chunhtai Sep 22, 2023

yjbanov Sep 20, 2023

yjbanov Sep 20, 2023

chunhtai commented Sep 27, 2023

yjbanov Sep 27, 2023

yjbanov Oct 11, 2023

chunhtai Oct 18, 2023 •

edited

Loading

yjbanov Oct 19, 2023

yjbanov Oct 19, 2023

yjbanov Oct 19, 2023

yjbanov left a comment

Add link support in web accessibility #46117

Add link support in web accessibility #46117

Conversation

chunhtai commented Sep 20, 2023

Pre-launch Checklist

chunhtai left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunhtai Sep 20, 2023 • edited by mdebbar Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunhtai commented Sep 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunhtai Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yjbanov left a comment

Choose a reason for hiding this comment

chunhtai left a comment •

edited

Loading

chunhtai Sep 20, 2023 •

edited by mdebbar

Loading

chunhtai Oct 18, 2023 •

edited

Loading