New issue
Advanced search Search tips

Issue 822000 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocking:
issue 821991



Sign in to add a comment

Add LayoutProvider support for various levels of emphasis in MD refresh

Project Member Reported by pbos@chromium.org, Mar 14 2018

Issue description

Add semantic constants to LayoutProvider to be able to query it for how large a corner radii to use for various surfaces.
 

Comment 1 by pbos@chromium.org, Mar 14 2018

This bug as written is confusing, see corresponding sections in design doc and spec.
This isn't talking about elevation, right?  Just roundedness of things.

I wonder if there is any generalization we can do beyond just "get a corner radius".  The dream (probably infeasible) world is that some object says "I have medium emphasis" and we get things like rounded corners and the right inkdrop/ripple behavior for free.  But anything on the road to that would be nice.

Comment 3 by pbos@chromium.org, Mar 14 2018

I think there would be a "I am medium emphasis" enum that you'd put into LayoutProvider and either get back a {rounded corners, elevation} struct back or GetCornerRadius(MEDIUM_EMPHASIS) / GetElevation(MEDIUM_EMPHASIS).

If we have an EmphasisProperties object returned for MEDIUM_EMPHASIS we can jam ripple behavior etc. into it. Does that sound sensible or are you thinking something else?
No idea, haven't thought through the implementation.

Comment 5 by pbos@chromium.org, Mar 14 2018

Spitballing: Hopefully it's not along the lines of GetCornerDp(SMALL_CORNER) but rather an object encompassing all emphasis for the dialog at hand. Better if that object can be handed to relevant constructors (say DialogDelegate::GetEmphasisProperties or whatever).

Comment 6 by pbos@chromium.org, Mar 15 2018

Labels: Proj-MdRefresh
Are those various styles of emphasis going to remain tied together for all time? Is it possible that some element might have EMPHASIS_MEDIUM for corner radius and EMPHASIS_LARGE for elevation?

I can envision some future UX request for some element to be "Bubble X should have 4dp corners with the highest elevation."

Comment 8 by pbos@chromium.org, Mar 16 2018

I think UX would have to phrase "Bubble X" as a specific style group. Bubble X would have to be of type Y and we would return {4dp, ELEVATION_HIGHEST} or something for type Y.

And then we should strive to have as few types as possible. Being very hand-wavy on purpose.
Owner: kylixrd@chromium.org
Status: Assigned (was: Available)
Project Member

Comment 11 by bugdroid1@chromium.org, Mar 23 2018

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

commit cfe1563c99200820535fb84735f03fab8fe7060e
Author: Allen Bauer <kylixrd@chromium.org>
Date: Fri Mar 23 20:47:41 2018

Add new MD Refresh related functions/enumeration to ChromeLayoutProvider.

Bug:  822000 
Change-Id: I950ec472da2b5b8f8b52c54b9d14adc0098b3804
Reviewed-on: https://chromium-review.googlesource.com/969092
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Allen Bauer <kylixrd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545572}
[modify] https://crrev.com/cfe1563c99200820535fb84735f03fab8fe7060e/chrome/browser/ui/views/harmony/chrome_layout_provider.cc
[modify] https://crrev.com/cfe1563c99200820535fb84735f03fab8fe7060e/chrome/browser/ui/views/harmony/chrome_layout_provider.h
[modify] https://crrev.com/cfe1563c99200820535fb84735f03fab8fe7060e/ui/views/layout/layout_provider.cc
[modify] https://crrev.com/cfe1563c99200820535fb84735f03fab8fe7060e/ui/views/layout/layout_provider.h

Status: Fixed (was: Assigned)
Labels: -Pri-3 Pri-1
Project Member

Comment 14 by bugdroid1@chromium.org, Apr 5 2018

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

commit 7a462a5db6eeeedd30c5e4abfa9f45b6a8aca7f6
Author: Allen Bauer <kylixrd@chromium.org>
Date: Thu Apr 05 14:07:38 2018

Added material refresh layout provider. Updated material design controller.

Added IsMaterialRefreshMode() function to ChromeLayoutProvider.

Bug:  822000 
Change-Id: I379d7603838cdec60b3f94fc0e163e9907995b61
Reviewed-on: https://chromium-review.googlesource.com/988052
Commit-Queue: Allen Bauer <kylixrd@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548413}
[modify] https://crrev.com/7a462a5db6eeeedd30c5e4abfa9f45b6a8aca7f6/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/7a462a5db6eeeedd30c5e4abfa9f45b6a8aca7f6/chrome/browser/ui/views/harmony/chrome_layout_provider.cc
[add] https://crrev.com/7a462a5db6eeeedd30c5e4abfa9f45b6a8aca7f6/chrome/browser/ui/views/harmony/material_refresh_layout_provider.cc
[add] https://crrev.com/7a462a5db6eeeedd30c5e4abfa9f45b6a8aca7f6/chrome/browser/ui/views/harmony/material_refresh_layout_provider.h
[modify] https://crrev.com/7a462a5db6eeeedd30c5e4abfa9f45b6a8aca7f6/ui/base/material_design/material_design_controller.cc
[modify] https://crrev.com/7a462a5db6eeeedd30c5e4abfa9f45b6a8aca7f6/ui/views/controls/scroll_view_unittest.cc
[modify] https://crrev.com/7a462a5db6eeeedd30c5e4abfa9f45b6a8aca7f6/ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc

