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

Conversation

@yjbanov
Copy link
Contributor

@yjbanov yjbanov commented Jul 2, 2024

Switch to using semantic heading tags (h1, h2, etc).

Fix missing secondary roles: focus, live region, route name, and label.
Improves indexability (flutter/flutter#46789)

@yjbanov yjbanov requested a review from chunhtai July 2, 2024 23:09
@github-actions github-actions bot added the platform-web Code specifically for the web engine label Jul 2, 2024
///
/// Normally, heading elements are not focusable as they do not receive
/// keyboard input. However, when a route is pushed (e.g. a dialog pops up),
/// the it may be desirable to move the screen reader focus to the heading
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the -> then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

void addHeadingRole() {
setAriaRole('heading');
}
DomElement createElement() => createDomElement('h${semanticsObject.headingLevel}');
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if the headingLevel changes? Does the update automatically create a new element and call this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heading level should not change. It confuses screen readers. Web crawlers snapshot the heading at its default value too, so changing has no effect either. We could issue a warning, but I'd rather such warnings are issued by the framework, and the engine will just do a graceful degradation of functionality (in this case the heading level stays the same forever).

@yjbanov yjbanov requested a review from harryterkelsen July 3, 2024 21:55
Copy link
Contributor

@harryterkelsen harryterkelsen left a comment

Choose a reason for hiding this comment

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

LGTM

@yjbanov yjbanov added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 5, 2024
@auto-submit auto-submit bot merged commit c6cb09d into flutter:main Jul 5, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 5, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 5, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jul 5, 2024
victorsanni pushed a commit to victorsanni/flutter that referenced this pull request Jul 8, 2024
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.

2 participants