[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 { | |||
| String getHref() { | |||
There was a problem hiding this comment.
I wanted to make sure first that the code is starting to look good before I spend time on writing and re-writing docs.
| // 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?
There was a problem hiding this comment.
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
stopPropagationon 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.
| /// 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.
There was a problem hiding this comment.
Good idea. I'll revisit this when I do the pass to fill in missing docs.
| _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).
There was a problem hiding this comment.
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.
| // 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?
There was a problem hiding this comment.
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.
This is a stopgap for issues like flutter/flutter#146291 until we land better link semantics (in flutter/flutter#150263 and flutter/packages#6711). For the time being, the `href="#"` isn't providing any extra value, and is causing the browser to navigate to to `#` whenever the semantics link is clicked, which is undesirable. Fixes flutter/flutter#146291
…3278) Make [`Semantics(identifier: '...')`](https://api.flutter.dev/flutter/semantics/SemanticsProperties/identifier.html) useful on the web. This PR plugs the Semantics `identifier` property as an HTML attribute `semantics-identifier` onto semantics elements. This is useful in some scenarios: - In testing to check if a certain semantics node has made it to the page ([example](flutter/flutter#97455)). - In apps and/or packages to be able to lookup the DOM element that corresponds to a certain semantics node ([example](flutter/packages#6711)). Fixes flutter/flutter#97455
The new property allows the user to specify a URI for their semantics link node. It's plumbed through for both web and non-web engines, but it's only used in the web engine currently. It sets the `href` of the anchor element associated with semantics node. This is going to unlock better semantics support in the Link widget on web ([PR](flutter/packages#6711)). Framework counterpart: flutter/flutter#150639 Part of flutter/flutter#150263
The new property allows the user to specify a URI for their semantics link node. On the web, it sets the `href` of the anchor element associated with semantics node. This is going to unlock better semantics support in the Link widget on web ([PR](flutter/packages#6711)). Engine counterpart: flutter/engine#53507 Fixes #150263
|
Re-opening this. It needs to land for flutter/flutter#143164 to be fixed |
|
From triage: What's the status of this issue? Is it still planned for further work? |
|
Waiting for this commit to make it to stable. |
stuartmorgan
added
the
waiting for stable update
|
(triage): flutter/flutter@8d01fe0 still hasn't made it to stable. |
stuartmorgan
removed
the
waiting for stable update
targetattribute on semantic links.Fixes flutter/flutter#143164
Fixes flutter/flutter#146291