Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1775533002:
Fixes incorrect clearing of hot-tracked state when context menu is opened from a menu item (Closed)
DescriptionFixes incorrect clearing of hot-tracked state when context menu is opened from a menu item
Broken with the last change here:
https://codereview.chromium.org/1741093002
Since menus can be nested using the same MenuController I think it would be correct to move |hot_button_| and its accessors into MenuItemView. This way there can be multiple views in hot-tracked state in the same MenuController as long as only one hot-tracked view is in any given menu item.
BUG=592359
Committed: https://crrev.com/34943cb4f328c269cb53091c0702c5380a675407
Cr-Commit-Position: refs/heads/master@{#383128}
Patch Set 1 : Moves hot_button tracking from MenuController to MenuItemView #Patch Set 2 : Moves hot_button tracking from MenuController to MenuItemView #
Total comments: 4
Patch Set 3 : Moves hot_button tracking from MenuController to MenuController::State #
Total comments: 4
Patch Set 4 : Moves hot_button tracking from MenuController to MenuController::State (comments) #Patch Set 5 : Moves hot_button tracking from MenuController to MenuController::State (unit test) #
Total comments: 2
Messages
Total messages: 38 (17 generated)
Description was changed from ========== Fixes incorrect clearing of hot-tracked state when context menu is opened from a menu item Broken with the last change here: https://codereview.chromium.org/1741093002/diff/60001/ui/views/controls/menu/... BUG=592359 ========== to ========== Fixes incorrect clearing of hot-tracked state when context menu is opened from a menu item Broken with the last change here: https://codereview.chromium.org/1741093002 Since menus can be nested using the same MenuController I think it would be correct to move |hot_button_| and its accessors into MenuItemView. This way there can be multiple views in hot-tracked state in the same MenuController as long as only one hot-tracked view is in any given menu item. BUG=592359 ==========
The CQ bit was checked by varkha@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/1775533002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1775533002/20001
varkha@chromium.org changed reviewers: + sky@chromium.org
sky@, can you please take a look? The bug got triggered because extension button context menu shares menu controller with the parent app menu. I have refactored most of the dealing with |hot_button_| into the MenuItemView. Thanks! https://codereview.chromium.org/1775533002/diff/20001/ui/views/controls/menu/... File ui/views/controls/menu/menu_controller.cc (left): https://codereview.chromium.org/1775533002/diff/20001/ui/views/controls/menu/... ui/views/controls/menu/menu_controller.cc:1068: SetHotTrackedButton(nullptr); This has been moved inside current_path[i]->SetSelected(false);
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by varkha@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/1775533002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1775533002/40001
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by varkha@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/1775533002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1775533002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
How about some test coverage? https://codereview.chromium.org/1775533002/diff/60001/ui/views/controls/menu/... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1775533002/diff/60001/ui/views/controls/menu/... ui/views/controls/menu/menu_controller.cc:562: button->GetAncestorWithClassName(MenuItemView::kViewClassName)); What if button is null? https://codereview.chromium.org/1775533002/diff/60001/ui/views/controls/menu/... File ui/views/controls/menu/menu_controller.h (right): https://codereview.chromium.org/1775533002/diff/60001/ui/views/controls/menu/... ui/views/controls/menu/menu_controller.h:238: struct State { This struct is meant to track state related to running of a nested menu. hot_button should move here.
Patchset #3 (id:80001) has been deleted
sky@, can you PTAL if this is what you had in mind? Still need to add some testing for this. https://codereview.chromium.org/1775533002/diff/60001/ui/views/controls/menu/... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1775533002/diff/60001/ui/views/controls/menu/... ui/views/controls/menu/menu_controller.cc:562: button->GetAncestorWithClassName(MenuItemView::kViewClassName)); On 2016/03/08 16:45:37, sky wrote: > What if button is null? Acknowledged (this is no longer necessary if I don't use menu items for hot-tracking). https://codereview.chromium.org/1775533002/diff/60001/ui/views/controls/menu/... File ui/views/controls/menu/menu_controller.h (right): https://codereview.chromium.org/1775533002/diff/60001/ui/views/controls/menu/... ui/views/controls/menu/menu_controller.h:238: struct State { On 2016/03/08 16:45:37, sky wrote: > This struct is meant to track state related to running of a nested menu. > hot_button should move here. Done.
https://codereview.chromium.org/1775533002/diff/100001/ui/views/controls/menu... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1775533002/diff/100001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:367: hot_button(NULL), nullptr (feel free to change NULL on the previous line too). https://codereview.chromium.org/1775533002/diff/100001/ui/views/controls/menu... File ui/views/controls/menu/menu_controller.h (left): https://codereview.chromium.org/1775533002/diff/100001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.h:663: CustomButton* hot_button_; I recommend keeping the member around and only adding to state when changing menu_stack_. I say that as the hot button should be updated immediately, where as pending_state_ is meant to be the state that will change after a delay.
The CQ bit was checked by varkha@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/1775533002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1775533002/140001
PTAL. Also added a test that checks that the hot-tracked button state as well as the |hot_button_| state in the MenuController are restored. https://codereview.chromium.org/1775533002/diff/100001/ui/views/controls/menu... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1775533002/diff/100001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:367: hot_button(NULL), On 2016/03/23 21:42:40, sky wrote: > nullptr (feel free to change NULL on the previous line too). Done. https://codereview.chromium.org/1775533002/diff/100001/ui/views/controls/menu... File ui/views/controls/menu/menu_controller.h (left): https://codereview.chromium.org/1775533002/diff/100001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.h:663: CustomButton* hot_button_; On 2016/03/23 21:42:40, sky wrote: > I recommend keeping the member around and only adding to state when changing > menu_stack_. I say that as the hot button should be updated immediately, where > as pending_state_ is meant to be the state that will change after a delay. Done.
jonross@chromium.org changed reviewers: + jonross@chromium.org
https://codereview.chromium.org/1775533002/diff/140001/ui/views/controls/menu... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1775533002/diff/140001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:2606: hot_button_->SetHotTracked(true); Is it possible for this hot_button_ to no longer be valid if we have CloseAllNestedMenus()? (line 2591)
One question though. If we cache the hot button before opening another menu and restore the hot button on closing the inner menu it means we'll be using the previously hot button. That really only makes sense if the mouse is currently in exactly the same place as it was when the inner menu opened. Seems like what we want is to lookup the hot button based on the mouse position when closing the inner menu. WDYT? https://codereview.chromium.org/1775533002/diff/140001/ui/views/controls/menu... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1775533002/diff/140001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:2606: hot_button_->SetHotTracked(true); On 2016/03/24 18:48:18, jonross wrote: > Is it possible for this hot_button_ to no longer be valid if we have > CloseAllNestedMenus()? (line 2591) In looking at it, this method is confusingly named. It's really HideAllNestedMenus. So, I believe you would still get a ViewHierarchyChanged() if after CloseAllNestedMenus() the button is deleted.
On 2016/03/24 19:12:56, sky wrote: > One question though. If we cache the hot button before opening another menu and > restore the hot button on closing the inner menu it means we'll be using the > previously hot button. That really only makes sense if the mouse is currently in > exactly the same place as it was when the inner menu opened. Seems like what we > want is to lookup the hot button based on the mouse position when closing the > inner menu. WDYT? I don't think this would be right. Consider purely keyboard operation. Open a menu (say Alt+E), [Down], [Down] to select an item, [Context menu / Alt+Space] to open a nested menu. [Esc] to close it. I would expect the hot-tracking to be still where it was before opening a context menu regardless of a mouse location. I thought hot-tracking is more following "current" menu focus position and not merely a hover effect.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Good point. LGTM
On 2016/03/24 19:55:37, sky wrote: > Good point. LGTM LGTM
The CQ bit was checked by varkha@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1775533002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1775533002/140001
Message was sent while issue was closed.
Description was changed from ========== Fixes incorrect clearing of hot-tracked state when context menu is opened from a menu item Broken with the last change here: https://codereview.chromium.org/1741093002 Since menus can be nested using the same MenuController I think it would be correct to move |hot_button_| and its accessors into MenuItemView. This way there can be multiple views in hot-tracked state in the same MenuController as long as only one hot-tracked view is in any given menu item. BUG=592359 ========== to ========== Fixes incorrect clearing of hot-tracked state when context menu is opened from a menu item Broken with the last change here: https://codereview.chromium.org/1741093002 Since menus can be nested using the same MenuController I think it would be correct to move |hot_button_| and its accessors into MenuItemView. This way there can be multiple views in hot-tracked state in the same MenuController as long as only one hot-tracked view is in any given menu item. BUG=592359 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Fixes incorrect clearing of hot-tracked state when context menu is opened from a menu item Broken with the last change here: https://codereview.chromium.org/1741093002 Since menus can be nested using the same MenuController I think it would be correct to move |hot_button_| and its accessors into MenuItemView. This way there can be multiple views in hot-tracked state in the same MenuController as long as only one hot-tracked view is in any given menu item. BUG=592359 ========== to ========== Fixes incorrect clearing of hot-tracked state when context menu is opened from a menu item Broken with the last change here: https://codereview.chromium.org/1741093002 Since menus can be nested using the same MenuController I think it would be correct to move |hot_button_| and its accessors into MenuItemView. This way there can be multiple views in hot-tracked state in the same MenuController as long as only one hot-tracked view is in any given menu item. BUG=592359 Committed: https://crrev.com/34943cb4f328c269cb53091c0702c5380a675407 Cr-Commit-Position: refs/heads/master@{#383128} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/34943cb4f328c269cb53091c0702c5380a675407 Cr-Commit-Position: refs/heads/master@{#383128} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
