New issue
Advanced search Search tips

Issue 838152 link

Starred by 7 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 15
Cc:
Components:
EstimatedDays: 2
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug

Blocking:
issue 821991
issue 856492



Sign in to add a comment

Paint the tab close button circle highlight programmatically.

Project Member Reported by kylixrd@chromium.org, Apr 30 2018

Issue description

Currently, the hover/pressed circle highlight the tab close button is a vector icon with the (X) icon superimposed. The circle can be painted from the code without the need for the vector icons.
 
Blocking: 821991
Cc: kylixrd@chromium.org
Owner: ----
Status: Available (was: Assigned)
EstimatedDays: 2
For context, the code is in TabCloseButton::GenerateImages() and the vector icons are chrome/app/vector_icons/tab_close_button_highlight.icon and friends.

Is using gfx::CanvasImageSource::MakeImageSkia() with a gfx::CanvasImageSource sub-class the right approach?
We probably should not use images at all.  Just draw a circle and two strokes directly.
Labels: M-70
Labels: -M-70 Group-Tab
Labels: M-70
Cc: -kylixrd@chromium.org pkasting@chromium.org
Owner: kylixrd@chromium.org
Status: Assigned (was: Available)
 Issue 856492  has been merged into this issue.
Project Member

Comment 11 by bugdroid1@chromium.org, Jul 23

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

commit deff1fee5c53b21cc2b653e0a3ff298fff2ab557
Author: Allen Bauer <kylixrd@chromium.org>
Date: Mon Jul 23 17:24:43 2018

Paint close tab button highlight circle programmatically.

Refactor, minimize and clean up code. Moved the non-refresh/non-touch
tab close button highlight colors to theme_properties.cc since there
was a theme element.

It's still using the close (X) icons instead of painting them since the
coordinates of the strokes are very odd. Also, the "touch" version of the
icon drawn by outlining the X with a complicated path.

Once actual specs for the icon are available, the code can be updated.

TBR=estade@chromium.org

Bug:  838152 
Change-Id: I2abca3be74f08649b3472f22fa31d0f897789e70
Reviewed-on: https://chromium-review.googlesource.com/1145590
Reviewed-by: Sidney San Martín <sdy@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: Allen Bauer <kylixrd@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Commit-Queue: Allen Bauer <kylixrd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577196}
[modify] https://crrev.com/deff1fee5c53b21cc2b653e0a3ff298fff2ab557/chrome/app/vector_icons/BUILD.gn
[delete] https://crrev.com/dcba678430c44543b62f9810286b3de4c4db22c9/chrome/app/vector_icons/tab_close_button_highlight.icon
[delete] https://crrev.com/dcba678430c44543b62f9810286b3de4c4db22c9/chrome/app/vector_icons/tab_close_button_touch_highlight.icon
[modify] https://crrev.com/deff1fee5c53b21cc2b653e0a3ff298fff2ab557/chrome/browser/themes/theme_properties.cc
[modify] https://crrev.com/deff1fee5c53b21cc2b653e0a3ff298fff2ab557/chrome/browser/ui/cocoa/hover_close_button.mm
[modify] https://crrev.com/deff1fee5c53b21cc2b653e0a3ff298fff2ab557/chrome/browser/ui/views/tabs/tab.cc
[modify] https://crrev.com/deff1fee5c53b21cc2b653e0a3ff298fff2ab557/chrome/browser/ui/views/tabs/tab_close_button.cc
[modify] https://crrev.com/deff1fee5c53b21cc2b653e0a3ff298fff2ab557/chrome/browser/ui/views/tabs/tab_close_button.h

Labels: -M-70 M-69 Target-69
Status: Started (was: Assigned)
Labels: -M-69 -Target-69 M-70 Target-70
Moving this to M70. We can live without the programatically drawn 'X'.
Blocking: 856492
Labels: Merge-Request-69
While on its own we wouldn't need this in M69, it's a blocker for  bug 856492 , which we want to merge, so requesting merge for the CL in comment 11.  This shouldn't have a noticeable visual effect on its own.
Project Member

Comment 16 by sheriffbot@chromium.org, Aug 15

Labels: -Merge-Request-69 Hotlist-Merge-Reject Merge-Reject-69
The bug is marked as P3 or Feature. It should not be merged as M69 is in beta. 
Please contact the approriate 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: -Pri-3 -Hotlist-Merge-Reject -Merge-Reject-69 Merge-Request-69 Pri-2
Elevating to P2 just so sheriffbot doesn't auto-reject
Project Member

Comment 18 by sheriffbot@chromium.org, Aug 15

