Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2119413004:
a11y: Exclude children of nested keyboard accessible controls from a11y tree. (Closed)
|
Created:
4 years, 5 months ago by Patti Lor Modified:
3 years, 7 months ago CC:
chromium-reviews, aboxhall+watch_chromium.org, tfarina, nektar+watch_chromium.org, yuzo+watch_chromium.org, je_julie, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptiona11y: Exclude children of nested keyboard accessible controls from a11y tree.
Currently, children of keyboard accessible controls, such as the Labels inside
LabelButtons, are not hidden from the accessibility tree. This is not useful
because the user only cares about the element that has focus, not any children
inside it, which are implementation details. Set any children of keyboard
accessible controls to have ignored roles, which indicates to the OS (works for
Windows, Mac, and Linux) that the ignored element should be excluded from the
accessibility tree.
BUG=610589, 723574
TEST=On Mac: Open the XCode Accessibility Inspector. On Windows: Open the
Windows SDK 'Inspect.exe' and make sure the icon for 'Watch Focus' is selected.
Then (on Mac, hover over; on Windows, focus) a views::LabelButton (e.g. on the
HTTP Authentication dialog). It should report an empty set of children and its
role ("Type"/"accessibilityRole" on Mac; "Role"/"ControlType" on Windows) should
say it is a button.
Review-Url: https://codereview.chromium.org/2119413004
Cr-Commit-Position: refs/heads/master@{#474867}
Committed: https://chromium.googlesource.com/chromium/src/+/ce2e11f2565db306765cc6f6412cf5bfb158fb70
Patch Set 1 #Patch Set 2 : Tentative switch to use IsAccessibilityFocusable() instead of a flag. #
Total comments: 2
Patch Set 3 : Implement ignored accessibility elements and hide them from accessibility clients, with tests. #Patch Set 4 : Fix compile errors. #Patch Set 5 : Fix last two tests on Mac, compile errors for Linux and Windows. #Patch Set 6 : Re-add compile error fixes. #Patch Set 7 : Fix compile error and final test in NativeWidgetMacTest. #
Total comments: 1
Patch Set 8 : Before revert of cross-platform ignored a11y elements. #Patch Set 9 : Move to a Mac-specific approach by ignoring elements with keyboard focusable parents. #
Total comments: 4
Patch Set 10 : Review comments, ignore View::enabled_. #Patch Set 11 : Don't ignore children if they're accessibility focusable, regardless of enabled state. #
Total comments: 16
Patch Set 12 : Review comments. #
Total comments: 2
Patch Set 13 : Review comments. #
Total comments: 14
Patch Set 14 : Switch back to "using LabelButton::label() and update comment. #Patch Set 15 : Make things cross-platform (with code from patchset 8). #Patch Set 16 : Fix Windows/Linux compilation, use insert instead of emplace. #Patch Set 17 : Don't use assignments inside if conditionals because Windows doesn't like it. #Patch Set 18 : Fix ChildAtIndex() for Widget children. #Patch Set 19 : Refactor to use ui::AX_ROLE_IGNORED for excluding a11y elements from the tree instead of focusabili… #
Total comments: 49
Patch Set 20 : Review comments (incomplete). #Patch Set 21 : Rebase. #Patch Set 22 : Fix compile error. #Patch Set 23 : Fix widget children. #
Total comments: 34
Patch Set 24 : Implement FromNativeViewAccessible() for Mac/Linux. #Patch Set 25 : Fix CrOS. #
Total comments: 6
Patch Set 26 : Address compilation issues + review comments. #Patch Set 27 : Don't use AXPlatformNode::FromNativeViewAccessible() on CrOS (unimplemented). #Patch Set 28 : Rebase on top of 2704263002. #
Total comments: 21
Patch Set 29 : Rebase again. #
Total comments: 13
Patch Set 30 : Review comments. #
Total comments: 9
Patch Set 31 : Fix easy review comments. #Patch Set 32 : Rebase. #Patch Set 33 : Switch to OS-specific methods of ignoring a11y elements. #
Total comments: 2
Patch Set 34 : Linting & review comments. #Messages
Total messages: 317 (275 generated)
patricialor@chromium.org changed reviewers: + tapted@chromium.org
Hi Trent, PTAL. One part of this CL I'm not sure about is whether to ignore child widgets as well as child views (which is what it does atm)? Thanks!
Did you see my comment at http://crbug.com/610589#c3 ? (is that a dead-end?) The notion would be try to infer that something is a leaf element under the conditions: - The view reports false for View::IsAccessibilityFocusable - There is a parent/ancestor View that reports true from View::IsAccessibilityFocusable
On 2016/07/07 04:27:26, tapted wrote: > Did you see my comment at http://crbug.com/610589#c3 ? (is that a dead-end?) > > The notion would be try to infer that something is a leaf element under the > conditions: > - The view reports false for View::IsAccessibilityFocusable > - There is a parent/ancestor View that reports true from > View::IsAccessibilityFocusable Ooh, yes I did see it but forgot about it. Will look into it now!
So, I think doing it the way you suggested is going to be a bit more
complicated. I have started doing it and uploaded a patchset, but haven't gone
through with it fully - some things I ran into while trying to implement it that
way:
a) If leaf elements are defined as elements which have entirely
non-accessibility focusable children:
- What should happen if new children are added or removed (can't override
AddChildView and there doesn't seem to be an observer event for this)? Should it
just be the responsibility of the adder/remover to make sure the FocusBehavior
of the relevant view is correct? I've assumed the answer is yes for patchset 2
here.
- Should there be a method to make a view a leaf node that just loops through
its children recursively and sets their FocusBehavior? I have not added this for
now as I don't think there will be very complex views that have more than one
layer of children that will need this, but not sure.
b) Leaf elements should probably also report their child count to be 0 in
GetChildCount(). This implies views which return false from
IsAccessibilityFocusable() should be ignored (i.e., that GetChildCount and
similar methods should ignore elements which aren't focusable). Should that be
the case (which then requires a refactoring for a bunch of other things)? I have
not gone ahead with this stuff since it seems a little complex for what we are
trying to achieve, but as it is now in patchset 2, it seems the behaviour is a
bit inconsistent (see NativeViewAccessibilityTest.{LabelIsChildOfButton,
HitTestSyncIgnoresAXLeafNodeChildren}, the last couple of lines where a new
grandchild is added).
The 'other things' I mention in (b) mostly deal with the test case where you
have a accessibility focusable root, a non-accessibility focusable child, and
maybe 3 focusable grandchildren - should calling GetChildCount on the root then
report 3? Similarly with ChildAtIndex() - should this also only return
accessibility focusable children?
Let me know what you think / if I didn't explain it well enough!
On 2016/07/11 07:16:30, Patti Lor wrote:
> So, I think doing it the way you suggested is going to be a bit more
> complicated. I have started doing it and uploaded a patchset, but haven't gone
> through with it fully - some things I ran into while trying to implement it
that
> way:
>
> a) If leaf elements are defined as elements which have entirely
> non-accessibility focusable children:
> - What should happen if new children are added or removed (can't override
> AddChildView and there doesn't seem to be an observer event for this)? Should
it
> just be the responsibility of the adder/remover to make sure the FocusBehavior
> of the relevant view is correct? I've assumed the answer is yes for patchset 2
> here.
yup - ideally we would have to ever actually do anything when adding/removing
views. Things should be given appropriate FocusBehavior traits already when the
a11y tree is generated.
> - Should there be a method to make a view a leaf node that just loops through
> its children recursively and sets their FocusBehavior? I have not added this
for
> now as I don't think there will be very complex views that have more than one
> layer of children that will need this, but not sure.
>
> b) Leaf elements should probably also report their child count to be 0 in
> GetChildCount(). This implies views which return false from
> IsAccessibilityFocusable() should be ignored (i.e., that GetChildCount and
> similar methods should ignore elements which aren't focusable). Should that be
> the case (which then requires a refactoring for a bunch of other things)?
Yup GetChildCount() will need to filter out non-a11y-focusable children.
> I have
> not gone ahead with this stuff since it seems a little complex for what we are
> trying to achieve, but as it is now in patchset 2, it seems the behaviour is a
> bit inconsistent (see NativeViewAccessibilityTest.{LabelIsChildOfButton,
> HitTestSyncIgnoresAXLeafNodeChildren}, the last couple of lines where a new
> grandchild is added).
>
> The 'other things' I mention in (b) mostly deal with the test case where you
> have a accessibility focusable root, a non-accessibility focusable child, and
> maybe 3 focusable grandchildren - should calling GetChildCount on the root
then
> report 3? Similarly with ChildAtIndex() - should this also only return
> accessibility focusable children?
>
> Let me know what you think / if I didn't explain it well enough!
The way to approach this might be to prioritize http://crbug.com/610585
"MacViews(a11y): Changing focus with Tab doesn't move VoiceOver cursor". I think
that's the "normal" way that a11y users navigate a11y information. If moving the
a11y cursor generates the right output, it might not matter too much that
hit-tests generated using the mouse pointer sometimes hit child views and report
weird stuff.
https://codereview.chromium.org/2119413004/diff/20001/ui/views/controls/butto... File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/2119413004/diff/20001/ui/views/controls/butto... ui/views/controls/button/label_button.cc:135: SetFocusBehavior(FocusBehavior::ACCESSIBLE_ONLY); The goal would be to make these lines unnecessary. The child views should already be FocusBehavior::NEVER and the LabelButotn should be FocusBehaviour::ALWAYS, which is a superset of ACCESSIBLE_ONLY.
Description was changed from ========== a11y: Allow elements to be marked as leaves in the accessibility tree. Add a new flag ax_leaf_element() to NativeViewAccessibility, which allows views to be marked as a leaf element in the accessibility tree. This means the children of that view will be ignored by accessibility clients. Also add expectations to NativeViewAccessibilityTest.LabelIsChildOfButton and a new test NativeViewAccessibilityTest.HitTestSyncIgnoresAXLeafNodeChildren to test this. BUG=610589 ========== to ========== a11y: Allow elements to be marked as leaves in the accessibility tree. Add a new flag ax_leaf_element() to NativeViewAccessibility, which allows views to be marked as a leaf element in the accessibility tree. This means the children of that view will be ignored by accessibility clients. Also add expectations to NativeViewAccessibilityTest.LabelIsChildOfButton and a new test NativeViewAccessibilityTest.HitTestSyncIgnoresAXLeafNodeChildren to test this. BUG=610589 NOPRESUBMIT=true ==========
Description was changed from ========== a11y: Allow elements to be marked as leaves in the accessibility tree. Add a new flag ax_leaf_element() to NativeViewAccessibility, which allows views to be marked as a leaf element in the accessibility tree. This means the children of that view will be ignored by accessibility clients. Also add expectations to NativeViewAccessibilityTest.LabelIsChildOfButton and a new test NativeViewAccessibilityTest.HitTestSyncIgnoresAXLeafNodeChildren to test this. BUG=610589 NOPRESUBMIT=true ========== to ========== a11y: Allow elements to be marked as leaves in the accessibility tree. Add a new flag ax_leaf_element() to NativeViewAccessibility, which allows views to be marked as a leaf element in the accessibility tree. This means the children of that view will be ignored by accessibility clients. Also add expectations to NativeViewAccessibilityTest.LabelIsChildOfButton and a new test NativeViewAccessibilityTest.HitTestSyncIgnoresAXLeafNodeChildren to test this. BUG=610589 ==========
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Description was changed from ========== a11y: Allow elements to be marked as leaves in the accessibility tree. Add a new flag ax_leaf_element() to NativeViewAccessibility, which allows views to be marked as a leaf element in the accessibility tree. This means the children of that view will be ignored by accessibility clients. Also add expectations to NativeViewAccessibilityTest.LabelIsChildOfButton and a new test NativeViewAccessibilityTest.HitTestSyncIgnoresAXLeafNodeChildren to test this. BUG=610589 ========== to ========== a11y: Allow elements to be marked as leaves in the accessibility tree. TODO: Change IsIgnored() to be manually set (default to false) instead of using FocusBehavior::ACCESSIBLE_ONLY - there are cases in which we want things to be visible to the accessibility client, but not be focusable (e.g., labels). Add a new flag ax_leaf_element() to NativeViewAccessibility, which allows views to be marked as a leaf element in the accessibility tree. This means the children of that view will be ignored by accessibility clients. Also add expectations to NativeViewAccessibilityTest.LabelIsChildOfButton and a new test NativeViewAccessibilityTest.HitTestSyncIgnoresAXLeafNodeChildren to test this. BUG=610589 ==========
Description was changed from ========== a11y: Allow elements to be marked as leaves in the accessibility tree. TODO: Change IsIgnored() to be manually set (default to false) instead of using FocusBehavior::ACCESSIBLE_ONLY - there are cases in which we want things to be visible to the accessibility client, but not be focusable (e.g., labels). Add a new flag ax_leaf_element() to NativeViewAccessibility, which allows views to be marked as a leaf element in the accessibility tree. This means the children of that view will be ignored by accessibility clients. Also add expectations to NativeViewAccessibilityTest.LabelIsChildOfButton and a new test NativeViewAccessibilityTest.HitTestSyncIgnoresAXLeafNodeChildren to test this. BUG=610589 ========== to ========== MacViews/a11y: Allow elements to be marked as leaves in the accessibility tree. TODO: Change IsIgnored() to be manually set (default to false) instead of using FocusBehavior::ACCESSIBLE_ONLY - there are cases in which we want things to be visible to the accessibility client, but not be focusable (e.g., labels). Add a new flag ax_leaf_element() to NativeViewAccessibility, which allows views to be marked as a leaf element in the accessibility tree. This means the children of that view will be ignored by accessibility clients. Also add expectations to NativeViewAccessibilityTest.LabelIsChildOfButton and a new test NativeViewAccessibilityTest.HitTestSyncIgnoresAXLeafNodeChildren to test this. BUG=610589 ==========
https://codereview.chromium.org/2119413004/diff/120001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2119413004/diff/120001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:283: if (NSPointInRect(point, child.boundsInScreen)) Perhaps we can tackle this logic. Something like if (NSPointInRect(point, [child boundsInScreen]) && ![child accessibilityIsIgnored]) return [child .. So we'll need the change just above still. However, a lot of the other changes to native_view_accessibility.cc may become unnecessary. I think this will still allow http://crbug.com/610589 to be fixed.
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== MacViews/a11y: Allow elements to be marked as leaves in the accessibility tree. TODO: Change IsIgnored() to be manually set (default to false) instead of using FocusBehavior::ACCESSIBLE_ONLY - there are cases in which we want things to be visible to the accessibility client, but not be focusable (e.g., labels). Add a new flag ax_leaf_element() to NativeViewAccessibility, which allows views to be marked as a leaf element in the accessibility tree. This means the children of that view will be ignored by accessibility clients. Also add expectations to NativeViewAccessibilityTest.LabelIsChildOfButton and a new test NativeViewAccessibilityTest.HitTestSyncIgnoresAXLeafNodeChildren to test this. BUG=610589 ========== to ========== MacViews/a11y: Ignore children of nested keyboard accessible controls. Currently, children of keyboard accessible controls, such as LabelButtons, are not hidden from the accessibility tree. This is not useful because the user only cares about element that has focus, not any children inside it, which are implementation details. Hide any children of keyboard accessible controls from the accessibility client by setting its role to ignored for MacViews. BUG=610589 TEST=Open the XCode Accessibility Inspector, hover over the text on a views::LabelButton (e.g. on the HTTP Authentication dialog). It should report an empty set of children and its role should be a button. ==========
Hi Trent, PTAL - scrapped the old approach entirely. Thanks! https://codereview.chromium.org/2119413004/diff/20001/ui/views/controls/butto... File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/2119413004/diff/20001/ui/views/controls/butto... ui/views/controls/button/label_button.cc:135: SetFocusBehavior(FocusBehavior::ACCESSIBLE_ONLY); On 2016/07/12 01:44:23, tapted wrote: > The goal would be to make these lines unnecessary. The child views should > already be FocusBehavior::NEVER and the LabelButotn should be > FocusBehaviour::ALWAYS, which is a superset of ACCESSIBLE_ONLY. Oh, you're right, this seems to already be the case, have deleted this!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
looks pretty good, but I'd loop in an a11y expert -- they might know better whether this is the right approach. https://codereview.chromium.org/2119413004/diff/160001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility.cc (right): https://codereview.chromium.org/2119413004/diff/160001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:105: if (HasFocusableAncestor()) should we do this only if the view itself is not accessibility focusable? I think it's allowed to traverse things inside something that is focusable. An example might be something like the omnibox which gets icons within it. https://codereview.chromium.org/2119413004/diff/160001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility.h (right): https://codereview.chromium.org/2119413004/diff/160001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.h:54: bool HasFocusableAncestor(); can this be moved to an anonymous namespace in the .cc only?
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Trent, PTAL. https://codereview.chromium.org/2119413004/diff/160001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility.cc (right): https://codereview.chromium.org/2119413004/diff/160001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:105: if (HasFocusableAncestor()) On 2016/11/25 04:36:27, tapted wrote: > should we do this only if the view itself is not accessibility focusable? I > think it's allowed to traverse things inside something that is focusable. > > An example might be something like the omnibox which gets icons within it. That sounds reasonable - I don't think it fixes the Omnibox problems (see below), but have added this in anyway. The star was always working with VO, but the LocationIconView (which is the lock icon/info icon and stuff to the left of the Omnibox) never did. A clean build makes it show up to VO separately as image + label, then with the changes in this CL, it's completely ignored. I've filed a bug to fix this in a follow up CL here: https://bugs.chromium.org/p/chromium/issues/detail?id=668930, and I think I know how to do it. (To be clear, the change you've suggested above doesn't fix it, it's still ignored.) https://codereview.chromium.org/2119413004/diff/160001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility.h (right): https://codereview.chromium.org/2119413004/diff/160001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.h:54: bool HasFocusableAncestor(); On 2016/11/25 04:36:27, tapted wrote: > can this be moved to an anonymous namespace in the .cc only? Good idea, thanks! I actually realised that just using IsAccessibilityFocusable() isn't good enough - doing that would mean the children of disabled controls (which would be accessibility focusable if they were enabled) would not be ignored. So I had to make View::focusable_behavior() public, not sure if that's an OK change?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2119413004/diff/200001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility.cc (right): https://codereview.chromium.org/2119413004/diff/200001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:24: bool ViewHasFocusableAncestor(View* parent) { nit: parent -> view (since it's not the parent that's passed in) https://codereview.chromium.org/2119413004/diff/200001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:31: } nit: // namespace (and and a blank line before, if clang-format lets you) https://codereview.chromium.org/2119413004/diff/200001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility_unittest.cc (right): https://codereview.chromium.org/2119413004/diff/200001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:76: button_->SetFocusBehavior(View::FocusBehavior::NEVER); We can add // On Mac, the label isn't given a role. Since it's a subview of |button_| (and the button is // focusable), the label is assumed to form part of the button and not have a role of its own. EXPECT_EQ(ui::AX_ROLE_<something - INGORED?>, label_accessibility_->GetData().role); and we need to say why making it unfocusable fixes it. https://codereview.chromium.org/2119413004/diff/200001/ui/views/view.h File ui/views/view.h (right): https://codereview.chromium.org/2119413004/diff/200001/ui/views/view.h#newcod... ui/views/view.h:783: FocusBehavior focus_behavior() const { return focus_behavior_; } I think this is OK. (although it raises the question of whether `IsAccessibilityFocusable`, or the FocusBehaviour enum values deserve better names..). OWNERS may have some other suggestions https://codereview.chromium.org/2119413004/diff/200001/ui/views/widget/native... File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2119413004/diff/200001/ui/views/widget/native... ui/views/widget/native_widget_mac_accessibility_unittest.mm:121: // The exception is if the child is explicitly marked accessibility focusable. it seems odd to say this but to not call SetFocusBehavior. Can we do the test without adding a second LabelButton. I think you just need to copy-paste the `TestLabelButton` from label_button_unittest.cc (it's small enough to not bother putting it in its own header). Then you can call SetFocusBehavior on the label inside the button itself. https://codereview.chromium.org/2119413004/diff/200001/ui/views/widget/native... ui/views/widget/native_widget_mac_accessibility_unittest.mm:124: child_button->SetSize(gfx::Size()); Is there an neat alternative to setting the size to zero? This is OK too, but the concern is that someone why decide to change `IsDrawn` to incorporate whether the view has zero size as well.. https://codereview.chromium.org/2119413004/diff/200001/ui/views/widget/native... ui/views/widget/native_widget_mac_accessibility_unittest.mm:135: // If the child is disabled, it should still work. nit: work -> be traversable?
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi Trent, PTAL! Thanks. https://codereview.chromium.org/2119413004/diff/200001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility.cc (right): https://codereview.chromium.org/2119413004/diff/200001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:24: bool ViewHasFocusableAncestor(View* parent) { On 2016/11/29 03:12:49, tapted wrote: > nit: parent -> view (since it's not the parent that's passed in) Done. https://codereview.chromium.org/2119413004/diff/200001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:31: } On 2016/11/29 03:12:49, tapted wrote: > nit: // namespace (and and a blank line before, if clang-format lets you) Thanks - it seems to not let you if you've only got 1 method inside the anon namespace, but two or more is fine. https://codereview.chromium.org/2119413004/diff/200001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility_unittest.cc (right): https://codereview.chromium.org/2119413004/diff/200001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:76: button_->SetFocusBehavior(View::FocusBehavior::NEVER); On 2016/11/29 03:12:49, tapted wrote: > We can add > > // On Mac, the label isn't given a role. Since it's a subview of |button_| (and > the button is > // focusable), the label is assumed to form part of the button and not have a > role of its own. > EXPECT_EQ(ui::AX_ROLE_<something - INGORED?>, > label_accessibility_->GetData().role); > > and we need to say why making it unfocusable fixes it. Done. https://codereview.chromium.org/2119413004/diff/200001/ui/views/view.h File ui/views/view.h (right): https://codereview.chromium.org/2119413004/diff/200001/ui/views/view.h#newcod... ui/views/view.h:783: FocusBehavior focus_behavior() const { return focus_behavior_; } On 2016/11/29 03:12:49, tapted wrote: > I think this is OK. (although it raises the question of whether > `IsAccessibilityFocusable`, or the FocusBehaviour enum values deserve better > names..). OWNERS may have some other suggestions Acknowledged. https://codereview.chromium.org/2119413004/diff/200001/ui/views/widget/native... File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2119413004/diff/200001/ui/views/widget/native... ui/views/widget/native_widget_mac_accessibility_unittest.mm:121: // The exception is if the child is explicitly marked accessibility focusable. On 2016/11/29 03:12:49, tapted wrote: > it seems odd to say this but to not call SetFocusBehavior. Can we do the test > without adding a second LabelButton. I think you just need to copy-paste the > `TestLabelButton` from label_button_unittest.cc (it's small enough to not bother > putting it in its own header). Then you can call SetFocusBehavior on the label > inside the button itself. Done. https://codereview.chromium.org/2119413004/diff/200001/ui/views/widget/native... ui/views/widget/native_widget_mac_accessibility_unittest.mm:124: child_button->SetSize(gfx::Size()); On 2016/11/29 03:12:49, tapted wrote: > Is there an neat alternative to setting the size to zero? This is OK too, but > the concern is that someone why decide to change `IsDrawn` to incorporate > whether the view has zero size as well.. Not sure, I think we could potentially use SetBoundsRect() and set the origin not-inside |button_| (like, to 0, 0 relative to screen coords, or something)? Not sure if that's very nice though. I've changed it to a 1x1 size now, let me know if you want that reverted. https://codereview.chromium.org/2119413004/diff/200001/ui/views/widget/native... ui/views/widget/native_widget_mac_accessibility_unittest.mm:135: // If the child is disabled, it should still work. On 2016/11/29 03:12:49, tapted wrote: > nit: work -> be traversable? Done.
https://codereview.chromium.org/2119413004/diff/200001/ui/views/widget/native... File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2119413004/diff/200001/ui/views/widget/native... ui/views/widget/native_widget_mac_accessibility_unittest.mm:124: child_button->SetSize(gfx::Size()); On 2016/11/29 23:26:18, Patti Lor wrote: > On 2016/11/29 03:12:49, tapted wrote: > > Is there an neat alternative to setting the size to zero? This is OK too, but > > the concern is that someone why decide to change `IsDrawn` to incorporate > > whether the view has zero size as well.. > > Not sure, I think we could potentially use SetBoundsRect() and set the origin > not-inside |button_| (like, to 0, 0 relative to screen coords, or something)? > Not sure if that's very nice though. I've changed it to a 1x1 size now, let me > know if you want that reverted. yeah - I like this better. thanks. https://codereview.chromium.org/2119413004/diff/220001/ui/views/widget/native... File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2119413004/diff/220001/ui/views/widget/native... ui/views/widget/native_widget_mac_accessibility_unittest.mm:134: LabelButton* child_button = new TestLabelButton(); Sorry - I meant that instead of adding child_button, we could make |button| a TestLabelButton. Then, here, we can just do button->label()->SetFocusBehavior(views::View::FocusBehavior::ACCESSIBLE_ONLY); EXPECT_EQ(1u, [[button->GetNativeViewAccessible()] children/count]) // If the button is disabled, the child should still be traversable. button->SetEnabled(false); EXPECT.. etc. You'll need the `using` declaration from the TestLabelButton label_button_unittest.cc (The issue is that having a button as a child view of another button is a bit weird -- that wouldn't happen in practise, but we can do a more "authentic" test by playing with the label subview of button that already exists). Except I wasn't sure if this would work, or would reduce the test coverage (there could be something I'm missing), so I left my other comments in.
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL, thanks https://codereview.chromium.org/2119413004/diff/200001/ui/views/widget/native... File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2119413004/diff/200001/ui/views/widget/native... ui/views/widget/native_widget_mac_accessibility_unittest.mm:124: child_button->SetSize(gfx::Size()); On 2016/11/29 23:43:20, tapted wrote: > On 2016/11/29 23:26:18, Patti Lor wrote: > > On 2016/11/29 03:12:49, tapted wrote: > > > Is there an neat alternative to setting the size to zero? This is OK too, > but > > > the concern is that someone why decide to change `IsDrawn` to incorporate > > > whether the view has zero size as well.. > > > > Not sure, I think we could potentially use SetBoundsRect() and set the origin > > not-inside |button_| (like, to 0, 0 relative to screen coords, or something)? > > Not sure if that's very nice though. I've changed it to a 1x1 size now, let me > > know if you want that reverted. > > yeah - I like this better. thanks. Updated the comment to reflect the new size too - forgot to do that. https://codereview.chromium.org/2119413004/diff/220001/ui/views/widget/native... File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2119413004/diff/220001/ui/views/widget/native... ui/views/widget/native_widget_mac_accessibility_unittest.mm:134: LabelButton* child_button = new TestLabelButton(); On 2016/11/29 23:43:20, tapted wrote: > Sorry - I meant that instead of adding child_button, we could make |button| a > TestLabelButton. Then, here, we can just do > > > button->label()->SetFocusBehavior(views::View::FocusBehavior::ACCESSIBLE_ONLY); > EXPECT_EQ(1u, [[button->GetNativeViewAccessible()] children/count]) > > // If the button is disabled, the child should still be traversable. > button->SetEnabled(false); > EXPECT.. etc. > > You'll need the `using` declaration from the TestLabelButton > label_button_unittest.cc > > > (The issue is that having a button as a child view of another button is a bit > weird -- that wouldn't happen in practise, but we can do a more "authentic" test > by playing with the label subview of button that already exists). > > Except I wasn't sure if this would work, or would reduce the test coverage > (there could be something I'm missing), so I left my other comments in. Oops, sorry for the misunderstanding. LabelButton::label() is protected, so I had to make a wrapper for it, but it seems to work otherwise.
lgtm with the following https://codereview.chromium.org/2119413004/diff/240001/ui/views/widget/native... File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2119413004/diff/240001/ui/views/widget/native... ui/views/widget/native_widget_mac_accessibility_unittest.mm:68: Label* GetLabel() { return LabelButton::label(); } try `using LabelButton::label;` here Then you should be able to access it via button->label() https://codereview.chromium.org/2119413004/diff/240001/ui/views/widget/native... ui/views/widget/native_widget_mac_accessibility_unittest.mm:146: // If the child is disabled, it should be traversable. should be -> should still be
patricialor@chromium.org changed reviewers: + dmazzoni@chromium.org
Hi dmazzoni, WDYT of this approach? Thanks! https://codereview.chromium.org/2119413004/diff/240001/ui/views/widget/native... File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2119413004/diff/240001/ui/views/widget/native... ui/views/widget/native_widget_mac_accessibility_unittest.mm:68: Label* GetLabel() { return LabelButton::label(); } On 2016/11/30 05:23:17, tapted wrote: > try `using LabelButton::label;` here > > Then you should be able to access it via button->label() Sorry >< Made a mistake the first time around doing this. Fixed. https://codereview.chromium.org/2119413004/diff/240001/ui/views/widget/native... ui/views/widget/native_widget_mac_accessibility_unittest.mm:146: // If the child is disabled, it should be traversable. On 2016/11/30 05:23:17, tapted wrote: > should be -> should still be Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2119413004/diff/240001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility.cc (right): https://codereview.chromium.org/2119413004/diff/240001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:110: #if defined(OS_MACOSX) I'd prefer to find a solution that works for all platforms. In general hiding children of focusable views sounds reasonable. https://codereview.chromium.org/2119413004/diff/240001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:116: data_.role = ui::AX_ROLE_UNKNOWN; Rather than setting the role to unknown, what if we just remove them from the accessibility tree by modifying GetChildCount(), below? https://codereview.chromium.org/2119413004/diff/240001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:123: int child_count = view_->child_count(); Right here, you could add: if (IsAccessibilityFocusableWhenEnabled(view_)) return 0; Would that work? If not, why not? https://codereview.chromium.org/2119413004/diff/240001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:195: // don't have to search further because AXPlatformNodeWin will do a recursive Is this specific only to AXPlatformNodeWin? https://codereview.chromium.org/2119413004/diff/240001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility_unittest.cc (right): https://codereview.chromium.org/2119413004/diff/240001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:73: #if defined(OS_MACOSX) I'm okay with removing this test or rewriting it to test something else useful, given this change
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the review dmazzoni - the new patchset is not for your comments, but for tapted's previously (forgot to reupload a new patchset). Please just look at the comments instead. https://codereview.chromium.org/2119413004/diff/240001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility.cc (right): https://codereview.chromium.org/2119413004/diff/240001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:110: #if defined(OS_MACOSX) On 2016/11/30 23:12:44, dmazzoni wrote: > I'd prefer to find a solution that works for all > platforms. In general hiding children of focusable > views sounds reasonable. See next comment. https://codereview.chromium.org/2119413004/diff/240001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:116: data_.role = ui::AX_ROLE_UNKNOWN; On 2016/11/30 23:12:44, dmazzoni wrote: > Rather than setting the role to unknown, what if we just remove them > from the accessibility tree by modifying GetChildCount(), below? Yes - this was my approach in patchset 8. Removing things in the accessibility tree has a couple implications, if we want to be consistent with the results returned from GetChildCount(), ChildAtIndex(), GetParent(), etc, which I will summarise here: - Children of deleted elements will need to be moved up the tree. - Hit tests will need to ignore those deleted elements. The diagram in https://codereview.chromium.org/2119413004/diff/140001/ui/views/accessibility... (you'll need to scroll a bit) is probably the most useful in illustrating this. (We can avoid having to implement this ourselves in this particular patchset because Mac does this for us with its NSAccessibilityUnignoredAncestor() function.) To make it platform independent, I could merge this patchset with patchset 8, but the issue was that it also requires a way to retrieve a NativeViewAccessibility from a View. I'd originally solved this by adding View::GetNativeViewAccessibility(), but tapted pointed out this might be difficult/confusing especially with GetNativeViewAccessible() there as well. What do you think? https://codereview.chromium.org/2119413004/diff/240001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:123: int child_count = view_->child_count(); On 2016/11/30 23:12:44, dmazzoni wrote: > Right here, you could add: > > if (IsAccessibilityFocusableWhenEnabled(view_)) > return 0; > > Would that work? If not, why not? See above comment. https://codereview.chromium.org/2119413004/diff/240001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:195: // don't have to search further because AXPlatformNodeWin will do a recursive On 2016/11/30 23:12:44, dmazzoni wrote: > Is this specific only to AXPlatformNodeWin? Yes, AFAICT, HitTestSync is only used for Windows (searching it in codesearch only turns up 1 override in the Linux code which returns nullptr). https://codereview.chromium.org/2119413004/diff/240001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility_unittest.cc (right): https://codereview.chromium.org/2119413004/diff/240001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:73: #if defined(OS_MACOSX) On 2016/11/30 23:12:44, dmazzoni wrote: > I'm okay with removing this test or rewriting it to test > something else useful, given this change Will go ahead and do this if it's decided we'll go with a platform independent approach as per my second comment.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/01 01:12:07, Patti Lor wrote: > Yes - this was my approach in patchset 8. Removing things in the accessibility > tree has a couple implications, if we want to be consistent with the results > returned from GetChildCount(), ChildAtIndex(), GetParent(), etc, which I will > summarise here: > - Children of deleted elements will need to be moved up the tree. > - Hit tests will need to ignore those deleted elements. Ah, I see - you want to hide individual elements but not the whole subtree. For the web part of the code, we call those elements "presentational" and they have a role of ui::AX_ROLE_PRESENTATIONAL or ui::AX_ROLE_NONE. https://rawgit.com/w3c/aria/master/aria/aria.html#presentation "the presentation role causes a given element to be treated as having no role or to be removed from the accessibility tree, but does not cause the content contained within the element to be removed from the accessibility tree." We actually implement removing the element from the tree in Blink so there's no browser-side code to follow, but I'd be in favor of adding something like this cross-platform, I'm sure it'd be useful for more things in the future. > The diagram in > https://codereview.chromium.org/2119413004/diff/140001/ui/views/accessibility... > (you'll need to scroll a bit) is probably the most useful in illustrating this. > (We can avoid having to implement this ourselves in this particular patchset > because Mac does this for us with its NSAccessibilityUnignoredAncestor() > function.) To make it platform independent, I could merge this patchset with > patchset 8, but the issue was that it also requires a way to retrieve a > NativeViewAccessibility from a View. I'd originally solved this by adding > View::GetNativeViewAccessibility(), but tapted pointed out this might be > difficult/confusing especially with GetNativeViewAccessible() there as well. > What do you think? I think there are two ways we could get a NativeViewAccessibility from a View: 1. Use AXPlatformNode::FromNativeViewAccessible() and then cast the delegate to a NativeViewAccessibility 2. Have NativeViewAccessibility keep a global map of all views so that NativeViewAccessibility::Create could be instead GetOrCreate and it would retrieve an existing To resolve the name confusion we could rename NativeViewAccessibility into AXPlatformNodeDelegateViews or something like that. The confusing names are just historical. In general Scott Violet has preferred we minimize the number of APIs on View directly, so I think either something like #1 or #2 would be preferable to adding a new interface to View. The reason we have GetNativeViewAccessible() is because some Views return an object that's not an AXPlatformNode and not based on NativeViewAccessibility at all - off the top of my head WebView is the most important example of this. I suppose another possibility is that all Views instead could return a NativeViewAccessibility (or AXPlatformNodeDelegateViews) instead of a NativeViewAccessible, and then the 2 - 3 View subclasses that need to do something special could subclass NativeViewAccessibility and have it delegate to some other non-views class instead. I haven't thought through all of the implications but I think that could work too. I'd favor trying #1 or #2 first.
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #16 (id:300001) has been deleted
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #18 (id:350001) has been deleted
Patchset #18 (id:370001) has been deleted
Patchset #18 (id:390001) has been deleted
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #18 (id:410001) has been deleted
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #18 (id:430001) has been deleted
Hi Trent, PTAL - it's changed a lot since you gave lgtm. Merged with the approach in patchset 8 to make this cross-platform. If you think it's too big, I could split it up into two patches, one for adding ui::AX_ROLE_IGNORED to non-focusable children of focusable views and one for actually excluding the elements from the a11y tree. @dmazzoni - thanks for your comments and sorry about the delay! I've taken your suggestion #2 of how to retrieve the NativeViewAccessibility from a View, so no need to add a new method to View any more. Up to you whether you'd like to take a look now or wait til tapted@ has given it a pass first :) Thanks both!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2119413004/diff/460001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2119413004/diff/460001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:289: ![child accessibilityIsIgnored]) nit: reorder these so accessibilityIsIgnored is checked first (it should be cheaper) https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility.cc (right): https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:71: // Remove this instance from the map when it's destroyed. In theory this should occur in NativeViewAccessibility::Destroy() -- that's what ~View() calls, and ~NativeViewAccessibility() may not be invoked immediately. But I don't think we need NativeViewAccessibility::Destroy() any more. I think the first step is to remove NativeViewAccessibility::Destroy(). views::View should have a std::unique_ptr to NativeViewAccessibility rather than a raw pointer. https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:72: NativeViewAccessibilityMap* views_to_nva = GetNativeViewAccessibilityMap(); We can just call GetNativeViewAccessibilityMap()->erase(view_); That will return 0 if it's not found, but since we don't guarantee that, we can just add a note above. https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:174: if (index < ax_child_count - child_widget_count) { This logic is pretty complicated.. I think we effectively want `NSAccessibilityUnignoredChildren` https://developer.apple.com/reference/appkit/1524571-nsaccessibilityunignored... I think my preference would be to create a helper function that returns a freshly-populated std::vector<gfx::NativeViewAccessible> of unignored children each time. It feels slow.. but it shouldn't be too bad, and the existence of NSAccessibilityUnignoredChildren justifies it to some extent. GetChildCount() then just returns GetUnignoredChildren().size(). and things are a lot simpler here. ::HitTestSync() can be updated to use this helper too (be sure to put child widgets in first) https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:207: else nit: "no else after return" - https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:256: for (int i = view_->child_count() - 1; i >= 0; --i) { Does this need to consider whether the child is ignored? https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:338: NativeViewAccessibilityMap* nit: Return a reference (sometimes returning a pointer suggests that it might return nullptr, but it never will) https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:339: NativeViewAccessibility::GetNativeViewAccessibilityMap() { can this go in an anonymous namespace? https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility.h (right): https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.h:91: static NativeViewAccessibility* CreateNewNativeViewAccessibilityImpl( Can we call this just `Create` still? (Also - I think we can remove NativeViewAccessibility::Destroy() and return a std::unique_ptr here -- see other comments) https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility_unittest.cc (right): https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:192: // A A I wonder if there's a naming scheme that could make the tests clearer. E.g. A -> root B -> b1a X1 -> b2x C -> b1a_c1a D -> b1a_c2a X2 -> b1a_c3x X3 -> b2x_c4x X4 -> b2x_c5x E -> b2x_c6a X5 -> b1a_c1a_d1x F -> b2x_c4x_d2a https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:194: // B X1 B F E ||| C | F | B || F | A | E ||| ||| C | F | B || F | A | E ||| should that first F be a D? https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:260: // inside a widget, which will handle their deletion. You can just say // Owned by |widget_|. https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:292: TEST_F(NativeViewAccessibilityIgnoredElementsTest, GetChildCount) { nit: all these should have a brief comment before https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:295: EXPECT_EQ(0, c_ax_->GetChildCount()); Perhaps just comment that this is a leaf? Then I think the IsLeafElement test is redundant - it's not adding anything new (unless I've missed something) https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:342: EXPECT_EQ(a_->GetNativeViewAccessible(), f_ax_->GetParent()); These need comments to describe what's happening. A better naming scheme may help. Then you can just add something like // Skips x3 and x1. The name will help make it clear what the direct parent is. https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:346: EXPECT_FALSE(a_ax_->GetChildCount() == 0); EXPECT_NE (or _EQ with the actual number?) https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:348: EXPECT_TRUE(c_ax_->GetChildCount() == 0); EXPECT_EQ https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:361: int min_width = a_->GetBoundsInScreen().width() / 6; const? https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility_win_unittest.cc (right): https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_win_unittest.cc:22: class NativeViewAccessibilityWinTest : public ViewsTestBase { this can be a separate patch I think :)
Looking really good! https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility.cc (right): https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:77: // This should only ever happen in tests where the NativeViewAccessibility Are there any such tests? It might be nice to fix them and add NOTREACHED here, unless it's a major pain to refactor. https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:189: Widget* child_widget = child_widgets[index - view_->child_count()]; This isn't correct anymore - taking [index - view_->child_count()] assumes that the number of children other than widgets is equal to view_->child_count() but it could be greater or less than that now. https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:244: return GetParent(); I think you should return nullptr here. HitTest is sometimes called repeatedly - do a hit test on the top-level window, get a child - then call HitTest on that again to get its child. Returning a parent could result in an endless loop. https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility.h (right): https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.h:86: // A static map of all the Views with an associated NativeViewAccessibility. Perhaps: // A static map from every View to its associated NativeViewAccessibility. https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility_unittest.cc (right): https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:365: // HitTestSync isn't recursive, so manually do the recursion until arriving at How about a helper that calls HitTestSync until reaching a leaf? Also, in case it wasn't clear, the reason it's not recursive is because sometimes the hit testing will lead you to something other than a View - for example if you hit test in web contents, we first return the WebView, but then the WebView's accessible is a totally different object like a BrowserAccessibilityCocoa, which implements its own hit testing from then on.
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks all, PTAL. https://codereview.chromium.org/2119413004/diff/460001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2119413004/diff/460001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:289: ![child accessibilityIsIgnored]) On 2016/12/22 02:56:36, tapted (OOO until 2017-01-03) wrote: > nit: reorder these so accessibilityIsIgnored is checked first (it should be > cheaper) Done. https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility.cc (right): https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:71: // Remove this instance from the map when it's destroyed. On 2016/12/22 02:56:36, tapted (OOO until 2017-01-03) wrote: > In theory this should occur in NativeViewAccessibility::Destroy() -- that's what > ~View() calls, and ~NativeViewAccessibility() may not be invoked immediately. > But I don't think we need NativeViewAccessibility::Destroy() any more. > > I think the first step is to remove NativeViewAccessibility::Destroy(). > views::View should have a std::unique_ptr to NativeViewAccessibility rather than > a raw pointer. Done. https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:72: NativeViewAccessibilityMap* views_to_nva = GetNativeViewAccessibilityMap(); On 2016/12/22 02:56:36, tapted (OOO until 2017-01-03) wrote: > We can just call > > GetNativeViewAccessibilityMap()->erase(view_); > > That will return 0 if it's not found, but since we don't guarantee that, we can > just add a note above. Oops, thanks for the tip! https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:77: // This should only ever happen in tests where the NativeViewAccessibility On 2016/12/28 18:02:35, dmazzoni wrote: > Are there any such tests? It might be nice to fix them and add > NOTREACHED here, unless it's a major pain to refactor. Yes, NativeViewAccessibilityTest.CrashOnWidgetDestroyed uses a custom NativeViewAccessibility subclass and won't be able to use GetOrCreate() to get an instance of that. I did try to fix it, but I don't think there's an easy solution, since NVA::Create() returns a platform specific subclass of NVA, and is guarded on the PLATFORM_HAS_NATIVE_VIEW_ACCESSIBILITY_IMPL hash define. To give NVATest its own constructor that avoids inserting it into the map would mean we'd have to define PLATFORM_HAS_NVA... for unit tests somehow. https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:174: if (index < ax_child_count - child_widget_count) { On 2016/12/22 02:56:36, tapted wrote: > This logic is pretty complicated.. I think we effectively want > `NSAccessibilityUnignoredChildren` > https://developer.apple.com/reference/appkit/1524571-nsaccessibilityunignored... > > I think my preference would be to create a helper function that returns a > freshly-populated std::vector<gfx::NativeViewAccessible> of unignored children > each time. > > It feels slow.. but it shouldn't be too bad, and the existence of > NSAccessibilityUnignoredChildren justifies it to some extent. > > GetChildCount() then just returns GetUnignoredChildren().size(). and things are > a lot simpler here. > > ::HitTestSync() can be updated to use this helper too (be sure to put child > widgets in first) Done. I'm not sure if HitTestSync can be updated to use this though, since it uses View::HitTestPoint() to do hit testing and there isn't a way to get the View from a gfx::NativeViewAccessible. https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:189: Widget* child_widget = child_widgets[index - view_->child_count()]; On 2016/12/28 18:02:35, dmazzoni wrote: > This isn't correct anymore - taking [index - view_->child_count()] assumes that > the number of children other than widgets is equal to view_->child_count() but > it could be greater or less than that now. Thanks - updated to index - ax_child_count. https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:207: else On 2016/12/22 02:56:36, tapted (OOO until 2017-01-03) wrote: > nit: "no else after return" - > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Done, thanks for always digging up the style guide links! https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:244: return GetParent(); On 2016/12/28 18:02:35, dmazzoni wrote: > I think you should return nullptr here. > > HitTest is sometimes called repeatedly - do a hit test > on the top-level window, get a child - then call HitTest > on that again to get its child. Returning a parent could > result in an endless loop. Done. https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:256: for (int i = view_->child_count() - 1; i >= 0; --i) { On 2016/12/22 02:56:36, tapted wrote: > Does this need to consider whether the child is ignored? No - skipping ignored children at this point of the hit test will skip the entire subtree. You'd fix that by making this method recursive, but since this was designed not to recurse I'm leaving it that way. The hunk just above this (line 242) should take care of ignored elements I think. https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:338: NativeViewAccessibilityMap* On 2016/12/22 02:56:36, tapted wrote: > nit: Return a reference (sometimes returning a pointer suggests that it might > return nullptr, but it never will) Done. https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:339: NativeViewAccessibility::GetNativeViewAccessibilityMap() { On 2016/12/22 02:56:36, tapted wrote: > can this go in an anonymous namespace? Done - I assumed you also meant to make it a function as well (as opposed to a method), so let me know if that was wrong. https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility.h (right): https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.h:86: // A static map of all the Views with an associated NativeViewAccessibility. On 2016/12/28 18:02:35, dmazzoni wrote: > Perhaps: > > // A static map from every View to its associated NativeViewAccessibility. Done. https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.h:91: static NativeViewAccessibility* CreateNewNativeViewAccessibilityImpl( On 2016/12/22 02:56:36, tapted (OOO until 2017-01-03) wrote: > Can we call this just `Create` still? (Also - I think we can remove > NativeViewAccessibility::Destroy() and return a std::unique_ptr here -- see > other comments) Renamed, but kept this returning a raw ptr as discussed. https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility_unittest.cc (right): https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:192: // A A On 2016/12/22 02:56:37, tapted (OOO until 2017-01-03) wrote: > I wonder if there's a naming scheme that could make the tests clearer. E.g. > > A -> root > B -> b1a > X1 -> b2x > C -> b1a_c1a > D -> b1a_c2a > X2 -> b1a_c3x > X3 -> b2x_c4x > X4 -> b2x_c5x > E -> b2x_c6a > X5 -> b1a_c1a_d1x > F -> b2x_c4x_d2a Yeah, that sounds like a good idea, but I'd like to preserve the single letters as well so that the tree/hit test diagrams don't get cluttered. I've tried using a compromise where all of them are prefixed with their letter, but also have the parents written in. I didn't know what the "a" at the end of them meant though (other than "not ignored") so I left them out. The numbers I also left out because they are confusing when you still have nodes named x1, x2 etc. Let me know if you think that's better or not :/ I'm not sure. https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:194: // B X1 B F E ||| C | F | B || F | A | E ||| On 2016/12/22 02:56:37, tapted (OOO until 2017-01-03) wrote: > ||| C | F | B || F | A | E ||| > > should that first F be a D? Yes, thanks for picking that up :) https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:260: // inside a widget, which will handle their deletion. On 2016/12/22 02:56:37, tapted (OOO until 2017-01-03) wrote: > You can just say > > // Owned by |widget_|. Done. https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:292: TEST_F(NativeViewAccessibilityIgnoredElementsTest, GetChildCount) { On 2016/12/22 02:56:37, tapted wrote: > nit: all these should have a brief comment before Done. https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:295: EXPECT_EQ(0, c_ax_->GetChildCount()); On 2016/12/22 02:56:37, tapted (OOO until 2017-01-03) wrote: > Perhaps just comment that this is a leaf? Then I think the IsLeafElement test is > redundant - it's not adding anything new (unless I've missed something) Done. https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:342: EXPECT_EQ(a_->GetNativeViewAccessible(), f_ax_->GetParent()); On 2016/12/22 02:56:37, tapted wrote: > These need comments to describe what's happening. A better naming scheme may > help. Then you can just add something like > > // Skips x3 and x1. > > The name will help make it clear what the direct parent is. Done. https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:346: EXPECT_FALSE(a_ax_->GetChildCount() == 0); On 2016/12/22 02:56:36, tapted (OOO until 2017-01-03) wrote: > EXPECT_NE (or _EQ with the actual number?) Deleted this test as you suggested previously. https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:348: EXPECT_TRUE(c_ax_->GetChildCount() == 0); On 2016/12/22 02:56:37, tapted (OOO until 2017-01-03) wrote: > EXPECT_EQ Acknowledged. https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:361: int min_width = a_->GetBoundsInScreen().width() / 6; On 2016/12/22 02:56:37, tapted (OOO until 2017-01-03) wrote: > const? Done. https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:365: // HitTestSync isn't recursive, so manually do the recursion until arriving at On 2016/12/28 18:02:35, dmazzoni wrote: > How about a helper that calls HitTestSync until reaching a leaf? > > Also, in case it wasn't clear, the reason it's not recursive is > because sometimes the hit testing will lead you to something > other than a View - for example if you hit test in web contents, > we first return the WebView, but then the WebView's accessible is > a totally different object like a BrowserAccessibilityCocoa, > which implements its own hit testing from then on. I'm not sure the helper is a good idea - it still needs a list of expected NVAs it will pass through on trying to get to the leaf because every call to HitTestSync() returns a NativeViewAccessible, and there's no way to retrieve a NativeViewAccessibility from a NativeViewAccessible on all platforms. You'd have to check at each step that the NVA->NativeObject() returned from the expected NVA list matches up with each NativeViewAccessible returned by HitTestSync. Ah, thanks for the clarification, I had just assumed it was that way because of a Windows specific implementation of hit testing or something. https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility_win_unittest.cc (right): https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_win_unittest.cc:22: class NativeViewAccessibilityWinTest : public ViewsTestBase { On 2016/12/22 02:56:37, tapted (OOO until 2017-01-03) wrote: > this can be a separate patch I think :) Oops - thanks, I think I needed to edit stuff in here before but it wasn't needed any more. Split off into https://codereview.chromium.org/2609443004/ which has already landed.
Description was changed from ========== MacViews/a11y: Ignore children of nested keyboard accessible controls. Currently, children of keyboard accessible controls, such as LabelButtons, are not hidden from the accessibility tree. This is not useful because the user only cares about element that has focus, not any children inside it, which are implementation details. Hide any children of keyboard accessible controls from the accessibility client by setting its role to ignored for MacViews. BUG=610589 TEST=Open the XCode Accessibility Inspector, hover over the text on a views::LabelButton (e.g. on the HTTP Authentication dialog). It should report an empty set of children and its role should be a button. ========== to ========== a11y: Exclude children of nested keyboard accessible controls from a11y tree. Currently, children of keyboard accessible controls, such as the Labels inside LabelButtons, are not hidden from the accessibility tree. This is not useful because the user only cares about the element that has focus, not any children inside it, which are implementation details. Set any children of keyboard accessible controls from to have ignored roles and exclude them from the accessibility tree. BUG=610589 TEST=Open the XCode Accessibility Inspector, hover over the text on a views::LabelButton (e.g. on the HTTP Authentication dialog). It should report an empty set of children and its role should be a button. ==========
Description was changed from ========== a11y: Exclude children of nested keyboard accessible controls from a11y tree. Currently, children of keyboard accessible controls, such as the Labels inside LabelButtons, are not hidden from the accessibility tree. This is not useful because the user only cares about the element that has focus, not any children inside it, which are implementation details. Set any children of keyboard accessible controls from to have ignored roles and exclude them from the accessibility tree. BUG=610589 TEST=Open the XCode Accessibility Inspector, hover over the text on a views::LabelButton (e.g. on the HTTP Authentication dialog). It should report an empty set of children and its role should be a button. ========== to ========== a11y: Exclude children of nested keyboard accessible controls from a11y tree. Currently, children of keyboard accessible controls, such as the Labels inside LabelButtons, are not hidden from the accessibility tree. This is not useful because the user only cares about the element that has focus, not any children inside it, which are implementation details. Set any children of keyboard accessible controls from to have ignored roles and exclude them from the accessibility tree. BUG=610589 TEST=Open the XCode Accessibility Inspector, hover over the text on a views::LabelButton (e.g. on the HTTP Authentication dialog). It should report an empty set of children and its role should be a button. ==========
lgtm https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility.cc (right): https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:247: // don't have to search further because AXPlatformNodeWin will do a recursive I think all platforms should be doing this, not just AXPlatformNodeWin. I just checked AXPlatformNodeMac and it seems to be calling hit test recursively on each child that matches.
Sorry... lots more comments - I'm still discovering lots in the a11y code :)
I just noticed that NativeViewAccessibility::PopulateChildWidgetVector() has a
way to get a NativeViewAccessibility from a NativeViewAccessible --
Can we do something like
NativeViewAccessibility* NativeViewAccessibility::GetForView(View* view) {
gfx::NativeViewAccessible native_object = view->GetNativeViewAccessible();
ui::AXPlatformNode* node =
ui::AXPlatformNode::FromNativeViewAccessible(native_object);
DCHECK(node);
return static_cast<NativeViewAccessibility*>(node->GetDelegate());
}
although AXPlatformNode::FromNativeViewAccessible() is only implemented on
Windows. On Mac, it would go something like
AXPlatformNode* AXPlatformNode::FromNativeViewAccessible(
gfx::NativeViewAccessible accessible) {
if ([accessible isKindOfClass:[AXPlatformNodeCocoa class]])
return [accessible getNode];
return nullptr;
}
Perhaps we need this anyway - PopulateChildWidgetVector() is probably broken on
Mac (and Linux?) with FromNativeViewAccessible just returning nullptr due to:
#if !defined(OS_WIN)
// This is the default implementation for platforms where native views
// accessibility is unsupported or unfinished.
//
// static
AXPlatformNode* AXPlatformNode::FromNativeViewAccessible(
gfx::NativeViewAccessible accessible) {
return nullptr;
}
#endif
https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility...
File ui/views/accessibility/native_view_accessibility.cc (right):
https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility...
ui/views/accessibility/native_view_accessibility.cc:339:
NativeViewAccessibility::GetNativeViewAccessibilityMap() {
On 2017/01/11 02:01:47, Patti Lor wrote:
> On 2016/12/22 02:56:36, tapted wrote:
> > can this go in an anonymous namespace?
>
> Done - I assumed you also meant to make it a function as well (as opposed to a
> method), so let me know if that was wrong.
yep
https://codereview.chromium.org/2119413004/diff/540001/ui/accessibility/platf...
File ui/accessibility/platform/ax_platform_node_mac.mm (right):
https://codereview.chromium.org/2119413004/diff/540001/ui/accessibility/platf...
ui/accessibility/platform/ax_platform_node_mac.mm:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
CL description nit "controls from to have" -> "controls to have"
https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility...
File ui/views/accessibility/native_view_accessibility.cc (right):
https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility...
ui/views/accessibility/native_view_accessibility.cc:18: typedef std::map<View*,
NativeViewAccessibility*> NativeViewAccessibilityMap;
Although this feels natural for me... I think the preference is for `using` now.
i.e.
using NativeViewAccessibilityMap = std::map<View*, NativeViewAccessibility*>;
https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility...
ui/views/accessibility/native_view_accessibility.cc:20: // static
remove comment
https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility...
ui/views/accessibility/native_view_accessibility.cc:80: }
since the mapped_type is a primitive type, I think you can just do:
NativeViewAccessibility*& nva = GetNativeViewAccessibilityMap()[view];
if (!nva)
nva = Create(view);
return nva;
https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility...
ui/views/accessibility/native_view_accessibility.cc:177: int child_widget_count
= static_cast<int>(child_widgets.size());
i don't know why this static_cast is here - I don't think it's needed
https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility...
ui/views/accessibility/native_view_accessibility.cc:178:
We can avoid multiple calls to GetUnignoredA11yChildren here by not calling
GetChildCount() above and below. PopulateChildWidgetVector is already repeated,
so I think it's fine to repeat GetUnignoredA11yChildren as well.
e.g., here, do
std::vector<gfx::NVA> a11y_children = GetUnignored..
int child_view_count = a11y_children.size();
if (index >= child_view_count + child_widget_count)
return nullptr; (note: also covers GetChildCount() == 0)
if (index < child_view_count)
return a11y_children[index];
Widget* child_widget = child_widgets[index - child_view_count];
return child_widget->GetRootView()->GetNativeViewAccessible();
https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility...
File ui/views/accessibility/native_view_accessibility.h (right):
https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility...
ui/views/accessibility/native_view_accessibility.h:84: typedef std::map<View*,
NativeViewAccessibility*> NativeViewAccessibilityMap;
can this (and the <map> #include) move into the .cc file as well? (actually I
think the typedef has already, so we can just move the comment perhaps)
https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility...
File ui/views/accessibility/native_view_accessibility_unittest.cc (right):
https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility...
ui/views/accessibility/native_view_accessibility_unittest.cc:18: class
NativeViewAccessibilityTest;
nit: (while you're here...) I don't think this forward dec is doing anything?
https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility...
ui/views/accessibility/native_view_accessibility_unittest.cc:28: View*
AddNewChildWithBoundsAndFocusability(View& parent,
style guide doesn't allow non-const by-ref arguments -- View* parent
https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility...
ui/views/accessibility/native_view_accessibility_unittest.cc:29: gfx::Rect
bounds,
nit: const ref for bounds
https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility...
ui/views/accessibility/native_view_accessibility_unittest.cc:32:
child->SetBounds(bounds.x(), bounds.y(), bounds.width(), bounds.height());
child->SetBoundsRect(bounds);
https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility...
ui/views/accessibility/native_view_accessibility_unittest.cc:35:
child->SetFocusBehavior(ClientView::FocusBehavior::ACCESSIBLE_ONLY);
nit:
child->SetFocusBehaviour(focusable ? View::FocusBehavior::ACCESSIBLE_ONLY :
...NEVER)
(+git cl-format)
https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility...
ui/views/accessibility/native_view_accessibility_unittest.cc:194:
~NativeViewAccessibilityIgnoredElementsTest() override {}
nit: I think it's safe to omit the destructor
https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility...
ui/views/accessibility/native_view_accessibility_unittest.cc:217:
a_root_->SetFocusBehavior(ClientView::FocusBehavior::ACCESSIBLE_ONLY);
ClientView -> View
(ClientView just happens to be a subclass of views::View. It's not really
related to focus/controls, but how a dialog widget is organised)
https://codereview.chromium.org/2119413004/diff/540001/ui/views/view.cc
File ui/views/view.cc (right):
https://codereview.chromium.org/2119413004/diff/540001/ui/views/view.cc#newco...
ui/views/view.cc:1392: NativeViewAccessibility::GetOrCreate(this));
it does seem a bit spooky that a call to GetOrCreate not done from here could
end up putting a View* into map that could be destroyed without removing that
map entry. But! I think I have a solution: `GetOrCreate` just needs to call
view->GetNativeViewAccessible(). Of course, that will be infinite recursion
right now. So here I think it should be:
native_view_accessiblity_ = base::WrapUnique(NVA::CreateForView(this));
and instead of GetOrCreate(view), something like
NVA* GetForView(View* view) {
view->GetNativeViewAccesible(); // Ensure a mapping exists.
NVA* entry = GetNativeViewAccessibilityMap()[view];
DCHECK(entry);
return entry;
}
I think then we can also check that the call to nvamap.erase() always erases
exactly 1 object too.
https://codereview.chromium.org/2119413004/diff/540001/ui/views/view.cc#newco...
ui/views/view.cc:1407: native_view_accessibility_.reset(
= base::WrapUnique(..)
Or, better, NVA::CreateForView might be able to return a std::unique_ptr and use
WrapUnique itself
Description was changed from ========== a11y: Exclude children of nested keyboard accessible controls from a11y tree. Currently, children of keyboard accessible controls, such as the Labels inside LabelButtons, are not hidden from the accessibility tree. This is not useful because the user only cares about the element that has focus, not any children inside it, which are implementation details. Set any children of keyboard accessible controls from to have ignored roles and exclude them from the accessibility tree. BUG=610589 TEST=Open the XCode Accessibility Inspector, hover over the text on a views::LabelButton (e.g. on the HTTP Authentication dialog). It should report an empty set of children and its role should be a button. ========== to ========== a11y: Exclude children of nested keyboard accessible controls from a11y tree. Currently, children of keyboard accessible controls, such as the Labels inside LabelButtons, are not hidden from the accessibility tree. This is not useful because the user only cares about the element that has focus, not any children inside it, which are implementation details. Set any children of keyboard accessible controls to have ignored roles and exclude them from the accessibility tree. BUG=610589 TEST=Open the XCode Accessibility Inspector, hover over the text on a views::LabelButton (e.g. on the HTTP Authentication dialog). It should report an empty set of children and its role should be a button. ==========
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Patchset #27 (id:620001) has been deleted
Patchset #26 (id:600001) has been deleted
Patchset #25 (id:580001) has been deleted
Patchset #24 (id:560001) has been deleted
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #24 (id:640001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2119413004/diff/680001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_mac.h (right): https://codereview.chromium.org/2119413004/diff/680001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.h:55: - (ui::AXPlatformNodeBase*)getNode; @property(nonatomic, readonly) ui::AXPlatformNodeBase* node; (and @synthesize node = node_; in the .mm) https://codereview.chromium.org/2119413004/diff/680001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility.cc (right): https://codereview.chromium.org/2119413004/diff/680001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:191: gfx::NativeViewAccessible NativeViewAccessibility::GetParent() { yeah NativeViewAccessible is `typedef void*` on ChromeOS, so this should either return null, or simply not get called on ChromeOS at all (i.e. disable the tests on ChromeOS that get here, or just remove the test.cc files from the sources list on cros) https://codereview.chromium.org/2119413004/diff/680001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:202: return (gfx::NativeViewAccessible)view_->GetWidget()->GetNativeView(); can the c-style cast be removed?
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #26 (id:700001) has been deleted
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #26 (id:720001) has been deleted
Patchset #26 (id:740001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #29 (id:820001) has been deleted
Patchset #28 (id:800001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ok, finally refactored the whole thing to use AXPlatformNode::FromNativeViewAccessible(), including implementing it for Linux and CrOS. CrOS actually just falls back to the original implementation (returning a new instance), since there is no gfx::NativeViewAccessible implementation there. PTAL, thanks again! https://codereview.chromium.org/2119413004/diff/540001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2119413004/diff/540001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2017/01/11 18:27:39, tapted wrote: > CL description nit "controls from to have" -> "controls to have" Done. https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility.cc (right): https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:18: typedef std::map<View*, NativeViewAccessibility*> NativeViewAccessibilityMap; On 2017/01/11 18:27:39, tapted wrote: > Although this feels natural for me... I think the preference is for `using` now. > i.e. > > using NativeViewAccessibilityMap = std::map<View*, NativeViewAccessibility*>; Done - sorry, you've told me this before >< https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:20: // static On 2017/01/11 18:27:39, tapted wrote: > remove comment Done. https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:80: } On 2017/01/11 18:27:39, tapted wrote: > since the mapped_type is a primitive type, I think you can just do: > > NativeViewAccessibility*& nva = GetNativeViewAccessibilityMap()[view]; > if (!nva) > nva = Create(view); > return nva; Deleted, not needed any more. https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:177: int child_widget_count = static_cast<int>(child_widgets.size()); On 2017/01/11 18:27:39, tapted wrote: > i don't know why this static_cast is here - I don't think it's needed Done. https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:178: On 2017/01/11 18:27:39, tapted wrote: > We can avoid multiple calls to GetUnignoredA11yChildren here by not calling > GetChildCount() above and below. PopulateChildWidgetVector is already repeated, > so I think it's fine to repeat GetUnignoredA11yChildren as well. > > e.g., here, do > > std::vector<gfx::NVA> a11y_children = GetUnignored.. > int child_view_count = a11y_children.size(); > if (index >= child_view_count + child_widget_count) > return nullptr; (note: also covers GetChildCount() == 0) > > if (index < child_view_count) > return a11y_children[index]; > > Widget* child_widget = child_widgets[index - child_view_count]; > return child_widget->GetRootView()->GetNativeViewAccessible(); > Done. https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:247: // don't have to search further because AXPlatformNodeWin will do a recursive On 2017/01/11 07:39:37, dmazzoni wrote: > I think all platforms should be doing this, not just AXPlatformNodeWin. > I just checked AXPlatformNodeMac and it seems to be calling > hit test recursively on each child that matches. Do you mean -accessibilityHitTest? It doesn't seem as if it's calling NVA::HitTestSync though, I think it just has its own implementation which is recursive. I'm not sure if it should be changed to use HitTestSync though, I think the difference would be that on Mac Widgets aren't included in the hit test. https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility.h (right): https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.h:84: typedef std::map<View*, NativeViewAccessibility*> NativeViewAccessibilityMap; On 2017/01/11 18:27:39, tapted wrote: > can this (and the <map> #include) move into the .cc file as well? (actually I > think the typedef has already, so we can just move the comment perhaps) Deleted as no longer needed :) Thanks for your suggestions Trent! https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility_unittest.cc (right): https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:18: class NativeViewAccessibilityTest; On 2017/01/11 18:27:39, tapted wrote: > nit: (while you're here...) I don't think this forward dec is doing anything? Deleted. https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:28: View* AddNewChildWithBoundsAndFocusability(View& parent, On 2017/01/11 18:27:39, tapted wrote: > style guide doesn't allow non-const by-ref arguments -- View* parent Done. https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:29: gfx::Rect bounds, On 2017/01/11 18:27:40, tapted wrote: > nit: const ref for bounds Done. https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:32: child->SetBounds(bounds.x(), bounds.y(), bounds.width(), bounds.height()); On 2017/01/11 18:27:40, tapted wrote: > child->SetBoundsRect(bounds); Done. https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:35: child->SetFocusBehavior(ClientView::FocusBehavior::ACCESSIBLE_ONLY); On 2017/01/11 18:27:39, tapted wrote: > nit: > child->SetFocusBehaviour(focusable ? View::FocusBehavior::ACCESSIBLE_ONLY : > ...NEVER) > > (+git cl-format) Done. https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:194: ~NativeViewAccessibilityIgnoredElementsTest() override {} On 2017/01/11 18:27:40, tapted wrote: > nit: I think it's safe to omit the destructor Done. https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:217: a_root_->SetFocusBehavior(ClientView::FocusBehavior::ACCESSIBLE_ONLY); On 2017/01/11 18:27:39, tapted wrote: > ClientView -> View > > (ClientView just happens to be a subclass of views::View. It's not really > related to focus/controls, but how a dialog widget is organised) Done. https://codereview.chromium.org/2119413004/diff/540001/ui/views/view.cc File ui/views/view.cc (right): https://codereview.chromium.org/2119413004/diff/540001/ui/views/view.cc#newco... ui/views/view.cc:1392: NativeViewAccessibility::GetOrCreate(this)); On 2017/01/11 18:27:40, tapted wrote: > it does seem a bit spooky that a call to GetOrCreate not done from here could > end up putting a View* into map that could be destroyed without removing that > map entry. But! I think I have a solution: `GetOrCreate` just needs to call > view->GetNativeViewAccessible(). Of course, that will be infinite recursion > right now. So here I think it should be: > > native_view_accessiblity_ = base::WrapUnique(NVA::CreateForView(this)); > > and instead of GetOrCreate(view), something like > > NVA* GetForView(View* view) { > view->GetNativeViewAccesible(); // Ensure a mapping exists. > NVA* entry = GetNativeViewAccessibilityMap()[view]; > DCHECK(entry); > return entry; > } > > > I think then we can also check that the call to nvamap.erase() always erases > exactly 1 object too. So I now have public: - static NVA::CreateForView() - should only be called by View - static NVA::GetForView() - should be called by everyone else Should we enforce this by making CreateForView protected also and friend View? Not sure when it's OK to do that. https://codereview.chromium.org/2119413004/diff/540001/ui/views/view.cc#newco... ui/views/view.cc:1407: native_view_accessibility_.reset( On 2017/01/11 18:27:40, tapted wrote: > = base::WrapUnique(..) > > Or, better, NVA::CreateForView might be able to return a std::unique_ptr and use > WrapUnique itself Done. https://codereview.chromium.org/2119413004/diff/680001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_mac.h (right): https://codereview.chromium.org/2119413004/diff/680001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.h:55: - (ui::AXPlatformNodeBase*)getNode; On 2017/02/15 02:10:45, tapted wrote: > @property(nonatomic, readonly) ui::AXPlatformNodeBase* node; > > (and > > @synthesize node = node_; > > in the .mm) Done, also moved the private declaration of |node_| to inside the .mm. https://codereview.chromium.org/2119413004/diff/680001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility.cc (right): https://codereview.chromium.org/2119413004/diff/680001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:191: gfx::NativeViewAccessible NativeViewAccessibility::GetParent() { On 2017/02/15 02:10:45, tapted wrote: > yeah NativeViewAccessible is `typedef void*` on ChromeOS, so this should either > return null, or simply not get called on ChromeOS at all (i.e. disable the tests > on ChromeOS that get here, or just remove the test.cc files from the sources > list on cros) Yep - so I ended up just returning a new instance of NativeViewAccessibility (consistent with the old behaviour) for CrOS in GetForView(), and verified that this still returns null on that platform. I'm not sure if keeping all the tests are useful (e.g. ChildAtIndex, GetParent, HitTestSync shouldn't be doing anything other than comparing nullptrs), do you think it's worth ifdefing them out or just leaving them? They still pass. https://codereview.chromium.org/2119413004/diff/680001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:202: return (gfx::NativeViewAccessible)view_->GetWidget()->GetNativeView(); On 2017/02/15 02:10:45, tapted wrote: > can the c-style cast be removed? Yep! Originally, I tried a static_cast instead, but that doesn't work because gfx::NativeViewAccessible/id/struct objc_object* and gfx::NativeView/NSView* "aren't related by inheritance", but now that https://crrev.com/2704263002 puts this bit of Mac-specific code in an .mm file, it works without casting at all. https://codereview.chromium.org/2119413004/diff/840001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility_mac.mm (right): https://codereview.chromium.org/2119413004/diff/840001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_mac.mm:24: ui::AX_ROLE_IGNORED) I just inlined IsViewA11yIgnored() from native_view_accessibility.cc here, hopefully that's OK.
https://codereview.chromium.org/2119413004/diff/840001/ui/gfx/native_widget_t... File ui/gfx/native_widget_types.h (right): https://codereview.chromium.org/2119413004/diff/840001/ui/gfx/native_widget_t... ui/gfx/native_widget_types.h:147: typedef void* NativeViewAccessible; I think we can get rid of this one too https://codereview.chromium.org/2119413004/diff/840001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility.cc (right): https://codereview.chromium.org/2119413004/diff/840001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:83: return Create(view); I think this means it will be leaked. I think we need that friend, so we can do view->GetNativeViewAccessible(); return view_->native_view_accessibility_; If we are only a friend on OS_CHROMEOS, maybe that will allow us to sell it to sky https://codereview.chromium.org/2119413004/diff/840001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:229: } Can you comment about this - I'm not sure it looks right. E.g. - We may return an object when we didn't previously since the HitTestPoint on line 233 is skipped - It seems inconsistent that GetChildCount() above checks for unignored children, and the loop below checks all child views without the ignored-check. (e.g. what if there is one unignored child and one ignored child: GetChildCount() returns 1, but the loop below might hit the ignored child?) https://codereview.chromium.org/2119413004/diff/840001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:237: // don't have to search further because AXPlatformNodeWin will do a recursive was there a comment earlier about how this applies on all platforms (not just AXPlatformNodeWin)? https://codereview.chromium.org/2119413004/diff/840001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility.h (right): https://codereview.chromium.org/2119413004/diff/840001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.h:45: static std::unique_ptr<NativeViewAccessibility> CreateForView(View* view); Let's move the unique_ptr/Destroy/CreateForView changes to a separate CL to land first - those have been constant throughout, and will make the review here simpler. https://codereview.chromium.org/2119413004/diff/840001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.h:48: // calls CreateForView() if it doesn't yet exist. Can we instead say "Invokes View::GetNativeViewAccessible() to ensure it exists." https://codereview.chromium.org/2119413004/diff/840001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility_mac.mm (right): https://codereview.chromium.org/2119413004/diff/840001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_mac.mm:24: ui::AX_ROLE_IGNORED) On 2017/02/21 03:29:17, Patti Lor wrote: > I just inlined IsViewA11yIgnored() from native_view_accessibility.cc here, > hopefully that's OK. Can this do something like gfx::NativeViewAccessible toolkit_parent = NativeViewAccessibility::GetToolkitParent(); if (toolkit_parent) return toolkit_parent; if (view_->GetWidget()) return view_->GetWidget()->GetNativeView(); if (parent_widget_) return parent_widget_->GetRootView()->GetNativeViewAccessible(); (maybe this should be in that other CL...) https://codereview.chromium.org/2119413004/diff/840001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility_unittest.cc (right): https://codereview.chromium.org/2119413004/diff/840001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:80: NativeViewAccessibility* button_accessibility_; if we convert these to unique_ptr can we still use Create? This will come out in the CL we split off and land first anyway (maybe we can make GetForView a protected method then) https://codereview.chromium.org/2119413004/diff/840001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility_win.cc (right): https://codereview.chromium.org/2119413004/diff/840001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_win.cc:61: } These reorderings (here and in auralinux.cc) might also be good candidates for separate CLs to make the review simpler. It also comes up occasionally that things need to get reverted/merged and keeping changes like this separate creates less confusion/churn when that happens. https://codereview.chromium.org/2119413004/diff/840001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc (right): https://codereview.chromium.org/2119413004/diff/840001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:161: ui::AXNodeData data = same here - can these just be unique_ptr?
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2119413004/diff/840001/ui/gfx/native_widget_t... File ui/gfx/native_widget_types.h (right): https://codereview.chromium.org/2119413004/diff/840001/ui/gfx/native_widget_t... ui/gfx/native_widget_types.h:147: typedef void* NativeViewAccessible; On 2017/02/21 06:01:11, tapted wrote: > I think we can get rid of this one too Done. https://codereview.chromium.org/2119413004/diff/840001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility.cc (right): https://codereview.chromium.org/2119413004/diff/840001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:83: return Create(view); On 2017/02/21 06:01:11, tapted wrote: > I think this means it will be leaked. I think we need that friend, so we can do > > view->GetNativeViewAccessible(); > return view_->native_view_accessibility_; > > If we are only a friend on OS_CHROMEOS, maybe that will allow us to sell it to > sky Done. https://codereview.chromium.org/2119413004/diff/840001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:229: } On 2017/02/21 06:01:11, tapted wrote: > Can you comment about this - I'm not sure it looks right. E.g. > > - We may return an object when we didn't previously since the HitTestPoint on > line 233 is skipped > - It seems inconsistent that GetChildCount() above checks for unignored > children, and the loop below checks all child views without the ignored-check. > (e.g. what if there is one unignored child and one ignored child: > GetChildCount() returns 1, but the loop below might hit the ignored child?) Yeah, it's kinda confusing (I had to work through it again). You're right about both points - for the first, I just moved that check down below line 233. For the second, I had initially thought this was OK because sometimes we want to return an ignored child (if it possesses unignored children) to recurse down in, but turns out this is also the case for ignored leaves. Following it through completely with AXPlatformodeWin::accHitTest() (its caller), turns out it will actually conclude there is nothing there and return false. I've refactored this now (including swapping the visible() check on line 241 to IsDrawn()), and the tests make more sense now (previously, I was assuming that callers of HitTestSync() would continue looking if I returned nullptr for an ignored leaf, which doesn't make sense because even the comment explicitly says it doesn't do this). Thanks for picking that up :) https://codereview.chromium.org/2119413004/diff/840001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.cc:237: // don't have to search further because AXPlatformNodeWin will do a recursive On 2017/02/21 06:01:11, tapted wrote: > was there a comment earlier about how this applies on all platforms (not just > AXPlatformNodeWin)? Yeah, see https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility.... I don't think this is true, so am leaving it as is. https://codereview.chromium.org/2119413004/diff/840001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility.h (right): https://codereview.chromium.org/2119413004/diff/840001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.h:45: static std::unique_ptr<NativeViewAccessibility> CreateForView(View* view); On 2017/02/21 06:01:11, tapted wrote: > Let's move the unique_ptr/Destroy/CreateForView changes to a separate CL to land > first - those have been constant throughout, and will make the review here > simpler. Done. https://codereview.chromium.org/2119413004/diff/840001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility.h:48: // calls CreateForView() if it doesn't yet exist. On 2017/02/21 06:01:11, tapted wrote: > Can we instead say "Invokes View::GetNativeViewAccessible() to ensure it > exists." Done. https://codereview.chromium.org/2119413004/diff/840001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility_mac.mm (right): https://codereview.chromium.org/2119413004/diff/840001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_mac.mm:24: ui::AX_ROLE_IGNORED) On 2017/02/21 06:01:12, tapted wrote: > On 2017/02/21 03:29:17, Patti Lor wrote: > > I just inlined IsViewA11yIgnored() from native_view_accessibility.cc here, > > hopefully that's OK. > > Can this do something like > > gfx::NativeViewAccessible toolkit_parent = > NativeViewAccessibility::GetToolkitParent(); > if (toolkit_parent) > return toolkit_parent; > > if (view_->GetWidget()) > return view_->GetWidget()->GetNativeView(); > > if (parent_widget_) > return parent_widget_->GetRootView()->GetNativeViewAccessible(); > > > (maybe this should be in that other CL...) Done, kinda - traded your GetToolkitParent() suggestion for a virtual method that retrieves the Widget parent, since both will be adding a new method anyway. https://codereview.chromium.org/2119413004/diff/840001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility_unittest.cc (right): https://codereview.chromium.org/2119413004/diff/840001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_unittest.cc:80: NativeViewAccessibility* button_accessibility_; On 2017/02/21 06:01:12, tapted wrote: > if we convert these to unique_ptr can we still use Create? > > This will come out in the CL we split off and land first anyway (maybe we can > make GetForView a protected method then) I think this part of the tests it's possible, but for the ignored tests the View's NVA and the NVA created by the test will be different instances. For now I just converted this to use CreateForView() and will friend the other test, hopefully that's ok. To make GetForView a protected method, I also had to move IsViewA11yIgnored() to a static method on NVA (again, this is on the other CL). https://codereview.chromium.org/2119413004/diff/840001/ui/views/accessibility... File ui/views/accessibility/native_view_accessibility_win.cc (right): https://codereview.chromium.org/2119413004/diff/840001/ui/views/accessibility... ui/views/accessibility/native_view_accessibility_win.cc:61: } On 2017/02/21 06:01:12, tapted wrote: > These reorderings (here and in auralinux.cc) might also be good candidates for > separate CLs to make the review simpler. It also comes up occasionally that > things need to get reverted/merged and keeping changes like this separate > creates less confusion/churn when that happens. Moved them to the other CL. https://codereview.chromium.org/2119413004/diff/840001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc (right): https://codereview.chromium.org/2119413004/diff/840001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:161: ui::AXNodeData data = On 2017/02/21 06:01:12, tapted wrote: > same here - can these just be unique_ptr? Done.
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
Patchset #32 (id:920001) has been deleted
Patchset #33 (id:960001) has been deleted
Patchset #31 (id:900001) has been deleted
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #30 (id:880001) has been deleted
Patchset #30 (id:940001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #31 (id:1000001) has been deleted
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
Patchset #29 (id:860001) has been deleted
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Patchset #29 (id:980001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #29 (id:1020001) has been deleted
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #29 (id:1040001) has been deleted
ping @ Trent? Thanks :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mostly nits, and some questions https://codereview.chromium.org/2119413004/diff/1060001/ui/views/accessibilit... File ui/views/accessibility/native_view_accessibility_base.cc (right): https://codereview.chromium.org/2119413004/diff/1060001/ui/views/accessibilit... ui/views/accessibility/native_view_accessibility_base.cc:25: // being a non-keyboard-focusable child of a keyboard-focusable ancestor. An example might be good here in the comment to clarify what this is used for (i.e. button labels) https://codereview.chromium.org/2119413004/diff/1060001/ui/views/accessibilit... ui/views/accessibility/native_view_accessibility_base.cc:170: return GetForView(parent_view)->GetParent(); (assuming it works..) things might be simpler if we don't recurse here. E.g. View* parent_view = view_->parent(); while (parent_view && IsIgnoredView(parent_view)) parent_view = parent_view->parent(); return parent_view ? parent_view->GetNativeViewAccessible() : GetNativeViewAccessibleForWidget(); https://codereview.chromium.org/2119413004/diff/1060001/ui/views/accessibilit... ui/views/accessibility/native_view_accessibility_base.cc:221: return child_nva->GetParent(); Can this get stuck in a loop? The comment above suggests to me that AXPlatformNodeWin will test again if something other than GetNativeObject() or nullptr is returned. So, what happens when this *does* jump multiple levels, to a parent - can it come back down, only to jump back up again? https://codereview.chromium.org/2119413004/diff/1060001/ui/views/accessibilit... File ui/views/accessibility/native_view_accessibility_base.h (right): https://codereview.chromium.org/2119413004/diff/1060001/ui/views/accessibilit... ui/views/accessibility/native_view_accessibility_base.h:38: static bool Ignored(View* view); perhaps IsIgnoredView()? https://codereview.chromium.org/2119413004/diff/1060001/ui/views/accessibilit... File ui/views/accessibility/native_view_accessibility_mac.mm (right): https://codereview.chromium.org/2119413004/diff/1060001/ui/views/accessibilit... ui/views/accessibility/native_view_accessibility_mac.mm:26: if (view_->GetWidget()) comment here why |parent_widget_| doesn't work for Mac? (e.g. is it a bug that we don't set a parent_widget_ on mac?)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #30 (id:1080001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #30 (id:1100001) has been deleted
Hi Trent, PTAL https://codereview.chromium.org/2119413004/diff/1060001/ui/views/accessibilit... File ui/views/accessibility/native_view_accessibility_base.cc (right): https://codereview.chromium.org/2119413004/diff/1060001/ui/views/accessibilit... ui/views/accessibility/native_view_accessibility_base.cc:25: // being a non-keyboard-focusable child of a keyboard-focusable ancestor. On 2017/04/03 05:50:36, tapted wrote: > An example might be good here in the comment to clarify what this is used for > (i.e. button labels) Done. https://codereview.chromium.org/2119413004/diff/1060001/ui/views/accessibilit... ui/views/accessibility/native_view_accessibility_base.cc:170: return GetForView(parent_view)->GetParent(); On 2017/04/03 05:50:36, tapted wrote: > (assuming it works..) things might be simpler if we don't recurse here. E.g. > > View* parent_view = view_->parent(); > while (parent_view && IsIgnoredView(parent_view)) > parent_view = parent_view->parent(); > > return parent_view ? parent_view->GetNativeViewAccessible() : > GetNativeViewAccessibleForWidget(); Oh, that's way better. It works, thanks! https://codereview.chromium.org/2119413004/diff/1060001/ui/views/accessibilit... ui/views/accessibility/native_view_accessibility_base.cc:221: return child_nva->GetParent(); On 2017/04/03 05:50:36, tapted wrote: > Can this get stuck in a loop? > > The comment above suggests to me that AXPlatformNodeWin will test again if > something other than GetNativeObject() or nullptr is returned. > > So, what happens when this *does* jump multiple levels, to a parent - can it > come back down, only to jump back up again? I think this is covered in the tests - if you look at NVABaseIgnoredElementsTest.HitTestSync, these two lines (451-452): EXPECT_EQ(b_nva_->GetNativeObject(), a_root_nva_->HitTestSync(curr_x, y)); EXPECT_EQ(b_nva_->GetNativeObject(), b_nva_->HitTestSync(curr_x, y)); Where this is testing a non-ignored element (B) with an ignored child (X2). AXPlatformNodeWin::accHitTest() has a base case for when the hit target returned is the same as itself, so as long as the second line above is passing, it won't loop. https://codereview.chromium.org/2119413004/diff/1060001/ui/views/accessibilit... File ui/views/accessibility/native_view_accessibility_base.h (right): https://codereview.chromium.org/2119413004/diff/1060001/ui/views/accessibilit... ui/views/accessibility/native_view_accessibility_base.h:38: static bool Ignored(View* view); On 2017/04/03 05:50:36, tapted wrote: > perhaps IsIgnoredView()? Done. https://codereview.chromium.org/2119413004/diff/1060001/ui/views/accessibilit... File ui/views/accessibility/native_view_accessibility_mac.mm (right): https://codereview.chromium.org/2119413004/diff/1060001/ui/views/accessibilit... ui/views/accessibility/native_view_accessibility_mac.mm:26: if (view_->GetWidget()) On 2017/04/03 05:50:36, tapted wrote: > comment here why |parent_widget_| doesn't work for Mac? > > (e.g. is it a bug that we don't set a parent_widget_ on mac?) I'm not sure why we are preferring to use the NativeView here over the NativeViewAccessible of the Widget's RootView. I think this part of the code was added before the |parent_widget_| NVA code in the current NVABase::GetNVAForWidget() - see https://crrev.com/420653003, so maybe it's actually just not needed any more. (Like, will there ever be a situation where |view_| is attached to a Widget, but its Widget doesn't have a RootView? I don't think that should happen, but you know more than me.) I think the only reason why we don't set |parent_widget_| on Mac is because we never happen to hit the part of the code in PopulateChildWidgetVector() that actually calls SetParentWidget() (I messed around a bit to try and find a situation where this might happen on Mac, including with MacViews browser, but couldn't really). But PopulateChildWidgetVector() doesn't actually seem to need parent_widget_ itself, I think maybe it just assumes that whatever calls it might need to access |parent_widget_| later (probably by calling GetParent()). WDYT of deleting this override and replacing the getter for parent_widget_ that will instead retrieve it on the fly? We might also have to DCHECK() in GetParent() that |view_| is a RootView when we try accessing |parent_widget_| (since PopulateChildWidgetVector() only sets |parent_widget_| if that's the case).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2119413004/diff/1060001/ui/views/accessibilit... File ui/views/accessibility/native_view_accessibility_base.cc (right): https://codereview.chromium.org/2119413004/diff/1060001/ui/views/accessibilit... ui/views/accessibility/native_view_accessibility_base.cc:221: return child_nva->GetParent(); On 2017/04/04 02:46:28, Patti Lor wrote: > On 2017/04/03 05:50:36, tapted wrote: > > Can this get stuck in a loop? > > > > The comment above suggests to me that AXPlatformNodeWin will test again if > > something other than GetNativeObject() or nullptr is returned. > > > > So, what happens when this *does* jump multiple levels, to a parent - can it > > come back down, only to jump back up again? > > I think this is covered in the tests - if you look at > NVABaseIgnoredElementsTest.HitTestSync, these two lines (451-452): > > EXPECT_EQ(b_nva_->GetNativeObject(), a_root_nva_->HitTestSync(curr_x, y)); > EXPECT_EQ(b_nva_->GetNativeObject(), b_nva_->HitTestSync(curr_x, y)); > > Where this is testing a non-ignored element (B) with an ignored child (X2). > AXPlatformNodeWin::accHitTest() has a base case for when the hit target returned > is the same as itself, so as long as the second line above is passing, it won't > loop. Acknowledged. https://codereview.chromium.org/2119413004/diff/1060001/ui/views/accessibilit... File ui/views/accessibility/native_view_accessibility_mac.mm (right): https://codereview.chromium.org/2119413004/diff/1060001/ui/views/accessibilit... ui/views/accessibility/native_view_accessibility_mac.mm:26: if (view_->GetWidget()) On 2017/04/04 02:46:29, Patti Lor wrote: > On 2017/04/03 05:50:36, tapted wrote: > > comment here why |parent_widget_| doesn't work for Mac? > > > > (e.g. is it a bug that we don't set a parent_widget_ on mac?) > > I'm not sure why we are preferring to use the NativeView here over the > NativeViewAccessible of the Widget's RootView. I think this part of the code was > added before the |parent_widget_| NVA code in the current > NVABase::GetNVAForWidget() - see https://crrev.com/420653003, so maybe it's > actually just not needed any more. (Like, will there ever be a situation where > |view_| is attached to a Widget, but its Widget doesn't have a RootView? I don't > think that should happen, but you know more than me.) > > I think the only reason why we don't set |parent_widget_| on Mac is because we > never happen to hit the part of the code in PopulateChildWidgetVector() that > actually calls SetParentWidget() (I messed around a bit to try and find a > situation where this might happen on Mac, including with MacViews browser, but > couldn't really). But PopulateChildWidgetVector() doesn't actually seem to need > parent_widget_ itself, I think maybe it just assumes that whatever calls it > might need to access |parent_widget_| later (probably by calling GetParent()). > > WDYT of deleting this override and replacing the getter for parent_widget_ that > will instead retrieve it on the fly? We might also have to DCHECK() in > GetParent() that |view_| is a RootView when we try accessing |parent_widget_| > (since PopulateChildWidgetVector() only sets |parent_widget_| if that's the > case). I think |parent_widget_| came about because in aura the NVA can teleport between widgets, and they need to set up WidgetObservers correctly.. I need to look at this some more. (Maybe dmazzoni has some ideas..) https://codereview.chromium.org/2119413004/diff/1110001/ui/views/accessibilit... File ui/views/accessibility/native_view_accessibility_base.cc (right): https://codereview.chromium.org/2119413004/diff/1110001/ui/views/accessibilit... ui/views/accessibility/native_view_accessibility_base.cc:64: return false; Comment here like // If the native accessibility object isn't backed by NativeViewAccessibility (e.g. when the View wraps a native object), assume it should not be ignored.
i've managed to get myself really lost - some suggestions below might allow an approach that fixes the bug without raising questions for other platforms - I think this currently breaks hit testing on Windows. If those suggestions don't work, I think to move forward on this, the next step could be to try to address the GetNativeViewAccessibleForWidget()/GetParent() changes in isolation. https://codereview.chromium.org/2119413004/diff/1110001/ui/accessibility/plat... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2119413004/diff/1110001/ui/accessibility/plat... ui/accessibility/platform/ax_platform_node_mac.mm:292: - (BOOL)accessibilityIsIgnored { Should this be updated to include ui::AX_ROLE_IGNORED ? (any chance that's a possible shortcut to fixing http://crbug.com/610589 ?) https://codereview.chromium.org/2119413004/diff/1110001/ui/views/accessibilit... File ui/views/accessibility/native_view_accessibility_base.cc (right): https://codereview.chromium.org/2119413004/diff/1110001/ui/views/accessibilit... ui/views/accessibility/native_view_accessibility_base.cc:60: bool NativeViewAccessibilityBase::IsIgnoredView(View* view) { Would would happen if we did something like (ax_platform_node_mac.mm) // static bool AXPlatformNode::IsIgnoredNode(gfx::NativeViewAccessible accessible) { return [accessible accessibilityIsIgnored]; } (ax_platform_node_win.cc) bool AXPlatformNode::IsIgnoredNode(gfx::NativeViewAccessible accessible) { return accessible->get_accState() & STATE_SYSTEM_INVISIBLE; } (you'd need to update AXPlatformNodeWin::MSAAState() to treat ui::AX_STATE_INVISIBLE and ui::AX_STATE_IGNORED the same) https://codereview.chromium.org/2119413004/diff/1110001/ui/views/accessibilit... ui/views/accessibility/native_view_accessibility_base.cc:216: if (child_nva) { I think this means that any webview child can't be hit? AXPlatformNodeWin::accHitTest(..) seems to rely on the NativeViewAccessible above a webview to return the non-NativeViewAccessible native node from its webview child in order to do a recursive hit test on the webview. With this change, I think hit-testing a the NativeViewAccessible above a webview will now return `this` and prevent the recursion into the webview's native accessibility object. That is, previously when doing if (child_view->HitTestPoint(point_in_child_coords)) return child_view->GetNativeViewAccessible(); that could return the result of WebView::GetNativeViewAccessible() but now, since WebView::GetNativeViewAccessible() doesn't return a NativeViewAccessibilityBase*, the method will return GetNativeObject() instead. https://codereview.chromium.org/2119413004/diff/1110001/ui/views/accessibilit... ui/views/accessibility/native_view_accessibility_base.cc:310: NativeViewAccessibilityBase::GetNativeViewAccessibleForWidget() { I think this needs an override in NativeViewAccessiblilityWin as well?
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Trent - I need to do more investigation for your other comments that I've missed, but I've replied to 3/5. https://codereview.chromium.org/2119413004/diff/1060001/ui/views/accessibilit... File ui/views/accessibility/native_view_accessibility_mac.mm (right): https://codereview.chromium.org/2119413004/diff/1060001/ui/views/accessibilit... ui/views/accessibility/native_view_accessibility_mac.mm:26: if (view_->GetWidget()) On 2017/04/04 08:13:13, tapted wrote: > On 2017/04/04 02:46:29, Patti Lor wrote: > > On 2017/04/03 05:50:36, tapted wrote: > > > comment here why |parent_widget_| doesn't work for Mac? > > > > > > (e.g. is it a bug that we don't set a parent_widget_ on mac?) > > > > I'm not sure why we are preferring to use the NativeView here over the > > NativeViewAccessible of the Widget's RootView. I think this part of the code > was > > added before the |parent_widget_| NVA code in the current > > NVABase::GetNVAForWidget() - see https://crrev.com/420653003, so maybe it's > > actually just not needed any more. (Like, will there ever be a situation where > > |view_| is attached to a Widget, but its Widget doesn't have a RootView? I > don't > > think that should happen, but you know more than me.) > > > > I think the only reason why we don't set |parent_widget_| on Mac is because we > > never happen to hit the part of the code in PopulateChildWidgetVector() that > > actually calls SetParentWidget() (I messed around a bit to try and find a > > situation where this might happen on Mac, including with MacViews browser, but > > couldn't really). But PopulateChildWidgetVector() doesn't actually seem to > need > > parent_widget_ itself, I think maybe it just assumes that whatever calls it > > might need to access |parent_widget_| later (probably by calling GetParent()). > > > > WDYT of deleting this override and replacing the getter for parent_widget_ > that > > will instead retrieve it on the fly? We might also have to DCHECK() in > > GetParent() that |view_| is a RootView when we try accessing |parent_widget_| > > (since PopulateChildWidgetVector() only sets |parent_widget_| if that's the > > case). > > I think |parent_widget_| came about because in aura the NVA can teleport between > widgets, and they need to set up WidgetObservers correctly.. I need to look at > this some more. (Maybe dmazzoni has some ideas..) O, ok. I think that's still OK though right? If we get |parent_widget_| on the fly instead of keeping a reference then we don't really need to set up WidgetObservers for it. According to codesearch there is nowhere else in the code that tries to use the getter we've got on NVABase, and if I delete it the only compile error is on PopulateChildWidgetVector(). But yeah, I'll link this thread on asking dmazzoni to look further. https://codereview.chromium.org/2119413004/diff/1110001/ui/accessibility/plat... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2119413004/diff/1110001/ui/accessibility/plat... ui/accessibility/platform/ax_platform_node_mac.mm:292: - (BOOL)accessibilityIsIgnored { On 2017/04/05 06:03:33, tapted wrote: > Should this be updated to include ui::AX_ROLE_IGNORED ? > > (any chance that's a possible shortcut to fixing http://crbug.com/610589 ?) I think this already happens - AX_ROLE_IGNORED is mapped to NSAccessibilityUnknownRole (see BuildRoleMap() above), because there isn't any NSAccessibility*Role equivalent, and we check for NS...UnknownRole here already. The change that actually ignores the ignored elements on Mac is line 299 below, where we check -accessibilityIsIgnored in the hit tests. We wouldn't need it if we used NVABase::HitTestSync() in -accessibilityHitTest since that would exclude ignored elements for us, but I think that might be out of scope for this CL (it might be needed to make hit testing go inside WebViews; currently the WebContents is just one giant green square when hovering over it with Accessibility Inspector on Mac). https://codereview.chromium.org/2119413004/diff/1110001/ui/views/accessibilit... File ui/views/accessibility/native_view_accessibility_base.cc (right): https://codereview.chromium.org/2119413004/diff/1110001/ui/views/accessibilit... ui/views/accessibility/native_view_accessibility_base.cc:64: return false; On 2017/04/04 08:13:13, tapted wrote: > Comment here like > > // If the native accessibility object isn't backed by NativeViewAccessibility > (e.g. when the View wraps a native object), assume it should not be ignored. Done. https://codereview.chromium.org/2119413004/diff/1110001/ui/views/accessibilit... ui/views/accessibility/native_view_accessibility_base.cc:310: NativeViewAccessibilityBase::GetNativeViewAccessibleForWidget() { On 2017/04/05 06:03:33, tapted wrote: > I think this needs an override in NativeViewAccessiblilityWin as well? I'm confused, can you explain why? A call to NVAWin::GetParent() would call NVABase::GetParent(), which looks for the View parent, then calls NVABase::GetNVAccessibleForWidget() to look for the Widget RootView, compared to having previously called NVABase::GetParent() to firstly look for the View parent, then the Widget RootView (which is the same)?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hey Trent, did you want to take a look at this again? It's now just using OS-specific ignoring-roles. As for ChromeOS, I tried ChromeVox on a Chromebook today and there doesn't seem to be a way to descend into things like buttons - any attempt to do so just starts reading out the text of the button character-by-character. So I'm not sure if having this change is important for CrOS, since ChromeVox takes care of it? This seems to be the case for ChromeVox as well as ChromeVox Next. https://codereview.chromium.org/2119413004/diff/1110001/ui/views/accessibilit... File ui/views/accessibility/native_view_accessibility_base.cc (right): https://codereview.chromium.org/2119413004/diff/1110001/ui/views/accessibilit... ui/views/accessibility/native_view_accessibility_base.cc:60: bool NativeViewAccessibilityBase::IsIgnoredView(View* view) { On 2017/04/05 06:03:33, tapted wrote: > Would would happen if we did something like > > (ax_platform_node_mac.mm) > // static > bool AXPlatformNode::IsIgnoredNode(gfx::NativeViewAccessible accessible) { > return [accessible accessibilityIsIgnored]; > } > > (ax_platform_node_win.cc) > bool AXPlatformNode::IsIgnoredNode(gfx::NativeViewAccessible accessible) { > return accessible->get_accState() & STATE_SYSTEM_INVISIBLE; > } > > > (you'd need to update AXPlatformNodeWin::MSAAState() to treat > ui::AX_STATE_INVISIBLE and ui::AX_STATE_IGNORED the same) So the invisible stuff works, but I think since then we've come to the conclusion that just marking ignore elements as invisible (Win) / redundant (Linux) / ignored (Mac) is better? (I.e. not try and ignore the elements in Views' a11y). Let me know if there was a specific reason you wanted Views a11y to know if a views::View is ignored.
This is awesome. lgtm, but dmazzoni should take a peek too Also does it fix http://crbug.com/723574 ? (can add to bug= if so) https://codereview.chromium.org/2119413004/diff/1170001/ui/views/accessibilit... File ui/views/accessibility/native_view_accessibility_base.cc (right): https://codereview.chromium.org/2119413004/diff/1170001/ui/views/accessibilit... ui/views/accessibility/native_view_accessibility_base.cc:18: bool IsAccessibilityFocusableWhenEnabled(View* view) { nit: (if clang format lets you) blank line before https://codereview.chromium.org/2119413004/diff/1170001/ui/views/accessibilit... ui/views/accessibility/native_view_accessibility_base.cc:37: } nit: // namespace
Description was changed from
==========
a11y: Exclude children of nested keyboard accessible controls from a11y tree.
Currently, children of keyboard accessible controls, such as the Labels inside
LabelButtons, are not hidden from the accessibility tree. This is not useful
because the user only cares about the element that has focus, not any children
inside it, which are implementation details. Set any children of keyboard
accessible controls to have ignored roles and exclude them from the
accessibility tree.
BUG=610589
TEST=Open the XCode Accessibility Inspector, hover over the text on a
views::LabelButton (e.g. on the HTTP Authentication dialog). It should report an
empty set of children and its role should be a button.
==========
to
==========
a11y: Exclude children of nested keyboard accessible controls from a11y tree.
Currently, children of keyboard accessible controls, such as the Labels inside
LabelButtons, are not hidden from the accessibility tree. This is not useful
because the user only cares about the element that has focus, not any children
inside it, which are implementation details. Set any children of keyboard
accessible controls to have ignored roles, which indicates to the OS (works for
Windows, Mac, and Linux) that the ignored element should be excluded from the
accessibility tree.
BUG=610589,723574
TEST=On Mac: Open the XCode Accessibility Inspector. On Windows: Open the
Windows SDK 'Inspect.exe' and make sure the icon for 'Watch Focus' is selected.
Then (on Mac, hover over; on Windows, focus) a views::LabelButton (e.g. on the
HTTP Authentication dialog). It should report an empty set of children and its
role ("Type"/"accessibilityRole" on Mac; "Role"/"ControlType" on Windows) should
say it is a button.
==========
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks Trent. Dominic, do you want to take a look now? One thing I forgot to mention in my previous email is that I haven't actually verified this is working on Linux since I've had trouble getting Accerciser to work. I think trying to land this is probably still better than blocking on that, though, so I'll try to see how do get a11y inspection to work later - I've left an email with some contacts Dominic pointed me to offline, so maybe someone else will know. But Windows/Mac are fine (see TEST= in description).
still lgtm Just a note that while this general logic (a child of a focusable control shouldn't be focusable), I can think of a few exceptions that don't currently affect views. On a webpage, for example, the document itself is focusable, as are all of its children. Similarly each iframe is focusable and so are its children - but the iframe isn't always part of the tab order. There are also some composite controls like list boxes where the container (the list box) can be focused, or one of the items in it can be focused. For example if the list box is empty the outer list box itself gets focus. I don't anticipate any problems with this change in Views now but I just think we should be aware that the rule isn't universal.
On 2017/05/25 21:12:43, dmazzoni wrote: > still lgtm > > Just a note that while this general logic (a child of a > focusable control shouldn't be focusable), I can think of > a few exceptions that don't currently affect views. > > On a webpage, for example, the document itself is focusable, > as are all of its children. Similarly each iframe is > focusable and so are its children - but the iframe isn't > always part of the tab order. > > There are also some composite controls like list boxes > where the container (the list box) can be focused, or > one of the items in it can be focused. For example if > the list box is empty the outer list box itself gets > focus. > > I don't anticipate any problems with this change in > Views now but I just think we should be aware that > the rule isn't universal. Thanks for the FYI. Any exceptions to the rule should be caught by IsViewUnfocusableChildOfFocusableAncestor(), so hopefully this is all fine for Views.
The CQ bit was checked by patricialor@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/2119413004/#ps1190001 (title: "Linting & review comments.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1190001, "attempt_start_ts": 1495761411921810,
"parent_rev": "35981e40fcdc003eec5cd83736b8b455082c4439", "commit_rev":
"ce2e11f2565db306765cc6f6412cf5bfb158fb70"}
Message was sent while issue was closed.
Description was changed from
==========
a11y: Exclude children of nested keyboard accessible controls from a11y tree.
Currently, children of keyboard accessible controls, such as the Labels inside
LabelButtons, are not hidden from the accessibility tree. This is not useful
because the user only cares about the element that has focus, not any children
inside it, which are implementation details. Set any children of keyboard
accessible controls to have ignored roles, which indicates to the OS (works for
Windows, Mac, and Linux) that the ignored element should be excluded from the
accessibility tree.
BUG=610589,723574
TEST=On Mac: Open the XCode Accessibility Inspector. On Windows: Open the
Windows SDK 'Inspect.exe' and make sure the icon for 'Watch Focus' is selected.
Then (on Mac, hover over; on Windows, focus) a views::LabelButton (e.g. on the
HTTP Authentication dialog). It should report an empty set of children and its
role ("Type"/"accessibilityRole" on Mac; "Role"/"ControlType" on Windows) should
say it is a button.
==========
to
==========
a11y: Exclude children of nested keyboard accessible controls from a11y tree.
Currently, children of keyboard accessible controls, such as the Labels inside
LabelButtons, are not hidden from the accessibility tree. This is not useful
because the user only cares about the element that has focus, not any children
inside it, which are implementation details. Set any children of keyboard
accessible controls to have ignored roles, which indicates to the OS (works for
Windows, Mac, and Linux) that the ignored element should be excluded from the
accessibility tree.
BUG=610589,723574
TEST=On Mac: Open the XCode Accessibility Inspector. On Windows: Open the
Windows SDK 'Inspect.exe' and make sure the icon for 'Watch Focus' is selected.
Then (on Mac, hover over; on Windows, focus) a views::LabelButton (e.g. on the
HTTP Authentication dialog). It should report an empty set of children and its
role ("Type"/"accessibilityRole" on Mac; "Role"/"ControlType" on Windows) should
say it is a button.
Review-Url: https://codereview.chromium.org/2119413004
Cr-Commit-Position: refs/heads/master@{#474867}
Committed:
https://chromium.googlesource.com/chromium/src/+/ce2e11f2565db306765cc6f6412c...
==========
Message was sent while issue was closed.
Committed patchset #34 (id:1190001) as https://chromium.googlesource.com/chromium/src/+/ce2e11f2565db306765cc6f6412c... |
