Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[url_launcher][web] Link should work when triggered by keyboard #6505

Merged
merged 6 commits into from
Apr 15, 2024

Conversation

mdebbar
Copy link
Contributor

@mdebbar mdebbar commented Apr 11, 2024

Background

You can think of the Link widget (on the web) as two components working together:

  1. The <a> 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 <a> element also has to receive an event to initiate the navigation.

The PR

We used to only handle click events on the <a> 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 <a> 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 flutter/flutter#97863

Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for adding this! Small question about attaching the keydown listener to the window, instead of the link element (or the view of the Widget)

@@ -192,6 +208,27 @@ class LinkViewController extends PlatformViewController {
await SystemChannels.platform_views.invokeMethod<void>('create', args);
}

void _onDomKeydown() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this method need to check if the keydown is "enter", "space" or something like that? This'd trigger the link pressing any key, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to assume what keys the framework and/or app are using as a trigger for the link.

As I explained in the PR description, there are 2 conditions that have to be met in order for the link to navigate. The code in _onDomKeydown is only one part. The other part is the app/framework calling followLink when they decide that the key should trigger the link. Here's roughly the sequence of events:

  1. User tabs through the app until the focus reaches the button wrapped by the Link widget.
    • (Keep in mind that this tabbing is happening purely in the framework; the browser has no idea what's focused)
  2. User hits "Enter".
  3. The framework received the keydown event.
    1. It decides to trigger the onPressed on the button (because the button has focus).
    2. The button's onPressed callback leads to a call to the Link's followLink.
    3. followLink calls LinkViewController.registerHitTest which sets _hitTestedViewId to the Link's platform view ID.
  4. The LinkViewController also receives keydown event.
    1. It checks if _hitTestedViewId is already set (indicating that the framework already decided to trigger the link).
    2. If _hitTestedViewId is set, it navigates to that Link's URI.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This should be part of the documentation of the widget :P)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added as comments to the _onGlobalKeydown method since that's where this information is relevant.

@ditman ditman added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 15, 2024
@auto-submit auto-submit bot merged commit 1d0bb4f into flutter:main Apr 15, 2024
78 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 16, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 16, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Apr 16, 2024
flutter/packages@6698b2d...90c876d

2024-04-16 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.24.10 to 3.25.0 (flutter/packages#6545)
2024-04-16 31859944+LongCatIsLooong@users.noreply.github.com [CupertinoIcons] fix broken icons and vertical alignment, bump the version to 1.08 (flutter/packages#6520)
2024-04-16 26625149+0xZOne@users.noreply.github.com [video_player] Calls `onDestroy` instead of `initialize` in onDetachedFromEngine (flutter/packages#6501)
2024-04-15 mikemcguiness@protonmail.com [image_picker] Adopt code excerpts in README (flutter/packages#5523)
2024-04-15 mdebbar@google.com [url_launcher][web] Link should work when triggered by keyboard (flutter/packages#6505)
2024-04-15 engine-flutter-autoroll@skia.org Manual roll Flutter from 53cba24 to 2e748e8 (19 revisions) (flutter/packages#6541)
2024-04-15 stuartmorgan@google.com Update local_auth_android minSdkVersion to 19 (flutter/packages#6537)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC flutter-ecosystem@google.com,rmistry@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
gilnobrega pushed a commit to gilnobrega/flutter that referenced this pull request Apr 22, 2024
flutter/packages@6698b2d...90c876d

2024-04-16 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.24.10 to 3.25.0 (flutter/packages#6545)
2024-04-16 31859944+LongCatIsLooong@users.noreply.github.com [CupertinoIcons] fix broken icons and vertical alignment, bump the version to 1.08 (flutter/packages#6520)
2024-04-16 26625149+0xZOne@users.noreply.github.com [video_player] Calls `onDestroy` instead of `initialize` in onDetachedFromEngine (flutter/packages#6501)
2024-04-15 mikemcguiness@protonmail.com [image_picker] Adopt code excerpts in README (flutter/packages#5523)
2024-04-15 mdebbar@google.com [url_launcher][web] Link should work when triggered by keyboard (flutter/packages#6505)
2024-04-15 engine-flutter-autoroll@skia.org Manual roll Flutter from 53cba24 to 2e748e8 (19 revisions) (flutter/packages#6541)
2024-04-15 stuartmorgan@google.com Update local_auth_android minSdkVersion to 19 (flutter/packages#6537)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC flutter-ecosystem@google.com,rmistry@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@mdebbar mdebbar deleted the link_keyboard branch May 9, 2024 17:11
TecHaxter pushed a commit to TecHaxter/flutter_packages that referenced this pull request May 22, 2024
…ter#6505)

### Background

You can think of the `Link` widget (on the web) as two components working together:

1. The `<a>` 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 `<a>` element also has to receive an event to initiate the navigation.

### The PR

We used to only handle click events on the `<a>` 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 `<a>` 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 flutter/flutter#97863
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
…ter#6505)

### Background

You can think of the `Link` widget (on the web) as two components working together:

1. The `<a>` 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 `<a>` element also has to receive an event to initiate the navigation.

### The PR

We used to only handle click events on the `<a>` 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 `<a>` 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 flutter/flutter#97863
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: url_launcher platform-web
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Link widget not working from keyboard on web
2 participants