Labels: -Merge-Request-69 Merge-Review-69 Hotlist-Merge-Review
This bug requires manual review: M69 has already been promoted to the beta branch, so this requires manual review
Please contact the 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: -Merge-Review-69 Merge-Approved-69
Approving merge to M69 branch 3497 based on comment #15.
Project Member

Comment 20 by bugdroid1@chromium.org, Aug 15

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

commit bae71fa5539890daf8a1cf4a419bebbf0f1de325
Author: Allen Bauer <kylixrd@chromium.org>
Date: Wed Aug 15 15:34:12 2018

Paint close tab button highlight circle programmatically.

Refactor, minimize and clean up code. Moved the non-refresh/non-touch
tab close button highlight colors to theme_properties.cc since there
was a theme element.

It's still using the close (X) icons instead of painting them since the
coordinates of the strokes are very odd. Also, the "touch" version of the
icon drawn by outlining the X with a complicated path.

Once actual specs for the icon are available, the code can be updated.

TBR=estade@chromium.org

Bug:  838152 
Change-Id: I2abca3be74f08649b3472f22fa31d0f897789e70
Reviewed-on: https://chromium-review.googlesource.com/1145590
Reviewed-by: Sidney San Martín <sdy@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: Allen Bauer <kylixrd@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Commit-Queue: Allen Bauer <kylixrd@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#577196}(cherry picked from commit deff1fee5c53b21cc2b653e0a3ff298fff2ab557)
Reviewed-on: https://chromium-review.googlesource.com/1175292
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#637}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/bae71fa5539890daf8a1cf4a419bebbf0f1de325/chrome/app/vector_icons/BUILD.gn
[delete] https://crrev.com/d880d8efe1bfe1204790a7bdf34674f374ffcb4d/chrome/app/vector_icons/tab_close_button_highlight.icon
[delete] https://crrev.com/d880d8efe1bfe1204790a7bdf34674f374ffcb4d/chrome/app/vector_icons/tab_close_button_touch_highlight.icon
[modify] https://crrev.com/bae71fa5539890daf8a1cf4a419bebbf0f1de325/chrome/browser/themes/theme_properties.cc
[modify] https://crrev.com/bae71fa5539890daf8a1cf4a419bebbf0f1de325/chrome/browser/ui/cocoa/hover_close_button.mm
[modify] https://crrev.com/bae71fa5539890daf8a1cf4a419bebbf0f1de325/chrome/browser/ui/views/tabs/tab.cc
[modify] https://crrev.com/bae71fa5539890daf8a1cf4a419bebbf0f1de325/chrome/browser/ui/views/tabs/tab_close_button.cc
[modify] https://crrev.com/bae71fa5539890daf8a1cf4a419bebbf0f1de325/chrome/browser/ui/views/tabs/tab_close_button.h

Status: Fixed (was: Started)
Project Member

Comment 22 by bugdroid1@chromium.org, Aug 15

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

commit 2d9d76e351a9867e05843c98fdb3ae5307793594
Author: Peter Kasting <pkasting@chromium.org>
Date: Wed Aug 15 15:51:47 2018

Paint close(X) icon and generate colors with specific contrast ratios.

TBR=estade@chromium.org, kylixrd@chromium.org

Bug:  838152 
Bug:  856492 
Change-Id: Ie16b4fd47d3e0f614a0f793300539729c905e5a0
Reviewed-on: https://chromium-review.googlesource.com/1162664
Commit-Queue: Allen Bauer <kylixrd@chromium.org>
Reviewed-by: Allen Bauer <kylixrd@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#581553}(cherry picked from commit 52e71c0e747870b84336ee33ba1c27fff5aafbcd)
Reviewed-on: https://chromium-review.googlesource.com/1175856
Cr-Commit-Position: refs/branch-heads/3497@{#640}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/2d9d76e351a9867e05843c98fdb3ae5307793594/chrome/app/vector_icons/BUILD.gn
[delete] https://crrev.com/f3a18f9491330d76058b2de0981cf97896286c30/chrome/app/vector_icons/tab_close_button_touch.icon
[modify] https://crrev.com/2d9d76e351a9867e05843c98fdb3ae5307793594/chrome/browser/ui/views/tabs/tab.cc
[modify] https://crrev.com/2d9d76e351a9867e05843c98fdb3ae5307793594/chrome/browser/ui/views/tabs/tab.h
[modify] https://crrev.com/2d9d76e351a9867e05843c98fdb3ae5307793594/chrome/browser/ui/views/tabs/tab_close_button.cc
[modify] https://crrev.com/2d9d76e351a9867e05843c98fdb3ae5307793594/chrome/browser/ui/views/tabs/tab_close_button.h

Sign in to add a comment