From 1d0bb4f85870df850a060d1848804a875fa9902f Mon Sep 17 00:00:00 2001 From: Mouad Debbar Date: Mon, 15 Apr 2024 15:51:18 -0400 Subject: [PATCH] [url_launcher][web] Link should work when triggered by keyboard (#6505) ### Background You can think of the `Link` widget (on the web) as two components working together: 1. The `` element created by the `Link` widget. This is essential to make all browser interactions feel natural (e.g. context menu, cmd+click, etc). 2. The children of `Link` widget. These are the widgets visible to the user (e.g. a button or a hyperlink text) and the user can interact with them the same way they would interact with any Flutter widgets (focus, pointer click, etc). In order for the Link widget to navigate to a URI, the two components from above have to indicate their intent of navigation: 1. Some widget has to call `followLink` to indicate that the click successfully landed (i.e. hit tested) on it. E.g. if it's a button, then the `onPressed` callback should lead to a call to the Link's `followLink`. 2. The `` element also has to receive an event to initiate the navigation. ### The PR We used to only handle click events on the `` element, and no handling for keyboard events was present. So when a user tabs their way to the Link, then hits "Enter", the following happens: 1. The focused widget (e.g. button) that received the "Enter" will correctly indicate its intent to navigate by calling `followLink`. 2. The intent from the `` element is lost because we were only handling clicks and not keyboard events. This PR adds handling of keyboard events so that it works similar to clicks. Fixes https://github.com/flutter/flutter/issues/97863 --- .../url_launcher_web/CHANGELOG.md | 6 +- .../integration_test/link_widget_test.dart | 231 +++++++++++++++++- .../url_launcher_web/lib/src/link.dart | 103 +++++++- .../url_launcher_web/pubspec.yaml | 2 +- 4 files changed, 331 insertions(+), 11 deletions(-) diff --git a/packages/url_launcher/url_launcher_web/CHANGELOG.md b/packages/url_launcher/url_launcher_web/CHANGELOG.md index 5e3d6d42b35e..ca4d9d86cc66 100644 --- a/packages/url_launcher/url_launcher_web/CHANGELOG.md +++ b/packages/url_launcher/url_launcher_web/CHANGELOG.md @@ -1,3 +1,7 @@ +## 2.3.1 + +* Implements correct handling of keyboard events with Link. + ## 2.3.0 * Updates web code to package `web: ^0.5.0`. @@ -24,7 +28,7 @@ ## 2.1.0 * Adds `launchUrl` implementation. -* Prevents _Tabnabbing_ and disallows `javascript:` URLs on `launch` and `launchUrl`. +* Prevents _Tabnabbing_ and disallows `javascript:` URLs on `launch` and `launchUrl`. ## 2.0.20 diff --git a/packages/url_launcher/url_launcher_web/example/integration_test/link_widget_test.dart b/packages/url_launcher/url_launcher_web/example/integration_test/link_widget_test.dart index 9cd76d03c80c..9c700c87d0eb 100644 --- a/packages/url_launcher/url_launcher_web/example/integration_test/link_widget_test.dart +++ b/packages/url_launcher/url_launcher_web/example/integration_test/link_widget_test.dart @@ -6,11 +6,13 @@ import 'dart:js_interop'; import 'dart:js_interop_unsafe'; import 'dart:ui_web' as ui_web; -import 'package:flutter/widgets.dart'; +import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:integration_test/integration_test.dart'; import 'package:url_launcher_platform_interface/link.dart'; +import 'package:url_launcher_platform_interface/url_launcher_platform_interface.dart'; import 'package:url_launcher_web/src/link.dart'; +import 'package:url_launcher_web/url_launcher_web.dart'; import 'package:web/web.dart' as html; void main() { @@ -171,6 +173,196 @@ void main() { await tester.pumpAndSettle(); }); }); + + group('Follows links', () { + late TestUrlLauncherPlugin testPlugin; + late UrlLauncherPlatform originalPlugin; + + setUp(() { + originalPlugin = UrlLauncherPlatform.instance; + testPlugin = TestUrlLauncherPlugin(); + UrlLauncherPlatform.instance = testPlugin; + }); + + tearDown(() { + UrlLauncherPlatform.instance = originalPlugin; + }); + + testWidgets('click to navigate to internal link', + (WidgetTester tester) async { + final TestNavigatorObserver observer = TestNavigatorObserver(); + final Uri uri = Uri.parse('/foobar'); + FollowLink? followLinkCallback; + + await tester.pumpWidget(MaterialApp( + navigatorObservers: [observer], + routes: { + '/foobar': (BuildContext context) => const Text('Internal route'), + }, + home: Directionality( + textDirection: TextDirection.ltr, + child: WebLinkDelegate(TestLinkInfo( + uri: uri, + target: LinkTarget.blank, + builder: (BuildContext context, FollowLink? followLink) { + followLinkCallback = followLink; + return const SizedBox(width: 100, height: 100); + }, + )), + ), + )); + // Platform view creation happens asynchronously. + await tester.pumpAndSettle(); + + expect(observer.currentRouteName, '/'); + expect(testPlugin.launches, isEmpty); + + final html.Element anchor = _findSingleAnchor(); + + await followLinkCallback!(); + _simulateClick(anchor); + await tester.pumpAndSettle(); + + // Internal links should navigate the app to the specified route. There + // should be no calls to `launchUrl`. + expect(observer.currentRouteName, '/foobar'); + expect(testPlugin.launches, isEmpty); + + // Needed when testing on on Chrome98 headless in CI. + // See https://github.com/flutter/flutter/issues/121161 + await tester.pumpAndSettle(); + }); + + testWidgets('keydown to navigate to internal link', + (WidgetTester tester) async { + final TestNavigatorObserver observer = TestNavigatorObserver(); + final Uri uri = Uri.parse('/foobar'); + FollowLink? followLinkCallback; + + await tester.pumpWidget(MaterialApp( + navigatorObservers: [observer], + routes: { + '/foobar': (BuildContext context) => const Text('Internal route'), + }, + home: Directionality( + textDirection: TextDirection.ltr, + child: WebLinkDelegate(TestLinkInfo( + uri: uri, + target: LinkTarget.blank, + builder: (BuildContext context, FollowLink? followLink) { + followLinkCallback = followLink; + return const SizedBox(width: 100, height: 100); + }, + )), + ), + )); + // Platform view creation happens asynchronously. + await tester.pumpAndSettle(); + + expect(observer.currentRouteName, '/'); + expect(testPlugin.launches, isEmpty); + + final html.Element anchor = _findSingleAnchor(); + + await followLinkCallback!(); + _simulateKeydown(anchor); + await tester.pumpAndSettle(); + + // Internal links should navigate the app to the specified route. There + // should be no calls to `launchUrl`. + expect(observer.currentRouteName, '/foobar'); + expect(testPlugin.launches, isEmpty); + + // Needed when testing on on Chrome98 headless in CI. + // See https://github.com/flutter/flutter/issues/121161 + await tester.pumpAndSettle(); + }); + + testWidgets('click to navigate to external link', + (WidgetTester tester) async { + final TestNavigatorObserver observer = TestNavigatorObserver(); + final Uri uri = Uri.parse('https://google.com'); + FollowLink? followLinkCallback; + + await tester.pumpWidget(MaterialApp( + navigatorObservers: [observer], + home: Directionality( + textDirection: TextDirection.ltr, + child: WebLinkDelegate(TestLinkInfo( + uri: uri, + target: LinkTarget.blank, + builder: (BuildContext context, FollowLink? followLink) { + followLinkCallback = followLink; + return const SizedBox(width: 100, height: 100); + }, + )), + ), + )); + // Platform view creation happens asynchronously. + await tester.pumpAndSettle(); + + expect(observer.currentRouteName, '/'); + expect(testPlugin.launches, isEmpty); + + final html.Element anchor = _findSingleAnchor(); + + await followLinkCallback!(); + _simulateClick(anchor); + await tester.pumpAndSettle(); + + // External links that are triggered by a click are left to be handled by + // the browser, so there should be no change to the app's route name, and + // no calls to `launchUrl`. + expect(observer.currentRouteName, '/'); + expect(testPlugin.launches, isEmpty); + + // Needed when testing on on Chrome98 headless in CI. + // See https://github.com/flutter/flutter/issues/121161 + await tester.pumpAndSettle(); + }); + + testWidgets('keydown to navigate to external link', + (WidgetTester tester) async { + final TestNavigatorObserver observer = TestNavigatorObserver(); + final Uri uri = Uri.parse('https://google.com'); + FollowLink? followLinkCallback; + + await tester.pumpWidget(MaterialApp( + navigatorObservers: [observer], + home: Directionality( + textDirection: TextDirection.ltr, + child: WebLinkDelegate(TestLinkInfo( + uri: uri, + target: LinkTarget.blank, + builder: (BuildContext context, FollowLink? followLink) { + followLinkCallback = followLink; + return const SizedBox(width: 100, height: 100); + }, + )), + ), + )); + // Platform view creation happens asynchronously. + await tester.pumpAndSettle(); + + expect(observer.currentRouteName, '/'); + expect(testPlugin.launches, isEmpty); + + final html.Element anchor = _findSingleAnchor(); + + await followLinkCallback!(); + _simulateKeydown(anchor); + await tester.pumpAndSettle(); + + // External links that are triggered by keyboard are handled by calling + // `launchUrl`, and there's no change to the app's route name. + expect(observer.currentRouteName, '/'); + expect(testPlugin.launches, ['https://google.com']); + + // Needed when testing on on Chrome98 headless in CI. + // See https://github.com/flutter/flutter/issues/121161 + await tester.pumpAndSettle(); + }); + }); } html.Element _findSingleAnchor() { @@ -199,6 +391,33 @@ html.Element _findSingleAnchor() { return foundAnchors.single; } +void _simulateClick(html.Element target) { + target.dispatchEvent( + html.MouseEvent( + 'click', + html.MouseEventInit()..bubbles = true, + ), + ); +} + +void _simulateKeydown(html.Element target) { + target.dispatchEvent( + html.KeyboardEvent( + 'keydown', + html.KeyboardEventInit()..bubbles = true, + ), + ); +} + +class TestNavigatorObserver extends NavigatorObserver { + String? currentRouteName; + + @override + void didPush(Route route, Route? previousRoute) { + currentRouteName = route.settings.name; + } +} + class TestLinkInfo extends LinkInfo { TestLinkInfo({ required this.uri, @@ -218,3 +437,13 @@ class TestLinkInfo extends LinkInfo { @override bool get isDisabled => uri == null; } + +class TestUrlLauncherPlugin extends UrlLauncherPlugin { + final List launches = []; + + @override + Future launchUrl(String url, LaunchOptions options) async { + launches.add(url); + return true; + } +} diff --git a/packages/url_launcher/url_launcher_web/lib/src/link.dart b/packages/url_launcher/url_launcher_web/lib/src/link.dart index 558f3a135a27..3a286fb40749 100644 --- a/packages/url_launcher/url_launcher_web/lib/src/link.dart +++ b/packages/url_launcher/url_launcher_web/lib/src/link.dart @@ -13,6 +13,7 @@ import 'package:flutter/material.dart'; import 'package:flutter/rendering.dart'; import 'package:flutter/services.dart'; import 'package:url_launcher_platform_interface/link.dart'; +import 'package:url_launcher_platform_interface/url_launcher_platform_interface.dart'; import 'package:web/web.dart' as html; /// The unique identifier for the view type to be used for link platform views. @@ -98,6 +99,8 @@ class WebLinkDelegateState extends State { } } +final JSAny _useCapture = {'capture': true}.jsify()!; + /// Controls link views. class LinkViewController extends PlatformViewController { /// Creates a [LinkViewController] instance with the unique [viewId]. @@ -106,10 +109,13 @@ class LinkViewController extends PlatformViewController { // This is the first controller being created, attach the global click // listener. - _clickSubscription = - const html.EventStreamProvider('click') - .forTarget(html.window) - .listen(_onGlobalClick); + // Why listen in the capture phase? + // + // To ensure we always receive the event even if the engine calls + // `stopPropagation`. + html.window + .addEventListener('keydown', _jsGlobalKeydownListener, _useCapture); + html.window.addEventListener('click', _jsGlobalClickListener); } _instances[viewId] = this; } @@ -142,13 +148,70 @@ class LinkViewController extends PlatformViewController { static int? _hitTestedViewId; - static late StreamSubscription _clickSubscription; + static final JSFunction _jsGlobalKeydownListener = _onGlobalKeydown.toJS; + static final JSFunction _jsGlobalClickListener = _onGlobalClick.toJS; + + static void _onGlobalKeydown(html.KeyboardEvent event) { + // Why not use `event.target`? + // + // Because the target is usually and not the element, so + // it's not very helpful. That's because focus management is handled by + // Flutter, and the browser doesn't always know which element is focused. In + // fact, in many cases, the focused widget is fully drawn on canvas and + // there's no corresponding HTML element to receive browser focus. + + // Why not check for "Enter" or "Space" keys? + // + // Because we don't know (nor do we want to assume) which keys the app + // considers to be "trigger" keys. So we let the app do its thing, and if it + // decides to "trigger" the link, it will call `followLink`, which will set + // `_hitTestedViewId` to the ID of the triggered Link. + + // Life of a keydown event: + // + // For simplicity, let's assume we are dealing with a Link widget setup with + // with a button widget like this: + // + // ```dart + // Link( + // uri: Uri.parse('...'), + // builder: (context, followLink) { + // return ElevatedButton( + // onPressed: followLink, + // child: const Text('Press me'), + // ); + // }, + // ); + // ``` + // + // 1. The user navigates through the UI using the Tab key until they reach + // the button in question. + // 2. The user presses the Enter key to trigger the link. + // 3. The framework receives the Enter keydown event: + // - The event is dispatched to the button widget. + // - The button widget calls `onPressed` and therefor `followLink`. + // - `followLink` calls `LinkViewController.registerHitTest`. + // - `LinkViewController.registerHitTest` sets `_hitTestedViewId`. + // 4. The `LinkViewController` also receives the keydown event: + // - We check the value of `_hitTestedViewId`. + // - If `_hitTestedViewId` is set, it means the app triggered the link. + // - We navigate to the Link's URI. + + // The keydown event is not directly associated with the target Link, so + // we need to look for the recently hit tested Link to handle the event. + if (_hitTestedViewId != null) { + _instances[_hitTestedViewId]?._onDomKeydown(); + } + // After the keyboard event has been received, clean up the hit test state + // so we can start fresh on the next event. + unregisterHitTest(); + } static void _onGlobalClick(html.MouseEvent event) { final int? viewId = getViewIdFromTarget(event); _instances[viewId]?._onDomClick(event); // After the DOM click event has been received, clean up the hit test state - // so we can start fresh on the next click. + // so we can start fresh on the next event. unregisterHitTest(); } @@ -192,6 +255,27 @@ class LinkViewController extends PlatformViewController { await SystemChannels.platform_views.invokeMethod('create', args); } + void _onDomKeydown() { + assert( + _hitTestedViewId == viewId, + 'Keydown event should only be handled by the hit tested Link', + ); + + if (_isExternalLink) { + // External links are not handled by the browser when triggered via a + // keydown, so we have to launch the url manually. + UrlLauncherPlatform.instance + .launchUrl(_uri.toString(), const LaunchOptions()); + return; + } + + // A uri that doesn't have a scheme is an internal route name. In this + // case, we push it via Flutter's navigation system instead of using + // `launchUrl`. + final String routeName = _uri.toString(); + pushRouteNameToFramework(null, routeName); + } + void _onDomClick(html.MouseEvent event) { final bool isHitTested = _hitTestedViewId == viewId; if (!isHitTested) { @@ -202,7 +286,7 @@ class LinkViewController extends PlatformViewController { return; } - if (_uri != null && _uri!.hasScheme) { + if (_isExternalLink) { // External links will be handled by the browser, so we don't have to do // anything. return; @@ -217,6 +301,7 @@ class LinkViewController extends PlatformViewController { } Uri? _uri; + bool get _isExternalLink => _uri != null && _uri!.hasScheme; /// Set the [Uri] value for this link. /// @@ -274,7 +359,9 @@ class LinkViewController extends PlatformViewController { assert(_instances[viewId] == this); _instances.remove(viewId); if (_instances.isEmpty) { - await _clickSubscription.cancel(); + html.window.removeEventListener('click', _jsGlobalClickListener); + html.window.removeEventListener( + 'keydown', _jsGlobalKeydownListener, _useCapture); } await SystemChannels.platform_views.invokeMethod('dispose', viewId); } diff --git a/packages/url_launcher/url_launcher_web/pubspec.yaml b/packages/url_launcher/url_launcher_web/pubspec.yaml index 35154b5a6d2b..51f320a32a5e 100644 --- a/packages/url_launcher/url_launcher_web/pubspec.yaml +++ b/packages/url_launcher/url_launcher_web/pubspec.yaml @@ -2,7 +2,7 @@ name: url_launcher_web description: Web platform implementation of url_launcher repository: https://github.com/flutter/packages/tree/main/packages/url_launcher/url_launcher_web issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+url_launcher%22 -version: 2.3.0 +version: 2.3.1 environment: sdk: ^3.3.0