[url_launcher] fix: Link widget Tab traversal

[pedromassango]

Tries to fix the TAB traversal issue on web by ensuring the semantics of the Link are merged with its children. The buttons are treated as LINKS only and this prevents semantic conflicts that would create extra focusable DOM nodes.

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

Fixes flutter/flutter#157689

Required engine fix PR:
flutter/flutter#174473

Pre-Review Checklist

• I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
• I read the [Tree Hygiene] page, which explains my responsibilities.
• I read and followed the [relevant style guides] and ran [the auto-formatter].
• I signed the [CLA].
• The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• I [linked to at least one issue that this PR fixes] in the description above.
• I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy], or I have commented below to indicate which [version change exemption] this PR falls under¹.
• I updated CHANGELOG.md to add a description of the change, [following repository CHANGELOG style], or I have commented below to indicate which [CHANGELOG exemption] this PR falls under¹.
• I updated/added any relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or I have commented below to indicate which [test exemption] this PR falls under¹.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on [Discord].

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the gemini-code-assist bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

Footnotes

1. Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩² ↩³

[gemini-code-assist]

Code Review

This pull request aims to fix a TAB traversal issue on the web for the Link widget by adding excludeSemantics: true to the wrapping Semantics widget. This prevents the child widget's semantics from conflicting and creating extra focusable DOM nodes. A new integration test is added to verify this behavior.

My review focuses on the new test case. I've found a bug where SemanticsHandle.dispose() is called twice, and I've also questioned a skipped assertion that seems critical for verifying the fix. I've provided a code suggestion to address both points.

[mdebbar]

Thanks for tackling this issue!

Unfortunately, excludeSemantics: true won't work because it removes all other info provided by the child (e.g. label).

@chunhtai do you have suggestions on how to make this work? Maybe making the child focusable: false or something?

[pedromassango]

@chunhtai do you have suggestions on how to make this work? Maybe making the child focusable: false or something?

Isn't focusable false by default?

[chunhtai]

I don't think the original use case makes sense. If their end goal is to create a link that looks like a button, they should style it themselves instead of wrapping button with a link.

The original issue is like doing

<a href="https://www.w3schools.com"><button type="button" onclick="alert('Hello world!')">Click Me!</button></a>

[pedromassango]

they should style it themselves instead of wrapping button with a link.

Can you give a POC showcasing how this is possible with the current implementation of the Link package? You're missing the focus of the issue: when focusing a widget wrapped with the Linkpackage, some widget deep within theLink` widget gets focused as well, requiring double TAB.

All Link customers want is to be able to provide this native right-click behavior to any widget. See the image below where you can have the same behavior on a Button in GitHub.

[chunhtai]

All Link customers want is to be able to provide this native right-click behavior to any widget.

on I forgot it doesn't register ontap. yeah in this case, Link is not a full fledged a tag equivalent.

The problem is elevated button itself will create a semantics container. You mentioned mergesemantics doesn't work, do you know which part it is not working?

[mdebbar]

I don't think the original use case makes sense. If their end goal is to create a link that looks like a button, they should style it themselves instead of wrapping button with a link.

The original issue is like doing

<a href="https://www.w3schools.com"><button type="button" onclick="alert('Hello world!')">Click Me!</button></a>

This is actually exactly the use case that the Link widget was designed for 🙂

In the past, people did:

SomeButton(
  onPressed: () {
    launchUrl("https://www.w3schools.com");
  },
  child: Text("Click here"),
)

and complained that they didn't get a proper web link behavior (cmd+click, context menu, etc).

So we wanted people to do this instead:

Link(
  uri: Uri.parse("https://www.w3schools.com"),
  builder: (ctx, onPressed) => SomeButton(
    onPressed: onPressed,
    child: Text("Click here"),
  ),
)

which lets them use all existing buttons and widgets, and makes them behave like real web links.

[chunhtai]

If you do this on the web

