Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1690133003:
DialogClientView: Correct the order in which subviews are focused. (Closed)
|
Created:
4 years, 10 months ago by karandeepb Modified:
4 years, 10 months ago CC:
chromium-reviews, tfarina, 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. |
DescriptionDialogClientView: Correct the order in which subviews are focused.
Currently, the child views of DialogClientView are not focused in the correct
order. This CL explicitly sets the next focusable view of each child view,
whenever the view hierarchy changes, thereby ensuring that the child views are
focused in the correct order.
BUG=586419
TEST=Open Edit Bookmark Dialog. Press Tab to move focus. Observe that the views
are focused in the correct order.
Committed: https://crrev.com/54989662f0ab36fe50b79a27abc362216f8eb85d
Cr-Commit-Position: refs/heads/master@{#376888}
Patch Set 1 #Patch Set 2 : Rebased #
Total comments: 6
Patch Set 3 : Addressed review comments. #
Total comments: 2
Patch Set 4 : Address review comments. #
Total comments: 2
Patch Set 5 : Address review comments. Add tests. #
Total comments: 2
Patch Set 6 : Address review comments. #Patch Set 7 : Fix failing test. #
Total comments: 2
Patch Set 8 : Address review comments. #
Messages
Total messages: 35 (15 generated)
Description was changed from ========== Initial commit BUG= ========== to ========== DialogClientView: Correct the order in which subviews are focused. A DialogClientView contains an "Extra View", OK button, and a cancel button as its subviews. The extra view is always on the left. On Windows and Chrome OS, this is followed by the OK button with the cancel button on the right, while on other platforms, the positions of OK and cancel buttons are reversed. However, currently, the order in which these views are focused is- OK button, cancel button followed by the extra view. This CL ensures that these views are focused from left to right by correcting the order in which they are added to the DialogClientView. BUG=586419 ==========
karandeepb@chromium.org changed reviewers: + tapted@chromium.org
Before I send it for owner's review, can you PTAL Trent. Thanks.
lgtm https://codereview.chromium.org/1690133003/diff/20001/ui/views/window/dialog_... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/1690133003/diff/20001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:118: // Add buttons in left to right order, so that they focus order is left to nit: they -> the is left to right -> matches https://codereview.chromium.org/1690133003/diff/20001/ui/views/window/dialog_... File ui/views/window/dialog_client_view.h (right): https://codereview.chromium.org/1690133003/diff/20001/ui/views/window/dialog_... ui/views/window/dialog_client_view.h:46: void UpdateOKButton(); nit: move to private: section + reorder in .cc
karandeepb@chromium.org changed reviewers: + sky@chromium.org
+sky for ui/views/window/ review. https://codereview.chromium.org/1690133003/diff/20001/ui/views/window/dialog_... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/1690133003/diff/20001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:118: // Add buttons in left to right order, so that they focus order is left to On 2016/02/15 06:24:14, tapted wrote: > nit: they -> the > is left to right -> matches Done. https://codereview.chromium.org/1690133003/diff/20001/ui/views/window/dialog_... File ui/views/window/dialog_client_view.h (right): https://codereview.chromium.org/1690133003/diff/20001/ui/views/window/dialog_... ui/views/window/dialog_client_view.h:46: void UpdateOKButton(); On 2016/02/15 06:24:14, tapted wrote: > nit: move to private: section + reorder in .cc Done.
(still lgtm) https://codereview.chromium.org/1690133003/diff/20001/ui/views/window/dialog_... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/1690133003/diff/20001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:118: // Add buttons in left to right order, so that they focus order is left to On 2016/02/15 06:59:15, karandeepb wrote: > On 2016/02/15 06:24:14, tapted wrote: > > nit: they -> the > > is left to right -> matches > > Done. (I think you missed the second line of this nit :) https://codereview.chromium.org/1690133003/diff/40001/ui/views/window/dialog_... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/1690133003/diff/40001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:356: ok_button_ = NULL; nit: now this is a move in the diff it should probably be updated to nullptr, same below
Thanks Trent. https://codereview.chromium.org/1690133003/diff/20001/ui/views/window/dialog_... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/1690133003/diff/20001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:118: // Add buttons in left to right order, so that they focus order is left to On 2016/02/15 10:22:20, tapted wrote: > On 2016/02/15 06:59:15, karandeepb wrote: > > On 2016/02/15 06:24:14, tapted wrote: > > > nit: they -> the > > > is left to right -> matches > > > > Done. > > (I think you missed the second line of this nit :) Done. https://codereview.chromium.org/1690133003/diff/40001/ui/views/window/dialog_... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/1690133003/diff/40001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:356: ok_button_ = NULL; On 2016/02/15 10:22:20, tapted wrote: > nit: now this is a move in the diff it should probably be updated to nullptr, > same below Done.
Also, test coverage would be nice here. https://codereview.chromium.org/1690133003/diff/60001/ui/views/window/dialog_... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/1690133003/diff/60001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:83: void DialogClientView::UpdateDialogButtons() { Because UpdateDialogButtons can be called at any time, and the delegate could return different values from GetDialogButtons, won't your change fail in certain cases? For example, if we want ok->cancel ordering but cancel is enabled first, then ok, won't the focus order be cancel->ok? Seems like the best solution is to explicitly set next focusable view?
Patchset #5 (id:80001) has been deleted
PTAL sky. Have added a test which fails on master and my earlier patch. Thanks. https://codereview.chromium.org/1690133003/diff/60001/ui/views/window/dialog_... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/1690133003/diff/60001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:83: void DialogClientView::UpdateDialogButtons() { On 2016/02/16 16:47:28, sky wrote: > Because UpdateDialogButtons can be called at any time, and the delegate could > return different values from GetDialogButtons, won't your change fail in certain > cases? For example, if we want ok->cancel ordering but cancel is enabled first, > then ok, won't the focus order be cancel->ok? Seems like the best solution is to > explicitly set next focusable view? Yeah. Thanks for the catch. I am now explicitly setting the next focusable view.
https://codereview.chromium.org/1690133003/diff/100001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/1690133003/diff/100001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:406: child_views.push_back(nullptr); I actually prefer you don't do this. Otherwise we have to worry about signed overflow on 409.
PTAL sky. https://codereview.chromium.org/1690133003/diff/100001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/1690133003/diff/100001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:406: child_views.push_back(nullptr); On 2016/02/17 18:18:26, sky wrote: > I actually prefer you don't do this. Otherwise we have to worry about signed > overflow on 409. Done.
Description was changed from ========== DialogClientView: Correct the order in which subviews are focused. A DialogClientView contains an "Extra View", OK button, and a cancel button as its subviews. The extra view is always on the left. On Windows and Chrome OS, this is followed by the OK button with the cancel button on the right, while on other platforms, the positions of OK and cancel buttons are reversed. However, currently, the order in which these views are focused is- OK button, cancel button followed by the extra view. This CL ensures that these views are focused from left to right by correcting the order in which they are added to the DialogClientView. BUG=586419 ========== to ========== DialogClientView: Correct the order in which subviews are focused. Currently, the child views of DialogClientView are not focused in the correct order. This CL explicitly sets the next focusable view of each child view, whenever the view hierarchy changes, thereby ensuring that the child views are focused in the correct order. BUG=586419 ==========
Description was changed from ========== DialogClientView: Correct the order in which subviews are focused. Currently, the child views of DialogClientView are not focused in the correct order. This CL explicitly sets the next focusable view of each child view, whenever the view hierarchy changes, thereby ensuring that the child views are focused in the correct order. BUG=586419 ========== to ========== DialogClientView: Correct the order in which subviews are focused. Currently, the child views of DialogClientView are not focused in the correct order. This CL explicitly sets the next focusable view of each child view, whenever the view hierarchy changes, thereby ensuring that the child views are focused in the correct order. BUG=586419 TEST=Open Edit Bookmark Dialog. Press Tab to move focus. Observe that the views are focused in the correct order. ==========
Thanks, LGTM
The CQ bit was checked by karandeepb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/1690133003/#ps120001 (title: "Address review comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1690133003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1690133003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
PTAL sky. Tests on the asan trybot had failed since the deletion of the contents_view caused the contents_view pointer to become dangling. This has now been fixed.
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1690133003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1690133003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/1690133003/diff/140001/ui/views/window/client... File ui/views/window/client_view.cc (right): https://codereview.chromium.org/1690133003/diff/140001/ui/views/window/client... ui/views/window/client_view.cc:95: } else if (!details.is_add && details.child == contents_view_) nit {} (because this an else block of a conditional that has {}
https://codereview.chromium.org/1690133003/diff/140001/ui/views/window/client... File ui/views/window/client_view.cc (right): https://codereview.chromium.org/1690133003/diff/140001/ui/views/window/client... ui/views/window/client_view.cc:95: } else if (!details.is_add && details.child == contents_view_) On 2016/02/22 16:38:57, sky wrote: > nit {} (because this an else block of a conditional that has {} Done.
The CQ bit was checked by karandeepb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1690133003/#ps160001 (title: "Address review comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1690133003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1690133003/160001
Message was sent while issue was closed.
Description was changed from ========== DialogClientView: Correct the order in which subviews are focused. Currently, the child views of DialogClientView are not focused in the correct order. This CL explicitly sets the next focusable view of each child view, whenever the view hierarchy changes, thereby ensuring that the child views are focused in the correct order. BUG=586419 TEST=Open Edit Bookmark Dialog. Press Tab to move focus. Observe that the views are focused in the correct order. ========== to ========== DialogClientView: Correct the order in which subviews are focused. Currently, the child views of DialogClientView are not focused in the correct order. This CL explicitly sets the next focusable view of each child view, whenever the view hierarchy changes, thereby ensuring that the child views are focused in the correct order. BUG=586419 TEST=Open Edit Bookmark Dialog. Press Tab to move focus. Observe that the views are focused in the correct order. ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== DialogClientView: Correct the order in which subviews are focused. Currently, the child views of DialogClientView are not focused in the correct order. This CL explicitly sets the next focusable view of each child view, whenever the view hierarchy changes, thereby ensuring that the child views are focused in the correct order. BUG=586419 TEST=Open Edit Bookmark Dialog. Press Tab to move focus. Observe that the views are focused in the correct order. ========== to ========== DialogClientView: Correct the order in which subviews are focused. Currently, the child views of DialogClientView are not focused in the correct order. This CL explicitly sets the next focusable view of each child view, whenever the view hierarchy changes, thereby ensuring that the child views are focused in the correct order. BUG=586419 TEST=Open Edit Bookmark Dialog. Press Tab to move focus. Observe that the views are focused in the correct order. Committed: https://crrev.com/54989662f0ab36fe50b79a27abc362216f8eb85d Cr-Commit-Position: refs/heads/master@{#376888} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/54989662f0ab36fe50b79a27abc362216f8eb85d Cr-Commit-Position: refs/heads/master@{#376888} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
