Layout-fighting between MenuItemView and ExtensionToolbarMenuView |
||
Issue descriptionThe extensions overflow bar in the main app menu is a turducken, and not the tasty kind. It is: - A BrowserActionsContainer - Wrapped in a ScrollView::Viewport - Wrapped in a ScrollView - Which is an ExtensionToolbarMenuView - Wrapped in a MenuItemView The problem is that there is "container" logic built into MenuItemView, which only seems to apply to this one case, but which doesn't position the ExtensionToolbarMenuView correctly. To fix this, ExtensionToolbarMenuView::Layout() changes both its size and position within its parent on Layout(). This means the order in which Layout() is called on these controls affects whether the inner control is positioned properly. This is an antipattern and should be fixed. No view should ever move itself during layout. I have put in a temporary fix to make the container behavior of MenuItemView call the child view's Layout() explicitly, but I want this to be fixed in a more robust way.
,
Jan 2
,
Jan 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dfeb060f50d5041e480d2f008cebb17d5307ad19 commit dfeb060f50d5041e480d2f008cebb17d5307ad19 Author: Collin Baker <collinbaker@chromium.org> Date: Fri Jan 11 22:31:01 2019 Use margins in ExtensionToolbarMenuView rather than laying out self Currently, ExtensionToolbarMenuView sets its own position in Layout(). This change sets the margins property instead, and modifies MenuItemView::Layout to use this property. Bug: 913998 Change-Id: Iab6305a0ad3dfc4fd936d17095ef6a65743e439c Reviewed-on: https://chromium-review.googlesource.com/c/1393461 Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Reviewed-by: Peter Boström <pbos@chromium.org> Reviewed-by: Dana Fried <dfried@chromium.org> Commit-Queue: Collin Baker <collinbaker@chromium.org> Cr-Commit-Position: refs/heads/master@{#622190} [modify] https://crrev.com/dfeb060f50d5041e480d2f008cebb17d5307ad19/chrome/browser/ui/views/toolbar/app_menu.cc [modify] https://crrev.com/dfeb060f50d5041e480d2f008cebb17d5307ad19/chrome/browser/ui/views/toolbar/app_menu_observer.h [modify] https://crrev.com/dfeb060f50d5041e480d2f008cebb17d5307ad19/chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc [modify] https://crrev.com/dfeb060f50d5041e480d2f008cebb17d5307ad19/chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.h [modify] https://crrev.com/dfeb060f50d5041e480d2f008cebb17d5307ad19/ui/views/controls/menu/menu_item_view.cc [modify] https://crrev.com/dfeb060f50d5041e480d2f008cebb17d5307ad19/ui/views/controls/menu/menu_item_view_unittest.cc |
||
►
Sign in to add a comment |
||
Comment 1 by robliao@chromium.org
, Dec 13