Modify the SemanticTab class to create <a> element when the node has … #180339
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
github-actions
bot
added
engine
![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 aims to enable link functionality for tabs with a linkUrl on Flutter Web by rendering them as <a> elements. While the implementation correctly handles tabs that have a linkUrl upon creation, it has a critical flaw in handling dynamic updates. If a linkUrl is added or removed after the tab is created, the underlying DOM element type is not updated, leading to incorrect behavior. I've provided a detailed comment explaining the issue and suggesting a more robust architectural solution.
| DomElement createElement() { | ||
| // When the tab has a link URL, render as an <a> element to enable | ||
| // proper link functionality (right-click copy, SEO indexing) while | ||
| // still using role="tab" for accessibility. | ||
| if (semanticsObject.hasLinkUrl) { | ||
| final DomElement element = domDocument.createElement('a'); | ||
| element.style.display = 'block'; | ||
| return element; | ||
| } | ||
| return super.createElement(); | ||
| } | ||
|
|
||
| @override | ||
| void update() { | ||
| super.update(); | ||
|
|
||
| // Update the href attribute when the link URL changes. | ||
| if (semanticsObject.isLinkUrlDirty) { | ||
| if (semanticsObject.hasLinkUrl) { | ||
| element.setAttribute('href', semanticsObject.linkUrl!); | ||
| } else { | ||
| element.removeAttribute('href'); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This implementation has a fundamental flaw regarding dynamic updates. The createElement method is only called once when the SemanticRole is instantiated. If a linkUrl is added to or removed from the SemanticsObject later, this method won't be called again, and the DOM element type will not change. This leads to incorrect behavior in the update method.
Specifically:
- If a tab is created without a
linkUrl, it gets a<flt-semantics>element. IflinkUrlis added later, theupdatemethod will incorrectly set anhrefattribute on this non-anchor element. - If a tab is created with a
linkUrl, it gets an<a>element. IflinkUrlis removed later, it remains an<a>element, just without anhref, which is not ideal.
To correctly handle dynamic changes, the element type needs to be swapped. This is typically done by changing the EngineSemanticsRole kind, which triggers disposal of the old role and creation of a new one with the correct element. This would likely require changes in _getEngineSemanticsRole in semantics.dart to return a different role kind based on hasLinkUrl for tabs.
1 participant
Enable link functionality for tabs with linkUrl on Flutter Web
This PR enables proper link functionality for tabs that have a
linkUrlset on Flutter Web. Previously, when using tabs for navigation menus with URLs (e.g., wrapping aTabwith theLinkwidget fromurl_launcher), the link semantics were lost becauseSemanticTabalways rendered as a generic<flt-semantics>element withrole="tab". fixes #180321Problem
When tabs are used for app navigation menus on web:
Solution
Modified
SemanticTabto:<a>element whenlinkUrlis present, while maintainingrole="tab"for accessibilityhrefattribute based on thelinkUrlvaluehrefattribute dynamically whenlinkUrlchangesThis results in HTML like
<a role="tab" href="https://example.com">Tab Label</a>, which provides:role="tab")Before
After (when linkUrl is set)
Fixes https://github.com/flutter/flutter/issues/XXXXX
Pre-launch Checklist
///).