Skip to content

[url_launcher] fix: tab traversal on web #10020

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.

Sign up for GitHub

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

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
+59 −21
Open

[url_launcher] fix: tab traversal on web #10020

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
06d560d
base
pedromassango May 20, 2025
fad6f19
Merge branch 'flutter:main' into master
pedromassango Aug 15, 2025
11e7825
fix: Link widget Tab traversal
pedromassango Aug 15, 2025
68ec601
wip
pedromassango Aug 15, 2025
0b2497c
wip
pedromassango Aug 15, 2025
dc9fec1
wip
pedromassango Aug 15, 2025
d8e410d
Merge branch 'flutter:main' into fix/link_tab_traversal
pedromassango Aug 26, 2025
5f2cb62
wip
pedromassango Aug 26, 2025
a376b78
nit
pedromassango Aug 26, 2025
4ac086d
nit
pedromassango Aug 26, 2025
a9b517e
update CHANGELOG.md
pedromassango Aug 28, 2025
26c49e0
Merge branch 'main' into fix/link_tab_traversal
pedromassango Aug 28, 2025
e1222f1
Merge branch 'main' into fix/link_tab_traversal
pedromassango Sep 4, 2025
abaaa01
nit
pedromassango Sep 4, 2025
30d7a0d
Merge branch 'main' into fix/tab_traversal_on_web
pedromassango Sep 16, 2025
c724783
Update packages/url_launcher/url_launcher_web/CHANGELOG.md
pedromassango Sep 16, 2025
30c0c73
update test case
pedromassango Sep 16, 2025
7b3465a
update existing test cases
pedromassango Sep 16, 2025
beb1acf
wip
pedromassango Sep 16, 2025
8d6d054
Merge branch 'main' into fix/tab_traversal_on_web
pedromassango Sep 16, 2025
7aa4726
fix: format
pedromassango Sep 16, 2025
cd430f5
Merge remote-tracking branch 'origin/fix/tab_traversal_on_web' into f…
pedromassango Sep 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/url_launcher/url_launcher_web/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
## NEXT
## 2.4.2

* Fixed an issue that caused duplicate semantic nodes for `Link` widgets.
* Updates minimum supported SDK version to Flutter 3.29/Dart 3.7.

## 2.4.1
Expand Down
59 changes: 47 additions & 12 deletions packages/url_launcher/url_launcher_web/example/integration_test/link_widget_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,43 @@ void main() {
maxScrolls: 1000,
);
});

testWidgets('MergeSemantics is always present to avoid duplicate nodes', (
WidgetTester tester,
) async {
await tester.pumpWidget(
MaterialApp(
home: Scaffold(
body: Column(
children: <Widget>[
WebLinkDelegate(
TestLinkInfo(
uri: Uri.parse('https://dart.dev/xyz'),
target: LinkTarget.blank,
builder: (BuildContext context, FollowLink? followLink) {
return ElevatedButton(
onPressed: followLink,
child: const Text('First Button'),
);
},
),
),
],
),
),
),
);

await tester.pumpAndSettle();

final Finder buttonFinder = find.byType(ElevatedButton);
expect(buttonFinder, findsOneWidget);

final Element buttonElement = tester.element(buttonFinder);
final MergeSemantics? parentWidget =
buttonElement.findAncestorWidgetOfExactType<MergeSemantics>();
expect(parentWidget, isNotNull);
});
});

group('Follows links', () {
Expand Down Expand Up @@ -897,17 +934,13 @@ void main() {
isLink: true,
identifier: 'test-link-12',
// linkUrl: 'https://foobar/example?q=1',
children: <Matcher>[
matchesSemantics(
hasTapAction: true,
hasEnabledState: true,
hasFocusAction: true,
isEnabled: true,
isButton: true,
isFocusable: true,
label: 'Button Link Text',
),
],
hasTapAction: true,
hasEnabledState: true,
hasFocusAction: true,
isEnabled: true,
isButton: true,
Copy link
Member Author

Choose a reason for hiding this comment

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

@chunhtai I was expecting this test to fail as the engine fix landed already 🤔

I mean now that the engine prioritize link, then ideally this test don't need isButton: true right? Are the tests not being ran against the master branch of the flutter?

isFocusable: true,
label: 'Button Link Text',
),
);

Expand Down Expand Up @@ -943,7 +976,9 @@ void main() {
final Finder linkFinder = find.byKey(linkKey);
expect(
tester.getSemantics(
find.descendant(of: linkFinder, matching: find.byType(Semantics)),
find
.descendant(of: linkFinder, matching: find.byType(Semantics))
.first,
),
matchesSemantics(
isLink: true,
Expand Down
16 changes: 9 additions & 7 deletions packages/url_launcher/url_launcher_web/lib/src/link.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,13 +117,15 @@ class WebLinkDelegateState extends State<WebLinkDelegate> {
}

Widget _buildChild(BuildContext context) {
return Semantics(
link: true,
identifier: _semanticsIdentifier,
linkUrl: widget.link.uri,
child: widget.link.builder(
context,
widget.link.isDisabled ? null : _followLink,
return MergeSemantics(
child: Semantics(
link: true,
identifier: _semanticsIdentifier,
linkUrl: widget.link.uri,
child: widget.link.builder(
context,
widget.link.isDisabled ? null : _followLink,
),
Comment on lines +120 to +128
Copy link
Contributor

Choose a reason for hiding this comment

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

I think tests run against master AND stable (@stuartmorgan-g to confirm).

If this code is expected to work with master but not with stable, then you need to wait until your flutter fix makes it to stable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Most tests (including web integration tests) are run against master and stable, yes.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mdebbar I believe it should be fine now since all tests are passing (had to update some existing ones to align with the new behavior)

My comment above (here) is really just a curiosity, as per my understanding after my engine fix, then ideally no isButton should be expeted

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe it should be fine now since all tests are passing

If a test that you expected to fail is passing, that doesn't sound fine; it sounds like there's a potential test issue here that should be investigated. Am I misunderstanding?

Copy link
Contributor

Choose a reason for hiding this comment

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

Reading the previous (reverted) PR:

With MergeSemantics, the semantics tree was not generated properly, e.g linkUrl value gets removed from the final semantics tree. This is being fixed in the engine, see flutter/flutter#174473

That sounds like we need to wait until the engine fix lands in stable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Am I misunderstanding?

I would say no, but, I'm only 50% sure about my statement and that is why I am looking for @chunhtai 's feedback as he has way more knowledge on this.

But yes, from my understanding, that test should fail as my engine fix now prioritize the link flag over the button one, meaning isButton: true should make it fail.

Anyway, will wait for the fix to land on stable and then, expectt the test to fail

Copy link
Collaborator

Choose a reason for hiding this comment

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

Anyway, will wait for the fix to land on stable and then, expectt the test to fail

The test passed CI on master, pinned to 8d0b31d81a02d13ee37d17c9387d7bbcd1c2baba (which is yesterday), so it's not clear to me what reason there would be to expect different results when the current master reaches stable.

),
);
}
Expand Down
2 changes: 1 addition & 1 deletion packages/url_launcher/url_launcher_web/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.4.1
version: 2.4.2

environment:
sdk: ^3.7.0
Expand Down