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

Issue 888204 link

Starred by 2 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Feature



Sign in to add a comment

Simplify inkdrops and focus rings

Project Member Reported by pbos@chromium.org, Sep 22

Issue description

Inkdrops and focus rings very often use the same shape (a rounded rectangle / pill shape). If we add this shape as a property of the view, the default focus ring / ink-drop behavior can be to use this shape instead of having the individual views override inkdrops and focus rings separately.

Internal doc: go/simplified-inkdrops
 
Project Member

Comment 1 by bugdroid1@chromium.org, Sep 25

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

commit e02b4364e6420e9b9cba7698be80c516a33c9c6b
Author: Peter Boström <pbos@chromium.org>
Date: Tue Sep 25 00:51:52 2018

Use highlight path for InkDropMask and FocusRing

Introduces a kHighlightPathKey property to views and uses it as a
default for FocusRing and CreateInkDropMask in InkDropHostView. If no
highlight path is added this fall backs to prior behavior, which allows
introducing this change in parts.

This highlight path is added to the bookmarks-bar buttons and the dialog
close button. CreateInkDropMask and installing the focus ring is removed
from the prior.

Bug:  chromium:861975 , chromium:888204
Change-Id: I7eb682efe82caea7e8088d7277733698967d0234
Reviewed-on: https://chromium-review.googlesource.com/1239715
Reviewed-by: Trent Apted <tapted@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593783}
[modify] https://crrev.com/e02b4364e6420e9b9cba7698be80c516a33c9c6b/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc
[modify] https://crrev.com/e02b4364e6420e9b9cba7698be80c516a33c9c6b/ui/views/animation/ink_drop_host_view.cc
[modify] https://crrev.com/e02b4364e6420e9b9cba7698be80c516a33c9c6b/ui/views/animation/ink_drop_mask.cc
[modify] https://crrev.com/e02b4364e6420e9b9cba7698be80c516a33c9c6b/ui/views/animation/ink_drop_mask.h
[modify] https://crrev.com/e02b4364e6420e9b9cba7698be80c516a33c9c6b/ui/views/bubble/bubble_frame_view.cc
[modify] https://crrev.com/e02b4364e6420e9b9cba7698be80c516a33c9c6b/ui/views/controls/focus_ring.cc
[modify] https://crrev.com/e02b4364e6420e9b9cba7698be80c516a33c9c6b/ui/views/controls/focus_ring.h
[modify] https://crrev.com/e02b4364e6420e9b9cba7698be80c516a33c9c6b/ui/views/view_properties.cc
[modify] https://crrev.com/e02b4364e6420e9b9cba7698be80c516a33c9c6b/ui/views/view_properties.h

Project Member

Comment 2 by bugdroid1@chromium.org, Sep 26

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

commit 93ca9993020d49cea34bde56e503726bccada8d2
Author: Daniel Bratell <bratell@opera.com>
Date: Wed Sep 26 12:37:00 2018

Add declaration for a newly defined highlight path property

A missing declaration broke some jumbo builds because in those
builds an implicit instantiation was seen before an explicit
specialization. This is illegal in C++. A forward declaration
solves the problem.

TBR=pbos@chromium.org,tapted@chromium.org

