Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(375)

Issue 1898633004: Views: Add new SetFocusBehavior method. (Closed)

Created:
4 years, 8 months ago by karandeepb
Modified:
4 years, 7 months ago
Reviewers:
tapted, sky, Evan Stade
CC:
chromium-reviews, asanka, msramek+watch_chromium.org, sadrul, yusukes+watch_chromium.org, bondd+autofillwatch_chromium.org, noyau+watch_chromium.org, markusheintz_, tapted, msw+watch_chromium.org, hcarmona+bubble_chromium.org, Matt Giuca, raymes+watch_chromium.org, nona+watch_chromium.org, rouslan+bubble_chromium.org, vabr+watchlistautofill_chromium.org, kalyank, vabr+watchlistpasswordmanager_chromium.org, rouslan+autofill_chromium.org, mlamouri+watch-notifications_chromium.org, oshima+watch_chromium.org, gcasto+watchlist_chromium.org, dbeam+watch-downloads_chromium.org, sync-reviews_chromium.org, mkwst+watchlist-passwords_chromium.org, groby+bubble_chromium.org, Peter Beverloo, tfarina, shuchen+watch_chromium.org, jdonnelly+autofillwatch_chromium.org, estade+watch_chromium.org, James Su, davemoore+watch_chromium.org, chrome-apps-syd-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Views: Add new SetFocusBehavior method. This CL adds a new SetFocusBehavior method to View class which takes a FocusBehavior enum, replacing the existing SetFocusable and SetAccessibilityFocusable methods. Motivation for this change: - SetFocusBehavior helps describe the focus behavior of a view completely and concisely. This is especially convenient considering the changes crrev.com/1894383002/ will bring. - It helps avoid a meaningless state where focusable_ is true and accessibility_focusable_ is false for a view. This is first in part of the CLs to implement Full Keyboard Access on MacViews. For prior discussion related to this CL, see crrev.com/1690543004. BUG=564912 Committed: https://crrev.com/e7ad02a20c1957caa8a014f17e090dde02573179 Cr-Commit-Position: refs/heads/master@{#390244}

Patch Set 1 : #

Patch Set 2 : Rebased. #

Total comments: 31

Patch Set 3 : Address review comments. #

Total comments: 6

Patch Set 4 : Address nits. #

Total comments: 4

Patch Set 5 : Rebased. #

Patch Set 6 : Removed unneeded SetFocusBehavior(FocusBehavior::NEVER) calls #

