Fix AXMenuListOption::GetRelativeBounds [chromium/src : main]
Luis Sanchez Padilla (Gerrit)
Attention is currently required from: Luis Sanchez Padilla.
Set Ready For Review
To view, visit change 4326938. To unsubscribe, or for help writing mail filters, visit settings.
Luis Sanchez Padilla (Gerrit)
Attention is currently required from: Ian Vollick, Mason Freed, Nektarios Paisios.
Luis Sanchez Padilla would like Nektarios Paisios, Ian Vollick and Mason Freed to review this change.
Fix AXMenuListOption::GetRelativeBounds
AXMenuListOption::GetRelativeBounds currently defaults to its owner select element's bounds, even when the select is expanded. This prevents accessibility tools to correctly represent the position of the option elements inside the popup menu. This change enables retrieving the right set of coordinates by caching them inside the corresponding AXMenuList when the popup's document is loaded.
Change-Id: I88e3bc1d2a9c1ea2076eea4da6d4a0f919c16cd7
---
A third_party/blink/manual_tests/forms/list-picker.html
M third_party/blink/renderer/core/accessibility/ax_object_cache.h
M third_party/blink/renderer/core/html/forms/internal_popup_menu.cc
M third_party/blink/renderer/core/html/forms/internal_popup_menu.h
M third_party/blink/renderer/core/html/forms/resources/list_picker.js
M third_party/blink/renderer/core/page/page_popup_client.h
M third_party/blink/renderer/core/page/page_popup_controller.cc
M third_party/blink/renderer/core/page/page_popup_controller.h
M third_party/blink/renderer/core/page/page_popup_controller.idl
M third_party/blink/renderer/modules/accessibility/ax_menu_list.h
M third_party/blink/renderer/modules/accessibility/ax_menu_list_option.cc
M third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.cc
M third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.h
13 files changed, 280 insertions(+), 4 deletions(-)
Aaron Leventhal (Gerrit)
Attention is currently required from: Ian Vollick, Luis Sanchez Padilla, Mason Freed.
Aaron Leventhal removed Nektarios Paisios from this change.
Aaron Leventhal (Gerrit)
Attention is currently required from: Ian Vollick, Luis Sanchez Padilla, Mason Freed.
1 comment:
Patchset:
Nektarios is away. Also, I'm the primary reviewer for any major changes to Blink a11y.
To view, visit change 4326938. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Ian Vollick, Luis Sanchez Padilla, Mason Freed.
2 comments:
Commit Message:
Patch Set #8, Line 9:
AXMenuListOption::GetRelativeBounds currently defaults to its owner select element's bounds, even when the select is expanded. This prevents accessibility tools to correctly represent the position of the option elements inside the popup menu. This change enables retrieving the right set of coordinates by caching them inside the corresponding AXMenuList when the popup's document is loaded.Are we still looking at making <select> not use AXMenuList on Windows? or does this CL replace the need for that?
File third_party/blink/renderer/modules/accessibility/ax_menu_list_option.cc:
Patch Set #8, Line 189:
if (options_bounds.size()) {We should early return or DCHECK if the index is larger than the bounds.
Have you tested that the indices don't get out of whack when there are some <optgroup> elements present? Or if some script inserts non-option elements that we didn't expect? Does a comment node throw it out of whack?
To view, visit change 4326938. To unsubscribe, or for help writing mail filters, visit settings.
Luis Sanchez Padilla (Gerrit)
Attention is currently required from: Ian Vollick, Luis Sanchez Padilla, Mason Freed.
Luis Sanchez Padilla uploaded patch set #9 to this change.
Fix AXMenuListOption::GetRelativeBounds
AXMenuListOption::GetRelativeBounds currently defaults to its owner
select element's bounds, even when the select is expanded. This prevents
accessibility tools to correctly represent the position of the option
elements inside the popup menu. This change enables retrieving the right
set of coordinates by caching them inside the corresponding AXMenuList
when the popup's document is loaded.
Change-Id: I88e3bc1d2a9c1ea2076eea4da6d4a0f919c16cd7
---
A third_party/blink/manual_tests/forms/list-picker.html
M third_party/blink/renderer/core/accessibility/ax_object_cache.h
M third_party/blink/renderer/core/html/forms/internal_popup_menu.cc
M third_party/blink/renderer/core/html/forms/internal_popup_menu.h
M third_party/blink/renderer/core/html/forms/resources/list_picker.js
M third_party/blink/renderer/core/page/page_popup_client.h
M third_party/blink/renderer/core/page/page_popup_controller.cc
M third_party/blink/renderer/core/page/page_popup_controller.h
M third_party/blink/renderer/core/page/page_popup_controller.idl
M third_party/blink/renderer/modules/accessibility/ax_menu_list.h
M third_party/blink/renderer/modules/accessibility/ax_menu_list_option.cc
M third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.cc
M third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.h
13 files changed, 280 insertions(+), 4 deletions(-)
To view, visit change 4326938. To unsubscribe, or for help writing mail filters, visit settings.
Luis Sanchez Padilla (Gerrit)
Attention is currently required from: Aaron Leventhal, Ian Vollick, Mason Freed.
3 comments:
Commit Message:
Patch Set #8, Line 9:
AXMenuListOption::GetRelativeBounds currently defaults to its owner select element's bounds, even when the select is expanded. This prevents accessibility tools to correctly represent the position of the option elements inside the popup menu. This change enables retrieving the right set of coordinates by caching them inside the corresponding AXMenuList when the popup's document is loaded.
Are we still looking at making <select> not use AXMenuList on Windows? or does this CL replace the n […]
This CL is not really a replacement as removing AXMenuList would still be beneficial for cases like https://bugs.chromium.org/p/chromium/issues/detail?id=1293030. This change seems less risky than the disablement of AXMenuList, so I think we can use this change to expose the correct bounds for the options and move forward with removing AXMenuList in order to fix other scenarios. What do you think?
Patchset:
Addressed feedback. Thank you!
File third_party/blink/renderer/modules/accessibility/ax_menu_list_option.cc:
Patch Set #8, Line 189:
if (options_bounds.size()) {
We should early return or DCHECK if the index is larger than the bounds. […]
Added the DCHECK on patchset 10, and also confirmed that indices to remain as expected even when options are inside optgroups. Other types of elements get ignored when computing the index of an option. https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/option_list.cc;l=13?q=OptionListIterator::Advance
To view, visit change 4326938. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Aaron Leventhal, Ian Vollick, Luis Sanchez Padilla.
5 comments:
Patchset:
Generally looks pretty good to me. It's really unfortunate that there is only a manual test for this. Is there any other way to verify the behavior?
File third_party/blink/manual_tests/forms/list-picker.html:
Cool, thanks for adding this!
File third_party/blink/renderer/core/html/forms/internal_popup_menu.cc:
Patch Set #10, Line 700:
SetMenuListOptionsBoundsI went back and forth on this. But it feels like this should be called something like `SetMenuListOptionsBoundsInAXTree()` or something to indicate where/how the options bounds are used.
I could be convinced otherwise, since the options bounds are kept in a correct state and could potentially be used for other purposes later.
Patch Set #10, Line 701:
WTF::Vector<gfx::Rect>& options_boundsHelp me understand why this gets modified by this function? I looked through the call stack and it doesn't look like the modified value gets used. I could have missed it. But if not, could you make this `const WTF::Vector<const gfx::Rect>&` and avoid changing it?
File third_party/blink/renderer/core/html/forms/resources/list_picker.js:
Patch Set #10, Line 521:
optionsBoundsCache_So if I'm reading this right, this is less of a "cache", and more of a global variable to hold the results as you recursively walk the children. In that case, would you mind breaking this function into two pieces, one that you call on the selectmenu, and the other that is just used locally for recursion, and then just pass in the holding variable to collect the option bounds before the call to setMenuListOptionsBounds?
To view, visit change 4326938. To unsubscribe, or for help writing mail filters, visit settings.
Benjamin Beaudry (Gerrit)
Attention is currently required from: Aaron Leventhal, Ian Vollick, Luis Sanchez Padilla.
1 comment:
Patchset:
Have you tried on monitors with different zoom levels?
To view, visit change 4326938. To unsubscribe, or for help writing mail filters, visit settings.
Luis Sanchez Padilla (Gerrit)
Attention is currently required from: Aaron Leventhal, Benjamin Beaudry, Ian Vollick, Mason Freed.
7 comments:
Patchset:
Have you tried on monitors with different zoom levels?
Yes, I tested with native ui zoom, magnifying tools, and internal zoom, and all work as expected.
Generally looks pretty good to me. […]
It is particularly complicated to test this fix, as it would require:
- The select element to be expanded, either programmatically or via injection.
- The ability to wait for JavaScript to run on the popup's document so that the menu can be populated with the options.
- Access to the AX tree so that the AXMenuListOptions are reachable and GetRelativeBounds is callable.
I couldn't find a current testing mechanism that enables that, would you have any suggestions?
Patchset:
Apologies for the delay. I was part of an on-call rotation for the last two weeks and it took all of my time. Thank you for your patience!
File third_party/blink/renderer/core/html/forms/internal_popup_menu.cc:
Patch Set #10, Line 700:
SetMenuListOptionsBounds
I went back and forth on this. […]
I had something similar originally and decided to drop the A11y part. Added it back in patchset 11.
Patch Set #10, Line 701:
WTF::Vector<gfx::Rect>& options_bounds
Help me understand why this gets modified by this function? I looked through the call stack and it d […]
InternalPopupMenu::SetMenuListOptionsBoundsInAXTree is called with the option bounds relative to the origin of the popup's frame, so they still need to be offset by the difference between the popup's frame's and the main frame's origins, which is done in this function. The modified value gets passed in AXObjectCache::SetMenuListOptionsBounds at the end of this function.
File third_party/blink/renderer/core/html/forms/resources/list_picker.js:
Patch Set #10, Line 521:
optionsBoundsCache_
So if I'm reading this right, this is less of a "cache", and more of a global variable to hold the r […]
Done in Patchset 11.
File third_party/blink/renderer/modules/accessibility/ax_menu_list_option.cc:
Patch Set #8, Line 189:
if (options_bounds.size()) {
Added the DCHECK on patchset 10, and also confirmed that indices to remain as expected even when opt […]
Done
To view, visit change 4326938. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Aaron Leventhal, Benjamin Beaudry, Ian Vollick, Luis Sanchez Padilla.
Patch set 11:Code-Review +1
3 comments:
Patchset:
It is particularly complicated to test this fix, as it would require: […]
No. 😞 I was hoping you'd know of some magic.
So, this is ok as-is. Thanks for trying.
Patchset:
LGTM! Thanks for the changes.
File third_party/blink/renderer/core/html/forms/internal_popup_menu.cc:
Patch Set #10, Line 701:
WTF::Vector<gfx::Rect>& options_boundsInternalPopupMenu::SetMenuListOptionsBoundsInAXTree is called with the option bounds relative to the […]
Acknowledged
To view, visit change 4326938. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Benjamin Beaudry, Ian Vollick, Luis Sanchez Padilla.
To view, visit change 4326938. To unsubscribe, or for help writing mail filters, visit settings.
Benjamin Beaudry (Gerrit)
Attention is currently required from: Ian Vollick, Luis Sanchez Padilla.
Patch set 11:Code-Review +1
To view, visit change 4326938. To unsubscribe, or for help writing mail filters, visit settings.
Luis Sanchez Padilla (Gerrit)
Attention is currently required from: Benjamin Beaudry, Ian Vollick, Luis Sanchez Padilla.
Patch set 11:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
Fix AXMenuListOption::GetRelativeBounds
AXMenuListOption::GetRelativeBounds currently defaults to its owner
select element's bounds, even when the select is expanded. This prevents
accessibility tools to correctly represent the position of the option
elements inside the popup menu. This change enables retrieving the right
set of coordinates by caching them inside the corresponding AXMenuList
when the popup's document is loaded.
Change-Id: I88e3bc1d2a9c1ea2076eea4da6d4a0f919c16cd7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4326938
Reviewed-by: Mason Freed <mas...@chromium.org>
Commit-Queue: Luis Sanchez Padilla <lusa...@microsoft.com>
Reviewed-by: Benjamin Beaudry <benjamin...@microsoft.com>
Reviewed-by: Aaron Leventhal <aleve...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1124967}
---
A third_party/blink/manual_tests/forms/list-picker.html
M third_party/blink/renderer/core/accessibility/ax_object_cache.h
M third_party/blink/renderer/core/html/forms/internal_popup_menu.cc
M third_party/blink/renderer/core/html/forms/internal_popup_menu.h
M third_party/blink/renderer/core/html/forms/resources/list_picker.js
M third_party/blink/renderer/core/page/page_popup_client.h
M third_party/blink/renderer/core/page/page_popup_controller.cc
M third_party/blink/renderer/core/page/page_popup_controller.h
M third_party/blink/renderer/core/page/page_popup_controller.idl
M third_party/blink/renderer/modules/accessibility/ax_menu_list.h
M third_party/blink/renderer/modules/accessibility/ax_menu_list_option.cc
M third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.cc
M third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.h
13 files changed, 285 insertions(+), 4 deletions(-)