From 245eb00111fedd656a8da5af99c2fd8f26e9c2c2 Mon Sep 17 00:00:00 2001 From: Mouad Debbar Date: Thu, 11 Apr 2024 15:51:02 -0400 Subject: [PATCH 1/5] [url_launcher][web] Link should work when triggered by keyboard --- .../url_launcher_web/CHANGELOG.md | 6 +- .../integration_test/link_widget_test.dart | 221 +++++++++++++++++- .../url_launcher_web/lib/src/link.dart | 53 ++++- .../url_launcher_web/pubspec.yaml | 2 +- 4 files changed, 271 insertions(+), 11 deletions(-) diff --git a/packages/url_launcher/url_launcher_web/CHANGELOG.md b/packages/url_launcher/url_launcher_web/CHANGELOG.md index 5e3d6d42b35..ca4d9d86cc6 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 9cd76d03c80..079283dee21 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,192 @@ 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 +387,27 @@ html.Element _findSingleAnchor() { return foundAnchors.single; } +void _simulateClick(html.Element target) { + final html.MouseEvent clickEvent = html.MouseEvent('click'); + clickEvent.initMouseEvent('click', /*bubbles*/ true); + target.dispatchEvent(clickEvent); +} + +void _simulateKeydown(html.Element target) { + final html.KeyboardEvent keydownEvent = html.KeyboardEvent('keydown'); + keydownEvent.initKeyboardEvent('keydown', /*bubbles*/ true); + target.dispatchEvent(keydownEvent); +} + +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 +427,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 558f3a135a2..d05df9e361a 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,10 @@ 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); + // We listen to `keydown` events in the capture phase to ensure that we + // 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 +145,25 @@ 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) { + // 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 +207,26 @@ 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 +237,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 +252,7 @@ class LinkViewController extends PlatformViewController { } Uri? _uri; + bool get _isExternalLink => _uri != null && _uri!.hasScheme; /// Set the [Uri] value for this link. /// @@ -274,7 +310,8 @@ 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 35154b5a6d2..51f320a32a5 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 From 2fac7302180c6e18407c2359eda717d4b99959d0 Mon Sep 17 00:00:00 2001 From: Mouad Debbar Date: Thu, 11 Apr 2024 16:26:02 -0400 Subject: [PATCH 2/5] fix formatting --- .../example/integration_test/link_widget_test.dart | 12 ++++++++---- .../url_launcher/url_launcher_web/lib/src/link.dart | 9 ++++++--- 2 files changed, 14 insertions(+), 7 deletions(-) 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 079283dee21..3f09bb7a63f 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 @@ -188,7 +188,8 @@ void main() { UrlLauncherPlatform.instance = originalPlugin; }); - testWidgets('click to navigate to internal link', (WidgetTester tester) async { + testWidgets('click to navigate to internal link', + (WidgetTester tester) async { final TestNavigatorObserver observer = TestNavigatorObserver(); final Uri uri = Uri.parse('/foobar'); FollowLink? followLinkCallback; @@ -232,7 +233,8 @@ void main() { await tester.pumpAndSettle(); }); - testWidgets('keydown to navigate to internal link', (WidgetTester tester) async { + testWidgets('keydown to navigate to internal link', + (WidgetTester tester) async { final TestNavigatorObserver observer = TestNavigatorObserver(); final Uri uri = Uri.parse('/foobar'); FollowLink? followLinkCallback; @@ -276,7 +278,8 @@ void main() { await tester.pumpAndSettle(); }); - testWidgets('click to navigate to external link', (WidgetTester tester) async { + testWidgets('click to navigate to external link', + (WidgetTester tester) async { final TestNavigatorObserver observer = TestNavigatorObserver(); final Uri uri = Uri.parse('https://google.com'); FollowLink? followLinkCallback; @@ -318,7 +321,8 @@ void main() { await tester.pumpAndSettle(); }); - testWidgets('keydown to navigate to external link', (WidgetTester tester) async { + testWidgets('keydown to navigate to external link', + (WidgetTester tester) async { final TestNavigatorObserver observer = TestNavigatorObserver(); final Uri uri = Uri.parse('https://google.com'); FollowLink? followLinkCallback; 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 d05df9e361a..428c3930a30 100644 --- a/packages/url_launcher/url_launcher_web/lib/src/link.dart +++ b/packages/url_launcher/url_launcher_web/lib/src/link.dart @@ -111,7 +111,8 @@ class LinkViewController extends PlatformViewController { // We listen to `keydown` events in the capture phase to ensure that we // receive the event even if the engine calls `stopPropagation`. - html.window.addEventListener('keydown', _jsGlobalKeydownListener, _useCapture); + html.window + .addEventListener('keydown', _jsGlobalKeydownListener, _useCapture); html.window.addEventListener('click', _jsGlobalClickListener); } _instances[viewId] = this; @@ -216,7 +217,8 @@ class LinkViewController extends PlatformViewController { 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()); + UrlLauncherPlatform.instance + .launchUrl(_uri.toString(), const LaunchOptions()); return; } @@ -311,7 +313,8 @@ class LinkViewController extends PlatformViewController { _instances.remove(viewId); if (_instances.isEmpty) { html.window.removeEventListener('click', _jsGlobalClickListener); - html.window.removeEventListener('keydown', _jsGlobalKeydownListener, _useCapture); + html.window.removeEventListener( + 'keydown', _jsGlobalKeydownListener, _useCapture); } await SystemChannels.platform_views.invokeMethod('dispose', viewId); } From 1932b39482e062b1ad3e8eb9b99488c539105bae Mon Sep 17 00:00:00 2001 From: Mouad Debbar Date: Fri, 12 Apr 2024 10:55:42 -0400 Subject: [PATCH 3/5] use non-deprecated api --- .../integration_test/link_widget_test.dart | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) 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 3f09bb7a63f..9c700c87d0e 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 @@ -392,15 +392,21 @@ html.Element _findSingleAnchor() { } void _simulateClick(html.Element target) { - final html.MouseEvent clickEvent = html.MouseEvent('click'); - clickEvent.initMouseEvent('click', /*bubbles*/ true); - target.dispatchEvent(clickEvent); + target.dispatchEvent( + html.MouseEvent( + 'click', + html.MouseEventInit()..bubbles = true, + ), + ); } void _simulateKeydown(html.Element target) { - final html.KeyboardEvent keydownEvent = html.KeyboardEvent('keydown'); - keydownEvent.initKeyboardEvent('keydown', /*bubbles*/ true); - target.dispatchEvent(keydownEvent); + target.dispatchEvent( + html.KeyboardEvent( + 'keydown', + html.KeyboardEventInit()..bubbles = true, + ), + ); } class TestNavigatorObserver extends NavigatorObserver { From 1614801bc7d8494effe374a6e9e63d0a674a8bee Mon Sep 17 00:00:00 2001 From: Mouad Debbar Date: Mon, 15 Apr 2024 11:48:42 -0400 Subject: [PATCH 4/5] add comments --- .../url_launcher_web/lib/src/link.dart | 52 ++++++++++++++++++- 1 file changed, 50 insertions(+), 2 deletions(-) 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 428c3930a30..f5fd8c4e09e 100644 --- a/packages/url_launcher/url_launcher_web/lib/src/link.dart +++ b/packages/url_launcher/url_launcher_web/lib/src/link.dart @@ -109,8 +109,10 @@ class LinkViewController extends PlatformViewController { // This is the first controller being created, attach the global click // listener. - // We listen to `keydown` events in the capture phase to ensure that we - // receive the event even if the engine calls `stopPropagation`. + // 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); @@ -150,6 +152,52 @@ class LinkViewController extends PlatformViewController { 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) { From 5c9289ca451da160bee404bfa8727823a6cded4d Mon Sep 17 00:00:00 2001 From: Mouad Debbar Date: Mon, 15 Apr 2024 15:33:00 -0400 Subject: [PATCH 5/5] format --- packages/url_launcher/url_launcher_web/lib/src/link.dart | 1 - 1 file changed, 1 deletion(-) 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 f5fd8c4e09e..3a286fb4074 100644 --- a/packages/url_launcher/url_launcher_web/lib/src/link.dart +++ b/packages/url_launcher/url_launcher_web/lib/src/link.dart @@ -197,7 +197,6 @@ class LinkViewController extends PlatformViewController { // - 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) {