Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2807653002:
Ensure default dialog button focus remains after a dialog update. (Closed)
DescriptionEnsure default dialog button focus remains after a dialog update.
BUG=709346
Review-Url: https://codereview.chromium.org/2807653002
Cr-Commit-Position: refs/heads/master@{#475165}
Committed: https://chromium.googlesource.com/chromium/src/+/db4ba3ee847c91f8d5120757389c96036b01c37d
Patch Set 1 #
Total comments: 2
Patch Set 2 : Update DialogClientView to track if a default button focus exists and ensure it remains after dialo… #
Total comments: 4
Patch Set 3 : Addressing Devlin's comments. #
Total comments: 4
Patch Set 4 : Using a view deletion observer to track focus. #
Total comments: 22
Patch Set 5 : Addressing comments. #
Total comments: 4
Patch Set 6 : Addressing sky's comments. #Patch Set 7 : Updating branch to be in sync with master. #Patch Set 8 : Showing widget for only the focus tests. #
Messages
Total messages: 41 (17 generated)
ackermanb@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
rdevlin.cronin@chromium.org changed reviewers: + sky@chromium.org
This is probably fine, but I'd like views-guru Sky to take a quick look. In particular, it actually seems weird to me that we have to do this, since the ExtensionInstallDialogView sets the default dialog button to be the cancel button. It seems that updating the dialog buttons causes that to be overridden, which is a bit odd. Sky, is that WAI? https://codereview.chromium.org/2807653002/diff/1/chrome/browser/ui/views/ext... File chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2807653002/diff/1/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:219: EXPECT_TRUE(delegate_view->IsDialogButtonEnabled(ui::DIALOG_BUTTON_CANCEL)); Let's add that focus check here, too (the cancel button should be focused each time).
On 2017/04/07 18:13:04, Devlin wrote: > This is probably fine, but I'd like views-guru Sky to take a quick look. In > particular, it actually seems weird to me that we have to do this, since the > ExtensionInstallDialogView sets the default dialog button to be the cancel > button. It seems that updating the dialog buttons causes that to be overridden, > which is a bit odd. Sky, is that WAI? > > https://codereview.chromium.org/2807653002/diff/1/chrome/browser/ui/views/ext... > File > chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc > (right): > > https://codereview.chromium.org/2807653002/diff/1/chrome/browser/ui/views/ext... > chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:219: > EXPECT_TRUE(delegate_view->IsDialogButtonEnabled(ui::DIALOG_BUTTON_CANCEL)); > Let's add that focus check here, too (the cancel button should be focused each > time). I agree this is a work around that shouldn't necessary. DialogClient::SetupLayout() should track if one of the known buttons has focus. If it does, when the children are added back it should restore focus.
Alright, made some updates to SetupLayout that tracks if there is a default focus and makes sure it stays after any dialog updates. You guys mind taking another look? https://codereview.chromium.org/2807653002/diff/1/chrome/browser/ui/views/ext... File chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2807653002/diff/1/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:219: EXPECT_TRUE(delegate_view->IsDialogButtonEnabled(ui::DIALOG_BUTTON_CANCEL)); On 2017/04/07 18:13:04, Devlin wrote: > Let's add that focus check here, too (the cancel button should be focused each > time). Done.
Description was changed from ========== [Extensions UI] Ensure focus is set on the cancel button after install button is enabled. BUG=709346 ========== to ========== Ensure default dialog button focus remains after a dialog update. BUG=709346 ==========
extensions lgtm https://codereview.chromium.org/2807653002/diff/20001/ui/views/window/dialog_... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2807653002/diff/20001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:405: default_focus_->RequestFocus(); Maybe add a comment why we need to do this. https://codereview.chromium.org/2807653002/diff/20001/ui/views/window/dialog_... File ui/views/window/dialog_client_view_unittest.cc (right): https://codereview.chromium.org/2807653002/diff/20001/ui/views/window/dialog_... ui/views/window/dialog_client_view_unittest.cc:412: EXPECT_EQ(GetInitiallyFocusedView(), client_view()->ok_button()); nitty nit: EXPECT_EQ(expected, actual)
Friendly nudge - I think we want this for M59, right?
Yes, I believe we do want it for M59, since my other changes that are affected by this landed there. Sky, can you take a look when you have chance? https://codereview.chromium.org/2807653002/diff/20001/ui/views/window/dialog_... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2807653002/diff/20001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:405: default_focus_->RequestFocus(); On 2017/04/12 19:36:31, Devlin wrote: > Maybe add a comment why we need to do this. Done. https://codereview.chromium.org/2807653002/diff/20001/ui/views/window/dialog_... File ui/views/window/dialog_client_view_unittest.cc (right): https://codereview.chromium.org/2807653002/diff/20001/ui/views/window/dialog_... ui/views/window/dialog_client_view_unittest.cc:412: EXPECT_EQ(GetInitiallyFocusedView(), client_view()->ok_button()); On 2017/04/12 19:36:31, Devlin wrote: > nitty nit: EXPECT_EQ(expected, actual) Done.
https://codereview.chromium.org/2807653002/diff/40001/ui/views/window/dialog_... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2807653002/diff/40001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:322: default_focus_ = GetDialogDelegate()->GetInitiallyFocusedView(); There should be no need for the member as it's only used in this function. I think you need to do the following: You'll need null checks here! View* focused_view = GetFocusManager()->GetFocusedView(); Then add an observer on focused_view. reset of function at end: // You'll need null check here too! if (!GetFocusManager()->GetFocusedView() && focused_view) focused_view->RequestFocus(); The observer tracks OnViewIsDeleting and sets focused_view to null. This is necessary as SetupLayout has side effects that could do weird things. https://codereview.chromium.org/2807653002/diff/40001/ui/views/window/dialog_... File ui/views/window/dialog_client_view_unittest.cc (right): https://codereview.chromium.org/2807653002/diff/40001/ui/views/window/dialog_... ui/views/window/dialog_client_view_unittest.cc:408: // Tests that the default focus of the button reamins after a dialog update. Just to be clear, does these tests fail without your patch?
ackermanb@google.com changed reviewers: + ackermanb@google.com
Hey sky, can you take another look?
https://codereview.chromium.org/2807653002/diff/40001/ui/views/window/dialog_... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2807653002/diff/40001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:322: default_focus_ = GetDialogDelegate()->GetInitiallyFocusedView(); On 2017/04/28 20:49:52, sky wrote: > There should be no need for the member as it's only used in this function. I > think you need to do the following: > > You'll need null checks here! > > View* focused_view = GetFocusManager()->GetFocusedView(); > Then add an observer on focused_view. > > reset of function > > at end: > // You'll need null check here too! > if (!GetFocusManager()->GetFocusedView() && focused_view) > focused_view->RequestFocus(); > > The observer tracks OnViewIsDeleting and sets focused_view to null. This is > necessary as SetupLayout has side effects that could do weird things. Done. https://codereview.chromium.org/2807653002/diff/40001/ui/views/window/dialog_... File ui/views/window/dialog_client_view_unittest.cc (right): https://codereview.chromium.org/2807653002/diff/40001/ui/views/window/dialog_... ui/views/window/dialog_client_view_unittest.cc:408: // Tests that the default focus of the button reamins after a dialog update. On 2017/04/28 20:49:52, sky wrote: > Just to be clear, does these tests fail without your patch? Yep, the tests do fail out without the patch.
(slgtm, just a few nits) https://codereview.chromium.org/2807653002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (right): https://codereview.chromium.org/2807653002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:84: int g_install_delay_in_ms = 500; nit: no need to change this, so let's not (to avoid busying the git history) https://codereview.chromium.org/2807653002/diff/60001/ui/views/window/dialog_... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2807653002/diff/60001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:48: views::View* observed_view_ = nullptr; DISALLOW_COPY_AND_ASSIGN() https://codereview.chromium.org/2807653002/diff/60001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:351: ViewDeletionObserver deletion_observer(GetFocusManager()->GetFocusedView()); nit: Since we use the FocusManager twice in this function, we can cache it in a variable. https://codereview.chromium.org/2807653002/diff/60001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:436: if (!GetFocusManager()->GetFocusedView() && focused_view) drive-by nit: I'd invert these, since checking focused_view is cheaper: if (focused_view && !GetFocusManaged()->GetFocusedView())
https://codereview.chromium.org/2807653002/diff/60001/ui/views/window/dialog_... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2807653002/diff/60001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:25: namespace { Move this code to around line 80ish, inside the view namespace so you don't need view:: everywhere. https://codereview.chromium.org/2807653002/diff/60001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:27: class ViewDeletionObserver : public views::ViewObserver { Add comment. https://codereview.chromium.org/2807653002/diff/60001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:29: ViewDeletionObserver(views::View* observed_view) explicit https://codereview.chromium.org/2807653002/diff/60001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:40: void OnViewIsDeleting(views::View* observed_view) override { Prefix with: // ViewObserver: https://codereview.chromium.org/2807653002/diff/60001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:42: observed_view_ = nullptr; Remove observer here. https://codereview.chromium.org/2807653002/diff/60001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:435: View* focused_view = deletion_observer.focused_view(); Name this previously_focused_view. https://codereview.chromium.org/2807653002/diff/60001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:436: if (!GetFocusManager()->GetFocusedView() && focused_view) On 2017/05/17 18:13:49, Devlin (catching up) wrote: > drive-by nit: I'd invert these, since checking focused_view is cheaper: > if (focused_view && !GetFocusManaged()->GetFocusedView()) Also, add a conditional of Contains(focused_view);
PTAL https://codereview.chromium.org/2807653002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (right): https://codereview.chromium.org/2807653002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:84: int g_install_delay_in_ms = 500; On 2017/05/17 18:13:49, Devlin (catching up) wrote: > nit: no need to change this, so let's not (to avoid busying the git history) I just moved it up a couple of lines because it was annoying me that this got sandwiched in between two functions. This just makes it so now it goes constants, g_install_delay_in_ms, functions. https://codereview.chromium.org/2807653002/diff/60001/ui/views/window/dialog_... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2807653002/diff/60001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:25: namespace { On 2017/05/17 19:08:30, sky wrote: > Move this code to around line 80ish, inside the view namespace so you don't need > view:: everywhere. Done. https://codereview.chromium.org/2807653002/diff/60001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:27: class ViewDeletionObserver : public views::ViewObserver { On 2017/05/17 19:08:30, sky wrote: > Add comment. Done. https://codereview.chromium.org/2807653002/diff/60001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:29: ViewDeletionObserver(views::View* observed_view) On 2017/05/17 19:08:30, sky wrote: > explicit Done. https://codereview.chromium.org/2807653002/diff/60001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:40: void OnViewIsDeleting(views::View* observed_view) override { On 2017/05/17 19:08:30, sky wrote: > Prefix with: > // ViewObserver: Done. https://codereview.chromium.org/2807653002/diff/60001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:42: observed_view_ = nullptr; On 2017/05/17 19:08:30, sky wrote: > Remove observer here. Done. https://codereview.chromium.org/2807653002/diff/60001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:48: views::View* observed_view_ = nullptr; On 2017/05/17 18:13:49, Devlin (catching up) wrote: > DISALLOW_COPY_AND_ASSIGN() Done. https://codereview.chromium.org/2807653002/diff/60001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:351: ViewDeletionObserver deletion_observer(GetFocusManager()->GetFocusedView()); On 2017/05/17 18:13:49, Devlin (catching up) wrote: > nit: Since we use the FocusManager twice in this function, we can cache it in a > variable. Done. https://codereview.chromium.org/2807653002/diff/60001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:435: View* focused_view = deletion_observer.focused_view(); On 2017/05/17 19:08:30, sky wrote: > Name this previously_focused_view. Done. https://codereview.chromium.org/2807653002/diff/60001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:436: if (!GetFocusManager()->GetFocusedView() && focused_view) On 2017/05/17 19:08:30, sky wrote: > On 2017/05/17 18:13:49, Devlin (catching up) wrote: > > drive-by nit: I'd invert these, since checking focused_view is cheaper: > > if (focused_view && !GetFocusManaged()->GetFocusedView()) > > Also, add a conditional of Contains(focused_view); Done. https://codereview.chromium.org/2807653002/diff/60001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:436: if (!GetFocusManager()->GetFocusedView() && focused_view) On 2017/05/17 18:13:49, Devlin (catching up) wrote: > drive-by nit: I'd invert these, since checking focused_view is cheaper: > if (focused_view && !GetFocusManaged()->GetFocusedView()) Done.
LGTM https://codereview.chromium.org/2807653002/diff/80001/ui/views/window/dialog_... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2807653002/diff/80001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:75: View* focused_view() { return observed_view_; } Style guide says accessors like this should match the name of the member. So, name this observed_view(). https://codereview.chromium.org/2807653002/diff/80001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:440: Contains(previously_focused_view)) As the conditional spans multiple lines use {}
Thanks guys! https://codereview.chromium.org/2807653002/diff/80001/ui/views/window/dialog_... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2807653002/diff/80001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:75: View* focused_view() { return observed_view_; } On 2017/05/18 16:05:29, sky wrote: > Style guide says accessors like this should match the name of the member. So, > name this observed_view(). Done. https://codereview.chromium.org/2807653002/diff/80001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:440: Contains(previously_focused_view)) On 2017/05/18 16:05:29, sky wrote: > As the conditional spans multiple lines use {} Done.
The CQ bit was checked by ackermanb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2807653002/#ps100001 (title: "Addressing sky's comments.")
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_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #7 (id:120001) has been deleted
ackermanb@chromium.org changed reviewers: - ackermanb@google.com
The CQ bit was checked by ackermanb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2807653002/#ps140001 (title: "Updating branch to be in sync with master.")
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_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
sky, can you take a quick look at dialog_client_view_unittest? I added widget_->Show() during the test setup so that views_mus_unittests passes
Patchset #8 (id:160001) has been deleted
SLGTM
The CQ bit was checked by ackermanb@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2807653002/#ps180001 (title: "Showing widget for only the focus tests.")
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": 180001, "attempt_start_ts": 1495834121371380,
"parent_rev": "8ae2026e07808d63a1722123f46a5d33e05064a0", "commit_rev":
"db4ba3ee847c91f8d5120757389c96036b01c37d"}
Message was sent while issue was closed.
Description was changed from ========== Ensure default dialog button focus remains after a dialog update. BUG=709346 ========== to ========== Ensure default dialog button focus remains after a dialog update. BUG=709346 Review-Url: https://codereview.chromium.org/2807653002 Cr-Commit-Position: refs/heads/master@{#475165} Committed: https://chromium.googlesource.com/chromium/src/+/db4ba3ee847c91f8d5120757389c... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:180001) as https://chromium.googlesource.com/chromium/src/+/db4ba3ee847c91f8d5120757389c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
