New issue
Advanced search Search tips

Issue 862005 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 20
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug



Sign in to add a comment

Extension icons disappear when pushed to dotted menu

Project Member Reported by agektmr@chromium.org, Jul 10

Issue description

Chrome Version: Version 69.0.3486.0 (Official Build) canary (64-bit)
OS: Mac OS X 10.13.5

What steps will reproduce the problem?
(1) Add multiple Chrome extensions
(2) Display them beside omnibar
(3) Push them onto menu bar by dragging left edge of left most extension to the right

What is the expected result?
See all hidden installed and active Chrome extensions in the menu.

What happens instead?
Only see couple of them in the menu.


 
full-list.png
12.2 KB View Download
hidden-list.png
15.5 KB View Download
Labels: -Pri-3 Needs-Bisect Pri-1
Looks like a regression.
Cc: viswa.karala@chromium.org
Labels: -Needs-Bisect hasbisect-per-revision Triaged-ET M-69 RegressedIn-69 ReleaseBlock-Beta Needs-Triage-M69 Target-69 FoundIn-69
Owner: pmonette@chromium.org
Status: Assigned (was: Untriaged)
Able to reproduce the issue on reported version 69.0.3486.0 and latest chrome 69.0.3488.0 using Mac 10.13.5, hence providing Bisect Info
Note: Issue is not seen on Linux and Windows
Bisect Info:
================
Good build: 69.0.3456.0
Bad build: 69.0.3457.0

You are probably looking for a change made after 566524 (known good), but no later than 566525 (first known bad).
https://chromium.googlesource.com/chromium/src/+log/843f4c596964746062da6786e6d4baac53720b3c..66e935ac491542bde00638e1484ca2c5fdbbf084
Reviewed-on: https://chromium-review.googlesource.com/1091497

@Patrick Monette: Please confirm the issue and help in re-assigning if it is not related to your change.
Adding ReleaseBlock-Beta as it is seems a recent break, feel free to remove it if not applicable.

Thanks!
Cc: lgrey@chromium.org
+cc lgrey@ - looking at the linked CL it seems odd that this would have broken only on Mac. Does this seem plausible to you?
Maybe, if it throws something out of whack with toolbar layout. The original report is from Cocoa browser;  viswa.karala@ are we sure bisect was Cocoa (#views-browser-windows disabled) the whole way?
M69 branch is coming VERY soon on July 19th, Your bug is marked as ReleaseBlock-Beta for M69. Please try to land the fix ASAP to trunk in order to prevent many merges going after M69 branch. This will also help us to branch M69 from high quality trunk. Thank you.
Friendly ping to get an update as it is marked as RBB.
Thanks..!
Labels: Needs-Feedback
Mac triage: needs feedback from reporter per #4
M69 branch is coming VERY soon on this Thursday, July 19th, Your bug is marked as ReleaseBlock-Beta for M69. Please try to land the fix ASAP to trunk in order to prevent many merges going after M69 branch. This will also help us to branch M69 from high quality trunk. Thank you.
Is there anyone knowledgeable with extension icons that can take this bug?

My CL should not have changed anything on Mac given that this was a Windows-only feature. I am not familiar with the code and thus can't really debug this issue.
Could some one from Extensions team please take a look into this as per C#9 & it is marked as RBB.

Thanks..!
Labels: -hasbisect-per-revision Needs-Bisect
Readding Needs-Bisect due to c#4
Labels: -Needs-Bisect
Tested the issue on chrome reported version# 69.0.3486.0 using Mac 10.12.6 and Mac 10.13.5 got same bisect range and change-log as mentioned in comment# 2.
Good build: 69.0.3456.0
Bad build: 69.0.3457.0

Also tested the issue on Ubuntu 14.04 and Windows-10, unable to reproduce the issue there. Able to reproduce the issue on mac by making the flag #views-browser-windows Default and Disable, but on Enabling the flag #views-browser-windows, we are unable to reproduce the issue. Hence removing Needs-Bisect label.

Thanks!

Cc: -lgrey@chromium.org pmonette@chromium.org
Owner: lgrey@chromium.org
Status: Started (was: Assigned)
Thanks for bisecting again. I think I've figured out the outline of what's going on here.
Project Member

Comment 14 by bugdroid1@chromium.org, Jul 20

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

commit 86341923722bec89a92a681b582d5e465c690541
Author: Leonard Grey <lgrey@chromium.org>
Date: Fri Jul 20 17:00:42 2018

Cocoa: ensure app menu makes space from browser action overflow container

https://chromium-review.googlesource.com/c/chromium/src/+/1091497 removed
an item from the app menu that never appeared on Mac. Unfortunately,
even though the item was never *displayed*, it appears to have factored
into NSMenu's calculation for what its max size should be.

When the item was removed, the max size is no longer able to account
for multiple rows in the browser action overflow container, so all
rows but the last are clipped.

This change removes and readds the extension overflow container when it's
finished sizing itself to prompt the menu to reevaluate its height.
We can't do this before we add the item to the menu because the menu's
width factors into the overflow container's size calculation. It seems
like there should be a less awkward way to do this, but I can't find it.

Bug:  862005 
Change-Id: Ib17ed810855041226786b16ca94f0a60d88dc892
Reviewed-on: https://chromium-review.googlesource.com/1145133
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Leonard Grey <lgrey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576905}
[modify] https://crrev.com/86341923722bec89a92a681b582d5e465c690541/chrome/browser/ui/cocoa/app_menu/app_menu_controller.mm

