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

Issue 831510 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

[Touchable] Extension icons in overflow menu jump when dragged

Project Member Reported by mgiuca@chromium.org, Apr 11 2018

Issue description

Chrome Version: 67 r549812
OS: Chrome

What steps will reproduce the problem?
With: --top-chrome-md=material-touch-optimized
(1) Add a bunch of extensions.
(2) Shrink the extensions toolbar so they appear in overflow menu.
(3) Drag to rearrange the order.

What is the expected result?
Looks normal.

What happens instead?
Upon completing the drag, ALL of the extension icons jump to a different offset / padding, until the menu is re-opened.

See video. (Note that the mouse cursor gets stuck when dragging; this is an artifact of the video recorder not seen in the real product.)
 

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

Forgot to attach video.
Extension overflow jump.webm
1.0 MB View Download
Cc: -afakhry@chromium.org pkasting@chromium.org
Owner: afakhry@chromium.org
Status: Started (was: Untriaged)
I can't repro this (See attached video). However, the weird thing is the drag indicator is not showing anymore! Looking into it....
drag_extensions-2018-04-11_16.25.47.mp4
188 KB View Download

Comment 3 by mgiuca@chromium.org, Apr 12 2018

Hmm. It doesn't repro on my dev Chromebook. But it does on r549974 (after a fresh sync).
I just sync'd this morning  ... let me sync again.
Cc: sky@chromium.org
After sync, I still can't repro! 

Regarding the drag indicator, it's no showing because it's being painted with a white color which matches the background color of the menu. The reason is that the menu's widget theme provider is the ui::DefaultThemeProvider which returns a default placeholder color for everything [1].

Blending that towards the opposite luma returns the white color [2].

We should always use the theme provider of the browser frame. Uploading a fix for that shortly...

[1]:  https://cs.chromium.org/chromium/src/ui/base/default_theme_provider.cc?type=cs&q=DefaultThemeProvider::GetColor&l=23

[2]: https://cs.chromium.org/chromium/src/chrome/browser/ui/views/toolbar/browser_actions_container.cc?type=cs&q=BrowserActionsContainer::OnPaint&l=656

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

Not sure what else to suggest. I'm using a plain Release build from Tip of Tree r549974, and running out/Release_cros/chrome --top-chrome-md=material-touch-optimized. I have 3 extensions installed and I'm accessing the menu from the browser menu (not PWA).

Good luck!
Project Member

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

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

commit 80485cd5d06d8b6b28a2dfd007ee6f26a5132dfb
Author: Ahmed Fakhry <afakhry@google.com>
Date: Fri Apr 13 19:56:52 2018

Fix browser actions drop indicator color

When browser actions are dragged and dropped inside the menu,
they used to use the menu's widget theme provider which is of
type ui::DefaultThemeProvider. Instead, it should use the browser's
frame theme provider, so it can get the correct color.

BUG= 831510 

Change-Id: If7f46e38e44bbfee04b6f67e8377b7d8dd3d9d7f
Reviewed-on: https://chromium-review.googlesource.com/1011374
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550730}
[modify] https://crrev.com/80485cd5d06d8b6b28a2dfd007ee6f26a5132dfb/chrome/browser/ui/views/toolbar/browser_actions_container.cc

Labels: Merge-Request-67
Project Member

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

Labels: merge-merged-testbranch
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/80485cd5d06d8b6b28a2dfd007ee6f26a5132dfb

commit 80485cd5d06d8b6b28a2dfd007ee6f26a5132dfb
Author: Ahmed Fakhry <afakhry@google.com>
Date: Fri Apr 13 19:56:52 2018

Fix browser actions drop indicator color

When browser actions are dragged and dropped inside the menu,
they used to use the menu's widget theme provider which is of
type ui::DefaultThemeProvider. Instead, it should use the browser's
frame theme provider, so it can get the correct color.

BUG= 831510 

Change-Id: If7f46e38e44bbfee04b6f67e8377b7d8dd3d9d7f
Reviewed-on: https://chromium-review.googlesource.com/1011374
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550730}
[modify] https://crrev.com/80485cd5d06d8b6b28a2dfd007ee6f26a5132dfb/chrome/browser/ui/views/toolbar/browser_actions_container.cc

The CL is about choosing colors. Is it addressing the regression noted with bug creation?  Was that reproducible? 
Cc: kbleicher@chromium.org
The color of the drop indicator bar not showing up is also a regression. 

However the jumping of icons as described in #0 cannot be repro'd so far.
Project Member

Comment 12 by sheriffbot@chromium.org, Apr 18 2018

Labels: -Merge-Request-67 Merge-Approved-67 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M67. Please go ahead and merge the CL to branch 3396 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 13 by bugdroid1@chromium.org, Apr 18 2018

Labels: -merge-approved-67 merge-merged-3396
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e66f847ee09e45298cad360c10e75e992679abb5

commit e66f847ee09e45298cad360c10e75e992679abb5
Author: Ahmed Fakhry <afakhry@google.com>
Date: Wed Apr 18 20:39:27 2018

[Merge to M-67] Fix browser actions drop indicator color

When browser actions are dragged and dropped inside the menu,
they used to use the menu's widget theme provider which is of
type ui::DefaultThemeProvider. Instead, it should use the browser's
frame theme provider, so it can get the correct color.

TBR=sky@chromium.org
BUG= 831510 

(cherry picked from commit 80485cd5d06d8b6b28a2dfd007ee6f26a5132dfb)

Change-Id: If7f46e38e44bbfee04b6f67e8377b7d8dd3d9d7f
Reviewed-on: https://chromium-review.googlesource.com/1011374
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#550730}
Reviewed-on: https://chromium-review.googlesource.com/1017807
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/branch-heads/3396@{#104}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/e66f847ee09e45298cad360c10e75e992679abb5/chrome/browser/ui/views/toolbar/browser_actions_container.cc

Status: Fixed (was: Started)

Sign in to add a comment