Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@mdebbar
Copy link
Contributor

@mdebbar mdebbar commented Jun 21, 2024

The new property allows the user to specify a URI for their semantics link node. It's plumbed through for both web and non-web engines, but it's only used in the web engine currently. It sets the href of the anchor element associated with semantics node.

This is going to unlock better semantics support in the Link widget on web (PR).

Framework counterpart: flutter/flutter#150639

Part of flutter/flutter#150263

@mdebbar mdebbar requested review from chunhtai and yjbanov June 21, 2024 20:36
@github-actions github-actions bot added the platform-web Code specifically for the web engine label Jun 21, 2024
final int headingLevel;

/// See [ui.SemanticsUpdateBuilder.updateNode].
final String? linkUri;
Copy link
Contributor

Choose a reason for hiding this comment

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

linkUri is non-nullable in the updateNode API. Does it need to be nullable here?

Copy link
Contributor

Choose a reason for hiding this comment

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

This mirrors the code for the nullable tooltip field. I think it is non-nullable in the updateNode API to make it easier for the C++ side to handle.

// TODO(mdebbar): Fill in the real link once the framework sends entire uri.
// https://github.com/flutter/flutter/issues/150263.
element.style.display = 'block';
if (semanticsObject.hasLinkUri) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the link change? If yes, this code should probably go into an override of the update method and it should also check for isLinkUriDirty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

childrenInHitTestOrder: childrenInHitTestOrder,
additionalActions: additionalActions,
headingLevel: 0,
linkUri: '',
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 "URL" is more accurate here than "URI". The <a> tag and the Link widget only work with URIs that are also fetchable over the network via the same URI. Using arbitrary URIs as abstract identifiers without being able to fetch them is not something that's supported by this property, AFAICT. Unless there's something on the mobile side that extends beyond that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

@harryterkelsen harryterkelsen changed the title Add Semantics Property linkUri Add Semantics Property linkUrl Jul 3, 2024
Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

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

lgtm

@harryterkelsen harryterkelsen added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 3, 2024
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jul 3, 2024
@auto-submit
Copy link
Contributor

auto-submit bot commented Jul 3, 2024

auto label is removed for flutter/engine/53507, due to - The status or check suite Linux Web Framework tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App platform-web Code specifically for the web engine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants