[url_launcher][web] Better support for semantics in the Link widget #6711

mdebbar · 2024-05-10T20:32:04Z

Better support for semantics.
Better support for clicks with a modifier key (e.g. cmd+click).
More robust handling of events (can now correctly handle events received in a different order).
Apply the target attribute on semantic links.
Improve tests.

To get the complete fix, the following engine PR is also needed (this PR doesn't strictly depend on the engine PR, but the fix won't be complete without it).

[web] Set the correct href on semantic links engine#52720

Fixes flutter/flutter#143164
Fixes flutter/flutter#146291

yjbanov · 2024-05-31T20:15:55Z

packages/url_launcher/url_launcher_web/lib/src/link.dart

@@ -42,13 +47,42 @@ class WebLinkDelegate extends StatefulWidget {
  WebLinkDelegateState createState() => WebLinkDelegateState();
 }

+extension on Uri {


nit: do extensions need docs?

Well, this is a "nameless" extension, which is private. So I don't think docs are needed, but I'll add docs to the method.

yjbanov · 2024-05-31T20:16:03Z

packages/url_launcher/url_launcher_web/lib/src/link.dart

@@ -42,13 +47,42 @@ class WebLinkDelegate extends StatefulWidget {
  WebLinkDelegateState createState() => WebLinkDelegateState();
 }

+extension on Uri {
+  String getHref() {


I wanted to make sure first that the code is starting to look good before I spend time on writing and re-writing docs.

yjbanov · 2024-05-31T20:17:41Z

packages/url_launcher/url_launcher_web/lib/src/link.dart

+
+    // In case an internal uri is given, the uri must be properly encoded
+    // using the currently used UrlStrategy.
+    return ui_web.urlStrategy!.prepareExternalUrl(toString());


Is this related to flutter/flutter#147857 by any chance?

Nope. That issue is purely a UrlStrategy and/or engine issue.

yjbanov · 2024-05-31T20:19:16Z

packages/url_launcher/url_launcher_web/lib/src/link.dart

 /// The link delegate used on the web platform.
 ///
 /// For external URIs, it lets the browser do its thing. For app route names, it
 /// pushes the route name to the framework.
 class WebLinkDelegateState extends State<WebLinkDelegate> {
  late LinkViewController _controller;

+  @visibleForTesting
+  late final String semanticIdentifier;


nit: we always use plural "semantics" even in singular context, e.g. SemanticsNode.

I keep jumping back and forth since I'm not sure if there's already a convention. I'll change all of them to plural.

yjbanov · 2024-05-31T20:21:55Z

packages/url_launcher/url_launcher_web/lib/src/link.dart

+  @override
+  void initState() {
+    super.initState();
+    semanticIdentifier = 'sem-id-${_nextSemanticsIdentifier++}';


I wonder if widget-specific identifiers should prefix their ids with their own names rather than the generic "sem". That it is a "semantics" and "ID" is already implied by the API through which this value flows. The useful extra information that could be provided here is that the ID belongs to a link widget. So maybe do 'link-${_nextSemanticsIdentifier++}'?

yjbanov · 2024-05-31T20:50:43Z

packages/url_launcher/url_launcher_web/lib/src/link.dart

+    // Why listen in the capture phase?
+    //
+    // To ensure we always receive the event even if the engine calls
+    // `stopPropagation`.


Why would the engine stop propagating the events? Seems like code smell in the engine, no?

For some reason, it already does here.

The problem is a combination of two things:

The engine is listening to keyboard events in the capture phase.

The engine is calling stopPropagation on the event when the framework handles it.

I wanted to make the Link widget robust in all kinds of scenarios, at least until we fix the above 2 issues.

yjbanov · 2024-05-31T20:54:26Z

packages/url_launcher/url_launcher_web/lib/src/link.dart

@@ -197,41 +343,99 @@ class LinkViewController extends PlatformViewController {
    //    - If `_hitTestedViewId` is set, it means the app triggered the link.


nit for line https://github.com/flutter/packages/pull/6711/files#diff-77a04b3b00b00a4480b7d5e75e089dde7cca99ccebcc4f43e0e25d2bba5346e0R338: typo in "therefor"

TIL; I always thought "therefor" and "therefore" were the same :)

yjbanov · 2024-05-31T20:59:25Z

packages/url_launcher/url_launcher_web/lib/src/link.dart

+/// Keeps track of the signals required to trigger a link.
+///
+/// Automatically resets the signals after a certain delay. This is to prevent
+/// the signals from getting stale.


This description is very generic, even though the class does something very specific. AFAICT this class is resolving some race where the viewId and the mouse event can come in at unpredictable times and in unpredictable order. The dart docs could talk about the specifics, and also explain how this race arises.

Good idea. I'll revisit this when I do the pass to fill in missing docs.

yjbanov · 2024-05-31T21:02:56Z

packages/url_launcher/url_launcher_web/lib/src/link.dart

-    _hitTestedViewId = null;
+    if (_triggerSignals.isReadyToTrigger) {
+      _triggerLink();
+    }


The pattern of this if block looks the same everywhere. Can we pass the _triggerLink function to LinkTriggerSignals and have it call it at the appropriate time automatically? It already has the knowledge of when it is appropriate to call it. LinkTriggerSignals will no longer need isReadyToTrigger (except maybe for testing).

There are cases where I also check for isValid first and do something else. But I think that also can be moved to the LinkTriggerSignals class. I like the idea so I'll take a stab at it.

yjbanov · 2024-05-31T21:11:21Z

packages/url_launcher/url_launcher_web/lib/src/link.dart

+    // We only want to handle clicks that land on *our* links, whether that's a
+    // platform view link or a semantics link.
+    final int? viewIdFromTarget = _getViewIdFromLink(target) ??
+        _getViewIdFromSemanticLink(target);


Does this work if <flutter-view> is inside shadow DOM? In embedded case in particular, we don't know how exactly a FlutterView is embedded. It could be inside a web component's shadow DOM. In that case event.target caught at window level will simply point to the web component. It won't tell you the exact element the mouse event happened on.

Another issue is that this relies on the semantic DOM internals, so it can easily break if the structure changes.

Can we use https://api.flutter.dev/flutter/semantics/SemanticsProperties/onTap.html instead?

Regarding <flutter-view> being inside shadow DOM, that's a very good point that I hadn't considered. I'll test it and fix it.

Regarding relying on the semantic DOM internals, I couldn't find a better way to implement it. Using onTap doesn't work because the children of the Link widget typically have their own onTap which absorbs the tap event so it never reaches the Link's onTap.

github-actions bot added platform-web p: url_launcher labels May 10, 2024

mdebbar mentioned this pull request May 16, 2024

[Web][A11y] Fix link onClick behavior on links flutter/engine#49984

Open

8 tasks

mdebbar force-pushed the link_semantics branch 2 times, most recently from 937ed37 to 979fc2a Compare May 23, 2024 20:30

mdebbar force-pushed the link_semantics branch from 979fc2a to 464f9bb Compare May 31, 2024 18:08

mdebbar added 6 commits May 31, 2024 15:04

[url_launcher][web] Better support for semantics in the Link widget

d3e16ea

tests

cce04c4

click on mismatched link

b3d16c7

more tests

f684065

semantics tests

c1556cc

apply the target attribute on semantic links

d98eab2

mdebbar force-pushed the link_semantics branch from 464f9bb to d98eab2 Compare May 31, 2024 19:04

mdebbar requested review from yjbanov and ditman May 31, 2024 19:27

yjbanov reviewed May 31, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[url_launcher][web] Better support for semantics in the Link widget #6711

[url_launcher][web] Better support for semantics in the Link widget #6711

mdebbar commented May 10, 2024 •

edited

yjbanov May 31, 2024

mdebbar Jun 3, 2024

yjbanov May 31, 2024

mdebbar Jun 3, 2024

yjbanov May 31, 2024

mdebbar Jun 3, 2024

yjbanov May 31, 2024

mdebbar Jun 3, 2024

yjbanov May 31, 2024

mdebbar Jun 3, 2024

yjbanov May 31, 2024

mdebbar Jun 3, 2024

yjbanov May 31, 2024

mdebbar Jun 3, 2024

yjbanov May 31, 2024

mdebbar Jun 3, 2024

yjbanov May 31, 2024

mdebbar Jun 3, 2024

yjbanov May 31, 2024

mdebbar Jun 3, 2024

		@@ -197,41 +343,99 @@ class LinkViewController extends PlatformViewController {
		// - If `_hitTestedViewId` is set, it means the app triggered the link.

[url_launcher][web] Better support for semantics in the Link widget #6711

Are you sure you want to change the base?

[url_launcher][web] Better support for semantics in the Link widget #6711

Conversation

mdebbar commented May 10, 2024 • edited

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

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

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

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

mdebbar commented May 10, 2024 •

edited