New issue
Advanced search Search tips

Issue 913998 link

Starred by 2 users

Issue metadata

Status: Started
Owner:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Layout-fighting between MenuItemView and ExtensionToolbarMenuView

Project Member Reported by dfried@chromium.org, Dec 11

Issue description

The 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.
 
Labels: Group-Platform
Owner: collinbaker@chromium.org
Status: Started (was: Available)
Project Member

Comment 3 by bugdroid1@chromium.org, 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