[web] Move announcement live elements to the end of the DOM and make them `div`s instead of `label`s. #42432

marcianx · 2023-05-30T22:04:48Z

Moving them to the end prevents the screen reader from landing on them before the relevant content.
Making them divs instead of labels prevents some screen readers (ChromeVox in particular) from landing on the live elements when the live elements are empty.

Pre-launch Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the [CLA].
All existing and new tests are passing.

…them `div`s instead of `label`s. - Moving them to the end prevents the screen reader from landing on them before the relevant content. - Making them `div`s instead of `label`s prevents some screen readers (ChromeVox in particular) from landing on the live elements when the live elements are empty. Fixes #127862.

yjbanov · 2023-05-30T22:20:55Z

lib/web_ui/lib/src/engine/embedder.dart

        .instance.semanticsHelper
        .prepareAccessibilityPlaceholder();

+    final DomElement announcementsElement = createDomElement('div');


Does it literally need to be <div>? For consistency with other top-level elements it would be good for it to have a custom name, such as <flt-announcement-host>.

Sure, updated per your suggestion and also updated the two live elements and confirmed that things still work fine on ChromeVox on our app.

…ouncements. And resolve some warnings.

yjbanov

auto-submit · 2023-06-01T19:33:09Z

auto label is removed for flutter/engine, pr: 42432, due to This PR has not met approval requirements for merging. You are not a member of flutter-hackers and need 1 more review(s) in order to merge this PR.

Merge guidelines: You need at least one approved review if you are already part of flutter-hackers or two member reviews if you are not a flutter-hacker before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

ditman

Thanks for de-singletonizing this! Very useful for the upcoming multi-view work!!

ditman · 2023-06-01T20:00:13Z

lib/web_ui/lib/src/engine/embedder.dart

+
    shadowRoot.append(accessibilityPlaceholder);
    shadowRoot.append(_sceneHostElement!);
+    shadowRoot.append(announcementsElement);


Do we need announcements on every possible view, or can we have a single global instance?

Where would we put that instance if it's not flutterViewEmbedder? Would that be a separate singleton object? In other words, would you like this to be re-singleton-ified? If so, then the question in the multi-view work is, what can be used to ensure that the live elements end up at/near the end of the DOM? (That being said placing them at the end might be slightly less critical after the two other fixes in this PR and the last.)

AFAIAA, there's also not a strict issue with having multiple live elements. Each live element is already implemented to support multiple announcements at once.

If we can have multiple live elements with no problem, let's go with this, that's why it's LGTM'd :)

(Without wanting to derail this PR: since you said that this needs to be near the end of the DOM, we could put it as the "last child" of the "body" tag. And this wouldn't need to be a re-singletonized: instances would need to either create the DOM object in the DOM if it wasn't there already, or reuse the one created by other instance previously, since all they do is adding some elements to the DOM when asked to announce something.)

The challenge is that inserting the live element div at the end of the DOM only works if subsequently initialized DOM elements don't also append themselves to document.body, thereby ending up after the live element. Effectively, you'd still need some view-level abstraction that can be used to append more elements to a view without adding them after the accessibility DOM for reliability.

mdebbar · 2023-06-01T20:49:14Z

lib/web_ui/lib/src/engine/embedder.dart

        .instance.semanticsHelper
        .prepareAccessibilityPlaceholder();

+    final DomElement announcementsElement = createDomElement('ftl-announcement-host');


Suggested change

final DomElement announcementsElement = createDomElement('ftl-announcement-host');

final DomElement announcementsElement = createDomElement('flt-announcement-host');

Ah, thanks! I'd used the same convention as accessibility.dart, but I guess that was also a typo originally. Fixed both.

ditman · 2023-06-02T01:49:21Z

(Restarted failing checks, they seemed to be an infra problem)

auto-submit · 2023-06-02T06:46:18Z

auto label is removed for flutter/engine, pr: 42432, 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.

The status or check suite Linux Framework Smoke Tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

marcianx · 2023-06-02T14:42:03Z

I've re-run the failing tests twice and they have consistently failed with what looks like infrastructural errors:

Linux Framework Smoke Tests: "Infra Failure: Step('Framework test.test: Framework test') (canceled) (retcode: 0)"
Linux Web Framework tests: "Empty summaryMarkdown"

Why is this PR showing these errors when some of the other PRs I sampled are not? Most of the others don't trigger "Linux Web Framework tests", but some do trigger "Linux Framework Smoke Tests" which seem to be passing for them.

mdebbar · 2023-06-02T17:57:22Z

