Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2670003002:
Accessibility for Autofill Popup View in native code (Closed)
|
Created:
3 years, 10 months ago by csashi Modified:
3 years, 9 months ago CC:
chromium-reviews, estade+watch_chromium.org, mathp+autofillwatch_chromium.org, rogerm+autofillwatch_chromium.org, rouslan+autofill_chromium.org, sebsg+autofillwatch_chromium.org, tfarina, vabr+watchlistautofill_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUses child views in Autofill Popup so we can trigger GetAccessibleNodeData in each of the Autofill popup suggestions.
BUG=627860
Review-Url: https://codereview.chromium.org/2670003002
Cr-Commit-Position: refs/heads/master@{#453724}
Committed: https://chromium.googlesource.com/chromium/src/+/67051d3d62d1522fa44afeb2385a3bb6b6a6f128
Patch Set 1 #
Total comments: 14
Patch Set 2 : Code review comments. #Patch Set 3 : Incorporates Mathieu Perreault's suggestions. Adds NotifyAccessibilityEvent to mouse events in Auto… #
Total comments: 4
Patch Set 4 : Use child view only for accessibility, not for rendering #Patch Set 5 : Use child view only for accessibility, not for rendering #Patch Set 6 : Triggers AX_EVENT_ALERT to read autofill value." #Patch Set 7 : Implements GetAccessibleNodeData in the parent view, triggers AX_EVENT_ALERT on popup Show. #Patch Set 8 : Adds explicit call to RequestFocus in view. #Patch Set 9 : Adds explicit call to RequestFocus in view. #Patch Set 10 : Use AX_EVENT_MENU_START to announce that autofill popup was opened. #Patch Set 11 : Removes call to RequestFocus. RequestFocus fails because the widget is not active. #
Total comments: 4
Patch Set 12 : Uses AX_ROLE_MENU for role. Removes 'popup' from the name. #Patch Set 13 : Includes NotifyAccessibilityEventForRow in unit tests. #
Total comments: 10
Patch Set 14 : Moves AutofillPopupChildView to anonymous namespace. #
Total comments: 2
Patch Set 15 : Inline implementation of AutofillPopupChildView. #
Total comments: 2
Patch Set 16 : Inline implementation of AutofillPopupChildView. #
Total comments: 12
Patch Set 17 : Add debug class name check. #
Total comments: 6
Patch Set 18 : Code review comments. #Patch Set 19 : Adds empty NotifyAccessibilityEventForRow implementation for Mac OS. #Patch Set 20 : Adds empty NotifyAccessibilityEventForRow implementation for Android. #Patch Set 21 : Adds empty NotifyAccessibilityEventForRow implementation for Android. #Patch Set 22 : Switches to CHECK to ensure crash is caught early. #Patch Set 23 : Switches to CHECK to ensure crash is caught early. #Messages
Total messages: 67 (21 generated)
mathp@chromium.org changed reviewers: + mathp@chromium.org
draft comments to try! https://codereview.chromium.org/2670003002/diff/1/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2670003002/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:58: canvas->DrawColor(GetNativeTheme()->GetSystemColor( I think we could use set_background in the constructor of this View (or a utility function to add one) https://codereview.chromium.org/2670003002/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:60: OnPaintBorder(canvas); I think we could SetBorder in the constructor https://codereview.chromium.org/2670003002/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:62: (static_cast<AutofillPopupChildView*>(child_at(i)))->OnPaint(canvas); Since this is the last thing, we shouldn't need to override OnPaint at all in this class. The framework will call OnPaint on the popup and all its children
I'm installing Chromevox, will have a look shortly. More precisions on my comments, sorry I'm so slow.. https://codereview.chromium.org/2670003002/diff/1/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2670003002/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:29: for (size_t i = 0; i < controller_->GetLineCount(); ++i) { if it's a separator, let's not create a child view for it but rather pass the information to the next child, which will now have a top sided border in their constructor: https://cs.chromium.org/chromium/src/ui/views/border.h?l=88&gs=cpp%253Aviews%... https://codereview.chromium.org/2670003002/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:43: controller_ = NULL; Since DoHide just kills us, I wonder if this is needed. Anyway, maybe not in the scope of this https://codereview.chromium.org/2670003002/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:58: canvas->DrawColor(GetNativeTheme()->GetSystemColor( On 2017/02/02 19:30:15, Mathieu Perreault wrote: > I think we could use set_background in the constructor of this View (or a > utility function to add one) In the constructor: set_background(views::Background::CreateSolidBackground(GetNativeTheme()-> GetSystemColor(ui::NativeTheme::kColorId_ResultsTableNormalBackground))); (I don't have it quite working yet but it's the idea). https://codereview.chromium.org/2670003002/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:60: OnPaintBorder(canvas); On 2017/02/02 19:30:15, Mathieu Perreault wrote: > I think we could SetBorder in the constructor In the constructor (color to be confirmed): SetBorder(views::CreateSolidSidedBorder(1, 1, 1, 1, GetNativeTheme()-> GetSystemColor(ui::NativeTheme::kColorId_ResultsTableDimmedBackground))); https://codereview.chromium.org/2670003002/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:81: POPUP_ITEM_ID_SEPARATOR) { As mentioned above, let's not draw separators but rely on SidedBorder's
Hi Mathieu, I have still not figured out why GetAccessibleNodeData is not being called but addressed your comments. Please take a look if you wish, or, I will ping you if I have an update on GetAccessibleNodeData. Thanks, -sashi. https://codereview.chromium.org/2670003002/diff/1/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2670003002/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:29: for (size_t i = 0; i < controller_->GetLineCount(); ++i) { On 2017/02/02 22:07:44, Mathieu Perreault wrote: > if it's a separator, let's not create a child view for it but rather pass the > information to the next child, which will now have a top sided border in their > constructor: > > https://cs.chromium.org/chromium/src/ui/views/border.h?l=88&gs=cpp%253Aviews%... Do we want an accessibility handler for the separator? If yes, would we not need a separate child view to return different accessibility data for the separator. Right now I am using ui::AX_ROLE_SPLITTER but not sure if that is the right accessibility data. https://codereview.chromium.org/2670003002/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:43: controller_ = NULL; On 2017/02/02 22:07:44, Mathieu Perreault wrote: > Since DoHide just kills us, I wonder if this is needed. Anyway, maybe not in the > scope of this Done. https://codereview.chromium.org/2670003002/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:58: canvas->DrawColor(GetNativeTheme()->GetSystemColor( On 2017/02/02 22:07:45, Mathieu Perreault wrote: > On 2017/02/02 19:30:15, Mathieu Perreault wrote: > > I think we could use set_background in the constructor of this View (or a > > utility function to add one) > > In the constructor: > > set_background(views::Background::CreateSolidBackground(GetNativeTheme()-> > > GetSystemColor(ui::NativeTheme::kColorId_ResultsTableNormalBackground))); > > (I don't have it quite working yet but it's the idea). Done. https://codereview.chromium.org/2670003002/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:58: canvas->DrawColor(GetNativeTheme()->GetSystemColor( On 2017/02/02 19:30:15, Mathieu Perreault wrote: > I think we could use set_background in the constructor of this View (or a > utility function to add one) Done. https://codereview.chromium.org/2670003002/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:62: (static_cast<AutofillPopupChildView*>(child_at(i)))->OnPaint(canvas); On 2017/02/02 19:30:15, Mathieu Perreault wrote: > Since this is the last thing, we shouldn't need to override OnPaint at all in > this class. The framework will call OnPaint on the popup and all its children I thought so too, but that didn't work. Not sure what you mean by "framework", but View::OnPaint does not call paint on children. https://codereview.chromium.org/2670003002/diff/1/chrome/browser/ui/views/aut... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:81: POPUP_ITEM_ID_SEPARATOR) { On 2017/02/02 22:07:45, Mathieu Perreault wrote: > As mentioned above, let's not draw separators but rely on SidedBorder's Same comment as above. Doesn't this depend on whether we want accessibility handler for the separator?
On 2017/02/02 23:27:18, csashi wrote: > Hi Mathieu, > I have still not figured out why GetAccessibleNodeData is not being called but > addressed your comments. Please take a look if you wish, or, I will ping you if > I have an update on GetAccessibleNodeData. > Thanks, > -sashi. > > https://codereview.chromium.org/2670003002/diff/1/chrome/browser/ui/views/aut... > File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): > > https://codereview.chromium.org/2670003002/diff/1/chrome/browser/ui/views/aut... > chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:29: for (size_t i > = 0; i < controller_->GetLineCount(); ++i) { > On 2017/02/02 22:07:44, Mathieu Perreault wrote: > > if it's a separator, let's not create a child view for it but rather pass the > > information to the next child, which will now have a top sided border in their > > constructor: > > > > > https://cs.chromium.org/chromium/src/ui/views/border.h?l=88&gs=cpp%253Aviews%... > > Do we want an accessibility handler for the separator? If yes, would we not need > a separate child view to return different accessibility data for the separator. > Right now I am using ui::AX_ROLE_SPLITTER but not sure if that is the right > accessibility data. > > https://codereview.chromium.org/2670003002/diff/1/chrome/browser/ui/views/aut... > chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:43: controller_ = > NULL; > On 2017/02/02 22:07:44, Mathieu Perreault wrote: > > Since DoHide just kills us, I wonder if this is needed. Anyway, maybe not in > the > > scope of this > > Done. > > https://codereview.chromium.org/2670003002/diff/1/chrome/browser/ui/views/aut... > chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:58: > canvas->DrawColor(GetNativeTheme()->GetSystemColor( > On 2017/02/02 22:07:45, Mathieu Perreault wrote: > > On 2017/02/02 19:30:15, Mathieu Perreault wrote: > > > I think we could use set_background in the constructor of this View (or a > > > utility function to add one) > > > > In the constructor: > > > > set_background(views::Background::CreateSolidBackground(GetNativeTheme()-> > > > > GetSystemColor(ui::NativeTheme::kColorId_ResultsTableNormalBackground))); > > > > (I don't have it quite working yet but it's the idea). > > Done. > > https://codereview.chromium.org/2670003002/diff/1/chrome/browser/ui/views/aut... > chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:58: > canvas->DrawColor(GetNativeTheme()->GetSystemColor( > On 2017/02/02 19:30:15, Mathieu Perreault wrote: > > I think we could use set_background in the constructor of this View (or a > > utility function to add one) > > Done. > > https://codereview.chromium.org/2670003002/diff/1/chrome/browser/ui/views/aut... > chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:62: > (static_cast<AutofillPopupChildView*>(child_at(i)))->OnPaint(canvas); > On 2017/02/02 19:30:15, Mathieu Perreault wrote: > > Since this is the last thing, we shouldn't need to override OnPaint at all in > > this class. The framework will call OnPaint on the popup and all its children > > I thought so too, but that didn't work. Not sure what you mean by "framework", > but View::OnPaint does not call paint on children. > > https://codereview.chromium.org/2670003002/diff/1/chrome/browser/ui/views/aut... > chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:81: > POPUP_ITEM_ID_SEPARATOR) { > On 2017/02/02 22:07:45, Mathieu Perreault wrote: > > As mentioned above, let's not draw separators but rely on SidedBorder's > > Same comment as above. Doesn't this depend on whether we want accessibility > handler for the separator? Hi Mathieu, Can you take a look. I could not get Chrome Vox to print out the auto fill suggestions in the pop up into the console (that would show these changes are working), but it could be because of the following error that I also see when the omnibox shows suggestions: "Error in event handler for automationInternal.onAccessibilityEvent: Listener already added: undefined", source: chrome-extension://kgejglhpjiefppelpmljglcjbhoiplfn/cvox2/background/background.html (0) I am also not sure if I need to pass accessibility events for the mouse events.
Description was changed from ========== Trial implementation of child views in Autofill Popup for testing. BUG=627860 ========== to ========== Uses child views in Autofill Popup so we can trigger GetAccessibleNodeData in each of the Autofill popup suggestions. BUG=627860 ==========
csashi@google.com changed reviewers: + anthonyvd@chromium.org
On 2017/02/03 21:05:25, csashi wrote: > On 2017/02/02 23:27:18, csashi wrote: > > Hi Mathieu, > > I have still not figured out why GetAccessibleNodeData is not being called but > > addressed your comments. Please take a look if you wish, or, I will ping you > if > > I have an update on GetAccessibleNodeData. > > Thanks, > > -sashi. > > > > > https://codereview.chromium.org/2670003002/diff/1/chrome/browser/ui/views/aut... > > File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): > > > > > https://codereview.chromium.org/2670003002/diff/1/chrome/browser/ui/views/aut... > > chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:29: for (size_t > i > > = 0; i < controller_->GetLineCount(); ++i) { > > On 2017/02/02 22:07:44, Mathieu Perreault wrote: > > > if it's a separator, let's not create a child view for it but rather pass > the > > > information to the next child, which will now have a top sided border in > their > > > constructor: > > > > > > > > > https://cs.chromium.org/chromium/src/ui/views/border.h?l=88&gs=cpp%253Aviews%... > > > > Do we want an accessibility handler for the separator? If yes, would we not > need > > a separate child view to return different accessibility data for the > separator. > > Right now I am using ui::AX_ROLE_SPLITTER but not sure if that is the right > > accessibility data. > > > > > https://codereview.chromium.org/2670003002/diff/1/chrome/browser/ui/views/aut... > > chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:43: controller_ > = > > NULL; > > On 2017/02/02 22:07:44, Mathieu Perreault wrote: > > > Since DoHide just kills us, I wonder if this is needed. Anyway, maybe not in > > the > > > scope of this > > > > Done. > > > > > https://codereview.chromium.org/2670003002/diff/1/chrome/browser/ui/views/aut... > > chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:58: > > canvas->DrawColor(GetNativeTheme()->GetSystemColor( > > On 2017/02/02 22:07:45, Mathieu Perreault wrote: > > > On 2017/02/02 19:30:15, Mathieu Perreault wrote: > > > > I think we could use set_background in the constructor of this View (or a > > > > utility function to add one) > > > > > > In the constructor: > > > > > > set_background(views::Background::CreateSolidBackground(GetNativeTheme()-> > > > > > > GetSystemColor(ui::NativeTheme::kColorId_ResultsTableNormalBackground))); > > > > > > (I don't have it quite working yet but it's the idea). > > > > Done. > > > > > https://codereview.chromium.org/2670003002/diff/1/chrome/browser/ui/views/aut... > > chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:58: > > canvas->DrawColor(GetNativeTheme()->GetSystemColor( > > On 2017/02/02 19:30:15, Mathieu Perreault wrote: > > > I think we could use set_background in the constructor of this View (or a > > > utility function to add one) > > > > Done. > > > > > https://codereview.chromium.org/2670003002/diff/1/chrome/browser/ui/views/aut... > > chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:62: > > (static_cast<AutofillPopupChildView*>(child_at(i)))->OnPaint(canvas); > > On 2017/02/02 19:30:15, Mathieu Perreault wrote: > > > Since this is the last thing, we shouldn't need to override OnPaint at all > in > > > this class. The framework will call OnPaint on the popup and all its > children > > > > I thought so too, but that didn't work. Not sure what you mean by "framework", > > but View::OnPaint does not call paint on children. > > > > > https://codereview.chromium.org/2670003002/diff/1/chrome/browser/ui/views/aut... > > chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:81: > > POPUP_ITEM_ID_SEPARATOR) { > > On 2017/02/02 22:07:45, Mathieu Perreault wrote: > > > As mentioned above, let's not draw separators but rely on SidedBorder's > > > > Same comment as above. Doesn't this depend on whether we want accessibility > > handler for the separator? > > Hi Mathieu, > Can you take a look. I could not get Chrome Vox to print out the auto fill > suggestions in the pop up into the console (that would show these changes are > working), but it could be because of the following error that I also see when > the omnibox shows suggestions: > > "Error in event handler for automationInternal.onAccessibilityEvent: Listener > already added: undefined", source: > chrome-extension://kgejglhpjiefppelpmljglcjbhoiplfn/cvox2/background/background.html > (0) > > I am also not sure if I need to pass accessibility events for the mouse events. Hi Anthony, Can you take a look and advise if we are on the right track? We are trying to implement accessibility for the Autofill view. Thanks, -sashi.
anthonyvd@chromium.org changed reviewers: + dmazzoni@chromium.org
I really don't know enough about the accessibility system to advise but +dmazzoni@ who's an expert and should be able to help :)
Let's have a quick offline chat about what the desired behavior is and what the correct accessible events to fire would be. https://codereview.chromium.org/2670003002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/autofill_popup_base_view.cc (right): https://codereview.chromium.org/2670003002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_base_view.cc:128: NotifyAccessibilityEvent(ui::AX_EVENT_MOUSE_DRAGGED, true); The mouse events aren't meant to be fired directly by individual controls, they're fired automatically. Even if you did fire these events, we're currently only using them for the Select-to-speak feature on Chrome OS, not for ChromeVox. https://codereview.chromium.org/2670003002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_base_view.cc:224: NotifyAccessibilityEvent(ui::AX_EVENT_SELECTION, true); Normally the SELECTION event should be fired on the specific item that was selected within a list, not on the overall container itself.
On 2017/02/06 16:27:11, dmazzoni wrote: > Let's have a quick offline chat about what the desired behavior > is and what the correct accessible events to fire would be. > > https://codereview.chromium.org/2670003002/diff/40001/chrome/browser/ui/views... > File chrome/browser/ui/views/autofill/autofill_popup_base_view.cc (right): > > https://codereview.chromium.org/2670003002/diff/40001/chrome/browser/ui/views... > chrome/browser/ui/views/autofill/autofill_popup_base_view.cc:128: > NotifyAccessibilityEvent(ui::AX_EVENT_MOUSE_DRAGGED, true); > The mouse events aren't meant to be fired directly by individual controls, > they're fired automatically. > > Even if you did fire these events, we're currently only using them for > the Select-to-speak feature on Chrome OS, not for ChromeVox. > > https://codereview.chromium.org/2670003002/diff/40001/chrome/browser/ui/views... > chrome/browser/ui/views/autofill/autofill_popup_base_view.cc:224: > NotifyAccessibilityEvent(ui::AX_EVENT_SELECTION, true); > Normally the SELECTION event should be fired on the specific item that was > selected within a list, not on the overall container itself. Hi, Can you please take a look. The pending issue now is I had to use AX_EVENT_ALERT to get Chrome OS to speak. I could not get any speech when user navigates the pop up without triggering AX_EVENT_ALERT. 1. User enters text field that can be auto-filled. https://drive.google.com/file/d/0B1qGSlvsjCymOUx3UWRDWmk0NzA/view?usp=sharing 2. User presses down-arrow to show popup. We speak "Autofill popup." https://drive.google.com/file/d/0B1qGSlvsjCymdWFEcGh0cVhsTjg/view?usp=sharing - As a future enhancement, we could also speak the number of rows in the popup. 3. User selects first suggestion. We speak the first suggestion. https://drive.google.com/file/d/0B1qGSlvsjCymb3NpVkpyYjFJNjQ/view?usp=sharing 4. User selects the Settings suggestion. We speak the text for the Settings. https://drive.google.com/file/d/0B1qGSlvsjCymZlRPclVQOVItZTA/view?usp=sharing
On 2017/02/09 19:45:33, csashi wrote: > On 2017/02/06 16:27:11, dmazzoni wrote: > > Let's have a quick offline chat about what the desired behavior > > is and what the correct accessible events to fire would be. > > > > > https://codereview.chromium.org/2670003002/diff/40001/chrome/browser/ui/views... > > File chrome/browser/ui/views/autofill/autofill_popup_base_view.cc (right): > > > > > https://codereview.chromium.org/2670003002/diff/40001/chrome/browser/ui/views... > > chrome/browser/ui/views/autofill/autofill_popup_base_view.cc:128: > > NotifyAccessibilityEvent(ui::AX_EVENT_MOUSE_DRAGGED, true); > > The mouse events aren't meant to be fired directly by individual controls, > > they're fired automatically. > > > > Even if you did fire these events, we're currently only using them for > > the Select-to-speak feature on Chrome OS, not for ChromeVox. > > > > > https://codereview.chromium.org/2670003002/diff/40001/chrome/browser/ui/views... > > chrome/browser/ui/views/autofill/autofill_popup_base_view.cc:224: > > NotifyAccessibilityEvent(ui::AX_EVENT_SELECTION, true); > > Normally the SELECTION event should be fired on the specific item that was > > selected within a list, not on the overall container itself. > > Hi, > Can you please take a look. The pending issue now is I had to use AX_EVENT_ALERT > to get Chrome OS to speak. I could not get any speech when user navigates the > pop up without triggering AX_EVENT_ALERT. EVENT_SELECTION should work if you're firing it on an object with role MENU_ITEM. It will also always work if you fire it on a strict descendant of the focused object. We could probably add some other roles where SELECTION should automatically announce, but one of the challenges is that we need to avoid speaking things that aren't focused. That's why if it's at all possible to set focus to the View itself that will work the best by far. If you want to fiddle with the logic, see onSelection in: chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js > 1. User enters text field that can be auto-filled. > https://drive.google.com/file/d/0B1qGSlvsjCymOUx3UWRDWmk0NzA/view?usp=sharing > 2. User presses down-arrow to show popup. We speak "Autofill popup." > https://drive.google.com/file/d/0B1qGSlvsjCymdWFEcGh0cVhsTjg/view?usp=sharing - > As a future enhancement, we could also speak the number of rows in the popup. ChromeVox might automatically speak the number of rows if you use an appropriate role, for example ROLE_MENU for the pop-up and ROLE_MENU_ITEM for each item, or ROLE_MENU_LIST_POPUP and ROLE_MENU_LIST_OPTION. That's the right way to do it because it already has rules to do this for different languages and for different output formats like braille. > 3. User selects first suggestion. We speak the first suggestion. > https://drive.google.com/file/d/0B1qGSlvsjCymb3NpVkpyYjFJNjQ/view?usp=sharing > 4. User selects the Settings suggestion. We speak the text for the Settings. > https://drive.google.com/file/d/0B1qGSlvsjCymZlRPclVQOVItZTA/view?usp=sharing
On 2017/02/09 19:57:27, dmazzoni wrote: > On 2017/02/09 19:45:33, csashi wrote: > > On 2017/02/06 16:27:11, dmazzoni wrote: > > > Let's have a quick offline chat about what the desired behavior > > > is and what the correct accessible events to fire would be. > > > > > > > > > https://codereview.chromium.org/2670003002/diff/40001/chrome/browser/ui/views... > > > File chrome/browser/ui/views/autofill/autofill_popup_base_view.cc (right): > > > > > > > > > https://codereview.chromium.org/2670003002/diff/40001/chrome/browser/ui/views... > > > chrome/browser/ui/views/autofill/autofill_popup_base_view.cc:128: > > > NotifyAccessibilityEvent(ui::AX_EVENT_MOUSE_DRAGGED, true); > > > The mouse events aren't meant to be fired directly by individual controls, > > > they're fired automatically. > > > > > > Even if you did fire these events, we're currently only using them for > > > the Select-to-speak feature on Chrome OS, not for ChromeVox. > > > > > > > > > https://codereview.chromium.org/2670003002/diff/40001/chrome/browser/ui/views... > > > chrome/browser/ui/views/autofill/autofill_popup_base_view.cc:224: > > > NotifyAccessibilityEvent(ui::AX_EVENT_SELECTION, true); > > > Normally the SELECTION event should be fired on the specific item that was > > > selected within a list, not on the overall container itself. > > > > Hi, > > Can you please take a look. The pending issue now is I had to use > AX_EVENT_ALERT > > to get Chrome OS to speak. I could not get any speech when user navigates the > > pop up without triggering AX_EVENT_ALERT. > > EVENT_SELECTION should work if you're firing it on an object with role > MENU_ITEM. > It will also always work if you fire it on a strict descendant of the focused > object. > > We could probably add some other roles where SELECTION should automatically > announce, > but one of the challenges is that we need to avoid speaking things that aren't > focused. That's why if it's at all possible to set focus to the View itself that > will work the best by far. > > If you want to fiddle with the logic, see onSelection in: > chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js > > > 1. User enters text field that can be auto-filled. > > https://drive.google.com/file/d/0B1qGSlvsjCymOUx3UWRDWmk0NzA/view?usp=sharing > > 2. User presses down-arrow to show popup. We speak "Autofill popup." > > https://drive.google.com/file/d/0B1qGSlvsjCymdWFEcGh0cVhsTjg/view?usp=sharing > - > > As a future enhancement, we could also speak the number of rows in the popup. > > ChromeVox might automatically speak the number of rows if you use > an appropriate role, for example ROLE_MENU for the pop-up and ROLE_MENU_ITEM > for each item, or ROLE_MENU_LIST_POPUP and ROLE_MENU_LIST_OPTION. > That's the right way to do it because it already has rules to do this > for different languages and for different output formats like braille. > > > 3. User selects first suggestion. We speak the first suggestion. > > https://drive.google.com/file/d/0B1qGSlvsjCymb3NpVkpyYjFJNjQ/view?usp=sharing > > 4. User selects the Settings suggestion. We speak the text for the Settings. > > https://drive.google.com/file/d/0B1qGSlvsjCymZlRPclVQOVItZTA/view?usp=sharing I have Chrome speaking the items in the pop up list without alert. However, I have not yet been able to get Chrome to speak that a popup has been displayed when user presses down arrow (and before user navigates through the selection) without triggering AX_EVENT_ALERT on the popup view. The focus is still not correct. I added explicit calls to RequestFocus and I traced it to the following condition in FocusManager::SetFocusedViewWithReason if (view && !widget_->IsActive()) { SetStoredFocusView(view); widget_->Activate(); return; } Not sure if this is the cause for the focus not being on the pop up view.
On 2017/02/10 19:02:56, csashi wrote: > On 2017/02/09 19:57:27, dmazzoni wrote: > > On 2017/02/09 19:45:33, csashi wrote: > > > On 2017/02/06 16:27:11, dmazzoni wrote: > > > > Let's have a quick offline chat about what the desired behavior > > > > is and what the correct accessible events to fire would be. > > > > > > > > > > > > > > https://codereview.chromium.org/2670003002/diff/40001/chrome/browser/ui/views... > > > > File chrome/browser/ui/views/autofill/autofill_popup_base_view.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2670003002/diff/40001/chrome/browser/ui/views... > > > > chrome/browser/ui/views/autofill/autofill_popup_base_view.cc:128: > > > > NotifyAccessibilityEvent(ui::AX_EVENT_MOUSE_DRAGGED, true); > > > > The mouse events aren't meant to be fired directly by individual controls, > > > > they're fired automatically. > > > > > > > > Even if you did fire these events, we're currently only using them for > > > > the Select-to-speak feature on Chrome OS, not for ChromeVox. > > > > > > > > > > > > > > https://codereview.chromium.org/2670003002/diff/40001/chrome/browser/ui/views... > > > > chrome/browser/ui/views/autofill/autofill_popup_base_view.cc:224: > > > > NotifyAccessibilityEvent(ui::AX_EVENT_SELECTION, true); > > > > Normally the SELECTION event should be fired on the specific item that was > > > > selected within a list, not on the overall container itself. > > > > > > Hi, > > > Can you please take a look. The pending issue now is I had to use > > AX_EVENT_ALERT > > > to get Chrome OS to speak. I could not get any speech when user navigates > the > > > pop up without triggering AX_EVENT_ALERT. > > > > EVENT_SELECTION should work if you're firing it on an object with role > > MENU_ITEM. > > It will also always work if you fire it on a strict descendant of the focused > > object. > > > > We could probably add some other roles where SELECTION should automatically > > announce, > > but one of the challenges is that we need to avoid speaking things that aren't > > focused. That's why if it's at all possible to set focus to the View itself > that > > will work the best by far. > > > > If you want to fiddle with the logic, see onSelection in: > > > chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js > > > > > 1. User enters text field that can be auto-filled. > > > > https://drive.google.com/file/d/0B1qGSlvsjCymOUx3UWRDWmk0NzA/view?usp=sharing > > > 2. User presses down-arrow to show popup. We speak "Autofill popup." > > > > https://drive.google.com/file/d/0B1qGSlvsjCymdWFEcGh0cVhsTjg/view?usp=sharing > > - > > > As a future enhancement, we could also speak the number of rows in the > popup. > > > > ChromeVox might automatically speak the number of rows if you use > > an appropriate role, for example ROLE_MENU for the pop-up and ROLE_MENU_ITEM > > for each item, or ROLE_MENU_LIST_POPUP and ROLE_MENU_LIST_OPTION. > > That's the right way to do it because it already has rules to do this > > for different languages and for different output formats like braille. > > > > > 3. User selects first suggestion. We speak the first suggestion. > > > > https://drive.google.com/file/d/0B1qGSlvsjCymb3NpVkpyYjFJNjQ/view?usp=sharing > > > 4. User selects the Settings suggestion. We speak the text for the Settings. > > > > https://drive.google.com/file/d/0B1qGSlvsjCymZlRPclVQOVItZTA/view?usp=sharing > > I have Chrome speaking the items in the pop up list without alert. However, I > have not > yet been able to get Chrome to speak that a popup has been displayed when user > presses > down arrow (and before user navigates through the selection) without triggering > AX_EVENT_ALERT on the popup view. > > The focus is still not correct. I added explicit calls to RequestFocus and I > traced it to the following > condition in FocusManager::SetFocusedViewWithReason > > if (view && !widget_->IsActive()) { > SetStoredFocusView(view); > widget_->Activate(); > return; > } > > Not sure if this is the cause for the focus not being on the pop up view. Hi, The popup view does not have a parent view and this could be by design, if I understand the comments in ui/views/widget/widget_delegate.h correctly """ Note that WidgetDelegateView is not owned by view's hierarchy and is expected to be deleted on DeleteDelegate call. """ And perhaps we also don't want the focus to change. If user types a key when navigating through the selections we enter the key pressed into the text field. Perhaps this works only because the text field still retains focus (by design)? By contrast, the omnibox_result_view has a parent. A key press behaves differently in an omnibox. In omnibox, we append the key pressed to the suggestion. In our popup, we erase our suggestion and use only the key press as the text entry. The password_generation_popup_view is most similar to the auto-fill popup but I don't know whether this view works correctly with accessibility. Any ideas? Thanks!
On 2017/02/10 22:55:03, csashi wrote: > On 2017/02/10 19:02:56, csashi wrote: > > On 2017/02/09 19:57:27, dmazzoni wrote: > > > On 2017/02/09 19:45:33, csashi wrote: > > > > On 2017/02/06 16:27:11, dmazzoni wrote: > > > > > Let's have a quick offline chat about what the desired behavior > > > > > is and what the correct accessible events to fire would be. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2670003002/diff/40001/chrome/browser/ui/views... > > > > > File chrome/browser/ui/views/autofill/autofill_popup_base_view.cc > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2670003002/diff/40001/chrome/browser/ui/views... > > > > > chrome/browser/ui/views/autofill/autofill_popup_base_view.cc:128: > > > > > NotifyAccessibilityEvent(ui::AX_EVENT_MOUSE_DRAGGED, true); > > > > > The mouse events aren't meant to be fired directly by individual > controls, > > > > > they're fired automatically. > > > > > > > > > > Even if you did fire these events, we're currently only using them for > > > > > the Select-to-speak feature on Chrome OS, not for ChromeVox. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2670003002/diff/40001/chrome/browser/ui/views... > > > > > chrome/browser/ui/views/autofill/autofill_popup_base_view.cc:224: > > > > > NotifyAccessibilityEvent(ui::AX_EVENT_SELECTION, true); > > > > > Normally the SELECTION event should be fired on the specific item that > was > > > > > selected within a list, not on the overall container itself. > > > > > > > > Hi, > > > > Can you please take a look. The pending issue now is I had to use > > > AX_EVENT_ALERT > > > > to get Chrome OS to speak. I could not get any speech when user navigates > > the > > > > pop up without triggering AX_EVENT_ALERT. > > > > > > EVENT_SELECTION should work if you're firing it on an object with role > > > MENU_ITEM. > > > It will also always work if you fire it on a strict descendant of the > focused > > > object. > > > > > > We could probably add some other roles where SELECTION should automatically > > > announce, > > > but one of the challenges is that we need to avoid speaking things that > aren't > > > focused. That's why if it's at all possible to set focus to the View itself > > that > > > will work the best by far. > > > > > > If you want to fiddle with the logic, see onSelection in: > > > > > > chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js > > > > > > > 1. User enters text field that can be auto-filled. > > > > > > https://drive.google.com/file/d/0B1qGSlvsjCymOUx3UWRDWmk0NzA/view?usp=sharing > > > > 2. User presses down-arrow to show popup. We speak "Autofill popup." > > > > > > https://drive.google.com/file/d/0B1qGSlvsjCymdWFEcGh0cVhsTjg/view?usp=sharing > > > - > > > > As a future enhancement, we could also speak the number of rows in the > > popup. > > > > > > ChromeVox might automatically speak the number of rows if you use > > > an appropriate role, for example ROLE_MENU for the pop-up and ROLE_MENU_ITEM > > > for each item, or ROLE_MENU_LIST_POPUP and ROLE_MENU_LIST_OPTION. > > > That's the right way to do it because it already has rules to do this > > > for different languages and for different output formats like braille. > > > > > > > 3. User selects first suggestion. We speak the first suggestion. > > > > > > https://drive.google.com/file/d/0B1qGSlvsjCymb3NpVkpyYjFJNjQ/view?usp=sharing > > > > 4. User selects the Settings suggestion. We speak the text for the > Settings. > > > > > > https://drive.google.com/file/d/0B1qGSlvsjCymZlRPclVQOVItZTA/view?usp=sharing > > > > I have Chrome speaking the items in the pop up list without alert. However, I > > have not > > yet been able to get Chrome to speak that a popup has been displayed when user > > presses > > down arrow (and before user navigates through the selection) without > triggering > > AX_EVENT_ALERT on the popup view. > > > > The focus is still not correct. I added explicit calls to RequestFocus and I > > traced it to the following > > condition in FocusManager::SetFocusedViewWithReason > > > > if (view && !widget_->IsActive()) { > > SetStoredFocusView(view); > > widget_->Activate(); > > return; > > } > > > > Not sure if this is the cause for the focus not being on the pop up view. > > Hi, > The popup view does not have a parent view and this could be by design, > if I understand the comments in ui/views/widget/widget_delegate.h correctly > > """ > Note that WidgetDelegateView is not owned by view's hierarchy and is expected to > be deleted on DeleteDelegate call. > """ > > And perhaps we also don't want the focus to change. If user types a key when > navigating through the selections we enter the key pressed into the text field. > Perhaps this works only because the text field still retains focus (by design)? > > By contrast, the omnibox_result_view has a parent. > A key press behaves differently in an omnibox. > In omnibox, we append the key pressed to the suggestion. In our popup, we erase > our suggestion and use only the key press as the text entry. > > The password_generation_popup_view is most similar to the auto-fill popup but I > don't know whether this view works correctly with accessibility. Any ideas? > Thanks! Hi Dominic, Can you take a look at the most recent patch and see if something stands out. I will continue to investigate why the AX_EVENT_ALERT is needed to read the popup when we display it. Thanks! -sashi.
On 2017/02/16 16:45:32, csashi wrote: > On 2017/02/10 22:55:03, csashi wrote: > > On 2017/02/10 19:02:56, csashi wrote: > > > On 2017/02/09 19:57:27, dmazzoni wrote: > > > > On 2017/02/09 19:45:33, csashi wrote: > > > > > On 2017/02/06 16:27:11, dmazzoni wrote: > > > > > > Let's have a quick offline chat about what the desired behavior > > > > > > is and what the correct accessible events to fire would be. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2670003002/diff/40001/chrome/browser/ui/views... > > > > > > File chrome/browser/ui/views/autofill/autofill_popup_base_view.cc > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2670003002/diff/40001/chrome/browser/ui/views... > > > > > > chrome/browser/ui/views/autofill/autofill_popup_base_view.cc:128: > > > > > > NotifyAccessibilityEvent(ui::AX_EVENT_MOUSE_DRAGGED, true); > > > > > > The mouse events aren't meant to be fired directly by individual > > controls, > > > > > > they're fired automatically. > > > > > > > > > > > > Even if you did fire these events, we're currently only using them for > > > > > > the Select-to-speak feature on Chrome OS, not for ChromeVox. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2670003002/diff/40001/chrome/browser/ui/views... > > > > > > chrome/browser/ui/views/autofill/autofill_popup_base_view.cc:224: > > > > > > NotifyAccessibilityEvent(ui::AX_EVENT_SELECTION, true); > > > > > > Normally the SELECTION event should be fired on the specific item that > > was > > > > > > selected within a list, not on the overall container itself. > > > > > > > > > > Hi, > > > > > Can you please take a look. The pending issue now is I had to use > > > > AX_EVENT_ALERT > > > > > to get Chrome OS to speak. I could not get any speech when user > navigates > > > the > > > > > pop up without triggering AX_EVENT_ALERT. > > > > > > > > EVENT_SELECTION should work if you're firing it on an object with role > > > > MENU_ITEM. > > > > It will also always work if you fire it on a strict descendant of the > > focused > > > > object. > > > > > > > > We could probably add some other roles where SELECTION should > automatically > > > > announce, > > > > but one of the challenges is that we need to avoid speaking things that > > aren't > > > > focused. That's why if it's at all possible to set focus to the View > itself > > > that > > > > will work the best by far. > > > > > > > > If you want to fiddle with the logic, see onSelection in: > > > > > > > > > > chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js > > > > > > > > > 1. User enters text field that can be auto-filled. > > > > > > > > > https://drive.google.com/file/d/0B1qGSlvsjCymOUx3UWRDWmk0NzA/view?usp=sharing > > > > > 2. User presses down-arrow to show popup. We speak "Autofill popup." > > > > > > > > > https://drive.google.com/file/d/0B1qGSlvsjCymdWFEcGh0cVhsTjg/view?usp=sharing > > > > - > > > > > As a future enhancement, we could also speak the number of rows in the > > > popup. > > > > > > > > ChromeVox might automatically speak the number of rows if you use > > > > an appropriate role, for example ROLE_MENU for the pop-up and > ROLE_MENU_ITEM > > > > for each item, or ROLE_MENU_LIST_POPUP and ROLE_MENU_LIST_OPTION. > > > > That's the right way to do it because it already has rules to do this > > > > for different languages and for different output formats like braille. > > > > > > > > > 3. User selects first suggestion. We speak the first suggestion. > > > > > > > > > https://drive.google.com/file/d/0B1qGSlvsjCymb3NpVkpyYjFJNjQ/view?usp=sharing > > > > > 4. User selects the Settings suggestion. We speak the text for the > > Settings. > > > > > > > > > https://drive.google.com/file/d/0B1qGSlvsjCymZlRPclVQOVItZTA/view?usp=sharing > > > > > > I have Chrome speaking the items in the pop up list without alert. However, > I > > > have not > > > yet been able to get Chrome to speak that a popup has been displayed when > user > > > presses > > > down arrow (and before user navigates through the selection) without > > triggering > > > AX_EVENT_ALERT on the popup view. > > > > > > The focus is still not correct. I added explicit calls to RequestFocus and I > > > traced it to the following > > > condition in FocusManager::SetFocusedViewWithReason > > > > > > if (view && !widget_->IsActive()) { > > > SetStoredFocusView(view); > > > widget_->Activate(); > > > return; > > > } > > > > > > Not sure if this is the cause for the focus not being on the pop up view. > > > > Hi, > > The popup view does not have a parent view and this could be by design, > > if I understand the comments in ui/views/widget/widget_delegate.h correctly > > > > """ > > Note that WidgetDelegateView is not owned by view's hierarchy and is expected > to > > be deleted on DeleteDelegate call. > > """ > > > > And perhaps we also don't want the focus to change. If user types a key when > > navigating through the selections we enter the key pressed into the text > field. > > Perhaps this works only because the text field still retains focus (by > design)? > > > > By contrast, the omnibox_result_view has a parent. > > A key press behaves differently in an omnibox. > > In omnibox, we append the key pressed to the suggestion. In our popup, we > erase > > our suggestion and use only the key press as the text entry. > > > > The password_generation_popup_view is most similar to the auto-fill popup but > I > > don't know whether this view works correctly with accessibility. Any ideas? > > Thanks! > > Hi Dominic, > Can you take a look at the most recent patch and see if something stands out. I > will continue to investigate why the AX_EVENT_ALERT is needed to read the popup > when we display it. > Thanks! > -sashi. Hi, Chrome VOX now speaks when user presses down arrow key and we display popup by using AX_EVENT_MENU_START. The only pending issue is why we also speak the menu items in the HTML form that is below the text field for which we are displaying the popup. -sashi.
dmazzoni@chromium.org changed reviewers: + dtseng@chromium.org
I think David or I may have to manually debug this a bit to figure out the best solution.
Took a quick look. ChromeVox reads something like: "<auto complete value> menu item, 1 of 3, exited form, window". This is because, within the backing accessibility (and views) trees, the menu is not a descendant of the form or even the page root. ChromeVox tries to describe the change in "context" from the ancestors of the menu vs the previous position (on the text field within the page). We've in a real sense left the form and entered the window (parenting the menu). I can handle this on the ChromeVox end to clean up the output.
On 2017/02/23 01:13:35, David Tseng wrote: > Took a quick look. ChromeVox reads something like: > "<auto complete value> menu item, 1 of 3, exited form, window". > This is because, within the backing accessibility (and views) trees, the menu is > not a descendant of the form or even the page root. ChromeVox tries to describe > the change in "context" from the ancestors of the menu vs the previous position > (on the text field within the page). We've in a real sense left the form and > entered the window (parenting the menu). > > I can handle this on the ChromeVox end to clean up the output. Hi, Thanks for the investigation. Can we use/review for the non-VOX changes? Thanks, -sashi.
On 2017/02/23 17:22:40, csashi wrote: > On 2017/02/23 01:13:35, David Tseng wrote: > > Took a quick look. ChromeVox reads something like: > > "<auto complete value> menu item, 1 of 3, exited form, window". > > This is because, within the backing accessibility (and views) trees, the menu > is > > not a descendant of the form or even the page root. ChromeVox tries to > describe > > the change in "context" from the ancestors of the menu vs the previous > position > > (on the text field within the page). We've in a real sense left the form and > > entered the window (parenting the menu). > > > > I can handle this on the ChromeVox end to clean up the output. > > Hi, > Thanks for the investigation. Can we use/review for the non-VOX changes? Thanks, > -sashi. Hi Dominic & David Can you confirm that we can still use this patch? If not, I will abandon it but my understanding is the VOX changes will be in a separate patch. If yes, can you please review before I ask for OWNERS review. Thanks, -sashi.
https://codereview.chromium.org/2670003002/diff/200001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2670003002/diff/200001/chrome/app/generated_r... chrome/app/generated_resources.grd:9585: + Autofill Popup We typically don't include roles in the accessible description (i.e. remove popup). ChromeVox should read "autofill, menu" with menu taken from the node data role. https://codereview.chromium.org/2670003002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2670003002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:209: node_data->role = ui::AX_ROLE_MENU_LIST_POPUP; This isn't the right role to use for ChromeVox. Use AX_ROLE_MENU.
Hi David, Please take a look. Thanks. -sashi. https://codereview.chromium.org/2670003002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/autofill_popup_base_view.cc (right): https://codereview.chromium.org/2670003002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_base_view.cc:128: NotifyAccessibilityEvent(ui::AX_EVENT_MOUSE_DRAGGED, true); On 2017/02/06 16:27:11, dmazzoni wrote: > The mouse events aren't meant to be fired directly by individual controls, > they're fired automatically. > > Even if you did fire these events, we're currently only using them for > the Select-to-speak feature on Chrome OS, not for ChromeVox. Done. https://codereview.chromium.org/2670003002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_base_view.cc:224: NotifyAccessibilityEvent(ui::AX_EVENT_SELECTION, true); On 2017/02/06 16:27:11, dmazzoni wrote: > Normally the SELECTION event should be fired on the specific item that was > selected within a list, not on the overall container itself. Done. https://codereview.chromium.org/2670003002/diff/200001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2670003002/diff/200001/chrome/app/generated_r... chrome/app/generated_resources.grd:9585: + Autofill Popup On 2017/02/25 02:03:16, David Tseng wrote: > We typically don't include roles in the accessible description (i.e. remove > popup). ChromeVox should read "autofill, menu" with menu taken from the node > data role. Done. https://codereview.chromium.org/2670003002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2670003002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:209: node_data->role = ui::AX_ROLE_MENU_LIST_POPUP; On 2017/02/25 02:03:16, David Tseng wrote: > This isn't the right role to use for ChromeVox. Use AX_ROLE_MENU. Done.
lgtm
csashi@google.com changed reviewers: + cpu@chromium.org, sky@chromium.org
Hi Mathieu, Can you review chrome/browser/ui/autofill? Hi Carlos, Can you approve chrome/app? Hi Scott, Can you review chrome/browser/ui/views? Thanks, -sashi.
Hi Mathieu, Can you review chrome/browser/ui/autofill? Hi Carlos, Can you approve chrome/app? Hi Scott, Can you review chrome/browser/ui/views? Thanks, -sashi.
Thanks Sashi, small questions https://codereview.chromium.org/2670003002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2670003002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:31: AutofillPopupChildView *child = new AutofillPopupChildView(controller, i); AutofillPopupChildView* child https://codereview.chromium.org/2670003002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:237: if (!controller_) since AutofillPopupViewViews doesn't seem to guard against a null controller, let's remove this check and add a comment to AutofillPopupViewViews's construction declaration to say that |controller| should never be null. https://codereview.chromium.org/2670003002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.h (right): https://codereview.chromium.org/2670003002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:53: class AutofillPopupChildView : public views::View { could this be defined only in the cc (i.e. anonymous namespace)?
Hi Mathieu, Please take a look. Thanks, -sashi. https://codereview.chromium.org/2670003002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2670003002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:31: AutofillPopupChildView *child = new AutofillPopupChildView(controller, i); On 2017/02/27 18:41:55, Mathieu Perreault wrote: > AutofillPopupChildView* child Is this an unfinished comment? https://codereview.chromium.org/2670003002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:237: if (!controller_) On 2017/02/27 18:41:55, Mathieu Perreault wrote: > since AutofillPopupViewViews doesn't seem to guard against a null controller, > let's remove this check and add a comment to AutofillPopupViewViews's > construction declaration to say that |controller| should never be null. The controller_ is set to NULL on Hide. Are we sure GetAccessibleNodeData will not be called after Hide? If yes, I can remove the check. https://codereview.chromium.org/2670003002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.h (right): https://codereview.chromium.org/2670003002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:53: class AutofillPopupChildView : public views::View { On 2017/02/27 18:41:55, Mathieu Perreault wrote: > could this be defined only in the cc (i.e. anonymous namespace)? Done.
https://codereview.chromium.org/2670003002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2670003002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:31: AutofillPopupChildView *child = new AutofillPopupChildView(controller, i); On 2017/02/27 18:49:44, csashi wrote: > On 2017/02/27 18:41:55, Mathieu Perreault wrote: > > AutofillPopupChildView* child > > Is this an unfinished comment? No the * is at the wrong location, sorry that wasn't clear https://codereview.chromium.org/2670003002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:237: if (!controller_) On 2017/02/27 18:49:44, csashi wrote: > On 2017/02/27 18:41:55, Mathieu Perreault wrote: > > since AutofillPopupViewViews doesn't seem to guard against a null controller, > > let's remove this check and add a comment to AutofillPopupViewViews's > > construction declaration to say that |controller| should never be null. > > The controller_ is set to NULL on Hide. Are we sure GetAccessibleNodeData will > not be called after Hide? If yes, I can remove the check. Hmm I don't see where the controller becomes null, can you be more specific? And if it is so, in that case your child view will keep the potentially invalid pointer even though its content is potentially destroyed elsewhere, right? https://codereview.chromium.org/2670003002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2670003002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:255: SetFocusBehavior(FocusBehavior::ALWAYS); I think we can have the function bodies in the anonymous namespace above too, inlined.
Hi Mathieu, Please take a look. -sashi. https://codereview.chromium.org/2670003002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2670003002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:31: AutofillPopupChildView *child = new AutofillPopupChildView(controller, i); On 2017/02/27 19:01:05, Mathieu Perreault wrote: > On 2017/02/27 18:49:44, csashi wrote: > > On 2017/02/27 18:41:55, Mathieu Perreault wrote: > > > AutofillPopupChildView* child > > > > Is this an unfinished comment? > > No the * is at the wrong location, sorry that wasn't clear Done. https://codereview.chromium.org/2670003002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:237: if (!controller_) On 2017/02/27 19:01:05, Mathieu Perreault wrote: > On 2017/02/27 18:49:44, csashi wrote: > > On 2017/02/27 18:41:55, Mathieu Perreault wrote: > > > since AutofillPopupViewViews doesn't seem to guard against a null > controller, > > > let's remove this check and add a comment to AutofillPopupViewViews's > > > construction declaration to say that |controller| should never be null. > > > > The controller_ is set to NULL on Hide. Are we sure GetAccessibleNodeData will > > not be called after Hide? If yes, I can remove the check. > > Hmm I don't see where the controller becomes null, can you be more specific? And > if it is so, in that case your child view will keep the potentially invalid > pointer even though its content is potentially destroyed elsewhere, right? Done. My mistake, I was referring to AutofillPopupViewViews. https://codereview.chromium.org/2670003002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2670003002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:255: SetFocusBehavior(FocusBehavior::ALWAYS); On 2017/02/27 19:01:05, Mathieu Perreault wrote: > I think we can have the function bodies in the anonymous namespace above too, > inlined. Done.
lgtm with nit https://codereview.chromium.org/2670003002/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.h (right): https://codereview.chromium.org/2670003002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:13: #include "ui/views/view.h" move to cc
https://codereview.chromium.org/2670003002/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.h (right): https://codereview.chromium.org/2670003002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:13: #include "ui/views/view.h" On 2017/02/27 19:13:34, Mathieu Perreault wrote: > move to cc Done.
https://codereview.chromium.org/2670003002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2670003002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:65: AddChildViewAt(child, static_cast<int>(i)); optional: AddChildViewAt(child); works the same here. https://codereview.chromium.org/2670003002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:77: void AutofillPopupViewViews::Hide() { As DoHide is public, might that be called instead of this? Also, what about the case of the widget being destroyed but hide not called? https://codereview.chromium.org/2670003002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:118: static_cast<AutofillPopupChildView*>(child_at(row)); This code makes me nervous. How do you know some other views haven't been added so that the cast is still valid? One option is to check GetClassName(). https://codereview.chromium.org/2670003002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.h (right): https://codereview.chromium.org/2670003002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:22: // |controller| should not be null. should->must But a DCHECK in the constructor makes this more obvious.
Hi Scott, Please take a look. Thanks, -sashi. https://codereview.chromium.org/2670003002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2670003002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:65: AddChildViewAt(child, static_cast<int>(i)); On 2017/02/27 21:05:57, sky wrote: > optional: AddChildViewAt(child); works the same here. Done. https://codereview.chromium.org/2670003002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:77: void AutofillPopupViewViews::Hide() { On 2017/02/27 21:05:57, sky wrote: > As DoHide is public, might that be called instead of this? Also, what about the > case of the widget being destroyed but hide not called? If I am reading correctly, DoHide is protected - I guess DoHide was meant as a helper function. If the widget gets destroyed but Hide not called, worst case we don't get an accessibility event. I could do something like the following in the destructor but I have to look into whether that would be safe (i.e. if NotifyAccessibilityEvent can access memory that has been destroyed). In the destructor: if (controller_) { // |Hide| was not called. NotifyAccessibilityEvent(ui::AX_EVENT_MENU_END, true); } https://codereview.chromium.org/2670003002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:118: static_cast<AutofillPopupChildView*>(child_at(row)); On 2017/02/27 21:05:57, sky wrote: > This code makes me nervous. How do you know some other views haven't been added > so that the cast is still valid? One option is to check GetClassName(). This class hierarchy is currently not deep. Hence, added a debug check. Please let me know if you wanted a check in opt mode also.
https://codereview.chromium.org/2670003002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2670003002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:77: void AutofillPopupViewViews::Hide() { On 2017/02/27 21:32:37, csashi wrote: > On 2017/02/27 21:05:57, sky wrote: > > As DoHide is public, might that be called instead of this? Also, what about > the > > case of the widget being destroyed but hide not called? > > If I am reading correctly, DoHide is protected - I guess DoHide was meant as a > helper function. > > If the widget gets destroyed but Hide not called, worst case we don't get an > accessibility event. I could do something like the following in the destructor > but I have to look into whether that would be safe (i.e. if > NotifyAccessibilityEvent can access memory that has been destroyed). > > In the destructor: > > if (controller_) { // |Hide| was not called. > NotifyAccessibilityEvent(ui::AX_EVENT_MENU_END, true); > } I'm not sure either. Maybe Dominic has an opinion here? https://codereview.chromium.org/2670003002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:118: static_cast<AutofillPopupChildView*>(child_at(row)); On 2017/02/27 21:32:38, csashi wrote: > On 2017/02/27 21:05:57, sky wrote: > > This code makes me nervous. How do you know some other views haven't been > added > > so that the cast is still valid? One option is to check GetClassName(). > > This class hierarchy is currently not deep. Hence, added a debug check. Please > let me know if you wanted a check in opt mode also. My concern is the static_cast, but I'm ok with it. https://codereview.chromium.org/2670003002/diff/320001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2670003002/diff/320001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:405: if (CanAccept(suggestions_[selected_line].frontend_id)) { no {} https://codereview.chromium.org/2670003002/diff/320001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:501: DCHECK(0 <= row); DCHECK_LE https://codereview.chromium.org/2670003002/diff/320001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:502: DCHECK(row < suggestions_.size()); DCHECK_LT
Hi Scott, Can you take a look? Thanks, -sashi. https://codereview.chromium.org/2670003002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2670003002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:118: static_cast<AutofillPopupChildView*>(child_at(row)); On 2017/02/27 23:31:04, sky wrote: > On 2017/02/27 21:32:38, csashi wrote: > > On 2017/02/27 21:05:57, sky wrote: > > > This code makes me nervous. How do you know some other views haven't been > > added > > > so that the cast is still valid? One option is to check GetClassName(). > > > > This class hierarchy is currently not deep. Hence, added a debug check. Please > > let me know if you wanted a check in opt mode also. > > My concern is the static_cast, but I'm ok with it. Acknowledged. https://codereview.chromium.org/2670003002/diff/320001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2670003002/diff/320001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:405: if (CanAccept(suggestions_[selected_line].frontend_id)) { On 2017/02/27 23:31:04, sky wrote: > no {} Done. https://codereview.chromium.org/2670003002/diff/320001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:501: DCHECK(0 <= row); On 2017/02/27 23:31:04, sky wrote: > DCHECK_LE Done. https://codereview.chromium.org/2670003002/diff/320001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:502: DCHECK(row < suggestions_.size()); On 2017/02/27 23:31:04, sky wrote: > DCHECK_LT Done.
https://codereview.chromium.org/2670003002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2670003002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:77: void AutofillPopupViewViews::Hide() { On 2017/02/27 23:31:04, sky wrote: > On 2017/02/27 21:32:37, csashi wrote: > > On 2017/02/27 21:05:57, sky wrote: > > > As DoHide is public, might that be called instead of this? Also, what about > > the > > > case of the widget being destroyed but hide not called? > > > > If I am reading correctly, DoHide is protected - I guess DoHide was meant as a > > helper function. > > > > If the widget gets destroyed but Hide not called, worst case we don't get an > > accessibility event. I could do something like the following in the destructor > > but I have to look into whether that would be safe (i.e. if > > NotifyAccessibilityEvent can access memory that has been destroyed). > > > > In the destructor: > > > > if (controller_) { // |Hide| was not called. > > NotifyAccessibilityEvent(ui::AX_EVENT_MENU_END, true); > > } > > I'm not sure either. Maybe Dominic has an opinion here? Did Dominic respond to this?
On 2017/02/28 04:03:34, sky wrote: > https://codereview.chromium.org/2670003002/diff/300001/chrome/browser/ui/view... > File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): > > https://codereview.chromium.org/2670003002/diff/300001/chrome/browser/ui/view... > chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:77: void > AutofillPopupViewViews::Hide() { > On 2017/02/27 23:31:04, sky wrote: > > On 2017/02/27 21:32:37, csashi wrote: > > > On 2017/02/27 21:05:57, sky wrote: > > > > As DoHide is public, might that be called instead of this? Also, what > about > > > the > > > > case of the widget being destroyed but hide not called? > > > > > > If I am reading correctly, DoHide is protected - I guess DoHide was meant as > a > > > helper function. > > > > > > If the widget gets destroyed but Hide not called, worst case we don't get an > > > accessibility event. I could do something like the following in the > destructor > > > but I have to look into whether that would be safe (i.e. if > > > NotifyAccessibilityEvent can access memory that has been destroyed). > > > > > > In the destructor: > > > > > > if (controller_) { // |Hide| was not called. > > > NotifyAccessibilityEvent(ui::AX_EVENT_MENU_END, true); > > > } > > > > I'm not sure either. Maybe Dominic has an opinion here? > > Did Dominic respond to this? HI Dominic, Can you take a look? Thanks, -sashi.
mathp@chromium.org changed reviewers: - anthonyvd@chromium.org
https://codereview.chromium.org/2670003002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2670003002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:77: void AutofillPopupViewViews::Hide() { On 2017/02/28 04:03:32, sky wrote: > On 2017/02/27 23:31:04, sky wrote: > > On 2017/02/27 21:32:37, csashi wrote: > > > On 2017/02/27 21:05:57, sky wrote: > > > > As DoHide is public, might that be called instead of this? Also, what > about > > > the > > > > case of the widget being destroyed but hide not called? > > > > > > If I am reading correctly, DoHide is protected - I guess DoHide was meant as > a > > > helper function. > > > > > > If the widget gets destroyed but Hide not called, worst case we don't get an > > > accessibility event. I could do something like the following in the > destructor > > > but I have to look into whether that would be safe (i.e. if > > > NotifyAccessibilityEvent can access memory that has been destroyed). > > > > > > In the destructor: > > > > > > if (controller_) { // |Hide| was not called. > > > NotifyAccessibilityEvent(ui::AX_EVENT_MENU_END, true); > > > } > > > > I'm not sure either. Maybe Dominic has an opinion here? > > Did Dominic respond to this? We should be cautious about calling NotifyAccessibilityEvent from a destructor. It will definitely trigger calling back into the View to query things like its location, visibility, etc. It's not necessary to notify MENU_END in exceptional circumstances like if the whole owning window is destroyed or during app shutdown. So as long as the normal code path (where the user closes the pop-up menu and returns to the same web page) calls Hide() and fires the event, I wouldn't worry about the exceptional cases.
lgtm
Ok, LGTM
On 2017/02/28 17:54:50, sky wrote: > Ok, LGTM Hi Carlos, Can you approve for chrome/app? Thanks, -sashi.
The CQ bit was checked by csashi@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org, mathp@chromium.org Link to the patchset: https://codereview.chromium.org/2670003002/#ps340001 (title: "Code review comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
mathp@chromium.org changed reviewers: - cpu@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by csashi@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, dmazzoni@chromium.org, dtseng@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2670003002/#ps360001 (title: "Adds empty NotifyAccessibilityEventForRow implementation for Mac OS.")
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by csashi@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, dmazzoni@chromium.org, dtseng@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2670003002/#ps380001 (title: "Adds empty NotifyAccessibilityEventForRow implementation for Android.")
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
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by csashi@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, dmazzoni@chromium.org, dtseng@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2670003002/#ps400001 (title: "Adds empty NotifyAccessibilityEventForRow implementation for Android.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 400001, "attempt_start_ts": 1488316744334750,
"parent_rev": "1be4b00ee4d9617054fe5698c05cf4d92f42bf17", "commit_rev":
"67051d3d62d1522fa44afeb2385a3bb6b6a6f128"}
Message was sent while issue was closed.
Description was changed from ========== Uses child views in Autofill Popup so we can trigger GetAccessibleNodeData in each of the Autofill popup suggestions. BUG=627860 ========== to ========== Uses child views in Autofill Popup so we can trigger GetAccessibleNodeData in each of the Autofill popup suggestions. BUG=627860 Review-Url: https://codereview.chromium.org/2670003002 Cr-Commit-Position: refs/heads/master@{#453724} Committed: https://chromium.googlesource.com/chromium/src/+/67051d3d62d1522fa44afeb2385a... ==========
Message was sent while issue was closed.
Committed patchset #21 (id:400001) as https://chromium.googlesource.com/chromium/src/+/67051d3d62d1522fa44afeb2385a... |
