New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[web:a11y] support dialogs described by descendants #42108
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a couple questions/comments.
// Setting the label for the first time. Wait for the DOM tree to be | ||
// established, then find the nearest dialog and update its label. | ||
semanticsObject.owner.addOneTimePostUpdateCallback(() { | ||
_lookUpNearestAncestorDialog(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to check that this
hasn't been disposed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
/// rely on this value in the middle of a semantics tree update. It is safe to | ||
/// use this value in post-update callback (see | ||
/// [EngineSemanticsOwner.addOneTimePostUpdateCallback]). | ||
SemanticsObject? get parent => _parent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to have an assert that throws if we are in the middle of an update. Not sure what's the best way to do that though. Maybe a flag EngineSemanticsOwner.isUpdating
and then:
SemanticsObject? get parent => _parent; | |
SemanticsObject? get parent { | |
assert(!owner.isUpdating); | |
return _parent; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
} | ||
return true; | ||
}()); | ||
semanticsObject.element.setAttribute('aria-label', label ?? ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see where the aria-label
is set for the case where namesRoute
is a descendant of scopesRoute
.
Does it happen through the LabelAndValue
role manager? If yes, then why are we doing it in the Dialog
role manager too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we don't set aria-label
when a descendant provides a description. We use aria-describedby="ID_OF_DESCENDANT"
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And then that descendant is supposed to set the aria-label
, right? I don't see RouteName
doing it, so I'm assuming that LabelAndValue
is doing it. But LabelAndValue
should be doing it for Dialog
as well. Maybe I'm missing something here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, the final state is like this:
<div role="dialog" aria-describedby="describer">
<div id="describer">Dialog description</div>
</div>
No aria-label
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. I was confused by this test:
expectSemanticsTree('''
<sem aria-describedby="flt-semantic-node-2" style="$rootSemanticStyle">
<sem-c>
<sem>
<sem-c>
<sem aria-label="$label"></sem>
</sem-c>
</sem>
</sem-c>
</sem>
''');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, it's because the descendant can provide the description by applying aria-label
to itself, but it can also be a group of elements that provide a description together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the improvements! LGTM again!
…127156) flutter/engine@9039c2d...bca11a4 2023-05-19 yjbanov@google.com [web:a11y] support dialogs described by descendants (flutter/engine#42108) 2023-05-18 47866232+chunhtai@users.noreply.github.com Makes android embedding to send full uri (flutter/engine#41836) 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 rmistry@google.com,zra@google.com 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
…lutter#127156) flutter/engine@9039c2d...bca11a4 2023-05-19 yjbanov@google.com [web:a11y] support dialogs described by descendants (flutter/engine#42108) 2023-05-18 47866232+chunhtai@users.noreply.github.com Makes android embedding to send full uri (flutter/engine#41836) 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 rmistry@google.com,zra@google.com 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
Add a new
RouteName
semantic role for nodes withnamesRoute
set without ascopesRoute
. Such nodes provide a description for the nearest ancestor dialog node. The web equivalent of this is when an elementrole="dialog"
is described by pointing to one of its children usingaria-labelledby
andaria-describedby
. Here's an example:(Source)
Flutter currently does not distinguish between "labelled by" and "described by". In my testing, only
aria-describedby
resulted in an announcement of the dialog's description upon opening.aria-labelledby
required that the dialog element itself be focusable, which is not the case. So I went witharia-describedby
.Fixes flutter/flutter#126030