Patch Set 7 : Rebased. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+205 lines, -219 lines) Patch
M ash/focus_cycler_unittest.cc View 1 chunk +4 lines, -4 lines 0 comments Download
M ash/shelf/overflow_button.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M ash/shelf/shelf_button.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M ash/shell/lock_view.cc View 1 chunk +1 line, -1 line 0 comments Download
M ash/sticky_keys/sticky_keys_overlay.cc View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
M ash/system/audio/volume_view.cc View 1 chunk +1 line, -1 line 0 comments Download
M ash/system/chromeos/network/network_state_list_detailed_view.cc View 1 2 3 4 2 chunks +5 lines, -5 lines 0 comments Download
M ash/system/date/date_view.cc View 1 2 4 chunks +3 lines, -3 lines 0 comments Download
M ash/system/toast/toast_overlay.cc View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
M ash/system/tray/actionable_view.cc View 1 chunk +1 line, -1 line 0 comments Download
M ash/system/tray/tray_details_view_unittest.cc View 1 chunk +1 line, -1 line 0 comments Download
M ash/system/tray/tray_popup_header_button.cc View 1 chunk +1 line, -1 line 0 comments Download
M ash/system/tray/tray_popup_label_button.cc View 1 chunk +1 line, -1 line 0 comments Download
M ash/system/user/button_from_view.cc View 1 chunk +1 line, -1 line 0 comments Download
M ash/wm/immersive_fullscreen_controller_unittest.cc View 1 2 3 4 5 6 2 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/apps/guest_view/web_view_browsertest.cc View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/chromeos/options/wifi_config_view.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/chromeos/options/wimax_config_view.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_panel.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/autofill/autofill_dialog_views.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/autofill/autofill_dialog_views_unittest.cc View 1 chunk +4 lines, -2 lines 0 comments Download
M chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/constrained_window_views_browsertest.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/desktop_media_picker_views.cc View 1 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/views/download/download_item_view.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/download/download_item_view_md.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/exclusive_access_bubble_views.cc View 1 2 3 4 5 3 chunks +3 lines, -4 lines 0 comments Download
M chrome/browser/ui/views/find_bar_view.cc View 1 3 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/ui/views/frame/browser_view.cc View 1 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/ui/views/ime/ime_window_view.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/infobars/infobar_view.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/views/location_bar/bubble_icon_view.cc View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/location_bar/content_setting_image_view.cc View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/location_bar/location_bar_decoration_view.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/location_bar/location_bar_view.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/location_bar/location_icon_view.cc View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/location_bar/open_pdf_in_reader_view.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/location_bar/page_action_image_view.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/passwords/credentials_item_view.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/passwords/manage_passwords_icon_views.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/profiles/profile_chooser_view.cc View 3 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/toolbar/app_menu.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/views/toolbar/toolbar_action_view.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/toolbar/toolbar_button.cc View 1 2 3 4 1 chunk +1 line, -2 lines 0 comments Download
M chrome/browser/ui/views/website_settings/chosen_object_view.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/website_settings/permission_selector_view.cc View 1 2 2 chunks +1 line, -1 line 0 comments Download
M ui/app_list/views/custom_launcher_page_view.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M ui/app_list/views/folder_header_view.cc View 1 chunk +1 line, -1 line 0 comments Download
M ui/app_list/views/search_result_actions_view.cc View 1 chunk +1 line, -1 line 0 comments Download
M ui/app_list/views/start_page_view.cc View 1 chunk +1 line, -1 line 0 comments Download
M ui/message_center/views/message_center_button_bar.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M ui/message_center/views/message_center_view.cc View 1 chunk +2 lines, -1 line 0 comments Download
M ui/message_center/views/message_view.cc View 1 chunk +1 line, -1 line 0 comments Download
M ui/message_center/views/notification_button.cc View 1 chunk +1 line, -1 line 0 comments Download
M ui/message_center/views/notifier_settings_view.cc View 5 chunks +5 lines, -5 lines 0 comments Download
M ui/message_center/views/padded_button.cc View 1 chunk +1 line, -1 line 0 comments Download
M ui/views/bubble/bubble_dialog_delegate_unittest.cc View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M ui/views/color_chooser/color_chooser_view.cc View 1 2 3 4 5 4 chunks +0 lines, -4 lines 0 comments Download
M ui/views/controls/button/button.cc View 1 chunk +1 line, -1 line 0 comments Download
M ui/views/controls/button/checkbox.cc View 1 chunk +1 line, -1 line 0 comments Download
M ui/views/controls/button/custom_button.h View 1 2 3 4 1 chunk +2 lines, -2 lines 0 comments Download
M ui/views/controls/button/image_button.h View 1 chunk +2 lines, -2 lines 0 comments Download
M ui/views/controls/button/label_button.cc View 1 chunk +1 line, -1 line 0 comments Download
M ui/views/controls/button/md_text_button.cc View 1 chunk +1 line, -1 line 0 comments Download
M ui/views/controls/combobox/combobox.cc View 3 chunks +2 lines, -3 lines 0 comments Download
M ui/views/controls/label_unittest.cc View 1 2 4 chunks +2 lines, -4 lines 0 comments Download
M ui/views/controls/link.cc View 1 2 2 chunks +5 lines, -4 lines 0 comments Download
M ui/views/controls/menu/menu_controller_unittest.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M ui/views/controls/scrollbar/native_scroll_bar_views.cc View 1 2 3 4 5 2 chunks +1 line, -4 lines 0 comments Download
M ui/views/controls/separator.cc View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
M ui/views/controls/slider.cc View 1 chunk +1 line, -1 line 0 comments Download
M ui/views/controls/tabbed_pane/tabbed_pane.cc View 1 chunk +1 line, -1 line 0 comments Download
M ui/views/controls/table/table_view.cc View 1 chunk +1 line, -1 line 0 comments Download
M ui/views/controls/textfield/textfield.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M ui/views/controls/textfield/textfield_unittest.cc View 1 1 chunk +2 lines, -2 lines 0 comments Download
M ui/views/controls/tree/tree_view.cc View 1 chunk +1 line, -1 line 0 comments Download
M ui/views/controls/webview/webview.cc View 1 chunk +2 lines, -1 line 0 comments Download
M ui/views/controls/webview/webview_unittest.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M ui/views/examples/button_example.cc View 3 chunks +5 lines, -3 lines 0 comments Download
M ui/views/examples/tree_view_example.cc View 1 chunk +3 lines, -3 lines 0 comments Download
M ui/views/examples/widget_example.cc View 1 chunk +1 line, -1 line 0 comments Download
M ui/views/focus/focus_manager_unittest.cc View 1 2 3 4 8 chunks +16 lines, -16 lines 0 comments Download
M ui/views/focus/focus_traversal_unittest.cc View 3 chunks +7 lines, -6 lines 0 comments Download
M ui/views/test/widget_test.cc View 1 chunk +1 line, -1 line 0 comments Download
M ui/views/touchui/touch_selection_menu_runner_views.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M ui/views/view.h View 1 2 3 4 chunks +20 lines, -21 lines 0 comments Download
M ui/views/view.cc View 1 2 3 4 5 6 2 chunks +6 lines, -15 lines 0 comments Download
M ui/views/view_targeter_unittest.cc View 1 chunk +1 line, -1 line 0 comments Download
M ui/views/view_unittest.cc View 1 2 4 chunks +12 lines, -11 lines 0 comments Download
M ui/views/widget/native_widget_mac_unittest.mm View 1 1 chunk +1 line, -1 line 0 comments Download
M ui/views/widget/root_view_unittest.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M ui/views/widget/widget_interactive_uitest.cc View 1 6 chunks +9 lines, -8 lines 0 comments Download
M ui/views/widget/widget_unittest.cc View 1 3 chunks +3 lines, -3 lines 0 comments Download
M ui/views/window/dialog_client_view.cc View 1 chunk +1 line, -1 line 0 comments Download

