New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 791399 link

Starred by 2 users
Status: Fixed
Owner:
█ mgiuca@chromium.org
Long OOO (go/where-is-mgiuca)
Closed: Apr 2018
Cc:
calamity@chromium.org
dominickn@chromium.org
█ bgalbs@google.com
pcovell@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

Desktop PWA: Three-dot menu overlaps zoom bubble

Project Member Reported by mgiuca@chromium.org, Dec 4 2017 
Chrome Version: (copy from chrome://version)
OS: (e.g. Win7, OSX 10.9.5, etc...)

What steps will reproduce the problem?
(1) Install and open a PWA app in a window.
(2) Click three-dot menu.
(3) Click Zoom + or -.

What is the expected result?
No zoom bubble shown.

What happens instead?
A zoom bubble pops underneath the context menu, partially visible (see screenshot).

This bubble isn't shown when the Chrome menu is used from a browser window. It should also not be shown here.
zoom-overlap.png
126 KB View Download

Comment 1 by calamity@chromium.org, Dec 4 2017 

The line that controls this is at https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_view.cc?l=802&rcl=73505ae3cd98481051c5285993713676467ac31d

BrowserView has no access to the HostedAppButtonContainer which holds the HostedAppMenuButton so we don't know if the menu is showing. This will be made accessible as we work to make the HostedAppButtonContainer available on multiple platforms.

i.e Fix is: Move HostedAppButtonContainer into BrowserNonClientFrameView, then ask it whether the menu is showing to prevent zoom bubble creation.

Comment 2 by mgiuca@chromium.org, Mar 26 2018

Labels: M-67

Comment 3 by mgiuca@chromium.org, Mar 27 2018

Cc: -mgiuca@chromium.org calamity@chromium.org
Owner: mgiuca@chromium.org
This sounds hard (moving HABC into BNCFV).

I took a quick look at this. Current setup is:

- HABC implements BrowserViewButtonProvider which sounds like the perfect way for telling the BrowserView about 3-dot-button related state.
- HABC calls "browser_view_->SetButtonProvider(this)", so BrowserView knows about HABC (as a BrowserViewButtonProvider).
- BrowserView uses "toolbar_->app_menu_button()->IsMenuShowing()" to tell whether the menu is showing, basically hard-wiring up the toolbar as the only way of showing the menu. (Which isn't true, because HABC can also show the menu.)
- ToolbarView also implements BrowserViewButtonProvider.

Thus it seems all we need to do is make IsMenuShowing() a virtual method on BrowserViewButtonProvider. ToolbarView and HABC can both implement it, and BrowserView should call that instead of directly calling ToolbarView.

Chris: If you think this is reasonable, I'll do it. *rolls up sleeves*

Comment 4 by mgiuca@chromium.org, Mar 28 2018

Status: Started (was: Assigned)
https://crrev.com/c/983252 -- WIP CL (it evolved into a large refactor which I'll split up later).

Probably need tests.

Comment 5 by mgiuca@chromium.org, Apr 6 2018

Labels: -Pri-2 Pri-1
Project Member

Comment 6 by bugdroid1@chromium.org, Apr 9 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8ba3e153b566e20156093dd96fc1134ff86b9640

commit 8ba3e153b566e20156093dd96fc1134ff86b9640
Author: Matt Giuca <mgiuca@chromium.org>
Date: Mon Apr 09 03:40:32 2018

Move AppMenuButton out of HostedAppButtonContainer to its own file.

Renames it to HostedAppMenuButton. This was requested in a review of a
larger refactor (https://crrev.com/c/983252).

Bug:  791399 
Change-Id: I173bb8fa11d427da428252dfcb8f19e779523b87
Reviewed-on: https://chromium-review.googlesource.com/999216
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Commit-Queue: Matt Giuca <mgiuca@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549101}
[modify] https://crrev.com/8ba3e153b566e20156093dd96fc1134ff86b9640/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/8ba3e153b566e20156093dd96fc1134ff86b9640/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc
[modify] https://crrev.com/8ba3e153b566e20156093dd96fc1134ff86b9640/chrome/browser/ui/views/frame/hosted_app_button_container.cc
[modify] https://crrev.com/8ba3e153b566e20156093dd96fc1134ff86b9640/chrome/browser/ui/views/frame/hosted_app_button_container.h
[add] https://crrev.com/8ba3e153b566e20156093dd96fc1134ff86b9640/chrome/browser/ui/views/frame/hosted_app_menu_button.cc
[add] https://crrev.com/8ba3e153b566e20156093dd96fc1134ff86b9640/chrome/browser/ui/views/frame/hosted_app_menu_button.h
Project Member

Comment 7 by bugdroid1@chromium.org, Apr 10 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/478049c65a9db363023b6d618562901bdca6fb39

commit 478049c65a9db363023b6d618562901bdca6fb39
Author: Matt Giuca <mgiuca@chromium.org>
Date: Tue Apr 10 06:13:43 2018

Refactor AppMenuButton common base class for browser/hosted app windows.

Moves browser-window-specific functionality from AppMenuButton into new
subclass BrowserAppMenuButton, and made AppMenuButton a base class of
HostedAppButtonContainer::AppMenuButton.

BrowserViewButtonProvider now provides an AppMenuButton, rather than a
generic MenuButton, allowing code that is common to browser and hosted
app windows to interact with the app menu (e.g., detecting whether it is
open, and closing it). This will be used in a future CL to fix the code
that hides the zoom bubble if the menu is open so that it works in
hosted app windows.

Tbr: nyquist@chromium.org
Bug:  791399 
Change-Id: I2bce579b2c220cd70c0a02e95f270e0f3d13ba8d
Reviewed-on: https://chromium-review.googlesource.com/983252
Commit-Queue: Matt Giuca <mgiuca@chromium.org>
Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549433}
[modify] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/feature_engagement/DEPS
[modify] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/feature_engagement/incognito_window/incognito_window_tracker.cc
[modify] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/accessibility/invert_bubble_view.cc
[modify] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/bubble_anchor_util_views.cc
[modify] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/extensions/extension_action_platform_delegate_views.cc
[modify] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc
[modify] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest_views.cc
[add] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/frame/app_menu_button.cc
[add] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/frame/app_menu_button.h
[modify] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc
[modify] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/frame/browser_view.cc
[modify] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/frame/browser_view_button_provider.h
[modify] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/frame/hosted_app_button_container.cc
[modify] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/frame/hosted_app_button_container.h
[modify] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/frame/hosted_app_menu_button.cc
[modify] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/frame/hosted_app_menu_button.h
[modify] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/global_error_bubble_view.cc
[modify] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/ime/ime_warning_bubble_view.cc
[modify] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/keyboard_access_browsertest.cc
[modify] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/media_router/app_menu_test_api_views.cc
[modify] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/network_profile_bubble_view.cc
[modify] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/relaunch_notification/relaunch_recommended_bubble_view.cc
[modify] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/session_crashed_bubble_view.cc
[modify] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/session_crashed_bubble_view_browsertest.cc
[modify] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/toolbar/app_menu.cc
[modify] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/toolbar/app_menu.h
[modify] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/toolbar/browser_action_test_util_views.cc
[modify] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/toolbar/browser_actions_container.cc
[rename] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/toolbar/browser_app_menu_button.cc
[rename] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/toolbar/browser_app_menu_button.h
[modify] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/toolbar/media_router_action_platform_delegate_views.cc
[modify] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/toolbar/toolbar_action_view_interactive_uitest.cc
[modify] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/toolbar/toolbar_view.cc
[modify] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/toolbar/toolbar_view.h
[modify] https://crrev.com/478049c65a9db363023b6d618562901bdca6fb39/chrome/browser/ui/views/toolbar/toolbar_view_interactive_uitest.cc
Project Member

Comment 8 by bugdroid1@chromium.org, Apr 10 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7e651fba081b5a1507f36a844cb33c09ed66d2f5

commit 7e651fba081b5a1507f36a844cb33c09ed66d2f5
Author: Matt Giuca <mgiuca@chromium.org>
Date: Tue Apr 10 08:43:23 2018

Fixed zoom bubble showing up behind app menu in hosted apps.

Previously, this only worked in browser windows. Now, it uses the button
provider to detect if the menu is open, in either browser or hosted app
windows.

Bug:  791399 
Change-Id: I885a4979e729eaedbb647903209c54b63286da22
Reviewed-on: https://chromium-review.googlesource.com/991892
Commit-Queue: Matt Giuca <mgiuca@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549458}
[modify] https://crrev.com/7e651fba081b5a1507f36a844cb33c09ed66d2f5/chrome/browser/ui/views/frame/browser_view.cc

Comment 9 by mgiuca@chromium.org, Apr 11 2018

Status: Fixed (was: Started)
►

Sign in to add a comment