Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2010493005:
a11y/Mac: Add screenreader support for SubtleNotificationView announcements.
|
Created:
4 years, 7 months ago by Patti Lor Modified:
4 years, 3 months ago CC:
aboxhall+watch_chromium.org, chrome-apps-syd-reviews_chromium.org, chromium-reviews, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, je_julie, nektar+watch_chromium.org, tfarina, yuzo+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptiona11y/Mac: Add screenreader support for SubtleNotificationView announcements.
Add accessibility alerts (ui::AX_EVENT_ALERT) for SubtleNotificationView on Mac,
which is used for the full screen, mouse/pointer lock, and new shortcut to go
back notification bubbles. Add a string for the Command key icon ⌘, which is
needed for translating the accelerator key icons to readable text for the
screenreader.
In theory, this change should also work on Windows and CrOS (since
ui::AX_EVENT_ALERT is already plumbed through for both of these platforms) but
this isn't the case (the cause of which is under investigation).
Additionally, declare NSAccessibilityPriorityKey in
base/mac/sdk_forward_declarations.h and update in other locations.
BUG=577011
Patch Set 1 #Patch Set 2 : Rebase. #Patch Set 3 : Rebase and use title instead of value. #
Total comments: 28
Patch Set 4 : Address review comments. #
Total comments: 2
Patch Set 5 : Fix announce to work for new backspace notification, not just ExclusiveAccessBubbleViews. #Patch Set 6 : Rebase. #
Total comments: 22
Patch Set 7 : Address review comments. #
Total comments: 4
Patch Set 8 : Address review comments. #
Total comments: 8
Patch Set 9 : Address review comments. #Patch Set 10 : Rebase #
Messages
Total messages: 46 (22 generated)
patricialor@chromium.org changed reviewers: + dmazzoni@chromium.org, tapted@chromium.org
Hi Trent, Dominic, PTAL! It's still a WIP - it might be worth adding strings for the announcement instead (currently it just speaks the text inside the bubble, which probably isn't very explanatory if you accidentally made the bubble appear). Dominic, I'm also unsure of how to test this in ChromeOS / Windows / Linux, which you might be able to advise on. I have tried using the ChromeVox text display on a CrOS build on Linux, and NVDA as well as the in-built screenreader on Windows 10. As far as I can tell nothing other than Mac works, but mguica@ forwarded me an email thread in which you recommended to use "NotifyAccessibilityEvent(ui::AX_EVENT_TEXT_CHANGED, true)", which apparently works for you? Please let me know! Thank you.
lgtm https://codereview.chromium.org/2010493005/diff/40001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2010493005/diff/40001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_mac.mm:310: // TODO(patricialor): Remove this when the deployment target is 10.9 or later. Just an idea: rather than a TODO, how about just making the compile fail, like: #if (MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_9) #error Delete the following line once the deployment target is >= 10.9 #endif
tapted@chromium.org changed reviewers: + mgiuca@chromium.org
+mgiuca for the the ExclusiveAccessBubbleViews::Show() comment below. Also maybe you have ideas for testing on CrOS/Win/Linux; Patti said: > how to test this in ChromeOS / Windows / Linux, > which you might be able to advise on. I have tried using the ChromeVox text > display on a CrOS build on Linux, and NVDA as well as the in-built screenreader > on Windows 10. I'm OK with rolling out just to Mac first though, if there's a deeper issue elsewhere. https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right): https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/exclusive_access_bubble_views.cc:89: view_->NotifyAccessibilityEvent(ui::AX_EVENT_ALERT, true); should this be in ExclusiveAccessBubbleViews::Show()? I see matt had it here in his earlier CL though, so he might have had a reason for that. https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/exclusive_access_bubble_views.cc:131: view_->NotifyAccessibilityEvent(ui::AX_EVENT_TEXT_CHANGED, true); If we start by fixing Mac and address other platforms in a follow-up, we shouldn't add this yet (since it's not plumbed through on mac). https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/subtle_notification_view.cc (right): https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/subtle_notification_view.cc:53: base::string16 text(); nit: declare const function https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/subtle_notification_view.cc:88: return text_; nit: declare this inline https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/subtle_notification_view.cc:218: // views::View: nit: no need for this comment https://codereview.chromium.org/2010493005/diff/40001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2010493005/diff/40001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_mac.mm:310: // TODO(patricialor): Remove this when the deployment target is 10.9 or later. On 2016/06/21 20:09:36, dmazzoni wrote: > Just an idea: rather than a TODO, how about just making the > compile fail, like: > > #if (MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_9) > #error Delete the following line once the deployment target is >= 10.9 > #endif This is a good suggestion! But I have an alternative which is a bit more idiomatic, and that's just to add this declaration to base/mac/sdk_forward_declarations.h around line 66. When we get around to bumping the deployment target, that file will be properly swept for anything stale https://codereview.chromium.org/2010493005/diff/40001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_mac.mm:347: NSAccessibilityPriorityKey : @(NSAccessibilityPriorityHigh) does NSAccessibilityPriorityKey : NSAccessibilityPriorityHigh work?
https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right): https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/exclusive_access_bubble_views.cc:89: view_->NotifyAccessibilityEvent(ui::AX_EVENT_ALERT, true); On 2016/06/22 00:23:38, tapted wrote: > should this be in ExclusiveAccessBubbleViews::Show()? I see matt had it here in > his earlier CL though, so he might have had a reason for that. Yeah I think it should be in Show(), not the constructor. https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/exclusive_access_bubble_views.cc:131: view_->NotifyAccessibilityEvent(ui::AX_EVENT_TEXT_CHANGED, true); On 2016/06/22 00:23:38, tapted wrote: > If we start by fixing Mac and address other platforms in a follow-up, we > shouldn't add this yet (since it's not plumbed through on mac). Are you saying this code is only relevant on non-Mac platforms? Does it work on non-Mac? This is basically what I had in my CL: https://codereview.chromium.org/1578293003/ which didn't end up reading out the text when the bubble opens, or only did in some cases IIRC. If this CL is about getting it working on Mac only then we shouldn't have code that isn't required on Mac. https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/subtle_notification_view.cc (right): https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/subtle_notification_view.cc:88: return text_; On 2016/06/22 00:23:38, tapted wrote: > nit: declare this inline This needs to have the pipes removed (http://www.officeworks.com.au/shop/officeworks/c/school-art/craft-materials/p... ?) before being read aloud. Few approaches: 1. Change have text() clean the pipes out of this string, and rename to GetText() to show that it's non-trivial. 2. Have GetAccessibleState do it. (Probably preferred; it's a bit weird that GetText() does not return what SetText() receives.) https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/subtle_notification_view.cc:222: state->name = instruction_view_->text(); This needs to have the pipes removed (http://www.officeworks.com.au/shop/officeworks/c/school-art/craft-materials/p... ?) before being read aloud. See SetText. Alternatively, could have a method on InstructionView that returns the text without pipes (but don't call it GetText, that would be confusing considering SetText is called with pipes). https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/subtle_notification_view.h (right): https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/subtle_notification_view.h:13: namespace views { Need a forward-declare of ui::AXViewState.
Description was changed from ========== Fullscreen/Mouselock bubble: Add support for screenreaders. WIP: Initial code for accessibility alerts (ui::AX_EVENT_ALERT) for SubtleNotificationView (inc. for OSX). TODO: Will probably need to split out code to make this work for Mac into a separate CL. BUG=577011 ========== to ========== Fullscreen/Mouselock bubble: Add support for screenreaders. WIP: Announce accessibility alerts (ui::AX_EVENT_ALERT) for SubtleNotificationView (including plumbing for accessibility alert support on Mac). This is the bubble that appears when entering full screen or on pointer lock. Additionally, declare NSAccessibilityPriorityKey in base/mac/sdk_forward_declarations.h and update in other locations. BUG=577011 ==========
Patchset #4 (id:60001) has been deleted
Thanks all, PTAL. Also, friendly ping to dmazzoni@, please see https://codereview.chromium.org/2010493005/#msg2! https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right): https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/exclusive_access_bubble_views.cc:89: view_->NotifyAccessibilityEvent(ui::AX_EVENT_ALERT, true); On 2016/06/22 02:59:20, Matt Giuca wrote: > On 2016/06/22 00:23:38, tapted wrote: > > should this be in ExclusiveAccessBubbleViews::Show()? I see matt had it here > in > > his earlier CL though, so he might have had a reason for that. > > Yeah I think it should be in Show(), not the constructor. Done. https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/exclusive_access_bubble_views.cc:131: view_->NotifyAccessibilityEvent(ui::AX_EVENT_TEXT_CHANGED, true); On 2016/06/22 02:59:20, Matt Giuca wrote: > On 2016/06/22 00:23:38, tapted wrote: > > If we start by fixing Mac and address other platforms in a follow-up, we > > shouldn't add this yet (since it's not plumbed through on mac). > > Are you saying this code is only relevant on non-Mac platforms? Does it work on > non-Mac? > > This is basically what I had in my CL: > https://codereview.chromium.org/1578293003/ > which didn't end up reading out the text when the bubble opens, or only did in > some cases IIRC. If this CL is about getting it working on Mac only then we > shouldn't have code that isn't required on Mac. Agreed - as far as I can tell, it doesn't work on other platforms, but I wanted to get clarification on this because the email thread you forwarded me seems to suggest that adding this particular line works for dmazzoni. https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/subtle_notification_view.cc (right): https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/subtle_notification_view.cc:53: base::string16 text(); On 2016/06/22 00:23:38, tapted wrote: > nit: declare const function Done. https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/subtle_notification_view.cc:88: return text_; On 2016/06/22 02:59:20, Matt Giuca wrote: > On 2016/06/22 00:23:38, tapted wrote: > > nit: declare this inline > > This needs to have the pipes removed > (http://www.officeworks.com.au/shop/officeworks/c/school-art/craft-materials/p... > ?) before being read aloud. > > Few approaches: > 1. Change have text() clean the pipes out of this string, and rename to > GetText() to show that it's non-trivial. > 2. Have GetAccessibleState do it. (Probably preferred; it's a bit weird that > GetText() does not return what SetText() receives.) Done (as per option 2). https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/subtle_notification_view.cc:218: // views::View: On 2016/06/22 00:23:38, tapted wrote: > nit: no need for this comment Done. https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/subtle_notification_view.cc:222: state->name = instruction_view_->text(); On 2016/06/22 02:59:20, Matt Giuca wrote: > This needs to have the pipes removed > (http://www.officeworks.com.au/shop/officeworks/c/school-art/craft-materials/p... > ?) before being read aloud. See SetText. > > Alternatively, could have a method on InstructionView that returns the text > without pipes (but don't call it GetText, that would be confusing considering > SetText is called with pipes). Done. https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/subtle_notification_view.h (right): https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/subtle_notification_view.h:13: namespace views { On 2016/06/22 02:59:20, Matt Giuca wrote: > Need a forward-declare of ui::AXViewState. Done, thanks. https://codereview.chromium.org/2010493005/diff/40001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2010493005/diff/40001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_mac.mm:310: // TODO(patricialor): Remove this when the deployment target is 10.9 or later. On 2016/06/22 00:23:38, tapted wrote: > On 2016/06/21 20:09:36, dmazzoni wrote: > > Just an idea: rather than a TODO, how about just making the > > compile fail, like: > > > > #if (MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_9) > > #error Delete the following line once the deployment target is >= 10.9 > > #endif > > This is a good suggestion! But I have an alternative which is a bit more > idiomatic, and that's just to add this declaration to > base/mac/sdk_forward_declarations.h around line 66. When we get around to > bumping the deployment target, that file will be properly swept for anything > stale Done, thanks for the suggestions. I stole that bit of code from ellyjones's CL (https://crrev.com/1999823002/), so I have also done the same thing there - not sure if it is worth doing that in a separate commit? dmazzoni, I think you missed my message about testing this on other platforms (see https://codereview.chromium.org/2010493005/#msg2), please advise? Thanks! https://codereview.chromium.org/2010493005/diff/40001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_mac.mm:347: NSAccessibilityPriorityKey : @(NSAccessibilityPriorityHigh) On 2016/06/22 00:23:38, tapted wrote: > does > > NSAccessibilityPriorityKey : NSAccessibilityPriorityHigh > > work? It complains "collection element of type 'NSAccessibilityPriorityLevel' is not an Objective-C object", so I'm guessing it's a C primitive that needs to be converted with the @(), have left as is.
https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right): https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/exclusive_access_bubble_views.cc:131: view_->NotifyAccessibilityEvent(ui::AX_EVENT_TEXT_CHANGED, true); On 2016/06/23 01:00:38, Patti Lor wrote: > On 2016/06/22 02:59:20, Matt Giuca wrote: > > On 2016/06/22 00:23:38, tapted wrote: > > > If we start by fixing Mac and address other platforms in a follow-up, we > > > shouldn't add this yet (since it's not plumbed through on mac). > > > > Are you saying this code is only relevant on non-Mac platforms? Does it work > on > > non-Mac? > > > > This is basically what I had in my CL: > > https://codereview.chromium.org/1578293003/ > > which didn't end up reading out the text when the bubble opens, or only did in > > some cases IIRC. If this CL is about getting it working on Mac only then we > > shouldn't have code that isn't required on Mac. > > Agreed - as far as I can tell, it doesn't work on other platforms, but I wanted > to get clarification on this because the email thread you forwarded me seems to > suggest that adding this particular line works for dmazzoni. I'm not sure (it's really hard to test on other platforms because I don't have any voice modules installed). I think if this isn't necessary for Mac we should just take it out of this CL. https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/subtle_notification_view.h (right): https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/subtle_notification_view.h:13: namespace views { On 2016/06/23 01:00:39, Patti Lor wrote: > On 2016/06/22 02:59:20, Matt Giuca wrote: > > Need a forward-declare of ui::AXViewState. > > Done, thanks. It's ui::AXViewState, not views::AXViewState. https://codereview.chromium.org/2010493005/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/subtle_notification_view.cc (right): https://codereview.chromium.org/2010493005/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/subtle_notification_view.cc:49: // more segments delimited by |kKeyNameDelimiter|; each of these segments I don't think this comment change is necessary. ("if kKeyNameDelimiter is set to..." implies that it's customizable, which it really isn't, you're just moving it out to a constant. From an interface perspective it just takes a pipe.)
https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right): https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/exclusive_access_bubble_views.cc:131: view_->NotifyAccessibilityEvent(ui::AX_EVENT_TEXT_CHANGED, true); On 2016/06/24 04:21:12, Matt Giuca wrote: > On 2016/06/23 01:00:38, Patti Lor wrote: > > On 2016/06/22 02:59:20, Matt Giuca wrote: > > > On 2016/06/22 00:23:38, tapted wrote: > > > > If we start by fixing Mac and address other platforms in a follow-up, we > > > > shouldn't add this yet (since it's not plumbed through on mac). > > > > > > Are you saying this code is only relevant on non-Mac platforms? Does it work > > on > > > non-Mac? > > > > > > This is basically what I had in my CL: > > > https://codereview.chromium.org/1578293003/ > > > which didn't end up reading out the text when the bubble opens, or only did > in > > > some cases IIRC. If this CL is about getting it working on Mac only then we > > > shouldn't have code that isn't required on Mac. > > > > Agreed - as far as I can tell, it doesn't work on other platforms, but I > wanted > > to get clarification on this because the email thread you forwarded me seems > to > > suggest that adding this particular line works for dmazzoni. > > I'm not sure (it's really hard to test on other platforms because I don't have > any voice modules installed). I think if this isn't necessary for Mac we should > just take it out of this CL. This is a reasonable event to fire on any platform when static text changes. I'd leave it in. Note that this would not typically cause the text to be announced. In general, only AX_EVENT_ALERT means to immediately announce something. Any other event is informative - it allows a client to know that something changed so that it can update its internal cache. If the object is currently focused I would expect it to announce - so an event fired on an editable text control or checkbox would be announced immediately, for example, when focused. Hope that helps.
Description was changed from ========== Fullscreen/Mouselock bubble: Add support for screenreaders. WIP: Announce accessibility alerts (ui::AX_EVENT_ALERT) for SubtleNotificationView (including plumbing for accessibility alert support on Mac). This is the bubble that appears when entering full screen or on pointer lock. Additionally, declare NSAccessibilityPriorityKey in base/mac/sdk_forward_declarations.h and update in other locations. BUG=577011 ========== to ========== a11y/Mac: Add screenreader support for SubtleNotificationView announcements. Add accessibility alerts (ui::AX_EVENT_ALERT) for SubtleNotificationView on Mac, which is used for the full screen, mouse/pointer lock, and new shortcut to go back notification bubbles. Add a string for the Command key icon ⌘, which is needed for translating the accelerator key icons to readable text for the screenreader. Additionally, declare NSAccessibilityPriorityKey in base/mac/sdk_forward_declarations.h and update in other locations. BUG=577011 ==========
Patchset #5 (id:100001) has been deleted
Hi all, PTAL - tapted@ suggested I test this change with the new shortcut to go back notification, which didn't work. To fix I have reverted all changes to exclusive_access_bubble_views.* and subtle_notification_view.* are doing the accessibility alerts instead. This includes adding a new string "Command" for translating ⌘ into something VoiceOver understands. (FYI, "Command+" is a string that exists. But I don't think we can take advantage of this because of the pipe delimiters.) Thanks! https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right): https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/exclusive_access_bubble_views.cc:131: view_->NotifyAccessibilityEvent(ui::AX_EVENT_TEXT_CHANGED, true); On 2016/06/24 05:26:03, dmazzoni wrote: > On 2016/06/24 04:21:12, Matt Giuca wrote: > > On 2016/06/23 01:00:38, Patti Lor wrote: > > > On 2016/06/22 02:59:20, Matt Giuca wrote: > > > > On 2016/06/22 00:23:38, tapted wrote: > > > > > If we start by fixing Mac and address other platforms in a follow-up, we > > > > > shouldn't add this yet (since it's not plumbed through on mac). > > > > > > > > Are you saying this code is only relevant on non-Mac platforms? Does it > work > > > on > > > > non-Mac? > > > > > > > > This is basically what I had in my CL: > > > > https://codereview.chromium.org/1578293003/ > > > > which didn't end up reading out the text when the bubble opens, or only > did > > in > > > > some cases IIRC. If this CL is about getting it working on Mac only then > we > > > > shouldn't have code that isn't required on Mac. > > > > > > Agreed - as far as I can tell, it doesn't work on other platforms, but I > > wanted > > > to get clarification on this because the email thread you forwarded me seems > > to > > > suggest that adding this particular line works for dmazzoni. > > > > I'm not sure (it's really hard to test on other platforms because I don't have > > any voice modules installed). I think if this isn't necessary for Mac we > should > > just take it out of this CL. > > This is a reasonable event to fire on any platform when static text changes. I'd > leave it in. > > Note that this would not typically cause the text to be announced. > > In general, only AX_EVENT_ALERT means to immediately announce something. > Any other event is informative - it allows a client to know that something > changed > so that it can update its internal cache. If the object is currently focused I > would > expect it to announce - so an event fired on an editable text control or > checkbox > would be announced immediately, for example, when focused. Hope that helps. Ended up plumbing ui::AX_EVENT_TEXT_CHANGED through for Mac as it was quite simple, so have left this line in. What dmazzoni@ said is also true though (as in, having this here won't cause the updated text to be announced again) so I have added a second notification for ui::AX_EVENT_ALERT here as well. https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/subtle_notification_view.h (right): https://codereview.chromium.org/2010493005/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/subtle_notification_view.h:13: namespace views { On 2016/06/24 04:21:12, Matt Giuca wrote: > On 2016/06/23 01:00:39, Patti Lor wrote: > > On 2016/06/22 02:59:20, Matt Giuca wrote: > > > Need a forward-declare of ui::AXViewState. > > > > Done, thanks. > > It's ui::AXViewState, not views::AXViewState. Done. Thanks again for picking up on that. D: https://codereview.chromium.org/2010493005/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/subtle_notification_view.cc (right): https://codereview.chromium.org/2010493005/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/subtle_notification_view.cc:49: // more segments delimited by |kKeyNameDelimiter|; each of these segments On 2016/06/24 04:21:12, Matt Giuca wrote: > I don't think this comment change is necessary. ("if kKeyNameDelimiter is set > to..." implies that it's customizable, which it really isn't, you're just moving > it out to a constant. From an interface perspective it just takes a pipe.) Done.
https://codereview.chromium.org/2010493005/diff/40001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2010493005/diff/40001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_mac.mm:347: NSAccessibilityPriorityKey : @(NSAccessibilityPriorityHigh) On 2016/06/23 01:00:39, Patti Lor wrote: > On 2016/06/22 00:23:38, tapted wrote: > > does > > > > NSAccessibilityPriorityKey : NSAccessibilityPriorityHigh > > > > work? > > It complains "collection element of type 'NSAccessibilityPriorityLevel' is not > an Objective-C object", so I'm guessing it's a C primitive that needs to be > converted with the @(), have left as is. Ah - right you are. [NSNumber numberWithInteger:NSAccessibilityPriorityHigh] is the old-school way of doing this, but @(foo) is better. https://codereview.chromium.org/2010493005/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/subtle_notification_view.cc (right): https://codereview.chromium.org/2010493005/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/subtle_notification_view.cc:41: const char kKeyNameDelimiter[] = "|"; hum - this should probably be a const char16 []. Then you shouldn't need to wrap it in base::ASCIIToUTF16(..) -- base::StringPiece16 will implicitly convert from a char16[]. Try a string literal, but to initialize it you might need something like const char16 kKeyNameDelimiter[] = { '|', 0 }; https://codereview.chromium.org/2010493005/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/subtle_notification_view.cc:51: // will be displayed as a keyboard key. e.g. "Press |Alt|+|Q| to exit" will (ubernit: technically, a comma should "always" come after "For example," even if, for example, it's abbreviated. For example, like this.) https://codereview.chromium.org/2010493005/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/subtle_notification_view.cc:58: base::string16 accessible_name() { return accessible_name_; } nit: const method https://codereview.chromium.org/2010493005/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/subtle_notification_view.cc:61: base::string16 text() { return text_; } nit: const https://codereview.chromium.org/2010493005/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/subtle_notification_view.cc:76: base::string16 accessible_name_; If it achieves the same thing, I think it makes more sense just to store this on SubtleNotificationView instead of the InstructionView, since it's SubtleNotificationView that's populating AXViewState https://codereview.chromium.org/2010493005/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/subtle_notification_view.cc:247: if (instruction_view_->accessible_name().empty()) Can this happen? https://codereview.chromium.org/2010493005/diff/140001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2010493005/diff/140001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:523: } break; nit: no need for break
Thanks Trent, PTAL! https://codereview.chromium.org/2010493005/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/subtle_notification_view.cc (right): https://codereview.chromium.org/2010493005/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/subtle_notification_view.cc:41: const char kKeyNameDelimiter[] = "|"; On 2016/07/11 03:46:47, tapted wrote: > hum - this should probably be a const char16 []. Then you shouldn't need to wrap > it in base::ASCIIToUTF16(..) -- base::StringPiece16 will implicitly convert from > a char16[]. Try a string literal, but to initialize it you might need something > like > > const char16 kKeyNameDelimiter[] = { '|', 0 }; > Done. https://codereview.chromium.org/2010493005/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/subtle_notification_view.cc:51: // will be displayed as a keyboard key. e.g. "Press |Alt|+|Q| to exit" will On 2016/07/11 03:46:47, tapted wrote: > (ubernit: technically, a comma should "always" come after "For example," even > if, for example, it's abbreviated. For example, like this.) Oops, didn't notice that! Noted for future reference and done :) https://codereview.chromium.org/2010493005/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/subtle_notification_view.cc:58: base::string16 accessible_name() { return accessible_name_; } On 2016/07/11 03:46:47, tapted wrote: > nit: const method Done. https://codereview.chromium.org/2010493005/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/subtle_notification_view.cc:61: base::string16 text() { return text_; } On 2016/07/11 03:46:47, tapted wrote: > nit: const Done. https://codereview.chromium.org/2010493005/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/subtle_notification_view.cc:76: base::string16 accessible_name_; On 2016/07/11 03:46:47, tapted wrote: > If it achieves the same thing, I think it makes more sense just to store this on > SubtleNotificationView instead of the InstructionView, since it's > SubtleNotificationView that's populating AXViewState Done. https://codereview.chromium.org/2010493005/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/subtle_notification_view.cc:247: if (instruction_view_->accessible_name().empty()) On 2016/07/11 03:46:47, tapted wrote: > Can this happen? Yes, currently the new backspace shortcut bubble is the only one that explicitly sets the accessible name (in NewBackShortcutBubble::UpdateContent). But since the exclusive access bubbles don't require a different string or anything, they will fall back to this. https://codereview.chromium.org/2010493005/diff/140001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2010493005/diff/140001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:523: } break; On 2016/07/11 03:46:47, tapted wrote: > nit: no need for break Done.
mostly nits. Make sure the subject of the CL matches the first line of the CL description. Also the CL description should say something about how this affects ChromeVox (Windows/CrOS). I.e. whether it does or doesn't change any behaviour on those platforms (and, if it doesn't, ideally why that is, but we can also say "still investigating") https://codereview.chromium.org/2010493005/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/subtle_notification_view.cc (right): https://codereview.chromium.org/2010493005/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/subtle_notification_view.cc:247: if (instruction_view_->accessible_name().empty()) On 2016/07/12 00:04:36, Patti Lor wrote: > On 2016/07/11 03:46:47, tapted wrote: > > Can this happen? > > Yes, currently the new backspace shortcut bubble is the only one that explicitly > sets the accessible name (in NewBackShortcutBubble::UpdateContent). But since > the exclusive access bubbles don't require a different string or anything, they > will fall back to this. Ah. So, it's "generally" good to avoid side-effects in getters. Some would consider it a bug that View::GetAccessibleState isn't a const method (and if it was, you wouldn't be able to set the accessible name here). I think this use is OK, but it needs a better comment - it's not clear why we set it instead of just assigning it. Something like // If there is no accessible name set, use the displayed text, but ensure the '|' delimiters are removed. https://codereview.chromium.org/2010493005/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/subtle_notification_view.cc (right): https://codereview.chromium.org/2010493005/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/subtle_notification_view.cc:58: const base::string16 text() { return text_; } `const` needs to come after the () (`const` on return types is also a thing, but generally not useful for by-value return types - you see it more for const-reference types, which is an option here, but not required) https://codereview.chromium.org/2010493005/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/subtle_notification_view.h (right): https://codereview.chromium.org/2010493005/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/subtle_notification_view.h:48: base::string16 accessible_name() const { return accessible_name_; } This might not be needed - see if you can update all the callers of accessible_name() to just access accessible_name_ directly.
Description was changed from ========== a11y/Mac: Add screenreader support for SubtleNotificationView announcements. Add accessibility alerts (ui::AX_EVENT_ALERT) for SubtleNotificationView on Mac, which is used for the full screen, mouse/pointer lock, and new shortcut to go back notification bubbles. Add a string for the Command key icon ⌘, which is needed for translating the accelerator key icons to readable text for the screenreader. Additionally, declare NSAccessibilityPriorityKey in base/mac/sdk_forward_declarations.h and update in other locations. BUG=577011 ========== to ========== a11y/Mac: Add screenreader support for SubtleNotificationView announcements. Add accessibility alerts (ui::AX_EVENT_ALERT) for SubtleNotificationView on Mac, which is used for the full screen, mouse/pointer lock, and new shortcut to go back notification bubbles. Add a string for the Command key icon ⌘, which is needed for translating the accelerator key icons to readable text for the screenreader. Additionally, declare NSAccessibilityPriorityKey in base/mac/sdk_forward_declarations.h and update in other locations. BUG=577011 ==========
https://codereview.chromium.org/2010493005/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/subtle_notification_view.cc (right): https://codereview.chromium.org/2010493005/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/subtle_notification_view.cc:192: NotifyAccessibilityEvent(ui::AX_EVENT_TEXT_CHANGED, true); I think AX_EVENT_CHILDREN_CHANGED would be better since |instruction_view_| and |link_| are the ones changing. https://codereview.chromium.org/2010493005/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/subtle_notification_view.cc:233: // '|' delimiters are removed. What code removes the | delimiters?
https://codereview.chromium.org/2010493005/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/subtle_notification_view.cc (right): https://codereview.chromium.org/2010493005/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/subtle_notification_view.cc:41: const char kKeyNameDelimiter[] = "|"; On 2016/07/12 00:04:36, Patti Lor wrote: > On 2016/07/11 03:46:47, tapted wrote: > > hum - this should probably be a const char16 []. Then you shouldn't need to > wrap > > it in base::ASCIIToUTF16(..) -- base::StringPiece16 will implicitly convert > from > > a char16[]. Try a string literal, but to initialize it you might need > something > > like > > > > const char16 kKeyNameDelimiter[] = { '|', 0 }; > > > > Done. Hmm, I don't really like this (I wrote a whole comment asking to change it then realised tapted had already asked for it). At first I read this as "here is an array of characters that we will split on: {'|', '\0'}", and thought, "why the heck are we splitting on NUL?" I think readability trumps performance here: I'd prefer going back to a char array string literal. Alternatively you can store it as a wchar_t array and use WideToUTF16 which will have no conversion overhead on Windows (but will on Mac and Linux). I don't really see the point of doing that. https://codereview.chromium.org/2010493005/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/subtle_notification_view.cc:51: // will be displayed as a keyboard key. e.g. "Press |Alt|+|Q| to exit" will On 2016/07/12 00:04:36, Patti Lor wrote: > On 2016/07/11 03:46:47, tapted wrote: > > (ubernit: technically, a comma should "always" come after "For example," even > > if, for example, it's abbreviated. For example, like this.) > > Oops, didn't notice that! Noted for future reference and done :) Also I had a comma there in the first place :) https://codereview.chromium.org/2010493005/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/subtle_notification_view.cc:247: if (instruction_view_->accessible_name().empty()) On 2016/07/12 00:51:39, tapted wrote: > On 2016/07/12 00:04:36, Patti Lor wrote: > > On 2016/07/11 03:46:47, tapted wrote: > > > Can this happen? > > > > Yes, currently the new backspace shortcut bubble is the only one that > explicitly > > sets the accessible name (in NewBackShortcutBubble::UpdateContent). But since > > the exclusive access bubbles don't require a different string or anything, > they > > will fall back to this. > > Ah. So, it's "generally" good to avoid side-effects in getters. Some would > consider it a bug that View::GetAccessibleState isn't a const method (and if it > was, you wouldn't be able to set the accessible name here). > > I think this use is OK, but it needs a better comment - it's not clear why we > set it instead of just assigning it. Something like > > // If there is no accessible name set, use the displayed text, but ensure the > '|' delimiters are removed. This is a good point. How about we *pretend* that GetAccessibleState is const and design accordingly? - Make |accessible_name_| mutable, which implies the correct semantics (it is a cached value that can be written by a const method). - Rename SetAccessibleName to GetAccessibleName and make it const. Move this if statement into GetAccessibleName (so GAN updates |accessible_name_| only if it's empty, and returns the value of |accessible_name_|. - Have GetAccessibleState just do state->name = GetAccessibleName(). Now GetAccessibleState has no visible side-effects, so behaves as a const method. https://codereview.chromium.org/2010493005/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/subtle_notification_view.cc (right): https://codereview.chromium.org/2010493005/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/subtle_notification_view.cc:190: link_->SetVisible(!link_text.empty()); I think you have to clear accessible_name_ here, or else GetAccessibleState may return stale data (it only updates the cache if it is empty). https://codereview.chromium.org/2010493005/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/subtle_notification_view.cc:233: // '|' delimiters are removed. On 2016/07/13 23:41:35, dmazzoni_ooo_until_7_20 wrote: > What code removes the | delimiters? SetAccessibleName, just above, does this. I agree this is a bit confusing and the comment probably should be split up across the two functions. Here: // If there is no accessible name set, use the displayed text. And above in SetAccessibleName: // Ensure the '|' delimiters are removed.
https://codereview.chromium.org/2010493005/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/subtle_notification_view.cc (right): https://codereview.chromium.org/2010493005/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/subtle_notification_view.cc:41: const char kKeyNameDelimiter[] = "|"; On 2016/07/14 01:16:02, Matt Giuca wrote: > On 2016/07/12 00:04:36, Patti Lor wrote: > > On 2016/07/11 03:46:47, tapted wrote: > > > hum - this should probably be a const char16 []. Then you shouldn't need to > > wrap > > > it in base::ASCIIToUTF16(..) -- base::StringPiece16 will implicitly convert > > from > > > a char16[]. Try a string literal, but to initialize it you might need > > something > > > like > > > > > > const char16 kKeyNameDelimiter[] = { '|', 0 }; > > > > > > > Done. > > Hmm, I don't really like this (I wrote a whole comment asking to change it then > realised tapted had already asked for it). > > At first I read this as "here is an array of characters that we will split on: > {'|', '\0'}", and thought, "why the heck are we splitting on NUL?" > > I think readability trumps performance here: I'd prefer going back to a char > array string literal. > > Alternatively you can store it as a wchar_t array and use WideToUTF16 which will > have no conversion overhead on Windows (but will on Mac and Linux). I don't > really see the point of doing that. my brain sees arrays and string literals as interchangeable :p I guess what we really need is this: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/gcoUbcjfsII/_Fndx... Then this could be something like const char16 kKeyNameDelimiter[] = u"|"; but that's currently on the banned list - https://chromium-cpp.appspot.com/ (although web_input_event_builders_mac_unittest.mm uses it already *gasp*)
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 ========== a11y/Mac: Add screenreader support for SubtleNotificationView announcements. Add accessibility alerts (ui::AX_EVENT_ALERT) for SubtleNotificationView on Mac, which is used for the full screen, mouse/pointer lock, and new shortcut to go back notification bubbles. Add a string for the Command key icon ⌘, which is needed for translating the accelerator key icons to readable text for the screenreader. Additionally, declare NSAccessibilityPriorityKey in base/mac/sdk_forward_declarations.h and update in other locations. BUG=577011 ========== to ========== a11y/Mac: Add screenreader support for SubtleNotificationView announcements. Add accessibility alerts (ui::AX_EVENT_ALERT) for SubtleNotificationView on Mac, which is used for the full screen, mouse/pointer lock, and new shortcut to go back notification bubbles. Add a string for the Command key icon ⌘, which is needed for translating the accelerator key icons to readable text for the screenreader. In theory, this change should also work on Windows and CrOS (since ui::AX_EVENT_ALERT is already plumbed through for both of these platforms) but this isn't the case (the cause of which is under investigation). Additionally, declare NSAccessibilityPriorityKey in base/mac/sdk_forward_declarations.h and update in other locations. BUG=577011 ==========
Thanks all, PTAL. I have also updated the CL description as per tapted's comment. https://codereview.chromium.org/2010493005/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/subtle_notification_view.cc (right): https://codereview.chromium.org/2010493005/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/subtle_notification_view.cc:41: const char kKeyNameDelimiter[] = "|"; On 2016/07/14 01:48:40, tapted wrote: > On 2016/07/14 01:16:02, Matt Giuca wrote: > > On 2016/07/12 00:04:36, Patti Lor wrote: > > > On 2016/07/11 03:46:47, tapted wrote: > > > > hum - this should probably be a const char16 []. Then you shouldn't need > to > > > wrap > > > > it in base::ASCIIToUTF16(..) -- base::StringPiece16 will implicitly > convert > > > from > > > > a char16[]. Try a string literal, but to initialize it you might need > > > something > > > > like > > > > > > > > const char16 kKeyNameDelimiter[] = { '|', 0 }; > > > > > > > > > > Done. > > > > Hmm, I don't really like this (I wrote a whole comment asking to change it > then > > realised tapted had already asked for it). > > > > At first I read this as "here is an array of characters that we will split on: > > {'|', '\0'}", and thought, "why the heck are we splitting on NUL?" > > > > I think readability trumps performance here: I'd prefer going back to a char > > array string literal. > > > > Alternatively you can store it as a wchar_t array and use WideToUTF16 which > will > > have no conversion overhead on Windows (but will on Mac and Linux). I don't > > really see the point of doing that. > > my brain sees arrays and string literals as interchangeable :p > > I guess what we really need is this: > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/gcoUbcjfsII/_Fndx... > > Then this could be something like > > const char16 kKeyNameDelimiter[] = u"|"; > > but that's currently on the banned list - https://chromium-cpp.appspot.com/ > (although web_input_event_builders_mac_unittest.mm uses it already *gasp*) Have taken Matt's suggestion to keep it as a char array string literal for readability, but yeah the banned one seems to be the best solution :( https://codereview.chromium.org/2010493005/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/subtle_notification_view.cc:51: // will be displayed as a keyboard key. e.g. "Press |Alt|+|Q| to exit" will On 2016/07/14 01:16:02, Matt Giuca wrote: > On 2016/07/12 00:04:36, Patti Lor wrote: > > On 2016/07/11 03:46:47, tapted wrote: > > > (ubernit: technically, a comma should "always" come after "For example," > even > > > if, for example, it's abbreviated. For example, like this.) > > > > Oops, didn't notice that! Noted for future reference and done :) > > Also I had a comma there in the first place :) Haha, yup, that's what I meant D: Didn't copy and paste properly! https://codereview.chromium.org/2010493005/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/subtle_notification_view.cc:247: if (instruction_view_->accessible_name().empty()) On 2016/07/14 01:16:02, Matt Giuca wrote: > On 2016/07/12 00:51:39, tapted wrote: > > On 2016/07/12 00:04:36, Patti Lor wrote: > > > On 2016/07/11 03:46:47, tapted wrote: > > > > Can this happen? > > > > > > Yes, currently the new backspace shortcut bubble is the only one that > > explicitly > > > sets the accessible name (in NewBackShortcutBubble::UpdateContent). But > since > > > the exclusive access bubbles don't require a different string or anything, > > they > > > will fall back to this. > > > > Ah. So, it's "generally" good to avoid side-effects in getters. Some would > > consider it a bug that View::GetAccessibleState isn't a const method (and if > it > > was, you wouldn't be able to set the accessible name here). > > > > I think this use is OK, but it needs a better comment - it's not clear why we > > set it instead of just assigning it. Something like > > > > // If there is no accessible name set, use the displayed text, but ensure the > > '|' delimiters are removed. > > This is a good point. How about we *pretend* that GetAccessibleState is const > and design accordingly? > > - Make |accessible_name_| mutable, which implies the correct semantics (it is a > cached value that can be written by a const method). > - Rename SetAccessibleName to GetAccessibleName and make it const. Move this if > statement into GetAccessibleName (so GAN updates |accessible_name_| only if it's > empty, and returns the value of |accessible_name_|. > - Have GetAccessibleState just do state->name = GetAccessibleName(). > > Now GetAccessibleState has no visible side-effects, so behaves as a const > method. A point Matt brought up in one of his other comments made me remove the side effect entirely and just calculate the |accessible_name_| on the fly when it's not set manually. Since |accessible_name_| can get stale, it means it will need to be cleared on calls to SetUpdate(). But this also means manually set |accessible_name_| will get cleared as well, which might be bad because the caller would have to call methods in a certain order (they would have to call UpdateContent() before calling SetAccessibleName() otherwise the |accessible_name_| they'd just set would be cleared by UpdateContent()). So either you would have to clear |accessible_name_| only if it had been set inside GetAccessibleState(), or just don't make the fallback text permanent (the solution I have chosen). Thanks for the suggestion though Matt! It was a good idea :) https://codereview.chromium.org/2010493005/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/subtle_notification_view.cc (right): https://codereview.chromium.org/2010493005/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/subtle_notification_view.cc:58: const base::string16 text() { return text_; } On 2016/07/12 00:51:39, tapted wrote: > `const` needs to come after the () (`const` on return types is also a thing, but > generally not useful for by-value return types - you see it more for > const-reference types, which is an option here, but not required) Done, sorry for the misunderstanding! https://codereview.chromium.org/2010493005/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/subtle_notification_view.h (right): https://codereview.chromium.org/2010493005/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/subtle_notification_view.h:48: base::string16 accessible_name() const { return accessible_name_; } On 2016/07/12 00:51:40, tapted wrote: > This might not be needed - see if you can update all the callers of > accessible_name() to just access accessible_name_ directly. Done. https://codereview.chromium.org/2010493005/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/subtle_notification_view.cc (right): https://codereview.chromium.org/2010493005/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/subtle_notification_view.cc:190: link_->SetVisible(!link_text.empty()); On 2016/07/14 01:16:02, Matt Giuca wrote: > I think you have to clear accessible_name_ here, or else GetAccessibleState may > return stale data (it only updates the cache if it is empty). :O Good catch thank you! I ended up changing the way |accessible_name_| works entirely because of this, see my other comment (on patch set 6). https://codereview.chromium.org/2010493005/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/subtle_notification_view.cc:192: NotifyAccessibilityEvent(ui::AX_EVENT_TEXT_CHANGED, true); On 2016/07/13 23:41:35, dmazzoni_ooo_until_7_20 wrote: > I think AX_EVENT_CHILDREN_CHANGED would be better since |instruction_view_| and > |link_| are the ones changing. Done, thanks. Would you say |link_| and |instruction_view_| need calls to NotifyAccessibilityEvent() in their SetText() implementations as well, or is that fine? https://codereview.chromium.org/2010493005/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/subtle_notification_view.cc:233: // '|' delimiters are removed. On 2016/07/14 01:16:02, Matt Giuca wrote: > On 2016/07/13 23:41:35, dmazzoni_ooo_until_7_20 wrote: > > What code removes the | delimiters? > > SetAccessibleName, just above, does this. > > I agree this is a bit confusing and the comment probably should be split up > across the two functions. > > Here: > // If there is no accessible name set, use the displayed text. > > And above in SetAccessibleName: > // Ensure the '|' delimiters are removed. Have removed the side effect here, and left the comment because the code it describes is now also in the same place. (See my other comment on patchset 6.)
https://codereview.chromium.org/2010493005/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/subtle_notification_view.cc (right): https://codereview.chromium.org/2010493005/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/subtle_notification_view.cc:192: NotifyAccessibilityEvent(ui::AX_EVENT_TEXT_CHANGED, true); On 2016/07/19 01:31:51, Patti Lor wrote: > On 2016/07/13 23:41:35, dmazzoni_ooo_until_7_20 wrote: > > I think AX_EVENT_CHILDREN_CHANGED would be better since |instruction_view_| > and > > |link_| are the ones changing. > > Done, thanks. Would you say |link_| and |instruction_view_| need calls to > NotifyAccessibilityEvent() in their SetText() implementations as well, or is > that fine? Yeah - this may depend on how we solve http://crbug.com/610585 (moving the VoiceOver cursor) and http://crbug.com/610589 (keeping the views::Label inside a views::Button out of the a11y tree). In this case, |instruction_view_| has alternating views::Labels and views::Views (containing a single views:Label). But we wouldn't want the VoiceOver cursor to iterate over those separately -- they should stop at SubtleNotificationView [or, possibly, InstructionView], and it should populate AXViewState. a11y hit-tests on the broken up labels should fail (e.g. by omitting them from as nodes from the a11y tree) |link_| isn't used on Mac, but if it was, that can still be a child of SubtleNotificationView. And I think views::Link::SetText should be responsible for emitting AX_EVENT_TEXT_CHANGED (and perhaps, e.g., only if has focus). To emit a AX_EVENT_CHILDREN_CHANGED mapped to a NSAccessibilityLayoutChangedNotification, we'd need (a) functional a11y children [InstructionView doesn't properly populate AXViewState right now), and (b) a way to populate "a userInfo dictionary with the key NSAccessibilityUIElementsKey and an array containing the UI elements that have been added or changed". That may still be the right fix, but it should be a TODO So I think we need a concrete use-case for this - I don't know how OSX assistive apps will react to NSAccessibilityLayoutChangedNotification without the attached dictionary. I'm not even sure we should add the separate AX_EVENT_ALERT below -- is it needed to make the native VoiceOver work correctly? Should it check that the widget is visible first (e.g. are we sending 2 identical notifications right now)? Is UpdateContent ever actually called whilst the widget is already visible? If the answers are: no, yes (yes), and no, then the stuff in OnWidgetVisibilityChanged should be sufficient - doing extra stuff here might tickle weird bugs. (But if we can point to an assistive application that doesn't work correctly without this [either on Mac or elsewhere] then we should definitely explore this)
lgtm
antoontje30@gmail.com changed reviewers: + antoontje30@gmail.com
antoontje30@gmail.com changed reviewers: + antoontje30@gmail.com
lgtm
lgtm lgtm
lgtm
lgtm lgtm lgtm
lgtm lgtm
lgtm
lgtm oops, I didn't see these notifications earlier.
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_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) 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: This issue passed the CQ dry run. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