Bug:  861975 ,888204
Change-Id: Iecf736bcf9748d3c68265c21f9351c56d9a23dd8
Reviewed-on: https://chromium-review.googlesource.com/1245785
Reviewed-by: Daniel Bratell <bratell@opera.com>
Commit-Queue: Daniel Bratell <bratell@opera.com>
Cr-Commit-Position: refs/heads/master@{#594289}
[modify] https://crrev.com/93ca9993020d49cea34bde56e503726bccada8d2/ui/views/view_properties.h

Labels: Hotlist-DesktopUIConsider
Labels: Group-Platform
Labels: M-72 Target-72
Labels: -Hotlist-DesktopUIConsider Hotlist-DesktopUITriaged
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 9

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

commit 09f8bdbd7d6228963e1ab3566bd9caab1c23731f
Author: Peter Boström <pbos@chromium.org>
Date: Tue Oct 09 19:03:23 2018

Use highlight paths for toolbar-button masks

Removes CreateInkDropMask overrides for toolbar buttons and
corresponding FocusRing::SetPath calls.

Bug: chromium:888204
Change-Id: Ife4f60de33d33df5cc34f008c79f49432ba29822
Reviewed-on: https://chromium-review.googlesource.com/c/1269805
Reviewed-by: Trent Apted <tapted@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598020}
[modify] https://crrev.com/09f8bdbd7d6228963e1ab3566bd9caab1c23731f/chrome/browser/ui/views/toolbar/browser_app_menu_button.cc
[modify] https://crrev.com/09f8bdbd7d6228963e1ab3566bd9caab1c23731f/chrome/browser/ui/views/toolbar/browser_app_menu_button.h
[modify] https://crrev.com/09f8bdbd7d6228963e1ab3566bd9caab1c23731f/chrome/browser/ui/views/toolbar/toolbar_action_view.cc
[modify] https://crrev.com/09f8bdbd7d6228963e1ab3566bd9caab1c23731f/chrome/browser/ui/views/toolbar/toolbar_action_view.h
[modify] https://crrev.com/09f8bdbd7d6228963e1ab3566bd9caab1c23731f/chrome/browser/ui/views/toolbar/toolbar_button.cc
[modify] https://crrev.com/09f8bdbd7d6228963e1ab3566bd9caab1c23731f/chrome/browser/ui/views/toolbar/toolbar_button.h
[modify] https://crrev.com/09f8bdbd7d6228963e1ab3566bd9caab1c23731f/chrome/browser/ui/views/toolbar/toolbar_ink_drop_util.h

Project Member

Comment 8 by bugdroid1@chromium.org, Oct 11

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

commit 1812d077133e5b46de10158cf530a2ec352bb54a
Author: Peter Boström <pbos@chromium.org>
Date: Thu Oct 11 00:33:57 2018

Fix ToolbarButton corner radius inconsistency

Before this change, ToolbarButton (and friends) would rely on ::Layout()
to set the inkdrop corner radius before it's indirectly being used from
::OnBoundsChanged(). This initial Layout() call did not reliably happen
before bounds were set, so several toolbar buttons had inconsistent
(non-circular) focus rings as well as inkdrop masks.

The fix for this is to poll the corner radius metric when creating
inkdrop components and path used for focus rings, so it doesn't rely on
a cached value being set from ::Layout().

Incidentally, the focus ring set for browser actions have been clipping
the extension icons. This now sets a separate path for ToolbarActionView
focus rings to prevent clipping and un-marks FocusRing::SetPath as
deprecated with a clarifying comment that it should only be used when
the inkdrop shape and FocusRing path needs to diverge.

Bug: chromium:888204
Change-Id: I0d345832e9c11c2e052840012b7f4c9a533b8712
Reviewed-on: https://chromium-review.googlesource.com/c/1272797
Commit-Queue: Peter Boström <pbos@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598592}
[modify] https://crrev.com/1812d077133e5b46de10158cf530a2ec352bb54a/chrome/browser/ui/views/toolbar/browser_app_menu_button.cc
[modify] https://crrev.com/1812d077133e5b46de10158cf530a2ec352bb54a/chrome/browser/ui/views/toolbar/toolbar_action_view.cc
[modify] https://crrev.com/1812d077133e5b46de10158cf530a2ec352bb54a/chrome/browser/ui/views/toolbar/toolbar_action_view.h
[modify] https://crrev.com/1812d077133e5b46de10158cf530a2ec352bb54a/chrome/browser/ui/views/toolbar/toolbar_button.cc
[modify] https://crrev.com/1812d077133e5b46de10158cf530a2ec352bb54a/chrome/browser/ui/views/toolbar/toolbar_button.h
[modify] https://crrev.com/1812d077133e5b46de10158cf530a2ec352bb54a/chrome/browser/ui/views/toolbar/toolbar_ink_drop_util.cc
[modify] https://crrev.com/1812d077133e5b46de10158cf530a2ec352bb54a/chrome/browser/ui/views/toolbar/toolbar_ink_drop_util.h
[modify] https://crrev.com/1812d077133e5b46de10158cf530a2ec352bb54a/ui/views/controls/focus_ring.h

Project Member

Comment 9 by bugdroid1@chromium.org, Oct 12

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

commit df755070086c231bea487bb50630debf9ac5362b
Author: Peter Boström <pbos@chromium.org>
Date: Fri Oct 12 18:28:43 2018

Add default ripples and highlights for paths

When kHighlightPath is in use the ink-drop effects are masked. As such
we can use a flood-fill ripple and default highlight path that cover the
entire view (that will be masked to the correct shape). This allows
removing custom ink-drop ripples and significantly simplify highlights
for toolbar-related buttons.

