[url_launcher][web] Better support for semantics in the Link widget #6711
Conversation
mdebbar
force-pushed
the
link_semantics
branch
2 times, most recently
from
937ed37
to
979fc2a
Compare
| @@ -42,13 +47,42 @@ class WebLinkDelegate extends StatefulWidget { | |||
| WebLinkDelegateState createState() => WebLinkDelegateState(); | |||
| } | |||
|
|
|||
| extension on Uri { | |||
There was a problem hiding this comment.
nit: do extensions need docs?
| @@ -42,13 +47,42 @@ class WebLinkDelegate extends StatefulWidget { | |||
| WebLinkDelegateState createState() => WebLinkDelegateState(); | |||
| } | |||
|
|
|||
| extension on Uri { | |||
| String getHref() { | |||
|
|
||
| // In case an internal uri is given, the uri must be properly encoded | ||
| // using the currently used UrlStrategy. | ||
| return ui_web.urlStrategy!.prepareExternalUrl(toString()); |
There was a problem hiding this comment.
Is this related to flutter/flutter#147857 by any chance?
| /// 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; |
There was a problem hiding this comment.
nit: we always use plural "semantics" even in singular context, e.g. SemanticsNode.
| @override | ||
| void initState() { | ||
| super.initState(); | ||
| semanticIdentifier = 'sem-id-${_nextSemanticsIdentifier++}'; |
There was a problem hiding this comment.
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++}'?
| // Why listen in the capture phase? | ||
| // | ||
| // To ensure we always receive the event even if the engine calls | ||
| // `stopPropagation`. |
There was a problem hiding this comment.
Why would the engine stop propagating the events? Seems like code smell in the engine, no?
| @@ -197,41 +343,99 @@ class LinkViewController extends PlatformViewController { | |||
| // - If `_hitTestedViewId` is set, it means the app triggered the link. | |||
There was a problem hiding this comment.
nit for line https://github.com/flutter/packages/pull/6711/files#diff-77a04b3b00b00a4480b7d5e75e089dde7cca99ccebcc4f43e0e25d2bba5346e0R338: typo in "therefor"
| /// 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. |
There was a problem hiding this comment.
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.
| _hitTestedViewId = null; | ||
| if (_triggerSignals.isReadyToTrigger) { | ||
| _triggerLink(); | ||
| } |
There was a problem hiding this comment.
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).
| // 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); |
There was a problem hiding this comment.
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?
targetattribute on semantic links.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).
Fixes flutter/flutter#143164
Fixes flutter/flutter#146291