Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2210763002:
MacViews a11y: Sync VoiceOver cursor with keyboard focus. (Closed)
|
Created:
4 years, 4 months ago by Patti Lor Modified:
4 years, 2 months ago CC:
aboxhall+watch_chromium.org, chrome-apps-syd-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, jam, je_julie, nektar+watch_chromium.org, yuzo+watch_chromium.org, Elly Fong-Jones Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews a11y: Sync VoiceOver cursor with keyboard focus.
Move the VoiceOver cursor when changing the keyboard focus if "VoiceOver cursor
follows keyboard focus" is turned on in VoiceOver Utility (System Preferences >
Accessibility > VoiceOver > Open VoiceOver Utility) with MacViews.
BUG=610585, 650118
TEST=With the #mac-views-native-dialogs flag turned on via chrome:flags, turn on
VoiceOver with Cmd+F5. Click the bookmark star on the right of the omnibox and
press tab to change focus inside it. The VoiceOver cursor should follow keyboard
focus.
Committed: https://crrev.com/f8686b58fdf56513efdf5ed2826018560d26bbe2
Cr-Commit-Position: refs/heads/master@{#423083}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Add accessibilityFocusedUIElement to BrowserCrApplication. #Patch Set 3 : Move accessibilityFocusedUIElement from BrowserCrApplication to BridgedContentView. #
Total comments: 22
Patch Set 4 : Review comments, delete accessibilityShouldUseUniqueId. #
Total comments: 9
Patch Set 5 : Delete view.* changes, other review comments. #
Total comments: 6
Patch Set 6 : Nits. #
Messages
Total messages: 59 (39 generated)
Description was changed from ========== MacViews a11y (WIP): Sync VoiceOver cursor with keyboard focus. Move the VoiceOver cursor when changing the keyboard focus if "VoiceOver cursor follows keyboard focus" is turned on in VoiceOver Utility (System Preferences > Accessibility > VoiceOver > Open VoiceOver Utility) with mac_views_browser=1. BUG=610585 TEST=On a Mac, turn on VoiceOver with Cmd+F5. Focus on the omnibox and press tab to move focus to the bookmark star and the hamburger menu. VoiceOver cursor should follow. ========== to ========== MacViews a11y (WIP): Sync VoiceOver cursor with keyboard focus. This is a WIP! Move the VoiceOver cursor when changing the keyboard focus if "VoiceOver cursor follows keyboard focus" is turned on in VoiceOver Utility (System Preferences > Accessibility > VoiceOver > Open VoiceOver Utility) with mac_views_browser=1. BUG=610585 TEST=On a Mac (and Chrome built with mac_views_browser=1), turn on VoiceOver with Cmd+F5. Focus on the omnibox and press tab to move focus to the bookmark star and the hamburger menu. VoiceOver cursor should follow. ==========
patricialor@chromium.org changed reviewers: + dtseng@chromium.org, tapted@chromium.org
Hi dtseng@, sending this to you to have a look because of https://codereview.chromium.org/7461104, which looks like it implemented the same feature for elements inside the WebContents - feel free to redirect this to someone else or remove yourself if you don't think you're the right person to ask. The CL as it is now doesn't work - pressing tab with VoiceOver on moves the keyboard focus but VoiceOver doesn't react at all. See https://www.diffchecker.com/czqnp5ke for a diff of what happens when the notification is posted for elements inside the WebContents compared to the notification posting for MacViews. (I've also left the tracing code I used in this patchset so please ignore those parts of the code.) https://codereview.chromium.org/2210763002/diff/1/content/browser/accessibili... File content/browser/accessibility/browser_accessibility_manager_mac.mm (right): https://codereview.chromium.org/2210763002/diff/1/content/browser/accessibili... content/browser/accessibility/browser_accessibility_manager_mac.mm:194: return; With tracing removed, this hunk is deleted. https://codereview.chromium.org/2210763002/diff/1/tmp-traceclass.h File tmp-traceclass.h (right): https://codereview.chromium.org/2210763002/diff/1/tmp-traceclass.h#newcode1 tmp-traceclass.h:1: #import <Foundation/Foundation.h> Temporary class for Objective C tracing. https://codereview.chromium.org/2210763002/diff/1/ui/accessibility/platform/a... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2210763002/diff/1/ui/accessibility/platform/a... ui/accessibility/platform/ax_platform_node_mac.mm:558: return; With tracing removed, this whole hunk shouldn't be here.
dmazzoni@chromium.org changed reviewers: + dmazzoni@chromium.org
I think I have an idea of why it's not working. VoiceOver doesn't really "trust" focus event notifications, so when it gets one, it starts at the top of the window and calls accessibilityFocusedUIElement on it, then checks if whatever that returns is different from what had focus before. If so, it announces it. Check the implementation of accessibilityFocusedUIElement in render_widget_host_view_mac.mm and see if you can implement something similar in the top-level mac views window.
On 2016/08/04 02:06:26, Patti Lor wrote: > Hi dtseng@, sending this to you to have a look because of > https://codereview.chromium.org/7461104, which looks like it implemented the > same feature for elements inside the WebContents - feel free to redirect this to > someone else or remove yourself if you don't think you're the right person to > ask. The CL as it is now doesn't work - pressing tab with VoiceOver on moves the > keyboard focus but VoiceOver doesn't react at all. > > See https://www.diffchecker.com/czqnp5ke for a diff of what happens when the > notification is posted for elements inside the WebContents compared to the > notification posting for MacViews. (I've also left the tracing code I used in > this patchset so please ignore those parts of the code.) > Happy to help if I can. IIRC, the trick to getting this to work was implementing - (id)accessibilityFocusedUIElement on a containing view. In the case of web, that was RenderWidgetHostViewMac. For native Mac Views, I would guess you need it for the top level Chrome view. I would check that's returning the appropriate thing first and go from there. I would also take a look at content/browser/accessibility/accessibility_event_recorder_mac.mm for how to programmatically view the event stream from VoiceOver's perspective and also how it introspects the Chrome accessibility tree for the focused ui element. HTH.
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...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) 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 #2 (id:20001) has been deleted
Patchset #2 (id:40001) 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: This issue passed the CQ dry run.
Thanks dmazzoni@ & dtseng@! Your suggestions worked. It unfortunately breaks the navigation for BrowserAccessibilityCocoa a little bit (works if you start VoiceOver while focused on something inside the WebContents, but not if you try to tab from a view in the top chrome to the WebContents). It also breaks this for Cocoa (in the top chrome, not WebContents), so will investigate a bit more. Do either of you have any ideas? :)
> > IMHO Usage of tab is low among voiceover users.Did you also test with VO > nav keys? > Ctrl-opt-left and right? Also try interacting with ctrl-opt-shft-down. > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Though usage of tab is lower than on other platforms, it's still very important to get focus right because VoiceOver sync's focus as you navigate via VoiceOver keys. Wrt other things to try to get tab to work correctly, did you look at the responder chainof your views? On Tue, Aug 9, 2016 at 9:25 AM, 'Nektarios Paisios' via Chromium-reviews < chromium-reviews@chromium.org> wrote: > IMHO Usage of tab is low among voiceover users.Did you also test with VO >> nav keys? >> Ctrl-opt-left and right? Also try interacting with ctrl-opt-shft-down. >> > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to chromium-reviews+unsubscribe@chromium.org. > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/08/09 at 08:08:28, patricialor wrote: > Thanks dmazzoni@ & dtseng@! Your suggestions worked. It unfortunately breaks the navigation for BrowserAccessibilityCocoa a little bit (works if you start VoiceOver while focused on something inside the WebContents, but not if you try to tab from a view in the top chrome to the WebContents). It also breaks this for Cocoa (in the top chrome, not WebContents), so will investigate a bit more. Do either of you have any ideas? :) It sounds like an interaction between the views and non-views code. What does "views" think is focused when focus is in the web content?
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 #3 (id:80001) has been deleted
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.
Description was changed from ========== MacViews a11y (WIP): Sync VoiceOver cursor with keyboard focus. This is a WIP! Move the VoiceOver cursor when changing the keyboard focus if "VoiceOver cursor follows keyboard focus" is turned on in VoiceOver Utility (System Preferences > Accessibility > VoiceOver > Open VoiceOver Utility) with mac_views_browser=1. BUG=610585 TEST=On a Mac (and Chrome built with mac_views_browser=1), turn on VoiceOver with Cmd+F5. Focus on the omnibox and press tab to move focus to the bookmark star and the hamburger menu. VoiceOver cursor should follow. ========== to ========== MacViews a11y: Sync VoiceOver cursor with keyboard focus. Move the VoiceOver cursor when changing the keyboard focus if "VoiceOver cursor follows keyboard focus" is turned on in VoiceOver Utility (System Preferences > Accessibility > VoiceOver > Open VoiceOver Utility) with mac_views_browser=1. BUG=610585, 650118 TEST=On a Mac (and Chrome built with mac_views_browser=1), turn on VoiceOver with Cmd+F5. Focus on the omnibox and press tab to move focus to the bookmark star and the hamburger menu. VoiceOver cursor should follow. ==========
Hi all, thanks for all your help! So I've decided just to try and get this landed (have taken the WIP labels out of the CL title and description), so PTAL - it's not perfect (see below for explanation), but this will be an improvement to the existing behaviour and it's been a bit of a time sink for me, so I'd like to move on. The only remaining issue now is that the rectangle representing the VO cursor will not be drawn in the correct position when moving from the TopChrome to the WebContents area. Instead, it will be stuck on the last keyboard-focused View (i.e., the black rectangle that represents the VO cursor will still be drawn around the last keyboard-focused View even when keyboard focus has moved to the WebContents). Note that this isn't the actual VO focus that is stuck, but is only the rectangle that is in the wrong place, so VO will still read out things that have focus inside the WebContents. As far as I can tell, this seems to affect things from a visual perspective. I've created a bug for this here: https://bugs.chromium.org/p/chromium/issues/detail?id=650118; maybe in the future I / someone else can revisit it. Hopefully this is OK with everyone! Thanks again :) @nektar: Yep, VO nav keys are working fine. @dtseng: Yes, I checked it out - it did help a bit in that now Cocoa is unbroken again (I moved some code to BridgedContentView). Thanks for the advice! @dmazzoni: Views thinks 'WebView' is focused (according to the FocusManager) when tabbing around inside the web content with VO on.
lgtm Thanks for the thorough investigation! There are other cases where VoiceOver's visual highlight isn't correct temporarily (but then resolves itself if you keep navigating). I've been working on a possible fix for a different case, so that might be fine. Everything in this change looks fine.
mostly nits - we'll have to check with an OWNER about the views.h change. I agree this is a great step forward, fixing the WebContents integration later is the right approach. https://codereview.chromium.org/2210763002/diff/100001/base/mac/sdk_forward_d... File base/mac/sdk_forward_declarations.h (right): https://codereview.chromium.org/2210763002/diff/100001/base/mac/sdk_forward_d... base/mac/sdk_forward_declarations.h:240: void NSAccessibilityUnregisterUniqueIdForUIElement(id element); sdk_forward_declarations.h isn't really the right place for exposing private API declarations - we want to make it really hard to use SPIs unintentionally. It feels like heresey, but I think the right way to do this is to leave the declaration in browser_accessibility_cocoa.mm and just copy+paste it into bridged_content_view.mm as well (there, you can say "Also used in BrowserAccessibilityCocoa.") https://codereview.chromium.org/2210763002/diff/100001/chrome/browser/chrome_... File chrome/browser/chrome_browser_application_mac.mm (right): https://codereview.chromium.org/2210763002/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_browser_application_mac.mm:29: #import "ui/accessibility/platform/ax_platform_node_mac.h" nit: we can revert this file back to how it was so it drops off the diff https://codereview.chromium.org/2210763002/diff/100001/content/browser/access... File content/browser/accessibility/browser_accessibility_cocoa.mm (right): https://codereview.chromium.org/2210763002/diff/100001/content/browser/access... content/browser/accessibility/browser_accessibility_cocoa.mm:2856: return YES; Comment here why this is done? Reading the comment on NSAccessibility.h suggests to me that this is the right thing to do - but does it solve a specific problem? (and it look slike OSX provided accessibilityNotifiesWhenDestroyed in the 10.9 SDK to support a particular use case - now that we use it, is there a possibility of conflict with the accessibilityShouldUseUniqueId stuff?) https://codereview.chromium.org/2210763002/diff/100001/content/browser/access... File content/browser/accessibility/browser_accessibility_manager_mac.mm (right): https://codereview.chromium.org/2210763002/diff/100001/content/browser/access... content/browser/accessibility/browser_accessibility_manager_mac.mm:7: #include "base/logging.h" this is a good change, but can probably be dropped off the diff too so it's a bit more focused. https://codereview.chromium.org/2210763002/diff/100001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2210763002/diff/100001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:215: void NotifyMacEvent(id target, ui::AXEvent event_type) { can this instead change from NSView* -> AXPlatformNodeCocoa*, rather than id? https://codereview.chromium.org/2210763002/diff/100001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:297: - (BOOL)accessibilityShouldUseUniqueId { This needs a note that it was never part of the informal protocol definition, but is required for <reason>. It's OK to repeat stuff from browser_accessibility_cocoa.mm https://codereview.chromium.org/2210763002/diff/100001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:533: if (!native_node_) nit: This extra check isn't needed - I'd go for just GetNativeViewAccessible(); or AXPlatformNodeCocoa* node = GetNativeViewAccessible(); https://codereview.chromium.org/2210763002/diff/100001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2210763002/diff/100001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:28: #import "ui/accessibility/platform/ax_platform_node_mac.h" nit: was this needed in the end? https://codereview.chromium.org/2210763002/diff/100001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:1377: return nil; accessibilityFocusedUIElement says the receiver can assume that it's already focused. widget->GetRootView() should be equivalent to |hostedView_| in that case. But a11y requests can come in at odd times. I think this method can do if (!hostedView_) return nil; return hostedView_->GetNativeViewAccessibility()->GetFocus() This ensures that we don't accidentally return a node in another window. https://codereview.chromium.org/2210763002/diff/100001/ui/views/view.h File ui/views/view.h (right): https://codereview.chromium.org/2210763002/diff/100001/ui/views/view.h#newcod... ui/views/view.h:950: NativeViewAccessibility* GetNativeViewAccessibility(); I'm unsure about this - we'll have to see what views OWNERS say. E.g. it's a bit confusing having bith GetNativeViewAccessibility() and GetNativeViewAccessible(). An alternative could be something like AXPlatformNodeCocoa root = hostedView_->GetNativeViewAccessible(); AXPlatformNodeDelegate* delegate = [root getDelegate]; // Needs an implementation of getDelegate. return delegate->GetFocus();
CCing ellyjones@ who works on Views accessibility for the 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: This issue passed the CQ dry run.
Thanks for the reviews, PTAL! https://codereview.chromium.org/2210763002/diff/100001/base/mac/sdk_forward_d... File base/mac/sdk_forward_declarations.h (right): https://codereview.chromium.org/2210763002/diff/100001/base/mac/sdk_forward_d... base/mac/sdk_forward_declarations.h:240: void NSAccessibilityUnregisterUniqueIdForUIElement(id element); On 2016/09/26 05:19:17, tapted wrote: > sdk_forward_declarations.h isn't really the right place for exposing private API > declarations - we want to make it really hard to use SPIs unintentionally. It > feels like heresey, but I think the right way to do this is to leave the > declaration in browser_accessibility_cocoa.mm and just copy+paste it into > bridged_content_view.mm as well (there, you can say "Also used in > BrowserAccessibilityCocoa.") See below comment, deleted this in both places. https://codereview.chromium.org/2210763002/diff/100001/chrome/browser/chrome_... File chrome/browser/chrome_browser_application_mac.mm (right): https://codereview.chromium.org/2210763002/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_browser_application_mac.mm:29: #import "ui/accessibility/platform/ax_platform_node_mac.h" On 2016/09/26 05:19:17, tapted wrote: > nit: we can revert this file back to how it was so it drops off the diff Done. https://codereview.chromium.org/2210763002/diff/100001/content/browser/access... File content/browser/accessibility/browser_accessibility_cocoa.mm (right): https://codereview.chromium.org/2210763002/diff/100001/content/browser/access... content/browser/accessibility/browser_accessibility_cocoa.mm:2856: return YES; On 2016/09/26 05:19:17, tapted wrote: > Comment here why this is done? Reading the comment on NSAccessibility.h suggests > to me that this is the right thing to do - but does it solve a specific problem? > (and it look slike OSX provided accessibilityNotifiesWhenDestroyed in the 10.9 > SDK to support a particular use case - now that we use it, is there a > possibility of conflict with the accessibilityShouldUseUniqueId stuff?) Ah - I think this might actually not be needed (at least, not while accessibilityShouldUseUniqueId is also there). I just double checked this (for Views) and having either or both accessibilityShouldUseUniqueId and accessibilityNotifiesWhenDestroyed make VO faster than without, so probably only one is needed. Inside the WebContents though I don't actually notice any difference in speed when deleting these, only in the Views parts, which is kind of strange - have no idea why it affects only Views. Since the former uses a private thing, I went ahead and deleted that in favour of accessibilityNotifiesWhenDestroyed (so am kind of guessing/assuming they do the same thing), but let me know if you want to just keep it as is here and use accessibilityNotifiesWhenDestroyed in AXPlatformNodeCocoa. I don't think they actually conflict though. (To be clear, not having either will make VO really slow in Views only - it will take at least a couple of seconds to catch up when keyboard focus is moved.) https://codereview.chromium.org/2210763002/diff/100001/content/browser/access... File content/browser/accessibility/browser_accessibility_manager_mac.mm (right): https://codereview.chromium.org/2210763002/diff/100001/content/browser/access... content/browser/accessibility/browser_accessibility_manager_mac.mm:7: #include "base/logging.h" On 2016/09/26 05:19:17, tapted wrote: > this is a good change, but can probably be dropped off the diff too so it's a > bit more focused. Done. https://codereview.chromium.org/2210763002/diff/100001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2210763002/diff/100001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:215: void NotifyMacEvent(id target, ui::AXEvent event_type) { On 2016/09/26 05:19:17, tapted wrote: > can this instead change from NSView* -> AXPlatformNodeCocoa*, rather than id? Done. https://codereview.chromium.org/2210763002/diff/100001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:297: - (BOOL)accessibilityShouldUseUniqueId { On 2016/09/26 05:19:17, tapted wrote: > This needs a note that it was never part of the informal protocol definition, > but is required for <reason>. It's OK to repeat stuff from > browser_accessibility_cocoa.mm See above comment - deleted this method entirely. https://codereview.chromium.org/2210763002/diff/100001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:533: if (!native_node_) On 2016/09/26 05:19:17, tapted wrote: > nit: This extra check isn't needed - I'd go for just > > GetNativeViewAccessible(); > > or > > AXPlatformNodeCocoa* node = GetNativeViewAccessible(); Done. https://codereview.chromium.org/2210763002/diff/100001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2210763002/diff/100001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:28: #import "ui/accessibility/platform/ax_platform_node_mac.h" On 2016/09/26 05:19:18, tapted wrote: > nit: was this needed in the end? Nope, have deleted it now - thanks. https://codereview.chromium.org/2210763002/diff/100001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:1377: return nil; On 2016/09/26 05:19:17, tapted wrote: > accessibilityFocusedUIElement says the receiver can assume that it's already > focused. widget->GetRootView() should be equivalent to |hostedView_| in that > case. But a11y requests can come in at odd times. I think this method can do > > if (!hostedView_) > return nil; > return hostedView_->GetNativeViewAccessibility()->GetFocus() > > This ensures that we don't accidentally return a node in another window. Done. https://codereview.chromium.org/2210763002/diff/100001/ui/views/view.h File ui/views/view.h (right): https://codereview.chromium.org/2210763002/diff/100001/ui/views/view.h#newcod... ui/views/view.h:950: NativeViewAccessibility* GetNativeViewAccessibility(); On 2016/09/26 05:19:18, tapted wrote: > I'm unsure about this - we'll have to see what views OWNERS say. E.g. it's a bit > confusing having bith GetNativeViewAccessibility() and > GetNativeViewAccessible(). An alternative could be something like > > AXPlatformNodeCocoa root = hostedView_->GetNativeViewAccessible(); > AXPlatformNodeDelegate* delegate = [root getDelegate]; // Needs an > implementation of getDelegate. > return delegate->GetFocus(); > I like your alternative, but I think it needs to be cross-platform - I use this a lot in some of my other CLs (e.g. ignoring accessibility elements, see https://codereview.chromium.org/2119413004/diff/100001/ui/views/accessibility... if you want context). Is it gross to have NativeViewAccessibility keep a static reference to all the instances? Or maybe just change the previously mentioned CL etc to work for Mac-only?
https://codereview.chromium.org/2210763002/diff/120001/content/browser/access... File content/browser/accessibility/browser_accessibility_cocoa.mm (right): https://codereview.chromium.org/2210763002/diff/120001/content/browser/access... content/browser/accessibility/browser_accessibility_cocoa.mm:15: #import "base/mac/sdk_forward_declarations.h" nit: remove? https://codereview.chromium.org/2210763002/diff/120001/content/browser/access... content/browser/accessibility/browser_accessibility_cocoa.mm:2884: // things more efficiently. Yeah it would be awesome if we can drop the private API use! But I'm a little wary of possible regressions.. accessibilityShouldUseUniqueId wasn't exposed for efficiency but because of tricky lifetime problems, and issues when this was changed from inheriting from an NSView to inheriting from just an NSObject. We'll need to go back to original bug to make sure nothing has regressed. https://codereview.chromium.org/2210763002/diff/120001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2210763002/diff/120001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:10: #import "base/mac/sdk_forward_declarations.h" nit: remove? https://codereview.chromium.org/2210763002/diff/120001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2210763002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:1369: return hostedView_->GetNativeViewAccessibility()->GetFocus(); What about `return [hostedView_->GetNativeViewAccessibility() accessibilityFocusedUIElement];` and implementing - (id)accessibilityFocusedUIElement; in ax_platform_node_mac.mm -- I think one of the objectives is that both BridgedNativeWidget and AXPlatformNodeCocoa have a fully-fleshed-out implementation of the NSAccessibility protocol. I think one problem is that NativeViewHostMac::GetNativeViewAccessible() probably won't actually have a views::NativeViewAccessibility to return. In fact, GetNativeViewAccessible() currently returns NULL -- it can probably return |native_view_|, on the assumption that the hosted view (probably a RenderWidgetHostViewCocoa) implements the NSAccessiblity protocol. Which it does, so the above suggestion should allow BridgedContentView to invoke the accessibilityFocusedUIElement in render_widget_host_view_mac.mm (maybe this is even a clue to the outstanding TODO?)
https://codereview.chromium.org/2210763002/diff/100001/ui/views/view.h File ui/views/view.h (right): https://codereview.chromium.org/2210763002/diff/100001/ui/views/view.h#newcod... ui/views/view.h:950: NativeViewAccessibility* GetNativeViewAccessibility(); On 2016/09/28 00:29:14, Patti Lor wrote: > On 2016/09/26 05:19:18, tapted wrote: > > I'm unsure about this - we'll have to see what views OWNERS say. E.g. it's a > bit > > confusing having bith GetNativeViewAccessibility() and > > GetNativeViewAccessible(). An alternative could be something like > > > > AXPlatformNodeCocoa root = hostedView_->GetNativeViewAccessible(); > > AXPlatformNodeDelegate* delegate = [root getDelegate]; // Needs an > > implementation of getDelegate. > > return delegate->GetFocus(); > > > > I like your alternative, but I think it needs to be cross-platform - I use this > a lot in some of my other CLs (e.g. ignoring accessibility elements, see > https://codereview.chromium.org/2119413004/diff/100001/ui/views/accessibility... > if you want context). Is it gross to have NativeViewAccessibility keep a static > reference to all the instances? Or maybe just change the previously mentioned CL > etc to work for Mac-only? For context, the reason I didn't add GetNativeViewAccessibility() before was that GetNativeViewAccessible() doesn't always return something based on NativeViewAccessibility. In particular, WebView and NativeViewHost both return something different. So my first inclination is to try to keep that design. See other comment. https://codereview.chromium.org/2210763002/diff/120001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2210763002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:1369: return hostedView_->GetNativeViewAccessibility()->GetFocus(); On 2016/09/28 01:03:20, tapted wrote: > What about > > `return [hostedView_->GetNativeViewAccessibility() > accessibilityFocusedUIElement];` > > and implementing - (id)accessibilityFocusedUIElement; in ax_platform_node_mac.mm > -- I think one of the objectives is that both BridgedNativeWidget and > AXPlatformNodeCocoa have a fully-fleshed-out implementation of the > NSAccessibility protocol. > > I think one problem is that > > NativeViewHostMac::GetNativeViewAccessible() > > probably won't actually have a views::NativeViewAccessibility to return. > > In fact, GetNativeViewAccessible() currently returns NULL -- it can probably > return |native_view_|, on the assumption that the hosted view (probably a > RenderWidgetHostViewCocoa) implements the NSAccessiblity protocol. Which it > does, so the above suggestion should allow BridgedContentView to invoke the > accessibilityFocusedUIElement in render_widget_host_view_mac.mm > > (maybe this is even a clue to the outstanding TODO?) Another alternative, this is the preferred method that I had in mind: First get hostedView_->GetNativeViewAccessible(). Then use AXPlatformNode::FromNativeViewAccessible, which will succeed if the NSAccessibility is backed by an AXPlatformNode. Then AXPlatformNode::delegate()->GetFocus(), which is implemented by NativeViewAccessibility in this specific case, but you don't have to assume that. Let me know if that will work. Right now this might seem slightly roundabout because every AXPlatformNode is backed by a NativeViewAccessibility, so why not just cut out the middleman? My eventual goal is that the web accessibility classes will eventually inherit from AXPlatformNodeDelegate, so 99% of the accessible objects will all be implemented by AXPlatformNode, but some will be views and some will be web.
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.
PTAL, thanks. https://codereview.chromium.org/2210763002/diff/100001/ui/views/view.h File ui/views/view.h (right): https://codereview.chromium.org/2210763002/diff/100001/ui/views/view.h#newcod... ui/views/view.h:950: NativeViewAccessibility* GetNativeViewAccessibility(); On 2016/09/29 16:06:15, dmazzoni wrote: > On 2016/09/28 00:29:14, Patti Lor wrote: > > On 2016/09/26 05:19:18, tapted wrote: > > > I'm unsure about this - we'll have to see what views OWNERS say. E.g. it's a > > bit > > > confusing having bith GetNativeViewAccessibility() and > > > GetNativeViewAccessible(). An alternative could be something like > > > > > > AXPlatformNodeCocoa root = hostedView_->GetNativeViewAccessible(); > > > AXPlatformNodeDelegate* delegate = [root getDelegate]; // Needs an > > > implementation of getDelegate. > > > return delegate->GetFocus(); > > > > > > > I like your alternative, but I think it needs to be cross-platform - I use > this > > a lot in some of my other CLs (e.g. ignoring accessibility elements, see > > > https://codereview.chromium.org/2119413004/diff/100001/ui/views/accessibility... > > if you want context). Is it gross to have NativeViewAccessibility keep a > static > > reference to all the instances? Or maybe just change the previously mentioned > CL > > etc to work for Mac-only? > > For context, the reason I didn't add GetNativeViewAccessibility() before was > that GetNativeViewAccessible() doesn't always return something based on > NativeViewAccessibility. In particular, WebView and NativeViewHost both > return something different. > > So my first inclination is to try to keep that design. > > See other comment. Ok, thanks for the context. I've deleted the changes to view.* for this CL since they're no longer needed, and will investigate alternatives for use in my other accessibility CLs. https://codereview.chromium.org/2210763002/diff/120001/content/browser/access... File content/browser/accessibility/browser_accessibility_cocoa.mm (right): https://codereview.chromium.org/2210763002/diff/120001/content/browser/access... content/browser/accessibility/browser_accessibility_cocoa.mm:15: #import "base/mac/sdk_forward_declarations.h" On 2016/09/28 01:03:19, tapted wrote: > nit: remove? Done. https://codereview.chromium.org/2210763002/diff/120001/content/browser/access... content/browser/accessibility/browser_accessibility_cocoa.mm:2884: // things more efficiently. On 2016/09/28 01:03:19, tapted wrote: > Yeah it would be awesome if we can drop the private API use! But I'm a little > wary of possible regressions.. accessibilityShouldUseUniqueId wasn't exposed for > efficiency but because of tricky lifetime problems, and issues when this was > changed from inheriting from an NSView to inheriting from just an NSObject. > We'll need to go back to original bug to make sure nothing has regressed. The decision to delete the private API stuff here was mostly done because of the information on the Apple radar filed (http://openradar.appspot.com/9896491), which said that VO wouldn't acknowledge not-NSView/NSWindow things as accessibility elements. It's working now, so I thought that probably isn't needed anymore. If the lifetime bug you are talking about is this: "This will hopefully set us up to solve the problem of VoiceOver not acknowledging page transitions. The remaining issue is that RWHV's get destroyed on page transitions, thereby destroying our BrowserAccessibilityManager. This is likely why VoiceOver won't honor our AXLoadComplete notification." (stolen from the CL description https://chromium.googlesource.com/chromium/src/+/cbd4ff785885f593c2ce45346ca9..., also see https://www.crbug.com/55658), then I think this is fine as well. VO re-reads the name of the HTML page when I navigate to another page with this CL patched in, and makes bloopy noises when I click/press enter on a link and again when the link loads. Adding log messages to ~BrowserAccessibilityManagerMac doesn't seem to trigger per page transition, either. https://codereview.chromium.org/2210763002/diff/120001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2210763002/diff/120001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:10: #import "base/mac/sdk_forward_declarations.h" On 2016/09/28 01:03:20, tapted wrote: > nit: remove? Done, thanks for pointing them all out >< https://codereview.chromium.org/2210763002/diff/120001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2210763002/diff/120001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:1369: return hostedView_->GetNativeViewAccessibility()->GetFocus(); On 2016/09/29 16:06:15, dmazzoni wrote: > On 2016/09/28 01:03:20, tapted wrote: > > What about > > > > `return [hostedView_->GetNativeViewAccessibility() > > accessibilityFocusedUIElement];` > > > > and implementing - (id)accessibilityFocusedUIElement; in > ax_platform_node_mac.mm > > -- I think one of the objectives is that both BridgedNativeWidget and > > AXPlatformNodeCocoa have a fully-fleshed-out implementation of the > > NSAccessibility protocol. > > > > I think one problem is that > > > > NativeViewHostMac::GetNativeViewAccessible() > > > > probably won't actually have a views::NativeViewAccessibility to return. > > > > In fact, GetNativeViewAccessible() currently returns NULL -- it can probably > > return |native_view_|, on the assumption that the hosted view (probably a > > RenderWidgetHostViewCocoa) implements the NSAccessiblity protocol. Which it > > does, so the above suggestion should allow BridgedContentView to invoke the > > accessibilityFocusedUIElement in render_widget_host_view_mac.mm > > > > (maybe this is even a clue to the outstanding TODO?) > > Another alternative, this is the preferred method that I had in mind: > > First get hostedView_->GetNativeViewAccessible(). > > Then use AXPlatformNode::FromNativeViewAccessible, which > will succeed if the NSAccessibility is backed by an AXPlatformNode. > > Then AXPlatformNode::delegate()->GetFocus(), which is implemented > by NativeViewAccessibility in this specific case, but you don't have to > assume that. > > Let me know if that will work. > > Right now this might seem slightly roundabout because every > AXPlatformNode is backed by a NativeViewAccessibility, so why not > just cut out the middleman? My eventual goal is that the web accessibility > classes will eventually inherit from AXPlatformNodeDelegate, so 99% of > the accessible objects will all be implemented by AXPlatformNode, but > some will be views and some will be web. dmazzoni@: I don't think that will work - as far as I can tell, the definition for non-Windows calls to FromNativeViewAccessible() just returns a nullptr (see https://cs.chromium.org/chromium/src/ui/accessibility/platform/ax_platform_no...). I went ahead and tried anyway just to double check, and it seems there's a couple of other things required too, like making FromNativeViewAccessible public (it's protected) and fixing a linker error for ui::AXPlatformNode::FromNativeViewAccessible not being found. That's as far as I got, because I wasn't sure if these were also intended consequences of your suggestion - let me know. tapted@: Have gone with your change for patchset 4, but it doesn't seem to make a difference with drawing the cursor in the right place. The NativeViewHostMac |native_view_| member variable always seems to be null, including when traversing focus inside the web content area.
lgtm % nits Also, since this fixes http://crbug.com/610585, we can form the TEST= line in terms of something to do on Canary rather than requiring a mac_views_browser=1 build - this way if QA come along they will have better luck verifying it. https://codereview.chromium.org/2210763002/diff/140001/content/browser/access... File content/browser/accessibility/browser_accessibility_cocoa.mm (right): https://codereview.chromium.org/2210763002/diff/140001/content/browser/access... content/browser/accessibility/browser_accessibility_cocoa.mm:2882: // destroyed (see - detach). This allows VoiceOver to do some internal nit: -detach (no space) https://codereview.chromium.org/2210763002/diff/140001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2210763002/diff/140001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:28: #include "ui/views/accessibility/native_view_accessibility.h" nit: this might not be required now https://codereview.chromium.org/2210763002/diff/140001/ui/views/controls/nati... File ui/views/controls/native/native_view_host_mac.mm (right): https://codereview.chromium.org/2210763002/diff/140001/ui/views/controls/nati... ui/views/controls/native/native_view_host_mac.mm:142: return native_view_; let's leave this off for now - there should be a nice way to test for it without requiring VoiceOver (also it needs different OWNERS :p)
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: Sync VoiceOver cursor with keyboard focus. Move the VoiceOver cursor when changing the keyboard focus if "VoiceOver cursor follows keyboard focus" is turned on in VoiceOver Utility (System Preferences > Accessibility > VoiceOver > Open VoiceOver Utility) with mac_views_browser=1. BUG=610585, 650118 TEST=On a Mac (and Chrome built with mac_views_browser=1), turn on VoiceOver with Cmd+F5. Focus on the omnibox and press tab to move focus to the bookmark star and the hamburger menu. VoiceOver cursor should follow. ========== to ========== MacViews a11y: Sync VoiceOver cursor with keyboard focus. Move the VoiceOver cursor when changing the keyboard focus if "VoiceOver cursor follows keyboard focus" is turned on in VoiceOver Utility (System Preferences > Accessibility > VoiceOver > Open VoiceOver Utility) with MacViews. BUG=610585, 650118 TEST=With the #mac-views-native-dialogs flag turned on via chrome:flags, turn on VoiceOver with Cmd+F5. Click the bookmark star on the right of the omnibox and press tab to change focus inside it. The VoiceOver cursor should follow keyboard focus. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2210763002/diff/140001/content/browser/access... File content/browser/accessibility/browser_accessibility_cocoa.mm (right): https://codereview.chromium.org/2210763002/diff/140001/content/browser/access... content/browser/accessibility/browser_accessibility_cocoa.mm:2882: // destroyed (see - detach). This allows VoiceOver to do some internal On 2016/10/04 08:17:39, tapted wrote: > nit: -detach (no space) Done. https://codereview.chromium.org/2210763002/diff/140001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2210763002/diff/140001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:28: #include "ui/views/accessibility/native_view_accessibility.h" On 2016/10/04 08:17:39, tapted wrote: > nit: this might not be required now Done. https://codereview.chromium.org/2210763002/diff/140001/ui/views/controls/nati... File ui/views/controls/native/native_view_host_mac.mm (right): https://codereview.chromium.org/2210763002/diff/140001/ui/views/controls/nati... ui/views/controls/native/native_view_host_mac.mm:142: return native_view_; On 2016/10/04 08:17:39, tapted wrote: > let's leave this off for now - there should be a nice way to test for it without > requiring VoiceOver (also it needs different OWNERS :p) Done.
The CQ bit was checked by patricialor@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dmazzoni@chromium.org, tapted@chromium.org Link to the patchset: https://codereview.chromium.org/2210763002/#ps160001 (title: "Nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== MacViews a11y: Sync VoiceOver cursor with keyboard focus. Move the VoiceOver cursor when changing the keyboard focus if "VoiceOver cursor follows keyboard focus" is turned on in VoiceOver Utility (System Preferences > Accessibility > VoiceOver > Open VoiceOver Utility) with MacViews. BUG=610585, 650118 TEST=With the #mac-views-native-dialogs flag turned on via chrome:flags, turn on VoiceOver with Cmd+F5. Click the bookmark star on the right of the omnibox and press tab to change focus inside it. The VoiceOver cursor should follow keyboard focus. ========== to ========== MacViews a11y: Sync VoiceOver cursor with keyboard focus. Move the VoiceOver cursor when changing the keyboard focus if "VoiceOver cursor follows keyboard focus" is turned on in VoiceOver Utility (System Preferences > Accessibility > VoiceOver > Open VoiceOver Utility) with MacViews. BUG=610585, 650118 TEST=With the #mac-views-native-dialogs flag turned on via chrome:flags, turn on VoiceOver with Cmd+F5. Click the bookmark star on the right of the omnibox and press tab to change focus inside it. The VoiceOver cursor should follow keyboard focus. Committed: https://crrev.com/f8686b58fdf56513efdf5ed2826018560d26bbe2 Cr-Commit-Position: refs/heads/master@{#423083} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/f8686b58fdf56513efdf5ed2826018560d26bbe2 Cr-Commit-Position: refs/heads/master@{#423083} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