Dependent Patchsets:

Messages

Total messages: 34 (14 generated)
karandeepb
PTAL sky@ and Trent. Thanks.
4 years, 8 months ago (2016-04-20 01:31:47 UTC) #9
tapted
checked all the call-sites https://codereview.chromium.org/1898633004/diff/120001/ash/system/date/date_view.cc File ash/system/date/date_view.cc (right): https://codereview.chromium.org/1898633004/diff/120001/ash/system/date/date_view.cc#newcode141 ash/system/date/date_view.cc:141: SetFocusBehavior(FocusBehavior::NEVER); (wait for sky to ...
4 years, 8 months ago (2016-04-20 04:16:28 UTC) #10
sky
https://codereview.chromium.org/1898633004/diff/120001/ash/system/date/date_view.cc File ash/system/date/date_view.cc (right): https://codereview.chromium.org/1898633004/diff/120001/ash/system/date/date_view.cc#newcode141 ash/system/date/date_view.cc:141: SetFocusBehavior(FocusBehavior::NEVER); On 2016/04/20 04:16:28, tapted wrote: > (wait for ...
4 years, 8 months ago (2016-04-20 19:26:02 UTC) #11
Evan Stade
forgive the driveby. I looked at the other CL but I couldn't easily find an ...
4 years, 8 months ago (2016-04-20 21:17:04 UTC) #13
karandeepb
PTAL Trent. Thanks. https://codereview.chromium.org/1898633004/diff/120001/ash/sticky_keys/sticky_keys_overlay.cc File ash/sticky_keys/sticky_keys_overlay.cc (right): https://codereview.chromium.org/1898633004/diff/120001/ash/sticky_keys/sticky_keys_overlay.cc#newcode70 ash/sticky_keys/sticky_keys_overlay.cc:70: SetFocusBehavior(FocusBehavior::NEVER); On 2016/04/20 21:17:03, Evan Stade ...
4 years, 8 months ago (2016-04-21 03:16:34 UTC) #15
karandeepb
On 2016/04/20 21:17:04, Evan Stade wrote: > forgive the driveby. I looked at the other ...
4 years, 8 months ago (2016-04-21 03:18:34 UTC) #16
tapted
lgtm - just some nits and a question https://codereview.chromium.org/1898633004/diff/140001/ui/views/view.h File ui/views/view.h (right): https://codereview.chromium.org/1898633004/diff/140001/ui/views/view.h#newcode120 ui/views/view.h:120: // ...
4 years, 8 months ago (2016-04-21 05:33:07 UTC) #17
karandeepb
PTAL sky@ and estade@. Thanks. https://codereview.chromium.org/1898633004/diff/140001/ui/views/view.h File ui/views/view.h (right): https://codereview.chromium.org/1898633004/diff/140001/ui/views/view.h#newcode120 ui/views/view.h:120: // Use when the ...
4 years, 8 months ago (2016-04-21 10:33:33 UTC) #18
sky
https://codereview.chromium.org/1898633004/diff/160001/chrome/browser/ui/views/website_settings/permission_selector_view.cc File chrome/browser/ui/views/website_settings/permission_selector_view.cc (right): https://codereview.chromium.org/1898633004/diff/160001/chrome/browser/ui/views/website_settings/permission_selector_view.cc#newcode74 chrome/browser/ui/views/website_settings/permission_selector_view.cc:74: SetFocusBehavior(FocusBehavior::ALWAYS); Why the change here? https://codereview.chromium.org/1898633004/diff/160001/ui/views/view.h File ui/views/view.h (right): ...
4 years, 8 months ago (2016-04-21 16:44:17 UTC) #19
karandeepb
https://codereview.chromium.org/1898633004/diff/160001/chrome/browser/ui/views/website_settings/permission_selector_view.cc File chrome/browser/ui/views/website_settings/permission_selector_view.cc (right): https://codereview.chromium.org/1898633004/diff/160001/chrome/browser/ui/views/website_settings/permission_selector_view.cc#newcode74 chrome/browser/ui/views/website_settings/permission_selector_view.cc:74: SetFocusBehavior(FocusBehavior::ALWAYS); On 2016/04/21 16:44:17, sky wrote: > Why the ...
4 years, 8 months ago (2016-04-22 08:38:29 UTC) #20
Evan Stade
thanks, no further comments from my end https://codereview.chromium.org/1898633004/diff/120001/ash/sticky_keys/sticky_keys_overlay.cc File ash/sticky_keys/sticky_keys_overlay.cc (right): https://codereview.chromium.org/1898633004/diff/120001/ash/sticky_keys/sticky_keys_overlay.cc#newcode70 ash/sticky_keys/sticky_keys_overlay.cc:70: SetFocusBehavior(FocusBehavior::NEVER); On ...
4 years, 8 months ago (2016-04-25 21:30:52 UTC) #21
Evan Stade
https://codereview.chromium.org/1898633004/diff/120001/ash/sticky_keys/sticky_keys_overlay.cc File ash/sticky_keys/sticky_keys_overlay.cc (right): https://codereview.chromium.org/1898633004/diff/120001/ash/sticky_keys/sticky_keys_overlay.cc#newcode70 ash/sticky_keys/sticky_keys_overlay.cc:70: SetFocusBehavior(FocusBehavior::NEVER); On 2016/04/25 21:30:52, Evan Stade wrote: > On ...
4 years, 8 months ago (2016-04-25 21:32:33 UTC) #22
sky
All container type views want NEVER, right? By container I mean a view that exists ...
4 years, 8 months ago (2016-04-25 23:00:56 UTC) #23
Evan Stade
On 2016/04/25 23:00:56, sky wrote: > All container type views want NEVER, right? By container ...
4 years, 8 months ago (2016-04-25 23:39:44 UTC) #24
sky
Got it. I thought you were suggesting we nuke NEVER. On Mon, Apr 25, 2016 ...
4 years, 8 months ago (2016-04-26 00:05:55 UTC) #25
karandeepb
PTAL sky@. I have removed some reduantant SetFocusBehavior(FocusBehavior::NEVER) calls. If it's ok can the removal ...
4 years, 8 months ago (2016-04-27 08:02:57 UTC) #26
sky
LGTM - you can remove focusable() separately.
4 years, 7 months ago (2016-04-27 16:31:07 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1898633004/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1898633004/220001
4 years, 7 months ago (2016-04-27 23:46:28 UTC) #30
commit-bot: I haz the power
Committed patchset #7 (id:220001)
4 years, 7 months ago (2016-04-27 23:57:38 UTC) #32
commit-bot: I haz the power
4 years, 7 months ago (2016-04-30 17:14:43 UTC) #33
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/e7ad02a20c1957caa8a014f17e090dde02573179
Cr-Commit-Position: refs/heads/master@{#390244}

Powered by Google App Engine
This is Rietveld 408576698