-
Notifications
You must be signed in to change notification settings - Fork 6k
Add link support in web accessibility #46117
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.
| } | ||
|
|
||
| /// The element used for link, i.e. `<a>`. | ||
| late DomHTMLElement anchorElement; |
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.
There are still some more work needed to wire up the semantics action to this element, but I would like to know whether this is the way we want to pursuit.
An alternative is to Make semanticsObject.element to be a directly, that way I can reuse all the RoleManager like Tappable or Focusable.
| ..height = '${semanticsObject.rect!.height}px'; | ||
| semanticsObject.element.append(anchorElement); | ||
| anchorElement.setAttribute('href', '#'); | ||
| anchorElement.addEventListener('focus', |
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.
Can you reuse the Focusable role manager instead of implementing custom focus management?
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.
The focusable role manager will only work on SemanticsObject.element I think? If I want to do that, I will need to add a way to create an <a> as SemanticsObject.element instead of the <flt-semantics-node> that it hardcoded to create. I am fine with that, but it seems to divert from what we were doing with text_field.dart
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, in that case you might want AccessibilityFocusManager.
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 updated the code to use RoleManager instead, PTAL
| EnginePlatformDispatcher.instance.invokeOnSemanticsAction( | ||
| semanticsObject.id, ui.SemanticsAction.didLoseAccessibilityFocus, null); | ||
| })); | ||
| anchorElement.addEventListener('click', |
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.
Similarly here, can we reuse the Tappable role manager?
| PrimaryRole.image => ImageRoleManager(this), | ||
| PrimaryRole.platformView => PlatformViewRoleManager(this), | ||
| PrimaryRole.generic => GenericRole(this), | ||
| PrimaryRole.link => Link(this), |
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.
Let's keep the generic role last to underscore that it's the fallback role that we only use as a last resort.
d346d35 to
9922887
Compare
6675306 to
58c57b7
Compare
|
a friendly bump |
| void _updateElement(PrimaryRole role) { | ||
| switch (role) { | ||
| case PrimaryRole.link: | ||
| element = domDocument.createElement('a'); |
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.
The element field is designed to be final, as it participates in the parent's DOM structure. Simply swapping the element field will result in a dangling element not attached to anywhere. Let's chat about what you are trying to do and find a better solution.
e1589f3 to
69fd141
Compare
| /// management intereferes with the widget's functionality. | ||
| PrimaryRoleManager.blank(this.role, this.semanticsObject); | ||
|
|
||
| late final DomElement element = _createAndInitElement(); |
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.
Does it need to be late if it is initialized immediately? late has runtime cost (it uses a null check behind-the-scenes) and complexity cost (it is initialized upon first access, which is less predictable than constructor initialization).
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.
non-late final can only accept static function as its initializer. In this case, there isn't a way for sub class to inject their logic
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 I see, you need to call this.createElement which not static. In that case late make sense. However, I'd still initialize it in the constructor. Otherwise, it gets initialized upon first read of the field, and since it's hard to predict when exactly that field is accessed we can get into race conditions, such as _createAndInitElement reading from semanticsObject that has changed its state after the constructor is finished. Can you move element = _createAndInitElement(); into the constructor body?
bc197b9 to
37126c3
Compare
| @override | ||
| DomElement createElement() { | ||
| final DomElement element = domDocument.createElement('a'); | ||
| // TODO(chunhtai): Fill in the real link once the framework sends entire uri. |
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.
Is there a Github issue this can point to?
| @protected | ||
| DomElement createElement() => domDocument.createElement('flt-semantics'); | ||
|
|
||
| DomElement _createAndInitElement() { |
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.
nit: actually, I'd still keep this static and pass all the information it needs to initialize the element (I think all it needs is the element created by createElement and semanticsObject). This way this method will not need access to this.
37126c3 to
1b71298
Compare
yjbanov
left a comment
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.
…136982) flutter/engine@d46933e...b27e1b3 2023-10-20 [email protected] Add link support in web accessibility (flutter/engine#46117) 2023-10-20 [email protected] [web] Support `flutterViewId` in platform view messages (flutter/engine#46891) 2023-10-20 [email protected] Fix async image loading issues in skwasm. (flutter/engine#47117) 2023-10-20 [email protected] Add option to save Impeller failure images in rendertests (flutter/engine#47142) 2023-10-20 [email protected] Roll Skia from b960e9140f56 to 9ffd5ef9a9ed (3 revisions) (flutter/engine#47167) 2023-10-20 [email protected] [macOS] Eliminate extraneous loadView calls (flutter/engine#47166) 2023-10-20 [email protected] Roll Dart SDK from ba96a157a8eb to 53fee35b299f (1 revision) (flutter/engine#47165) 2023-10-20 [email protected] [Impeller] GPU Tracer for GLES. (flutter/engine#47080) 2023-10-20 [email protected] Roll Skia from de628929015d to b960e9140f56 (2 revisions) (flutter/engine#47164) 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://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
fixes flutter/flutter#134795 ## 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 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. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat

fixes flutter/flutter#134795
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.