@marcianx you might be stuck on a bad commit or something. I rebased the PR, let's see if things go green now 🤞

… and make them `div`s instead of `label`s. (flutter/engine#42432)

…128142) flutter/engine@c6e9383...b4250ac 2023-06-02 [email protected] Manually roll ANGLE (flutter/engine#42169) 2023-06-02 [email protected] [Impeller] Fix a bugprone-unchecked-optional-access warning in SurfaceMTL (flutter/engine#42518) 2023-06-02 [email protected] [web] Move announcement live elements to the end of the DOM and make them `div`s instead of `label`s. (flutter/engine#42432) 2023-06-02 [email protected] Roll Fuchsia Mac SDK from JQRQ1nH1ILNA--N_b... to gcm-vsCu6IPUZZnN0... (flutter/engine#42515) 2023-06-02 [email protected] Add cpu as drone dimension to mac ios build. (flutter/engine#42514) 2023-06-02 [email protected] Move benchmarks no upload to a test. (flutter/engine#42356) 2023-06-02 [email protected] [Android] Change Linux Android Emulator Tests to run on a device running API 33 (flutter/engine#42492) 2023-06-02 [email protected] Roll Skia from 47b0db43f6a4 to daf2219fec54 (1 revision) (flutter/engine#42513) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from JQRQ1nH1ILNA to gcm-vsCu6IPU 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],[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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

flutter-dashboard bot added the platform-web Code specifically for the web engine label May 30, 2023

yjbanov approved these changes May 30, 2023

View reviewed changes

marcianx added 2 commits May 30, 2023 23:56

Used <flt-announcement-*> for announcement-related elements.

5748fe4

Update additional engine tests that required mocking AccessibilityAnn…

418bc86

…ouncements. And resolve some warnings.

yjbanov approved these changes May 31, 2023

View reviewed changes

yjbanov added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 1, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 1, 2023

ditman approved these changes Jun 1, 2023

View reviewed changes

mdebbar reviewed Jun 1, 2023

View reviewed changes

Rename 'ftl-' prefix to 'flt-' prefix as originally intended.

e504652

marcianx added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 2, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 2, 2023

Merge branch 'main' into live-to-end

9c9f869

mdebbar approved these changes Jun 2, 2023

View reviewed changes

marcianx added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 2, 2023

auto-submit bot merged commit 274064e into flutter:main Jun 2, 2023

engine-flutter-autoroll mentioned this pull request Jun 2, 2023

Roll Flutter Engine from c6e9383f2143 to b4250acbf3ca (8 revisions) flutter/flutter#128142

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 2, 2023

274064ed1 [web] Move announcement live elements to the end of the DOM…

c933e9c

… and make them `div`s instead of `label`s. (flutter/engine#42432)

marcianx deleted the live-to-end branch June 5, 2023 22:05

	final DomElement announcementsElement = createDomElement('ftl-announcement-host');
	final DomElement announcementsElement = createDomElement('flt-announcement-host');

[web] Move announcement live elements to the end of the DOM and make them divs instead of labels. #42432

[web] Move announcement live elements to the end of the DOM and make them divs instead of labels. #42432

Uh oh!

Conversation

marcianx commented May 30, 2023 • edited by yjbanov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

yjbanov May 30, 2023

Choose a reason for hiding this comment

Uh oh!

marcianx May 31, 2023

Choose a reason for hiding this comment

Uh oh!

yjbanov left a comment

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Jun 1, 2023

Uh oh!

ditman left a comment

Choose a reason for hiding this comment

Uh oh!

ditman Jun 1, 2023

Choose a reason for hiding this comment

Uh oh!

marcianx Jun 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ditman Jun 2, 2023

Choose a reason for hiding this comment

Uh oh!

marcianx Jun 2, 2023

Choose a reason for hiding this comment

Uh oh!

mdebbar Jun 1, 2023

Choose a reason for hiding this comment

Uh oh!

marcianx Jun 1, 2023

Choose a reason for hiding this comment

Uh oh!

ditman commented Jun 2, 2023

Uh oh!

auto-submit bot commented Jun 2, 2023

Uh oh!

marcianx commented Jun 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdebbar commented Jun 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[web] Move announcement live elements to the end of the DOM and make them `div`s instead of `label`s. #42432

[web] Move announcement live elements to the end of the DOM and make them `div`s instead of `label`s. #42432

marcianx commented May 30, 2023 •

edited by yjbanov

Loading

marcianx Jun 1, 2023 •

edited

Loading

marcianx commented Jun 2, 2023 •

edited

Loading