This change makes use of these simpler inkdrops in for bookmark-bar
buttons and menu buttons.

Bug: chromium:888204
Change-Id: I32f21dde80783cdd262351596a4a4a504505e21c
Reviewed-on: https://chromium-review.googlesource.com/c/1277268
Commit-Queue: Peter Boström <pbos@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599300}
[modify] https://crrev.com/df755070086c231bea487bb50630debf9ac5362b/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc
[modify] https://crrev.com/df755070086c231bea487bb50630debf9ac5362b/chrome/browser/ui/views/toolbar/browser_app_menu_button.cc
[modify] https://crrev.com/df755070086c231bea487bb50630debf9ac5362b/chrome/browser/ui/views/toolbar/browser_app_menu_button.h
[modify] https://crrev.com/df755070086c231bea487bb50630debf9ac5362b/chrome/browser/ui/views/toolbar/toolbar_action_view.cc
[modify] https://crrev.com/df755070086c231bea487bb50630debf9ac5362b/chrome/browser/ui/views/toolbar/toolbar_action_view.h
[modify] https://crrev.com/df755070086c231bea487bb50630debf9ac5362b/chrome/browser/ui/views/toolbar/toolbar_button.cc
[modify] https://crrev.com/df755070086c231bea487bb50630debf9ac5362b/chrome/browser/ui/views/toolbar/toolbar_button.h
[modify] https://crrev.com/df755070086c231bea487bb50630debf9ac5362b/chrome/browser/ui/views/toolbar/toolbar_ink_drop_util.cc
[modify] https://crrev.com/df755070086c231bea487bb50630debf9ac5362b/chrome/browser/ui/views/toolbar/toolbar_ink_drop_util.h
[modify] https://crrev.com/df755070086c231bea487bb50630debf9ac5362b/ui/views/animation/ink_drop_host_view.cc
[modify] https://crrev.com/df755070086c231bea487bb50630debf9ac5362b/ui/views/controls/button/label_button.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Oct 15

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

commit 0e982e2098181f9d155558339fde858d01c27a50
Author: Peter Boström <pbos@chromium.org>
Date: Mon Oct 15 22:41:38 2018

Use highlight path for DownloadItemView

Moves color derivation to GetInkDropBaseColor() and removes ripple and
highlight overrides. This change also makes use of the newer FocusRing
class instead of using a FocusableBorder. FocusRing automatically
follows the highlight path and has a more consistent style with the rest
of the UI.

This change also removes an override of AddInkDropLayer which clipped
the layer to the view bounds. This is not required for this ink drop as
it only paints within the highlight path which is within the view
bounds.

Bug: chromium:888204
Change-Id: I7b388f1fe52aea6b14c32d27c0f39d5e2cff527e
Reviewed-on: https://chromium-review.googlesource.com/c/1279289
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599761}
[modify] https://crrev.com/0e982e2098181f9d155558339fde858d01c27a50/chrome/browser/ui/views/download/download_item_view.cc
[modify] https://crrev.com/0e982e2098181f9d155558339fde858d01c27a50/chrome/browser/ui/views/download/download_item_view.h

Project Member

Comment 11 by bugdroid1@chromium.org, Oct 16

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

commit b3a2c60cc34c512fe6b27939f34dc338f0dae31a
Author: Peter Boström <pbos@chromium.org>
Date: Tue Oct 16 00:01:44 2018

Use highlight paths for HoverButton

Removes the need for custom ink-drop highlights. This change (as tested
locally) should have no visual impact. It only reduces custom ink-drop
usage.

Bug: chromium:888204
Change-Id: Iefb2c6adf9f95a6d1265fb1dbd95ff0f0e7ecf97
Reviewed-on: https://chromium-review.googlesource.com/c/1279169
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599785}
[modify] https://crrev.com/b3a2c60cc34c512fe6b27939f34dc338f0dae31a/chrome/browser/ui/views/hover_button.cc
[modify] https://crrev.com/b3a2c60cc34c512fe6b27939f34dc338f0dae31a/chrome/browser/ui/views/hover_button.h

Project Member

Comment 12 by bugdroid1@chromium.org, Oct 19

Labels: Hotlist-DesktopUIChecked
Status: WontFix (was: Started)
** Mass UI Triage **

