[url_launcher] fix: tab traversal on web #10020
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes an accessibility issue in url_launcher_web where the Link widget created duplicate semantic nodes. The change wraps the existing Semantics widget in _buildChild with a MergeSemantics widget to combine the semantic information of the link and its child. A new integration test has been added to verify that a MergeSemantics widget is present in the widget tree for a WebLinkDelegate. The package version has been bumped to 2.4.2 and the CHANGELOG.md has been updated to reflect this fix. My review includes a suggestion to align the new changelog entry with the repository's style guide.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
# Conflicts: # packages/url_launcher/url_launcher/example/pubspec.yaml # packages/url_launcher/url_launcher/pubspec.yaml # packages/url_launcher/url_launcher_ios/pubspec.yaml # packages/url_launcher/url_launcher_linux/pubspec.yaml # packages/url_launcher/url_launcher_platform_interface/pubspec.yaml # packages/url_launcher/url_launcher_web/CHANGELOG.md # packages/url_launcher/url_launcher_web/pubspec.yaml # packages/url_launcher/url_launcher_windows/pubspec.yaml
chunhtai
left a comment
chunhtai
left a comment
There was a problem hiding this comment.
LGTM, just a unrelated question not blocking the merge.
| matchesSemantics( | ||
| isLink: true, | ||
| identifier: 'test-link-12', | ||
| // linkUrl: 'https://foobar/example?q=1', |
There was a problem hiding this comment.
This is not introduced in this pr, do you know why the semantics node didn't set the link Url? it feels like it should
There was a problem hiding this comment.
Hmm, I have no idea, it looks like the dev was missing the fix I am trying to land here in order for the test to pass with this line uncommented 🤔 I would need more time to trace it tho and have a proper answer
There was a problem hiding this comment.
or... it was missing the engine fix I landed last year (as part of this PR), see this comment: https://github.com/flutter/flutter/pull/174473/changes#r2307776968 or your reasoning behind that fix here
github-actions
bot
added
platform-ios
and removed
platform-linux
platform-windows
labels
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
The random formatting change in url_launcher_macos (maybe you ran format there for some reason, but didn't pub get first?) will also need to be reverted to pass CI.
Soo confusing, the test was failing because that macos file wasn't formatter correct, so I formatted it according to the CI/Luci output and still failing |
|
There's no reason for this PR to touch that file or package at all, so the solution is to revert the file, not to reformat it. |
|
This usually means you have an out of date pub, try to do flutter update-packages in flutter checkout and make sure it is the latest master branch |
4 participants
Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
Continuation of #9815
Fixes flutter/flutter#157689
Pre-Review Checklist
[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///).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-assistbot 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
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3