Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1826433002:
DialogClientView: Fix regression in Chrome Task Manager focusing. (Closed)
|
Created:
4 years, 9 months ago by karandeepb Modified:
4 years, 8 months ago Reviewers:
sky 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: Fix regression in Chrome Task Manager focusing.
This CL fixes a regression introduced in
https://codereview.chromium.org/1690133003/. The regression is caused since the
TaskManagerView, adds child views to the DialogClientView. However,
DialogClientView::SetupFocusChain does not account for externally added child
views.
This CL uses View::ReorderChildView to reorder child views and ensure that
appropriate focus order is maintained. Also the redundant contents_ view in
DialogClientViewTest is removed.
BUG=596045, 586419
Committed: https://crrev.com/e5b001fd75dc0fc7dbe265020ffbd84be9916d5c
Cr-Commit-Position: refs/heads/master@{#386324}
Patch Set 1 : #
Total comments: 2
Patch Set 2 : Back to old approach. #Patch Set 3 : #
Total comments: 4
Patch Set 4 : Address review comments. #
Total comments: 2
Patch Set 5 : Address review comments. #Messages
Total messages: 25 (15 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Dialog client view regression. ========== to ========== DialogClientView: Fix regression in Chrome Task Manager focusing. This CL fixes a regression introduced in https://codereview.chromium.org/1690133003/. The regression is caused since the TaskManagerView, adds child views to the DialogClientView. However, DialogClientView::SetupFocusChain does not account for externally added child views. This CL uses a different approach to get the correct focus order amongst the children of DialogClientView, by ensuring "mutable" views like the ok and cancel buttons are added to the view hierarchy in the appropriate focus order. BUG=596045, 586419 ==========
Description was changed from ========== DialogClientView: Fix regression in Chrome Task Manager focusing. This CL fixes a regression introduced in https://codereview.chromium.org/1690133003/. The regression is caused since the TaskManagerView, adds child views to the DialogClientView. However, DialogClientView::SetupFocusChain does not account for externally added child views. This CL uses a different approach to get the correct focus order amongst the children of DialogClientView, by ensuring "mutable" views like the ok and cancel buttons are added to the view hierarchy in the appropriate focus order. BUG=596045, 586419 ========== to ========== DialogClientView: Fix regression in Chrome Task Manager focusing. This CL fixes a regression introduced in https://codereview.chromium.org/1690133003/. The regression is caused since the TaskManagerView, adds child views to the DialogClientView. However, DialogClientView::SetupFocusChain does not account for externally added child views. This CL uses a different approach to get the correct focus order amongst the children of DialogClientView, by ensuring "mutable" views like the ok and cancel buttons are added to the view hierarchy in the appropriate focus order. BUG=596045, 586419 ==========
karandeepb@chromium.org changed reviewers: + sky@chromium.org
PTAL sky@. Thanks.
https://codereview.chromium.org/1826433002/diff/40001/ui/views/window/dialog_... File ui/views/window/dialog_client_view.cc (left): https://codereview.chromium.org/1826433002/diff/40001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:345: void DialogClientView::SetupFocusChain() { This code seems simpler to me. Is there a reason you can't add extra_view_ to child_views? An alternative to SetNextFocusableView is to reorder the views using ReorderChildView.
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:140001) has been deleted
Description was changed from ========== DialogClientView: Fix regression in Chrome Task Manager focusing. This CL fixes a regression introduced in https://codereview.chromium.org/1690133003/. The regression is caused since the TaskManagerView, adds child views to the DialogClientView. However, DialogClientView::SetupFocusChain does not account for externally added child views. This CL uses a different approach to get the correct focus order amongst the children of DialogClientView, by ensuring "mutable" views like the ok and cancel buttons are added to the view hierarchy in the appropriate focus order. BUG=596045, 586419 ========== to ========== DialogClientView: Fix regression in Chrome Task Manager focusing. This CL fixes a regression introduced in https://codereview.chromium.org/1690133003/. The regression is caused since the TaskManagerView, adds child views to the DialogClientView. However, DialogClientView::SetupFocusChain does not account for externally added child views. This CL uses View::ReorderChildView to reorder child views and ensure that appropriate focus order is maintained. BUG=596045, 586419 ==========
PTAL sky@. Thanks. https://codereview.chromium.org/1826433002/diff/40001/ui/views/window/dialog_... File ui/views/window/dialog_client_view.cc (left): https://codereview.chromium.org/1826433002/diff/40001/ui/views/window/dialog_... ui/views/window/dialog_client_view.cc:345: void DialogClientView::SetupFocusChain() { On 2016/03/22 16:19:23, sky wrote: > This code seems simpler to me. Is there a reason you can't add extra_view_ to > child_views? > An alternative to SetNextFocusableView is to reorder the views using > ReorderChildView. extra_view_ is being currently added to child views. The regression occurred since other clients may add views to the DialogClientView, but SetupFocusChain did not take that into account. The changes in dialog_client_view_unittest.cc illustrate this. ReorderChildView is a good suggestion. However I have to modify the SetupFocusChain call site since it's not safe to call ReorderChildView while a view is being removed. For example the code at https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/view.cc&s... assumes that the call to PropagateRemoveNotifications does not alter the position of the view being removed.
https://codereview.chromium.org/1826433002/diff/160001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/1826433002/diff/160001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:364: // Setup focus by reordering views. You should document why this doesn't use SetNextFocusableView. https://codereview.chromium.org/1826433002/diff/160001/ui/views/window/dialog... File ui/views/window/dialog_client_view_unittest.cc (right): https://codereview.chromium.org/1826433002/diff/160001/ui/views/window/dialog... ui/views/window/dialog_client_view_unittest.cc:112: View* contents_; Why do you need contents_? This is a DialogDelegateView that is a view?
Description was changed from ========== DialogClientView: Fix regression in Chrome Task Manager focusing. This CL fixes a regression introduced in https://codereview.chromium.org/1690133003/. The regression is caused since the TaskManagerView, adds child views to the DialogClientView. However, DialogClientView::SetupFocusChain does not account for externally added child views. This CL uses View::ReorderChildView to reorder child views and ensure that appropriate focus order is maintained. BUG=596045, 586419 ========== to ========== DialogClientView: Fix regression in Chrome Task Manager focusing. This CL fixes a regression introduced in https://codereview.chromium.org/1690133003/. The regression is caused since the TaskManagerView, adds child views to the DialogClientView. However, DialogClientView::SetupFocusChain does not account for externally added child views. This CL uses View::ReorderChildView to reorder child views and ensure that appropriate focus order is maintained. Also the redundant contents_ view in DialogClientViewTest is removed. BUG=596045, 586419 ==========
PTAL sky@. Thanks. https://codereview.chromium.org/1826433002/diff/160001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/1826433002/diff/160001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:364: // Setup focus by reordering views. On 2016/04/07 19:09:37, sky wrote: > You should document why this doesn't use SetNextFocusableView. Done. https://codereview.chromium.org/1826433002/diff/160001/ui/views/window/dialog... File ui/views/window/dialog_client_view_unittest.cc (right): https://codereview.chromium.org/1826433002/diff/160001/ui/views/window/dialog... ui/views/window/dialog_client_view_unittest.cc:112: View* contents_; On 2016/04/07 19:09:38, sky wrote: > Why do you need contents_? This is a DialogDelegateView that is a view? Yeah I also thought this was weird. Have refactored the code to remove contents_.
LGTM https://codereview.chromium.org/1826433002/diff/180001/ui/views/window/dialog... File ui/views/window/dialog_client_view_unittest.cc (right): https://codereview.chromium.org/1826433002/diff/180001/ui/views/window/dialog... ui/views/window/dialog_client_view_unittest.cc:20: TestDialogClientView(DialogDelegateView* dialog_delegate_view) explicit
https://codereview.chromium.org/1826433002/diff/180001/ui/views/window/dialog... File ui/views/window/dialog_client_view_unittest.cc (right): https://codereview.chromium.org/1826433002/diff/180001/ui/views/window/dialog... ui/views/window/dialog_client_view_unittest.cc:20: TestDialogClientView(DialogDelegateView* dialog_delegate_view) On 2016/04/08 16:14:59, sky wrote: > explicit Done.
The CQ bit was checked by karandeepb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/1826433002/#ps200001 (title: "Address review comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1826433002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1826433002/200001
Message was sent while issue was closed.
Description was changed from ========== DialogClientView: Fix regression in Chrome Task Manager focusing. This CL fixes a regression introduced in https://codereview.chromium.org/1690133003/. The regression is caused since the TaskManagerView, adds child views to the DialogClientView. However, DialogClientView::SetupFocusChain does not account for externally added child views. This CL uses View::ReorderChildView to reorder child views and ensure that appropriate focus order is maintained. Also the redundant contents_ view in DialogClientViewTest is removed. BUG=596045, 586419 ========== to ========== DialogClientView: Fix regression in Chrome Task Manager focusing. This CL fixes a regression introduced in https://codereview.chromium.org/1690133003/. The regression is caused since the TaskManagerView, adds child views to the DialogClientView. However, DialogClientView::SetupFocusChain does not account for externally added child views. This CL uses View::ReorderChildView to reorder child views and ensure that appropriate focus order is maintained. Also the redundant contents_ view in DialogClientViewTest is removed. BUG=596045, 586419 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== DialogClientView: Fix regression in Chrome Task Manager focusing. This CL fixes a regression introduced in https://codereview.chromium.org/1690133003/. The regression is caused since the TaskManagerView, adds child views to the DialogClientView. However, DialogClientView::SetupFocusChain does not account for externally added child views. This CL uses View::ReorderChildView to reorder child views and ensure that appropriate focus order is maintained. Also the redundant contents_ view in DialogClientViewTest is removed. BUG=596045, 586419 ========== to ========== DialogClientView: Fix regression in Chrome Task Manager focusing. This CL fixes a regression introduced in https://codereview.chromium.org/1690133003/. The regression is caused since the TaskManagerView, adds child views to the DialogClientView. However, DialogClientView::SetupFocusChain does not account for externally added child views. This CL uses View::ReorderChildView to reorder child views and ensure that appropriate focus order is maintained. Also the redundant contents_ view in DialogClientViewTest is removed. BUG=596045, 586419 Committed: https://crrev.com/e5b001fd75dc0fc7dbe265020ffbd84be9916d5c Cr-Commit-Position: refs/heads/master@{#386324} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e5b001fd75dc0fc7dbe265020ffbd84be9916d5c Cr-Commit-Position: refs/heads/master@{#386324} |