Since fix is landed, closing this issue. If this bug still reproduces for you, please reopen or file a new issue. Thanks!
Labels: -Type-Bug -Hotlist-DesktopUIChecked Type-Feature
Status: Started (was: WontFix)
Not a bug, not sure we're done so reopening.
Labels: -M-72 -Target-72 M-73 Target-73
Project Member

Comment 16 by bugdroid1@chromium.org, Dec 20

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

commit b95fe36bf1caa5821aec8507792fbb7b60d68430
Author: Peter Boström <pbos@chromium.org>
Date: Thu Dec 20 16:18:43 2018

Make use of highlight path in TopShortcutButton

Removes the need for overrides of most CreateInkDropX methods as the
ink-drop shape instead gets generated from the highlight path.

This adds set_ink_drop_highlight_opacity to prevent the need of using
TrayPopupUtils to provide a different highlight only to use a different
opacity.

Bug: chromium:888204
Change-Id: I2f147c1f16081dee73418899669a7164d73322dd
Reviewed-on: https://chromium-review.googlesource.com/c/1385023
Reviewed-by: Michael Wasserman <msw@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618233}
[modify] https://crrev.com/b95fe36bf1caa5821aec8507792fbb7b60d68430/ash/system/tray/tray_popup_utils.cc
[modify] https://crrev.com/b95fe36bf1caa5821aec8507792fbb7b60d68430/ash/system/unified/top_shortcut_button.cc
[modify] https://crrev.com/b95fe36bf1caa5821aec8507792fbb7b60d68430/ash/system/unified/top_shortcut_button.h
[modify] https://crrev.com/b95fe36bf1caa5821aec8507792fbb7b60d68430/ui/views/animation/ink_drop_host_view.cc
[modify] https://crrev.com/b95fe36bf1caa5821aec8507792fbb7b60d68430/ui/views/animation/ink_drop_host_view.h

Project Member

Comment 17 by bugdroid1@chromium.org, Jan 2

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

commit 9c8b10b5f102aac1db5ce7a2ac99a450381ee7c2
Author: Peter Boström <pbos@chromium.org>
Date: Wed Jan 02 23:27:40 2019

Unify ink-drop style to flood fill with masks.

This change makes flood fill the default ink-drop style. Square InkDrops
are currently bugs in the browser that are kind-of hidden by InkDrop
masks.

It makes use of the same path that FocusRing uses for masks to make sure
that the InkDrop and FocusRing are in sync by default. These mismatch
bugs keep popping up, especially on Mac, as testers verify new parts of
the UI.

Follow-up changes will be to reduce the number of classes that override
CreateInkDrop methods and make sure that they explicitly request a
SquareInkDrop when they want it.

Bug: chromium:888204,  chromium:907833 ,  chromium:915284 