<a href="https://www.w3schools.com"><button type="button" onclick="alert('Hello world!')">Click Me!</button></a>

you will also need two tabs

[pedromassango]

You mentioned mergesemantics doesn't work, do you know which part it is not working?

Yes, the problem is that when you merge them, the semantics's Uri of the link gets lost.

It looks like the engine is removing it. Likely because the child of the Link widget has its semantics button and is is taking precedence over the link's semantics properties. Interesting enough, the Link's semantics link: true is keeped properly.

Will share the semantics tree diff here in a few hours

[chunhtai]

I can think of several way.

1. use MergeSemantics and adjust web engine to let link take priority (bandage fix)
2. expose isSemanticsButton in elevatedButton, use Mergesemantics, and ask developer to set ElevateButton(isSemanticsButton: false)
3. Convert link:true to SemanticsRole (possibly button too but may be a bigger breaking change), in this case mergeSemantics will prioritize the role of parent widget

[pedromassango]

I can think of several way.

1. use MergeSemantics and adjust web engine to let link take priority (bandage fix)
2. expose isSemanticsButton in elevatedButton, use Mergesemantics, and ask developer to set ElevateButton(isSemanticsButton: false)
3. Convert link:true to SemanticsRole (possibly button too but may be a bigger breaking change), in this case mergeSemantics will prioritize the role of parent widget

I like the first suggestion, tho I don't think it is a "bandage" fix. "Natively", buttons with a link (aka have a href) are treated as a link and even VoiceOver reads them as Link. Take GitHub for instance, any component with a href regradless of their appearance, is treated as link, as long as it have a href.

Back button

Looks like a button but is treated like a link and VoiceOver reads it as link

Pull requests button

Looks like a an icon button but is treated like a link and VoiceOver reads it as a link

What looks like a primary button...

Is treated like a link as well, because it have a href

That said, I will modify this PR to fix it by modifying the engine

[pedromassango]

@chunhtai any way you could help me with the fix in Flutter's framework/engine? Initially I though by adding the missing _linkUrl ??= child._linkUrl; in the void absorb(...) would ge me closer to the fix.

I would say, I'm now bakc to square zero as I ran out of ideas, would appreciate some hints on where to start

https://github.com/flutter/flutter/blob/9dd1ccb7010a7441b6d44fc5fc1be6842cfd1361/packages/flutter/lib/src/semantics/semantics.dart#L5994-L6007

[pedromassango]

After you make changes in engine sources, you have to rebuild the engine using felt build. And when you run your app, you have to use --local-web-sdk=wasm_release in order to pick the locally built engine.

Yeah, I totally missed this, let me try again.
Will also share a draft PR so you can have a look

btw, I am mostly familiar with the framework & pretty new to the engine

[pedromassango]

Ok, this is ready for review, I still would love the Link widget to merge the semantics itself, if you allow for it. Reason being without this, the framework/engine fix is somewhat useless and we would still need to educate customers in how to "workaround the issue".

By letting Link merge semantics, we ensure there will always be only one semantic node.

[pedromassango]

Finally have a draft PR for the engine: flutter/flutter#174473. Pleas have a look!

Turns out I was missing the commands #9815 (comment) to very my fixes 🫠

[chunhtai]

LGTM, except for the change log and ci

[pedromassango]

@chunhtai any hints on why some tests are failing? 🤔 It is curious because:

1. The one I added works fine
2. The other tests are failing even after removing my changes which means this PR don't cause them to break

Is it ok to merge with the unrelated failing tests?

[stuartmorgan-g]

Is it ok to merge with the unrelated failing tests?

No, absolutely not. If you believe there are unrelated failures, escalate to someone, don't just force-land your PR. If you are wrong, then you will break the tree, and if you are right, then you are effectively landing a PR into a broken tree for a purpose other than fixing the tree, which isn't allowed either.

[stuartmorgan-g]

2. The other tests are failing even after removing my changes which means this PR don't cause them to break

All presubmits are passing in the revert PR.