Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2341633006:
MacViews/a11y: Allow accessibility clients to update the selected text. (Closed)
|
Created:
4 years, 3 months ago by Patti Lor Modified:
4 years, 1 month ago CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, aboxhall+watch_chromium.org, tfarina, nektar+watch_chromium.org, yuzo+watch_chromium.org, nona+watch_chromium.org, je_julie, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, James Su, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews/a11y: Allow accessibility clients to update the selected text.
Currently, the `selected text` a11y attribute of views::Textfields is reported
as unmodifiable, which differs from native NSTextFields on Mac.
Add a callback to views::Textfield and OmniboxViewViews to allow accessibility
clients to replace the currently selected text, and plumb through for Mac. When
no text is selected, new text is inserted at the current cursor position. This
also fixes a problem where selection information provided to accessibility
clients was incorrect for the Omnibox when it still had selected text, but focus
was not on the window the Omnibox was currently attached to.
BUG=610591
TEST=With #mac-views-webui-dialogs turned on in chrome:flags, open the XCode
Accessibility Inspector and bring up the HTTP Authentication dialog. Type some
text into the username Textfield, then select some text in it. Click on the
Accessibility Inspector to move focus off the Chrome window and hover over the
Omnibox with the mouse. The Accessibility Inspector should report the correct
"accessibilitySelectedTextRange" and "accessibilitySelectedText" attributes.
Then press Cmd+F7 to lock onto the Textfield, click on
"accessibilitySelectedText" and enter a value at the bottom. The selected text
in the Textfield should be replaced with the new string with no text selected.
Committed: https://crrev.com/0aa1406119dc85097efebbb282895b543e4b0c26
Cr-Commit-Position: refs/heads/master@{#428645}
Patch Set 1 #Patch Set 2 : Add to TestAXNodeWrapper. #Patch Set 3 : Add to AuraLinuxApplication. #Patch Set 4 : Put the cursor back at the end of the newly inserted string. #Patch Set 5 : Move accessibility notification for text selection being changed to SelectRange() instead. #
Total comments: 26
Patch Set 6 : Review comments. #Patch Set 7 : Fix other platforms. #Patch Set 8 : Fix windows. #Patch Set 9 : Delete unused method declarations and revert redundant a11y notification. #
Total comments: 15
Patch Set 10 : Review comments. #Patch Set 11 : Rebase, fix OmniboxViewViews::AccessibilitySetValue. #
Total comments: 2
Patch Set 12 : Make set value callbacks not have a const bool |replace| arg. #
Total comments: 8
Patch Set 13 : Review comments. #
Total comments: 8
Patch Set 14 : Review comments (inc. using InsertOrReplaceText() for Omnibox). #Patch Set 15 : Rebase. #
Total comments: 2
Patch Set 16 : Fix Windows. #
Total comments: 12
Patch Set 17 : Review comments. #Messages
Total messages: 107 (90 generated)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
patricialor@chromium.org changed reviewers: + tapted@chromium.org
Hi Trent, PTAL, thanks!
https://codereview.chromium.org/2341633006/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/2341633006/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:427: SelectRange(saved_selection_for_focus_change_); I don't think we need to select it (I'm actually not sure what it means to select when there is no focus), but I think we can just apply the selection like gfx::Range selection_range = saved_selection_for_focus_change_.IsValid() ? saved_selection_for_focus_change_ : GetSelectedRange(); https://codereview.chromium.org/2341633006/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:434: current_text.substr(selection_range.GetMax(), current_text.length()); `current_text.length()` shouldn't be needed on the third component https://codereview.chromium.org/2341633006/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:435: SetUserText(new_string, true); Calling AccessibilitySetValue(new_string) is probably a slight improvement https://codereview.chromium.org/2341633006/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:436: SetSelectionRange(gfx::Range(selection_range.GetMin() + new_value.length())); Same here - I don't think we can set a selection without focus. Perhaps selection_range.set_end( selection_range.end() - selection_range.length() + new_value.length()); if (HasFocus()) SetSelectionRange(selection_range) else saved_selection_for_focus_change_ = selection_range; https://codereview.chromium.org/2341633006/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:761: GetSelectionBounds(&entry_start, &entry_end); move to an `else`? https://codereview.chromium.org/2341633006/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:764: if (!HasFocus() && saved_selection_for_focus_change_.IsValid()) { `!HasFocus()` might not be required -- pretty sure `saved_selection_for_focus_change_.IsValid()` \implies{} !HasFocus() https://codereview.chromium.org/2341633006/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_view_views.h (right): https://codereview.chromium.org/2341633006/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.h:132: // Same as above, but replaces selected text/inserts new text at the cursor. nit: blank line before. "above" -> AccessibilitySetValue https://codereview.chromium.org/2341633006/diff/80001/ui/accessibility/ax_vie... File ui/accessibility/ax_view_state.h (right): https://codereview.chromium.org/2341633006/diff/80001/ui/accessibility/ax_vie... ui/accessibility/ax_view_state.h:83: base::Callback<void(const base::string16&)> set_value_callback; Does much change if we require the Callback to have a `bool` "replace" argument? I think that will be neater https://codereview.chromium.org/2341633006/diff/80001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2341633006/diff/80001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_mac.mm:387: if ([attribute isEqualToString:NSAccessibilitySelectedTextAttribute] && else if (or alter to support the `bool` replace arg as required https://codereview.chromium.org/2341633006/diff/80001/ui/views/accessibility/... File ui/views/accessibility/native_view_accessibility.cc (right): https://codereview.chromium.org/2341633006/diff/80001/ui/views/accessibility/... ui/views/accessibility/native_view_accessibility.cc:236: } the `bool` arg would make it easier to share code with SetStringValue https://codereview.chromium.org/2341633006/diff/80001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2341633006/diff/80001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1787: ClearSelection(); do we want to extend the selection here too? https://codereview.chromium.org/2341633006/diff/80001/ui/views/widget/native_... File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2341633006/diff/80001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:373: } are there cross-platform tests for AccessibilitySetText? They might need an update too..
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
PTAL, thanks Trent! https://codereview.chromium.org/2341633006/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/2341633006/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:427: SelectRange(saved_selection_for_focus_change_); On 2016/09/27 07:16:30, tapted wrote: > I don't think we need to select it (I'm actually not sure what it means to > select when there is no focus), but I think we can just apply the selection like > > gfx::Range selection_range = saved_selection_for_focus_change_.IsValid() > ? saved_selection_for_focus_change_ > : GetSelectedRange(); Done. https://codereview.chromium.org/2341633006/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:434: current_text.substr(selection_range.GetMax(), current_text.length()); On 2016/09/27 07:16:30, tapted wrote: > `current_text.length()` shouldn't be needed on the third component Done. https://codereview.chromium.org/2341633006/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:435: SetUserText(new_string, true); On 2016/09/27 07:16:30, tapted wrote: > Calling AccessibilitySetValue(new_string) is probably a slight improvement No longer needed after your |replace| arg suggestion :) https://codereview.chromium.org/2341633006/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:436: SetSelectionRange(gfx::Range(selection_range.GetMin() + new_value.length())); On 2016/09/27 07:16:30, tapted wrote: > Same here - I don't think we can set a selection without focus. Perhaps > > selection_range.set_end( > selection_range.end() - selection_range.length() + new_value.length()); > if (HasFocus()) > SetSelectionRange(selection_range) > else > saved_selection_for_focus_change_ = selection_range; Sorry - this is an inconsistency that I've now deleted. As per the other comment, Cocoa doesn't actually re-apply the selection after replacing it (the behaviour is the same as you had typed something while having selected text focused). https://codereview.chromium.org/2341633006/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:761: GetSelectionBounds(&entry_start, &entry_end); On 2016/09/27 07:16:30, tapted wrote: > move to an `else`? Done. https://codereview.chromium.org/2341633006/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:764: if (!HasFocus() && saved_selection_for_focus_change_.IsValid()) { On 2016/09/27 07:16:30, tapted wrote: > `!HasFocus()` might not be required -- pretty sure > `saved_selection_for_focus_change_.IsValid()` \implies{} !HasFocus() Done. https://codereview.chromium.org/2341633006/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_view_views.h (right): https://codereview.chromium.org/2341633006/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_view_views.h:132: // Same as above, but replaces selected text/inserts new text at the cursor. On 2016/09/27 07:16:30, tapted wrote: > nit: blank line before. "above" -> AccessibilitySetValue Done. https://codereview.chromium.org/2341633006/diff/80001/ui/accessibility/ax_vie... File ui/accessibility/ax_view_state.h (right): https://codereview.chromium.org/2341633006/diff/80001/ui/accessibility/ax_vie... ui/accessibility/ax_view_state.h:83: base::Callback<void(const base::string16&)> set_value_callback; On 2016/09/27 07:16:30, tapted wrote: > Does much change if we require the Callback to have a `bool` "replace" argument? > I think that will be neater Done. https://codereview.chromium.org/2341633006/diff/80001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2341633006/diff/80001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_mac.mm:387: if ([attribute isEqualToString:NSAccessibilitySelectedTextAttribute] && On 2016/09/27 07:16:30, tapted wrote: > else if (or alter to support the `bool` replace arg as required Done. https://codereview.chromium.org/2341633006/diff/80001/ui/views/accessibility/... File ui/views/accessibility/native_view_accessibility.cc (right): https://codereview.chromium.org/2341633006/diff/80001/ui/views/accessibility/... ui/views/accessibility/native_view_accessibility.cc:236: } On 2016/09/27 07:16:30, tapted wrote: > the `bool` arg would make it easier to share code with SetStringValue Done. https://codereview.chromium.org/2341633006/diff/80001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2341633006/diff/80001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1787: ClearSelection(); On 2016/09/27 07:16:30, tapted wrote: > do we want to extend the selection here too? No - Cocoa doesn't, so I was following that behaviour. https://codereview.chromium.org/2341633006/diff/80001/ui/views/widget/native_... File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2341633006/diff/80001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:373: } On 2016/09/27 07:16:30, tapted wrote: > are there cross-platform tests for AccessibilitySetText? They might need an > update too.. No, I don't think so - I can add some? Not sure which approach is more useful though, adding tests on NativeViewAccessibilityTest, or on all the classes that register a set_value_callback? The latter just consists of OmniboxViewViews and Textfield.
https://codereview.chromium.org/2341633006/diff/80001/ui/views/widget/native_... File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2341633006/diff/80001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:373: } On 2016/10/20 04:19:34, Patti Lor wrote: > On 2016/09/27 07:16:30, tapted wrote: > > are there cross-platform tests for AccessibilitySetText? They might need an > > update too.. > > No, I don't think so - I can add some? Not sure which approach is more useful > though, adding tests on NativeViewAccessibilityTest, or on all the classes that > register a set_value_callback? The latter just consists of OmniboxViewViews and > Textfield. Hm - there's a windows-specific test in NativeViewAcccessibilityWinTest.TextfieldAccessibility But Windows doesn't seem to have the equivalent of NSAccessibilitySelectedTextAttribute -- just "put value". This is probably enough. https://codereview.chromium.org/2341633006/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views.h (right): https://codereview.chromium.org/2341633006/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. CL description I don't think we need the "when mac_views_browser=true" bit (since that's true just for the omnibox stuff). I'd get rid of the "Tests are also added" bit as well (otherwise it sounds like there's a reason behind only doing it for Textfield, but it's more of a cost/benefit thing [dealing with focus is hard in tests and some level of manual testing is OK]). The TEST= Can we phrase this in terms of things we can do in Canary with chrome://flags? (e.g. bookmark bubble). Otherwise someone might try to repro just with the Cocoa browser, which we're not changing. Also, the first paragraph of a CL description is usually a "current behaviour" summary. The linked bug isn't really specific enough. So there should be a para at the start something like "Currently, the `selected text` a11y attribute of views::Textfields is reported as unmodifiable, which differs from native NSTextFields on Mac." https://codereview.chromium.org/2341633006/diff/160001/ui/accessibility/ax_vi... File ui/accessibility/ax_view_state.h (right): https://codereview.chromium.org/2341633006/diff/160001/ui/accessibility/ax_vi... ui/accessibility/ax_view_state.h:81: // These callbacks are only valid for the lifetime of the view, and should These callbacks are -> This callback is https://codereview.chromium.org/2341633006/diff/160001/ui/accessibility/ax_vi... ui/accessibility/ax_view_state.h:84: base::Callback<void(const base::string16&, const bool&)> set_value_callback; `const bool&` -> just `bool` -- typically only classes/structs get passed by const-reference https://codereview.chromium.org/2341633006/diff/160001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_delegate.h (right): https://codereview.chromium.org/2341633006/diff/160001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_delegate.h:90: const bool& replace) = 0; const bool& -> bool https://codereview.chromium.org/2341633006/diff/160001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_delegate.h:91: // Whether SetStringValue() and ReplaceSelectedText() are callable, i.e. if nit: blank line before https://codereview.chromium.org/2341633006/diff/160001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2341633006/diff/160001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:398: [value isKindOfClass:[NSString class]]) perhaps at the top of the function, if (![value isKindOfClass:[NSString class]]) return; https://codereview.chromium.org/2341633006/diff/160001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:400: false); now that the body spans 2 lines, the `if` (and else) needs curlies
Description was changed from ========== MacViews/a11y: Allow accessibility clients to update the selected text. Add a callback to views::Textfield and OmniboxViewViews to allow accessibility clients to replace the currently selected text, and plumb through for Mac (when mac_views_browser=true). When no text is selected, new text is inserted at the current cursor position. This also fixes a problem where selection information provided to accessibility clients was incorrect for the Omnibox when it still had selected text, but focus was not on the window the Omnibox was currently attached to. Tests are also added for this change for views::Textfield only. BUG=610591 TEST=Open the XCode Accessibility Inspector and go to "google.com" in Chrome. Select some text in the URL, click on the Accessibility Inspector to move focus off the Chrome window and hover over the Omnibox with the mouse. The Accessibility Inspector should report the correct "accessibilitySelectedTextRange" and "accessibilitySelectedText" attributes. Then press Cmd+F7 to lock onto the Omnibox, click on "accessibilitySelectedText" and enter a value at the bottom. The selected text in the Omnibox should be replaced with the new string and should not revert to the URL when focus is moved back to Chrome. ========== to ========== MacViews/a11y: Allow accessibility clients to update the selected text. Currently, the `selected text` a11y attribute of views::Textfields is reported as unmodifiable, which differs from native NSTextFields on Mac. Add a callback to views::Textfield and OmniboxViewViews to allow accessibility clients to replace the currently selected text, and plumb through for Mac. When no text is selected, new text is inserted at the current cursor position. This also fixes a problem where selection information provided to accessibility clients was incorrect for the Omnibox when it still had selected text, but focus was not on the window the Omnibox was currently attached to. BUG=610591 TEST=With #mac-views-webui-dialogs turned on in chrome:flags, open the XCode Accessibility Inspector and bring up the HTTP Authentication dialog. Type some text into the username Textfield, then select some text in it. Click on the Accessibility Inspector to move focus off the Chrome window and hover over the Omnibox with the mouse. The Accessibility Inspector should report the correct "accessibilitySelectedTextRange" and "accessibilitySelectedText" attributes. Then press Cmd+F7 to lock onto the Textfield, click on "accessibilitySelectedText" and enter a value at the bottom. The selected text in the Textfield should be replaced with the new string with no text selected. ==========
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #11 (id:200001) has been deleted
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Patchset #11 (id:220001) has been deleted
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL, thanks! https://codereview.chromium.org/2341633006/diff/80001/ui/views/widget/native_... File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2341633006/diff/80001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:373: } On 2016/10/20 05:54:58, tapted wrote: > On 2016/10/20 04:19:34, Patti Lor wrote: > > On 2016/09/27 07:16:30, tapted wrote: > > > are there cross-platform tests for AccessibilitySetText? They might need an > > > update too.. > > > > No, I don't think so - I can add some? Not sure which approach is more useful > > though, adding tests on NativeViewAccessibilityTest, or on all the classes > that > > register a set_value_callback? The latter just consists of OmniboxViewViews > and > > Textfield. > > Hm - there's a windows-specific test in > NativeViewAcccessibilityWinTest.TextfieldAccessibility > > But Windows doesn't seem to have the equivalent of > NSAccessibilitySelectedTextAttribute -- just "put value". > > This is probably enough. Ok :) I missed the Windows one - thanks for investigating that! https://codereview.chromium.org/2341633006/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views.h (right): https://codereview.chromium.org/2341633006/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2016/10/20 05:54:58, tapted wrote: > CL description > > I don't think we need the "when mac_views_browser=true" bit (since that's true > just for the omnibox stuff). > > I'd get rid of the "Tests are also added" bit as well (otherwise it sounds like > there's a reason behind only doing it for Textfield, but it's more of a > cost/benefit thing [dealing with focus is hard in tests and some level of manual > testing is OK]). > > > The TEST= Can we phrase this in terms of things we can do in Canary with > chrome://flags? (e.g. bookmark bubble). Otherwise someone might try to repro > just with the Cocoa browser, which we're not changing. > > > Also, the first paragraph of a CL description is usually a "current behaviour" > summary. The linked bug isn't really specific enough. So there should be a para > at the start something like > > "Currently, the `selected text` a11y attribute of views::Textfields is reported > as unmodifiable, which differs from native NSTextFields on Mac." Done. https://codereview.chromium.org/2341633006/diff/160001/ui/accessibility/ax_vi... File ui/accessibility/ax_view_state.h (right): https://codereview.chromium.org/2341633006/diff/160001/ui/accessibility/ax_vi... ui/accessibility/ax_view_state.h:81: // These callbacks are only valid for the lifetime of the view, and should On 2016/10/20 05:54:58, tapted wrote: > These callbacks are -> This callback is Done. https://codereview.chromium.org/2341633006/diff/160001/ui/accessibility/ax_vi... ui/accessibility/ax_view_state.h:84: base::Callback<void(const base::string16&, const bool&)> set_value_callback; On 2016/10/20 05:54:59, tapted wrote: > `const bool&` -> just `bool` -- typically only classes/structs get passed by > const-reference Done. https://codereview.chromium.org/2341633006/diff/160001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_delegate.h (right): https://codereview.chromium.org/2341633006/diff/160001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_delegate.h:90: const bool& replace) = 0; On 2016/10/20 05:54:59, tapted wrote: > const bool& -> bool Done. https://codereview.chromium.org/2341633006/diff/160001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_delegate.h:91: // Whether SetStringValue() and ReplaceSelectedText() are callable, i.e. if On 2016/10/20 05:54:59, tapted wrote: > nit: blank line before Done. https://codereview.chromium.org/2341633006/diff/160001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2341633006/diff/160001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:398: [value isKindOfClass:[NSString class]]) On 2016/10/20 05:54:59, tapted wrote: > perhaps at the top of the function, > > if (![value isKindOfClass:[NSString class]]) > return; This doesn't work for all attributes - e.g., NSAccessibilityFocusAttribute is normally writable, but will be passed as a NSNumber which wraps a bool value. https://codereview.chromium.org/2341633006/diff/160001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:400: false); On 2016/10/20 05:54:59, tapted wrote: > now that the body spans 2 lines, the `if` (and else) needs curlies Done.
lgtm with `const bool` -> bool https://codereview.chromium.org/2341633006/diff/160001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2341633006/diff/160001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:398: [value isKindOfClass:[NSString class]]) On 2016/10/24 04:31:48, Patti Lor wrote: > On 2016/10/20 05:54:59, tapted wrote: > > perhaps at the top of the function, > > > > if (![value isKindOfClass:[NSString class]]) > > return; > > This doesn't work for all attributes - e.g., NSAccessibilityFocusAttribute is > normally writable, but will be passed as a NSNumber which wraps a bool value. Acknowledged. https://codereview.chromium.org/2341633006/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views.h (right): https://codereview.chromium.org/2341633006/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.h:133: const bool replace); nit: remove const on |replace| (typically APIs don't put const on by-value arguments, since it doesn't really affect callers of the API and limits what the callee can do with it).
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #12 (id:260001) has been deleted
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
patricialor@chromium.org changed reviewers: + dmazzoni@chromium.org, msw@chromium.org
dmazzoni@chromium.org: Please review changes in ui/views/accessibility/ ui/accessibility/ msw@chromium.org: Please review changes in ui/views/controls/textfield/ chrome/browser/ui/views/omnibox/ Thanks all! https://codereview.chromium.org/2341633006/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views.h (right): https://codereview.chromium.org/2341633006/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.h:133: const bool replace); On 2016/10/24 06:11:57, tapted wrote: > nit: remove const on |replace| (typically APIs don't put const on by-value > arguments, since it doesn't really affect callers of the API and limits what the > callee can do with it). Done.
https://codereview.chromium.org/2341633006/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/2341633006/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:425: base::string16 new_string = This definitely looks wrong. You're declaring a new variable |new_string| inside the block that shadows |new_string| declared above, but then you're not doing anything with it. https://codereview.chromium.org/2341633006/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views.h (right): https://codereview.chromium.org/2341633006/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.h:130: // don't normally use this). Sets the value and clears the selection. If I'd reword. If |replace| is false, sets the value and clears the selection. If |replace| is true, deletes whatever's selected and inserts the value. Hmmm, that seems unclear, |replace| doesn't really do what I'd expect it to do. Let's rename it so that it's more clear. How about flipping the boolean like this: // Replaces the current selection with the text to insert. // If |clear_first| is true, clears the text field first so // that it's completely replaced with |text_to_insert|. AccessibilityInsertText(const base::string16& text_to_insert, bool clear_first); https://codereview.chromium.org/2341633006/diff/280001/ui/accessibility/ax_vi... File ui/accessibility/ax_view_state.h (right): https://codereview.chromium.org/2341633006/diff/280001/ui/accessibility/ax_vi... ui/accessibility/ax_view_state.h:84: base::Callback<void(const base::string16&, bool)> set_value_callback; Maybe the same here, the bool could be replace all. Just an FYI, I just recently added ui/accessibility/ax_action_data.h as a more generic way to encapsulate different actions that could be taken on accessible objects. We should probably replace this with a callback that takes an AXActionData as an argument and handles that action. That will allow us to share a lot more code between platforms. OK to leave things as they are for this change, but let's switch to that before we add another callback if we find ourselves tempted to add another one. https://codereview.chromium.org/2341633006/diff/280001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_delegate.h (right): https://codereview.chromium.org/2341633006/diff/280001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_delegate.h:88: // Optionally replace it or insert at the cursor if |replace| is true. Same here, flip the boolean around and rename it so that it inserts text by default, and clears the selection if the bool is set. Don't say "optionally" because it's a required parameter, the caller needs to decide for sure whether to pass true or false.
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the review dmazzoni, PTAL! https://codereview.chromium.org/2341633006/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/2341633006/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:425: base::string16 new_string = On 2016/10/25 17:21:49, dmazzoni wrote: > This definitely looks wrong. You're declaring a new variable |new_string| inside > the block that shadows |new_string| declared above, but then you're not doing > anything with it. Ah - thanks for picking up on that! I'm surprised this wasn't a compile error - turns out this is something called "shadowing". Fixed. https://codereview.chromium.org/2341633006/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views.h (right): https://codereview.chromium.org/2341633006/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.h:130: // don't normally use this). Sets the value and clears the selection. If On 2016/10/25 17:21:49, dmazzoni wrote: > I'd reword. If |replace| is false, sets the value and clears the selection. If > |replace| is true, deletes whatever's selected and inserts the value. > > Hmmm, that seems unclear, |replace| doesn't really do what I'd expect > it to do. Let's rename it so that it's more clear. How about flipping the > boolean like this: > > // Replaces the current selection with the text to insert. > // If |clear_first| is true, clears the text field first so > // that it's completely replaced with |text_to_insert|. > AccessibilityInsertText(const base::string16& text_to_insert, bool clear_first); > I've changed it to |clear_first|, with the following comment - let me know if that's clearer: // If |clear_first| is true, clears the existing // value first and sets it to |new_value|. If false, deletes whatever's // selected and replaces it with |new_value|. Then clears the text selection. https://codereview.chromium.org/2341633006/diff/280001/ui/accessibility/ax_vi... File ui/accessibility/ax_view_state.h (right): https://codereview.chromium.org/2341633006/diff/280001/ui/accessibility/ax_vi... ui/accessibility/ax_view_state.h:84: base::Callback<void(const base::string16&, bool)> set_value_callback; On 2016/10/25 17:21:49, dmazzoni wrote: > Maybe the same here, the bool could be replace all. > > Just an FYI, I just recently added ui/accessibility/ax_action_data.h > as a more generic way to encapsulate different actions that could > be taken on accessible objects. We should probably replace this > with a callback that takes an AXActionData as an argument and > handles that action. That will allow us to share a lot more code > between platforms. > > OK to leave things as they are for this change, but let's switch > to that before we add another callback if we find ourselves tempted > to add another one. Ah, thanks for the FYI - will definitely be needing more callbacks in future CLs I think, so this is useful. https://codereview.chromium.org/2341633006/diff/280001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_delegate.h (right): https://codereview.chromium.org/2341633006/diff/280001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_delegate.h:88: // Optionally replace it or insert at the cursor if |replace| is true. On 2016/10/25 17:21:49, dmazzoni wrote: > Same here, flip the boolean around and rename it so that it > inserts text by default, and clears the selection if the bool > is set. > > Don't say "optionally" because it's a required parameter, > the caller needs to decide for sure whether to pass true or > false. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
lgtm https://codereview.chromium.org/2341633006/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/2341633006/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:428: SetUserText(new_string, true); Could you use InsertOrReplaceText() when clear_first is false, rather than doing your own string replace here? I'm hoping that might have the right cursor behavior too. https://codereview.chromium.org/2341633006/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views.h (right): https://codereview.chromium.org/2341633006/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.h:132: // selected and replaces it with |new_value|. Then clears the text selection. Doesn't it leave the cursor at the end of |new_value|? Suppose the omnibox contains "bread turkey bread" and "turkey" is selected, and |new_value| is "tofu". If |clear_first| is true, I'd expect the omnibox to contain "tofu" with the cursor at the end. If |clear_first| is false, I'd expect the omnibox to contain "bread tofu bread" with the cursor at the end of "tofu", not at the end of the whole sandwich. https://codereview.chromium.org/2341633006/diff/300001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_delegate.h (right): https://codereview.chromium.org/2341633006/diff/300001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_delegate.h:93: // Whether SetStringValue() and ReplaceSelectedText() are callable, i.e. if Update this comment, you don't have a separate ReplaceSelectedText method anymore https://codereview.chromium.org/2341633006/diff/300001/ui/views/widget/native... File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2341633006/diff/300001/ui/views/widget/native... ui/views/widget/native_widget_mac_accessibility_unittest.mm:363: // Replace a section only. It'd be nice to try replacing something in the middle and check the selection range after the call
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, PTAL. I'm not 100% about whether OmniboxViewViews::AccessibilitySetValue() is correct, but hopefully msw can help out with this. https://codereview.chromium.org/2341633006/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/2341633006/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:428: SetUserText(new_string, true); On 2016/10/26 04:26:02, dmazzoni wrote: > Could you use InsertOrReplaceText() when clear_first is false, rather than doing > your own string replace here? I'm hoping that might have the right cursor > behavior too. Thanks for trying to dig deeper on this - I did originally try this, but the text would always revert back to the original URL whenever InsertOrReplaceText() was used. I had another look after seeing this comment though and I think model()->SetInputInProgress() was the thing I was missing - this seems to prevent the text from being reverted after refocusing on the Omnibox. Hopefully msw@ can confirm if this is the right way to do it. https://codereview.chromium.org/2341633006/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views.h (right): https://codereview.chromium.org/2341633006/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.h:132: // selected and replaces it with |new_value|. Then clears the text selection. On 2016/10/26 04:26:02, dmazzoni wrote: > Doesn't it leave the cursor at the end of |new_value|? > > Suppose the omnibox contains "bread turkey bread" and "turkey" is selected, and > |new_value| is "tofu". > > If |clear_first| is true, I'd expect the omnibox to contain "tofu" with the > cursor at the end. > > If |clear_first| is false, I'd expect the omnibox to contain "bread tofu bread" > with the cursor at the end of "tofu", not at the end of the whole sandwich. That's true - replaced "Then clears the text selection" with "The cursor is then placed at the end of |new_value|.". (Both here and in textfield.h). https://codereview.chromium.org/2341633006/diff/300001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_delegate.h (right): https://codereview.chromium.org/2341633006/diff/300001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_delegate.h:93: // Whether SetStringValue() and ReplaceSelectedText() are callable, i.e. if On 2016/10/26 04:26:02, dmazzoni wrote: > Update this comment, you don't have a separate ReplaceSelectedText method > anymore Done, thanks. https://codereview.chromium.org/2341633006/diff/300001/ui/views/widget/native... File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2341633006/diff/300001/ui/views/widget/native... ui/views/widget/native_widget_mac_accessibility_unittest.mm:363: // Replace a section only. On 2016/10/26 04:26:02, dmazzoni wrote: > It'd be nice to try replacing something in the middle and check the selection > range after the call Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2341633006/diff/340001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_win.cc (right): https://codereview.chromium.org/2341633006/diff/340001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_win.cc:457: if (delegate_->SetStringValue(new_value, false)) should this be 'true' to preserve existing behaviour?
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
lgtm with minor nits/q. https://codereview.chromium.org/2341633006/diff/360001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/2341633006/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:422: SelectRange(saved_selection_for_focus_change_); Should this check if saved_selection_for_focus_change_ is valid? https://codereview.chromium.org/2341633006/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:425: model()->SetInputInProgress(true); nit: call this at the top of the block, to match SetUserText. https://codereview.chromium.org/2341633006/diff/360001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views.h (right): https://codereview.chromium.org/2341633006/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.h:130: // don't normally use this). If |clear_first| is true, clears the existing optional nit: "If |clear_first| is true, this replaces all text with the |new_value|. Otherwise this inserts |new_value| at the cursor position, replacing any selected text. The cursor is placed at the end of |new_value|." https://codereview.chromium.org/2341633006/diff/360001/ui/accessibility/ax_vi... File ui/accessibility/ax_view_state.h (right): https://codereview.chromium.org/2341633006/diff/360001/ui/accessibility/ax_vi... ui/accessibility/ax_view_state.h:78: // things like logging into portals or filling forms. Set the second argument optional nit: make comments regarding this flag consistent. https://codereview.chromium.org/2341633006/diff/360001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_delegate.h (right): https://codereview.chromium.org/2341633006/diff/360001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_delegate.h:93: // Whether SetStringValue() is callable, i.e. if the string value is read only nit: do you mean "if the textfield is not read-only"? https://codereview.chromium.org/2341633006/diff/360001/ui/views/controls/text... File ui/views/controls/textfield/textfield.h (right): https://codereview.chromium.org/2341633006/diff/360001/ui/views/controls/text... ui/views/controls/textfield/textfield.h:368: // don't normally use this). If |clear_first| is true, clears the existing nit: keep this consistent with any changes to the omnibox function comment.
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Thanks all for the reviews! https://codereview.chromium.org/2341633006/diff/340001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_win.cc (right): https://codereview.chromium.org/2341633006/diff/340001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_win.cc:457: if (delegate_->SetStringValue(new_value, false)) On 2016/10/27 02:09:56, tapted wrote: > should this be 'true' to preserve existing behaviour? Yes, thank you! Fixed now. https://codereview.chromium.org/2341633006/diff/360001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/2341633006/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:422: SelectRange(saved_selection_for_focus_change_); On 2016/10/27 17:55:09, msw wrote: > Should this check if saved_selection_for_focus_change_ is valid? Yes - thanks for picking that up! https://codereview.chromium.org/2341633006/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:425: model()->SetInputInProgress(true); On 2016/10/27 17:55:08, msw wrote: > nit: call this at the top of the block, to match SetUserText. Done. https://codereview.chromium.org/2341633006/diff/360001/chrome/browser/ui/view... File chrome/browser/ui/views/omnibox/omnibox_view_views.h (right): https://codereview.chromium.org/2341633006/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/omnibox/omnibox_view_views.h:130: // don't normally use this). If |clear_first| is true, clears the existing On 2016/10/27 17:55:09, msw wrote: > optional nit: "If |clear_first| is true, this replaces all text with the > |new_value|. Otherwise this inserts |new_value| at the cursor position, > replacing any selected text. The cursor is placed at the end of |new_value|." Done, thank you! https://codereview.chromium.org/2341633006/diff/360001/ui/accessibility/ax_vi... File ui/accessibility/ax_view_state.h (right): https://codereview.chromium.org/2341633006/diff/360001/ui/accessibility/ax_vi... ui/accessibility/ax_view_state.h:78: // things like logging into portals or filling forms. Set the second argument On 2016/10/27 17:55:09, msw wrote: > optional nit: make comments regarding this flag consistent. Done (with some edits because using the variable names doesn't make sense in this context). https://codereview.chromium.org/2341633006/diff/360001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_delegate.h (right): https://codereview.chromium.org/2341633006/diff/360001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_delegate.h:93: // Whether SetStringValue() is callable, i.e. if the string value is read only On 2016/10/27 17:55:09, msw wrote: > nit: do you mean "if the textfield is not read-only"? Done, thanks! https://codereview.chromium.org/2341633006/diff/360001/ui/views/controls/text... File ui/views/controls/textfield/textfield.h (right): https://codereview.chromium.org/2341633006/diff/360001/ui/views/controls/text... ui/views/controls/textfield/textfield.h:368: // don't normally use this). If |clear_first| is true, clears the existing On 2016/10/27 17:55:09, msw wrote: > nit: keep this consistent with any changes to the omnibox function comment. Done.
The CQ bit was checked by patricialor@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, dmazzoni@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/2341633006/#ps380001 (title: "Review comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #17 (id:380001)
Message was sent while issue was closed.
Description was changed from ========== MacViews/a11y: Allow accessibility clients to update the selected text. Currently, the `selected text` a11y attribute of views::Textfields is reported as unmodifiable, which differs from native NSTextFields on Mac. Add a callback to views::Textfield and OmniboxViewViews to allow accessibility clients to replace the currently selected text, and plumb through for Mac. When no text is selected, new text is inserted at the current cursor position. This also fixes a problem where selection information provided to accessibility clients was incorrect for the Omnibox when it still had selected text, but focus was not on the window the Omnibox was currently attached to. BUG=610591 TEST=With #mac-views-webui-dialogs turned on in chrome:flags, open the XCode Accessibility Inspector and bring up the HTTP Authentication dialog. Type some text into the username Textfield, then select some text in it. Click on the Accessibility Inspector to move focus off the Chrome window and hover over the Omnibox with the mouse. The Accessibility Inspector should report the correct "accessibilitySelectedTextRange" and "accessibilitySelectedText" attributes. Then press Cmd+F7 to lock onto the Textfield, click on "accessibilitySelectedText" and enter a value at the bottom. The selected text in the Textfield should be replaced with the new string with no text selected. ========== to ========== MacViews/a11y: Allow accessibility clients to update the selected text. Currently, the `selected text` a11y attribute of views::Textfields is reported as unmodifiable, which differs from native NSTextFields on Mac. Add a callback to views::Textfield and OmniboxViewViews to allow accessibility clients to replace the currently selected text, and plumb through for Mac. When no text is selected, new text is inserted at the current cursor position. This also fixes a problem where selection information provided to accessibility clients was incorrect for the Omnibox when it still had selected text, but focus was not on the window the Omnibox was currently attached to. BUG=610591 TEST=With #mac-views-webui-dialogs turned on in chrome:flags, open the XCode Accessibility Inspector and bring up the HTTP Authentication dialog. Type some text into the username Textfield, then select some text in it. Click on the Accessibility Inspector to move focus off the Chrome window and hover over the Omnibox with the mouse. The Accessibility Inspector should report the correct "accessibilitySelectedTextRange" and "accessibilitySelectedText" attributes. Then press Cmd+F7 to lock onto the Textfield, click on "accessibilitySelectedText" and enter a value at the bottom. The selected text in the Textfield should be replaced with the new string with no text selected. Committed: https://crrev.com/0aa1406119dc85097efebbb282895b543e4b0c26 Cr-Commit-Position: refs/heads/master@{#428645} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/0aa1406119dc85097efebbb282895b543e4b0c26 Cr-Commit-Position: refs/heads/master@{#428645} |
