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

[web] scale semantic text elements to match the desired focus ring size #52586

Merged
merged 9 commits into from
May 9, 2024

Conversation

yjbanov
Copy link
Contributor

@yjbanov yjbanov commented May 6, 2024

Due to https://g-issues.chromium.org/issues/40875151?pli=1&authuser=0 and a lack of an ARIA role for plain text nodes (after the removal of role="text" in WebKit recently), there is no way to customize the size of the screen reader focus ring for a plain text element. The focus ring always tightly hugs the text itself.

A workaround implemented in this PR is to match the size of the text exactly to the desired focus ring size. This is done by measuring the size of the text in the DOM, then scaling it both vertically and horizontally to match the size requested by the framework for the corresponding semantics node.

To avoid serious performance penalty, the following optimizations were included:

  • Nodes that are satisfiable by just an aria-label do not need this workaround, and are skipped.
  • Nodes that must use DOM text (e.g. links, buttons) but have ARIA roles that size them based on the element, do not need this workaround, and are skipped.
  • Nodes that do need the workaround are first measured in a single batch, incurring only one page reflow. Then they are all updated in a single batch without taking any further DOM measurements. This ensures that no matter how many text spans are rendered, only one reflow is needed to measure them all.
  • Nodes that need the workaround cache the previous label and size, and if they do not change, the size is not updated.

Other changes:

  • Rename LeafLabelRepresentation to LabelRepresentation, because labels also apply to non-leaf nodes (e.g. aria-label may be applied to a container).
  • Rename labelRepresentation to preferredLabelRepresentation, because a particular label representation cannot be guaranteed. A node that currently looks like a leaf text node may turn out to be an empty container, and after adding children to it must switch from using DOM text to an aria-label. Therefore, role manager only specify a preference, but LabelAndValue ultimately decides which representation is usable.
  • Introduce void initState() in PrimaryRoleManager to be used for one-time initialization of state and DOM structure after all objects that are in a one-to-one relationship with each other create all the references needed to establish that relationship (PrimaryRoleManager, SemanticsObject, element, owner, etc). This is not available at the time the constructors are called.

Fixes flutter/flutter#146774.

@github-actions github-actions bot added the platform-web Code specifically for the web engine label May 6, 2024
@yjbanov yjbanov requested a review from mdebbar May 7, 2024 03:27
// the offsetWidth of the element, and therefore not fully cover the rect
// of the parent. "justify" will ensure the text is stretched edge to edge
// covering full offsetWidth.
..textAlign = 'justify'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still needed after you apply the scaling?

If you make the span inline-block and you scale it to the desired size, I don't see why text-align: justify is needed. Am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me double-check this. This is because I observed offsetWidth != text width when text broke up into multiple lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confirmed that justify is needed: https://jsfiddle.net/yjbanov/mhgdrzek/13/

On my laptop I see this:

Screenshot 2024-05-07 at 3 15 33 PM

Take a look at parent widths 50px, 60px, and 70px. The text is laid out identically, and the screen reader width is the same for all, tightly hugging the text. However, the measured width (based on offsetWidth) is different. If I add text-align: justify, the aaa bbb will stretch the full width of the paragraph, matching offsetWidth.

Comment on lines +346 to +349
final SizedSpanRepresentation representation = measurement.representation;
final double domWidth = measurement.domSize.width;
final double domHeight = measurement.domSize.height;
final ui.Size targetSize = measurement.targetSize;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: use destructuring.

Suggested change
final SizedSpanRepresentation representation = measurement.representation;
final double domWidth = measurement.domSize.width;
final double domHeight = measurement.domSize.height;
final ui.Size targetSize = measurement.targetSize;
final _Measurement(
:SizedSpanRepresentation representation,
:double domWidth,
:double domHeight,
:ui.Size targetSize,
) = measurement;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't. measurement.domSize.width doesn't destructure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops! It can still be done like this:

Suggested change
final SizedSpanRepresentation representation = measurement.representation;
final double domWidth = measurement.domSize.width;
final double domHeight = measurement.domSize.height;
final ui.Size targetSize = measurement.targetSize;
final _Measurement(
:SizedSpanRepresentation representation,
domSize: ui.Size(width: double domWidth, height: double domHeight),
:ui.Size targetSize,
) = measurement;

but I think it's less readable than what you had originally.

@yjbanov yjbanov requested a review from mdebbar May 7, 2024 22:27
Copy link
Contributor

@mdebbar mdebbar left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 53 to 55
LabelRepresentation.ariaLabel => AriaLabelRepresentation._(owner),
LabelRepresentation.domText => DomTextRepresentation._(owner),
LabelRepresentation.sizedSpan => SizedSpanRepresentation._(owner),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is also possible:

Suggested change
LabelRepresentation.ariaLabel => AriaLabelRepresentation._(owner),
LabelRepresentation.domText => DomTextRepresentation._(owner),
LabelRepresentation.sizedSpan => SizedSpanRepresentation._(owner),
ariaLabel => AriaLabelRepresentation._(owner),
domText => DomTextRepresentation._(owner),
sizedSpan => SizedSpanRepresentation._(owner),

@yjbanov yjbanov added the autosubmit Merge PR when tree becomes green via auto submit App label May 9, 2024
@yjbanov yjbanov merged commit 1c12368 into flutter:main May 9, 2024
29 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 10, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 10, 2024
zanderso pushed a commit to flutter/flutter that referenced this pull request May 10, 2024
…148137)

flutter/engine@c0917b1...1ccd0c3

2024-05-10 [email protected] Fixed constness
of display list storage. (flutter/engine#52705)
2024-05-10 [email protected] Revert "Various
documentation improvements (#52600)" (flutter/engine#52709)
2024-05-10 [email protected] Manual roll Dart SDK from
b7cad2edae4b to 01121c008f4d (3 revisions) (flutter/engine#52706)
2024-05-10 [email protected] [iOS] Fix App crash when use WebView with iOS
VoiceOver (flutter/engine#52484)
2024-05-09 [email protected] Various documentation improvements (#52600)
(flutter/engine#52623)
2024-05-09 [email protected] [web] scale semantic text elements to
match the desired focus ring size (flutter/engine#52586)
2024-05-09 [email protected] [Impeller] Adds
impeller display list golden tests (flutter/engine#52690)
2024-05-09 [email protected] Roll buildroot to
70a42312a688 (flutter/engine#52675)
2024-05-09 [email protected] When `et` is not attached
to a terminal, still split lines for status updates.
(flutter/engine#52681)
2024-05-09 [email protected] updated analysis
exclusion (flutter/engine#52699)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that
a human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
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.

[web] a11y focus ring rect frequently broken for plain text
2 participants