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

Issue 895519 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 16
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug-Regression

Blocking:
issue 737995
issue 881371



Sign in to add a comment

Toolbar trusted area icon highlights are ovals in touchable UI mode

Project Member Reported by taku...@chromium.org, Oct 15

Issue description

Chrome Version: 71.0.3578.0
OS: tested on Linux, ChromeOS

What steps will reproduce the problem?
(1) Set #top-chrome-md to touchable refresh in chrome:flags
(2) Hover on the toolbar trusted area icons

What is the expected result?
The icons' inkdrop highlights should be circles

What happens instead?
They are ovals of varying shapes. Also the spacing between the icons is off


Peter, could you take a look?

 
screen_capture-19.ogv
43.4 KB View Download
Cc: pbos@chromium.org
Owner: thomasanderson@chromium.org
Reverting crrev.com/ab847d5f047c23bcf820efa745b5b9944c725c54 fixes it, thomasanderson@ can you take a look? This'll need a merge to M71.

If you're swamped let me know, I can pitch in some cycles.
> If you're swamped let me know, I can pitch in some cycles.

Well I can't say no to that :)  I could definitely use some help
Lol, do you wanna take a look first? Otherwise just assign it back to me.
Blocking: 737995 881371
Cc: thomasanderson@chromium.org
Labels: -Type-Bug -Pri-2 Pri-1 Type-Bug-Regression
Owner: pbos@chromium.org
Labels: Merge-Request-71
Fixed in 019423bf4be51d4547a2309741eb666a39a7b718 but I referred to the wrong bug.


Awesome, thanks pbos!
Pls apply appropriate OSs label. 
Pls apply appropriate OSs label ASAP. Thank you.
Labels: OS-Chrome OS-Linux OS-Mac OS-Windows
Cc: jrw@chromium.org
 Issue 893304  has been merged into this issue.
Labels: -OS-Linux -OS-Windows -OS-Mac
This should really only be CrOS, nothing really uses touchable in M71 outside that platform (unless you change chrome://flags).
Project Member

Comment 12 by sheriffbot@chromium.org, Oct 16

Labels: -Merge-Request-71 Hotlist-Merge-Approved Merge-Approved-71
Your change meets the bar and is auto-approved for M71. Please go ahead and merge the CL to branch 3578 manually. Please contact milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(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, Oct 16

Labels: -merge-approved-71 merge-merged-3578
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ed63ea8113ced76f20aad0e07d80947b1fbdb86b

commit ed63ea8113ced76f20aad0e07d80947b1fbdb86b
Author: Peter Boström <pbos@chromium.org>
Date: Tue Oct 16 23:43:49 2018

Fix touchable insets for Avatar/Cast toolbar icons

An earlier refactoring set zero-insets for touchable AvatarToolbarButton
instead of insets for TOOLBAR_BUTTON. This API was confusing to use so
SetLayoutInsets was changed and renamed to SetLayoutInsetDelta to make
it easier to use. Callers no longer need to include the call to
GetLayoutInsets(TOOLBAR_BUTTON).

CastToolbarButton having incorrect ink-drops was not a regression but
has always used a 16dp icon in both touchable and non-touchable modes.
This change also calls SetInsetDelta to make up for the missing 8dp.

Bug:  chromium:895519 
Change-Id: I30595913dfb8a19358d198d9f4e0dbcb37993117
Reviewed-on: https://chromium-review.googlesource.com/c/1281903
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#599765}(cherry picked from commit 019423bf4be51d4547a2309741eb666a39a7b718)
Reviewed-on: https://chromium-review.googlesource.com/c/1285321
Reviewed-by: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#74}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
[modify] https://crrev.com/ed63ea8113ced76f20aad0e07d80947b1fbdb86b/chrome/browser/ui/views/media_router/cast_toolbar_button.cc
[modify] https://crrev.com/ed63ea8113ced76f20aad0e07d80947b1fbdb86b/chrome/browser/ui/views/profiles/avatar_toolbar_button.cc
[modify] https://crrev.com/ed63ea8113ced76f20aad0e07d80947b1fbdb86b/chrome/browser/ui/views/toolbar/toolbar_button.cc
[modify] https://crrev.com/ed63ea8113ced76f20aad0e07d80947b1fbdb86b/chrome/browser/ui/views/toolbar/toolbar_button.h

Labels: -Hotlist-Merge-Approved
Status: Fixed (was: Assigned)
Labels: Merge-Merged-71-3578
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/ed63ea8113ced76f20aad0e07d80947b1fbdb86b

Commit: ed63ea8113ced76f20aad0e07d80947b1fbdb86b
Author: pbos@chromium.org
Commiter: pbos@chromium.org
Date: 2018-10-16 23:43:49 +0000 UTC

Fix touchable insets for Avatar/Cast toolbar icons

An earlier refactoring set zero-insets for touchable AvatarToolbarButton
instead of insets for TOOLBAR_BUTTON. This API was confusing to use so
SetLayoutInsets was changed and renamed to SetLayoutInsetDelta to make
it easier to use. Callers no longer need to include the call to
GetLayoutInsets(TOOLBAR_BUTTON).

CastToolbarButton having incorrect ink-drops was not a regression but
has always used a 16dp icon in both touchable and non-touchable modes.
This change also calls SetInsetDelta to make up for the missing 8dp.

Bug:  chromium:895519 
Change-Id: I30595913dfb8a19358d198d9f4e0dbcb37993117
Reviewed-on: https://chromium-review.googlesource.com/c/1281903
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#599765}(cherry picked from commit 019423bf4be51d4547a2309741eb666a39a7b718)
Reviewed-on: https://chromium-review.googlesource.com/c/1285321
Reviewed-by: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#74}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}

Sign in to add a comment