Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2563423006:
MacViews/a11y: Ignore invisible accessibility elements. (Closed)
|
Created:
4 years ago by Patti Lor Modified:
4 years ago CC:
chromium-reviews, aboxhall+watch_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org, je_julie, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, mac-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews/a11y: Ignore invisible accessibility elements.
BubbleFrameView can optionally show a close button based on the return value of
WidgetDelegate::ShouldShowCloseButton() and overrides. In cases when it is set
to false, the close button still appears in the accessibility tree on Mac.
Fix this by ignoring invisible accessibility elements for macOS only. This fix
is not cross-platform because the ui::AX_STATE_INVISIBLE flag seems to be useful
for Windows and Android accessibility.
BUG=660230
Committed: https://crrev.com/506491c05a0dd41bc68604c9b34fe56d93705017
Cr-Commit-Position: refs/heads/master@{#438342}
Patch Set 1 #
Total comments: 4
Messages
Total messages: 18 (10 generated)
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.
patricialor@chromium.org changed reviewers: + tapted@chromium.org
Hi Trent, PTAL.
lgtm - I can't think of a better approach, but a11y OWNERS might know what's up :) https://codereview.chromium.org/2563423006/diff/1/ui/accessibility/platform/a... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2563423006/diff/1/ui/accessibility/platform/a... ui/accessibility/platform/ax_platform_node_mac.mm:309: NSAccessibilityChildrenAttribute, There's also NSAccessibilityVisibleChildrenAttribute . browser_accessibility_cocoa.mm uses this. Perhaps by providing it, AppKit will prefer it over AXChildren? But also AXVisibleChildren says "Recommended for elements whose child elements can be occluded or scrolled out of view." so that might not be what we want. There's also AXHidden.. but I think that's "is the app hidden?" and not intended for UI elements :/
patricialor@chromium.org changed reviewers: + dmazzoni@chromium.org
Hi dmazzoni, PTAL. Is there something better that should be done with invisible UI elements? https://codereview.chromium.org/2563423006/diff/1/ui/accessibility/platform/a... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2563423006/diff/1/ui/accessibility/platform/a... ui/accessibility/platform/ax_platform_node_mac.mm:309: NSAccessibilityChildrenAttribute, On 2016/12/13 05:57:51, tapted wrote: > There's also NSAccessibilityVisibleChildrenAttribute . > browser_accessibility_cocoa.mm uses this. Perhaps by providing it, AppKit will > prefer it over AXChildren? But also AXVisibleChildren says "Recommended for > elements whose child elements can be occluded or scrolled out of view." so that > might not be what we want. > > There's also AXHidden.. but I think that's "is the app hidden?" and not intended > for UI elements :/ Yeah, not sure either - thanks for looking into it though, I hadn't known about those attributes.
lgtm This solution seems fine to me. I think it would also be fine just to exclude invisible children at the time we return children for a node. The web is tricky but for Views I can't see a case where we'd ever need to expose an invisible child - it just happens to work fine on Windows now. https://codereview.chromium.org/2563423006/diff/1/ui/accessibility/platform/a... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2563423006/diff/1/ui/accessibility/platform/a... ui/accessibility/platform/ax_platform_node_mac.mm:309: NSAccessibilityChildrenAttribute, On 2016/12/13 06:13:19, Patti Lor wrote: > On 2016/12/13 05:57:51, tapted wrote: > > There's also NSAccessibilityVisibleChildrenAttribute . > > browser_accessibility_cocoa.mm uses this. Perhaps by providing it, AppKit will > > prefer it over AXChildren? But also AXVisibleChildren says "Recommended for > > elements whose child elements can be occluded or scrolled out of view." so > that > > might not be what we want. > > > > There's also AXHidden.. but I think that's "is the app hidden?" and not > intended > > for UI elements :/ > > Yeah, not sure either - thanks for looking into it though, I hadn't known about > those attributes. Yeah, I believe AXVisibleChildren is only used for things like list boxes and tables.
Thanks dmazzoni for the review. It seems there's a 'STATE_SYSTEM_INVISIBLE' flag that is set for invisible elements on Windows, so it might be best to keep the invisible state for other platforms - I checked whether the close button (see the bug attached) is also being exposed to the screen reader (Narrator) on Windows and it doesn't seem to be, so will leave this change as-is. https://codereview.chromium.org/2563423006/diff/1/ui/accessibility/platform/a... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2563423006/diff/1/ui/accessibility/platform/a... ui/accessibility/platform/ax_platform_node_mac.mm:309: NSAccessibilityChildrenAttribute, On 2016/12/13 17:37:01, dmazzoni wrote: > On 2016/12/13 06:13:19, Patti Lor wrote: > > On 2016/12/13 05:57:51, tapted wrote: > > > There's also NSAccessibilityVisibleChildrenAttribute . > > > browser_accessibility_cocoa.mm uses this. Perhaps by providing it, AppKit > will > > > prefer it over AXChildren? But also AXVisibleChildren says "Recommended for > > > elements whose child elements can be occluded or scrolled out of view." so > > that > > > might not be what we want. > > > > > > There's also AXHidden.. but I think that's "is the app hidden?" and not > > intended > > > for UI elements :/ > > > > Yeah, not sure either - thanks for looking into it though, I hadn't known > about > > those attributes. > > Yeah, I believe AXVisibleChildren is only used for things like list boxes and > tables. Ack - thanks for the info, will keep that in mind for future changes.
The CQ bit was checked by patricialor@chromium.org
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": 1, "attempt_start_ts": 1481672412955750, "parent_rev":
"d25f525c557fe87ba649364f8b5ce2dbdb98360a", "commit_rev":
"2b270c086ba28280ee1ccf5278bfdce95c442e3d"}
Message was sent while issue was closed.
Description was changed from ========== MacViews/a11y: Ignore invisible accessibility elements. BubbleFrameView can optionally show a close button based on the return value of WidgetDelegate::ShouldShowCloseButton() and overrides. In cases when it is set to false, the close button still appears in the accessibility tree on Mac. Fix this by ignoring invisible accessibility elements for macOS only. This fix is not cross-platform because the ui::AX_STATE_INVISIBLE flag seems to be useful for Windows and Android accessibility. BUG=660230 ========== to ========== MacViews/a11y: Ignore invisible accessibility elements. BubbleFrameView can optionally show a close button based on the return value of WidgetDelegate::ShouldShowCloseButton() and overrides. In cases when it is set to false, the close button still appears in the accessibility tree on Mac. Fix this by ignoring invisible accessibility elements for macOS only. This fix is not cross-platform because the ui::AX_STATE_INVISIBLE flag seems to be useful for Windows and Android accessibility. BUG=660230 Review-Url: https://codereview.chromium.org/2563423006 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== MacViews/a11y: Ignore invisible accessibility elements. BubbleFrameView can optionally show a close button based on the return value of WidgetDelegate::ShouldShowCloseButton() and overrides. In cases when it is set to false, the close button still appears in the accessibility tree on Mac. Fix this by ignoring invisible accessibility elements for macOS only. This fix is not cross-platform because the ui::AX_STATE_INVISIBLE flag seems to be useful for Windows and Android accessibility. BUG=660230 Review-Url: https://codereview.chromium.org/2563423006 ========== to ========== MacViews/a11y: Ignore invisible accessibility elements. BubbleFrameView can optionally show a close button based on the return value of WidgetDelegate::ShouldShowCloseButton() and overrides. In cases when it is set to false, the close button still appears in the accessibility tree on Mac. Fix this by ignoring invisible accessibility elements for macOS only. This fix is not cross-platform because the ui::AX_STATE_INVISIBLE flag seems to be useful for Windows and Android accessibility. BUG=660230 Committed: https://crrev.com/506491c05a0dd41bc68604c9b34fe56d93705017 Cr-Commit-Position: refs/heads/master@{#438342} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/506491c05a0dd41bc68604c9b34fe56d93705017 Cr-Commit-Position: refs/heads/master@{#438342} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