Change-Id: I681b93d6f5bdfe7f78421ebc8f2fbd522fb6eee4
Reviewed-on: https://chromium-review.googlesource.com/c/1367186
Commit-Queue: Peter Boström <pbos@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Reviewed-by: Mohsen Izadi <mohsen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619533}
[modify] https://crrev.com/9c8b10b5f102aac1db5ce7a2ac99a450381ee7c2/chrome/browser/ui/views/download/download_item_view.cc
[modify] https://crrev.com/9c8b10b5f102aac1db5ce7a2ac99a450381ee7c2/chrome/browser/ui/views/download/download_item_view.h
[modify] https://crrev.com/9c8b10b5f102aac1db5ce7a2ac99a450381ee7c2/chrome/browser/ui/views/hover_button.cc
[modify] https://crrev.com/9c8b10b5f102aac1db5ce7a2ac99a450381ee7c2/chrome/browser/ui/views/toolbar/browser_app_menu_button.cc
[modify] https://crrev.com/9c8b10b5f102aac1db5ce7a2ac99a450381ee7c2/chrome/browser/ui/views/toolbar/browser_app_menu_button.h
[modify] https://crrev.com/9c8b10b5f102aac1db5ce7a2ac99a450381ee7c2/chrome/browser/ui/views/toolbar/toolbar_action_view.cc
[modify] https://crrev.com/9c8b10b5f102aac1db5ce7a2ac99a450381ee7c2/chrome/browser/ui/views/toolbar/toolbar_action_view.h
[modify] https://crrev.com/9c8b10b5f102aac1db5ce7a2ac99a450381ee7c2/chrome/browser/ui/views/toolbar/toolbar_button.cc
[modify] https://crrev.com/9c8b10b5f102aac1db5ce7a2ac99a450381ee7c2/chrome/browser/ui/views/toolbar/toolbar_button.h
[modify] https://crrev.com/9c8b10b5f102aac1db5ce7a2ac99a450381ee7c2/chrome/browser/ui/views/toolbar/toolbar_ink_drop_util.cc
[modify] https://crrev.com/9c8b10b5f102aac1db5ce7a2ac99a450381ee7c2/chrome/browser/ui/views/toolbar/toolbar_ink_drop_util.h
[modify] https://crrev.com/9c8b10b5f102aac1db5ce7a2ac99a450381ee7c2/ui/views/animation/ink_drop_host_view.cc
[modify] https://crrev.com/9c8b10b5f102aac1db5ce7a2ac99a450381ee7c2/ui/views/animation/ink_drop_host_view.h
[modify] https://crrev.com/9c8b10b5f102aac1db5ce7a2ac99a450381ee7c2/ui/views/animation/ink_drop_impl.cc
[modify] https://crrev.com/9c8b10b5f102aac1db5ce7a2ac99a450381ee7c2/ui/views/animation/ink_drop_impl.h
[modify] https://crrev.com/9c8b10b5f102aac1db5ce7a2ac99a450381ee7c2/ui/views/controls/button/button.cc
[modify] https://crrev.com/9c8b10b5f102aac1db5ce7a2ac99a450381ee7c2/ui/views/controls/button/checkbox.cc
[modify] https://crrev.com/9c8b10b5f102aac1db5ce7a2ac99a450381ee7c2/ui/views/controls/button/label_button.cc
[modify] https://crrev.com/9c8b10b5f102aac1db5ce7a2ac99a450381ee7c2/ui/views/controls/button/label_button.h
[modify] https://crrev.com/9c8b10b5f102aac1db5ce7a2ac99a450381ee7c2/ui/views/controls/button/md_text_button.cc
[modify] https://crrev.com/9c8b10b5f102aac1db5ce7a2ac99a450381ee7c2/ui/views/controls/button/md_text_button.h
[modify] https://crrev.com/9c8b10b5f102aac1db5ce7a2ac99a450381ee7c2/ui/views/controls/focus_ring.cc
[modify] https://crrev.com/9c8b10b5f102aac1db5ce7a2ac99a450381ee7c2/ui/views/controls/focus_ring.h

Project Member

Comment 18 by bugdroid1@chromium.org, Jan 9

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

commit 2edafd1198b94ab3f31537d7cd2ac09aaf011cba
Author: Peter Boström <pbos@chromium.org>
Date: Wed Jan 09 17:13:48 2019

Use highlight paths in ActionableView

This replaces TrayPopupUtils::CreateInkDropMask with
TrayPopupUtils::CreateHighlightPath. Reusing CreateHighlightPath should
make it easier to replace CreateInkDropX overrides that currently use
TrayPopupUtils with highlight paths.

Bug: chromium:888204
Change-Id: I92dd533eb2fd8d89d59ed7d95a8838bc8e5abf63
Reviewed-on: https://chromium-review.googlesource.com/c/1388744
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: Mohsen Izadi <mohsen@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621193}
[modify] https://crrev.com/2edafd1198b94ab3f31537d7cd2ac09aaf011cba/ash/system/tray/actionable_view.cc
[modify] https://crrev.com/2edafd1198b94ab3f31537d7cd2ac09aaf011cba/ash/system/tray/actionable_view.h
[modify] https://crrev.com/2edafd1198b94ab3f31537d7cd2ac09aaf011cba/ash/system/tray/system_menu_button.cc
[modify] https://crrev.com/2edafd1198b94ab3f31537d7cd2ac09aaf011cba/ash/system/tray/system_menu_button.h
[modify] https://crrev.com/2edafd1198b94ab3f31537d7cd2ac09aaf011cba/ash/system/tray/tray_background_view.cc
[modify] https://crrev.com/2edafd1198b94ab3f31537d7cd2ac09aaf011cba/ash/system/tray/tray_background_view.h
[modify] https://crrev.com/2edafd1198b94ab3f31537d7cd2ac09aaf011cba/ash/system/tray/tray_popup_utils.cc
[modify] https://crrev.com/2edafd1198b94ab3f31537d7cd2ac09aaf011cba/ash/system/tray/tray_popup_utils.h

Sign in to add a comment