Project Member

Comment 15 by bugdroid1@chromium.org, Apr 5 2018

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

commit 95daf3e977127b37f176be7eb35ca4ffbf15f66a
Author: Takumi Fujimoto <takumif@chromium.org>
Date: Thu Apr 05 21:01:10 2018

Revert "Added material refresh layout provider. Updated material design controller."

This reverts commit 7a462a5db6eeeedd30c5e4abfa9f45b6a8aca7f6.

Reason for revert: This CL seems to be the cause for a
views_unittests failure:
https://ci.chromium.org/buildbot/chromium.memory/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/26901

Original change's description:
> Added material refresh layout provider. Updated material design controller.
> 
> Added IsMaterialRefreshMode() function to ChromeLayoutProvider.
> 
> Bug:  822000 
> Change-Id: I379d7603838cdec60b3f94fc0e163e9907995b61
> Reviewed-on: https://chromium-review.googlesource.com/988052
> Commit-Queue: Allen Bauer <kylixrd@chromium.org>
> Reviewed-by: Scott Violet <sky@chromium.org>
> Reviewed-by: Peter Kasting <pkasting@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#548413}

TBR=sky@chromium.org,pkasting@chromium.org,kylixrd@chromium.org

Change-Id: I173a8e69440ee564accc1824d6f1406dc7b1cb92
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  822000 
Reviewed-on: https://chromium-review.googlesource.com/998852
Reviewed-by: Takumi Fujimoto <takumif@chromium.org>
Commit-Queue: Takumi Fujimoto <takumif@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548554}
[modify] https://crrev.com/95daf3e977127b37f176be7eb35ca4ffbf15f66a/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/95daf3e977127b37f176be7eb35ca4ffbf15f66a/chrome/browser/ui/views/harmony/chrome_layout_provider.cc
[delete] https://crrev.com/cb7df4866c1a44631949bea1fc1f53c708b3046b/chrome/browser/ui/views/harmony/material_refresh_layout_provider.cc
[delete] https://crrev.com/cb7df4866c1a44631949bea1fc1f53c708b3046b/chrome/browser/ui/views/harmony/material_refresh_layout_provider.h
[modify] https://crrev.com/95daf3e977127b37f176be7eb35ca4ffbf15f66a/ui/base/material_design/material_design_controller.cc
[modify] https://crrev.com/95daf3e977127b37f176be7eb35ca4ffbf15f66a/ui/views/controls/scroll_view_unittest.cc
[modify] https://crrev.com/95daf3e977127b37f176be7eb35ca4ffbf15f66a/ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc

Project Member

Comment 16 by bugdroid1@chromium.org, Apr 6 2018

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

commit 95f066e0a41d1d87220aa093b40e3d57330b29bc
Author: Allen Bauer <kylixrd@chromium.org>
Date: Fri Apr 06 18:35:36 2018

Reland "Added material refresh layout provider. Updated material design controller."

This is a reland of 7a462a5db6eeeedd30c5e4abfa9f45b6a8aca7f6

Original change's description:
> Added material refresh layout provider. Updated material design controller.
> 
> Added IsMaterialRefreshMode() function to ChromeLayoutProvider.
> 
> Bug:  822000 
> Change-Id: I379d7603838cdec60b3f94fc0e163e9907995b61
> Reviewed-on: https://chromium-review.googlesource.com/988052
> Commit-Queue: Allen Bauer <kylixrd@chromium.org>
> Reviewed-by: Scott Violet <sky@chromium.org>
> Reviewed-by: Peter Kasting <pkasting@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#548413}

Bug:  822000 
Change-Id: I10e27e8294308d01415664adccfac7c4a7206b38
Reviewed-on: https://chromium-review.googlesource.com/999335
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Allen Bauer <kylixrd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548871}
[modify] https://crrev.com/95f066e0a41d1d87220aa093b40e3d57330b29bc/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/95f066e0a41d1d87220aa093b40e3d57330b29bc/chrome/browser/ui/views/harmony/chrome_layout_provider.cc
[add] https://crrev.com/95f066e0a41d1d87220aa093b40e3d57330b29bc/chrome/browser/ui/views/harmony/material_refresh_layout_provider.cc
[add] https://crrev.com/95f066e0a41d1d87220aa093b40e3d57330b29bc/chrome/browser/ui/views/harmony/material_refresh_layout_provider.h
[modify] https://crrev.com/95f066e0a41d1d87220aa093b40e3d57330b29bc/ui/base/material_design/material_design_controller.cc
[modify] https://crrev.com/95f066e0a41d1d87220aa093b40e3d57330b29bc/ui/views/controls/progress_bar_unittest.cc
[modify] https://crrev.com/95f066e0a41d1d87220aa093b40e3d57330b29bc/ui/views/controls/scroll_view_unittest.cc
[modify] https://crrev.com/95f066e0a41d1d87220aa093b40e3d57330b29bc/ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc

Sign in to add a comment