Status: Fixed (was: Started)
Labels: Merge-TBD
[Auto-generated comment by a script] We noticed that this issue is targeted for M-69; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-69 label, otherwise remove Merge-TBD label. Thanks.
Labels: -Merge-TBD Merge-Request-69
Project Member

Comment 18 by sheriffbot@chromium.org, Jul 21

Labels: -Merge-Request-69 Hotlist-Merge-Approved Merge-Approved-69
Your change meets the bar and is auto-approved for M69. Please go ahead and merge the CL to branch 3497 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: TE-Verified-M70 TE-Verified-70.0.3500.0
Able to reproduce the issue on chrome reported version 69.0.3486.0(Build without fix)
Verified the fix on Mac 10.12.6 on Chrome version #70.0.3500.0 as per the comment#0
Attaching screenshot for reference.
Observed "able to see all hidden installed and active Chrome extensions in the menu"
Hence, the fix is working as expected.
Adding the verified label.

Thanks!
862005.mp4
1.8 MB View Download
Project Member

Comment 20 by bugdroid1@chromium.org, Jul 23

Labels: -merge-approved-69 merge-merged-3497
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d3524fc6789095cfe23bee396f26e7c458033552

commit d3524fc6789095cfe23bee396f26e7c458033552
Author: Leonard Grey <lgrey@chromium.org>
Date: Mon Jul 23 13:47:58 2018

Cocoa: ensure app menu makes space from browser action overflow container

https://chromium-review.googlesource.com/c/chromium/src/+/1091497 removed
an item from the app menu that never appeared on Mac. Unfortunately,
even though the item was never *displayed*, it appears to have factored
into NSMenu's calculation for what its max size should be.

When the item was removed, the max size is no longer able to account
for multiple rows in the browser action overflow container, so all
rows but the last are clipped.

This change removes and readds the extension overflow container when it's
finished sizing itself to prompt the menu to reevaluate its height.
We can't do this before we add the item to the menu because the menu's
width factors into the overflow container's size calculation. It seems
like there should be a less awkward way to do this, but I can't find it.

Bug:  862005 
Change-Id: Ib17ed810855041226786b16ca94f0a60d88dc892
Reviewed-on: https://chromium-review.googlesource.com/1145133
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Leonard Grey <lgrey@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#576905}(cherry picked from commit 86341923722bec89a92a681b582d5e465c690541)
Reviewed-on: https://chromium-review.googlesource.com/1146780
Reviewed-by: Leonard Grey <lgrey@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#12}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/d3524fc6789095cfe23bee396f26e7c458033552/chrome/browser/ui/cocoa/app_menu/app_menu_controller.mm

Labels: TE-Verified-M69 TE-Verified-69.0.3497.12
Able to reproduce the issue on chrome reported version 69.0.3486.0(Build without fix)
Verified the fix on Mac 10.12.6 on Chrome version #69.0.3497.12 as per the comment#0
Attaching screencast for reference.
Observed "able to see all hidden installed and active Chrome extensions in the menu"
Hence, the fix is working as expected.
Adding the verified label.

Thanks!
862005.mp4
3.0 MB View Download

Sign in to add a comment