Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2174643002:
[ash-md] Updates ash shelf tooltips to MD (Closed)
|
Created:
4 years, 5 months ago by varkha Modified:
4 years, 4 months ago CC:
chromium-reviews, msw+watch_chromium.org, sadrul, tfarina, groby+bubble_chromium.org, hcarmona+bubble_chromium.org, kalyank, rouslan+bubble_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[ash-md] Updates ash shelf tooltips to MD
Makes shelf tooltip appear as in MD spec.
- rounded corners with a radius of 2px
- height = 24px
- margins of 8px
- offset from the shelf 8px
- black background at 80% opacity
- white text color at 87% opacity
Note:
Sizing of the shelf tooltips (and other chrome bubbles) is broken because of
http://crbug.com/630444. This CL was debugged with a local rollback of
https://codereview.chromium.org/2148963002.
BUG=595011
Committed: https://crrev.com/4196fbdf697ccefa236d9b331698cdd34f8f1e18
Cr-Commit-Position: refs/heads/master@{#407898}
Patch Set 1 #
Total comments: 7
Patch Set 2 : [ash-md] Updates ash shelf tooltips to MD (not under flag) #Patch Set 3 : [ash-md] Updates ash shelf tooltips to MD (no new shadow type) #
Total comments: 8
Patch Set 4 : [ash-md] Updates ash shelf tooltips to MD (nits) #
Messages
Total messages: 36 (21 generated)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
varkha@chromium.org changed reviewers: + tdanderson@chromium.org
tdanderson@, can you please take a look? Thanks! Next steps (not in this CL): Make clock tooltip have a gap (it is not using ShelfTooltipManager). Investigate if line [1] is necessary. It seems like it is not since the bubble border has its own background [2] and painting both results in double opacity. We haven't noticed that because we were only using opaque colors. [1] - https://cs.chromium.org/chromium/src/ui/views/bubble/bubble_dialog_delegate.c... [2] - https://cs.chromium.org/chromium/src/ui/views/bubble/bubble_border.cc?sq=pack... Thanks!
Description was changed from ========== [ash-md] Updates ash shelf tooltips to MD Makes shelf tooltip appear as in MD spec. - rounded corners with a radius of 2px - height = 24px - margins of 8px - offset from the shelf 8px - black background at 80% opacity - white text color at 87% opacity BUG=595011 ========== to ========== [ash-md] Updates ash shelf tooltips to MD Makes shelf tooltip appear as in MD spec. - rounded corners with a radius of 2px - height = 24px - margins of 8px - offset from the shelf 8px - black background at 80% opacity - white text color at 87% opacity Note: Sizing of the shelf tooltips (and other chrome bubbles) is broken because of http://crbug.com/630444. This CL was debugged with a local rollback of https://codereview.chromium.org/2148963002. BUG=595011 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
msw@chromium.org changed reviewers: + msw@chromium.org
https://codereview.chromium.org/2174643002/diff/1/ash/shelf/shelf_tooltip_man... File ash/shelf/shelf_tooltip_manager.cc (right): https://codereview.chromium.org/2174643002/diff/1/ash/shelf/shelf_tooltip_man... ash/shelf/shelf_tooltip_manager.cc:59: set_shadow(material ? views::BubbleBorder::NO_ASSETS_TOOLTIP driveby: Can you use the existing NO_ASSETS setting with different (smaller) margins? Avoiding a new bubble setting would be nice.
I'll re-review pending your opinion of my suggestion below: https://codereview.chromium.org/2174643002/diff/1/ash/common/material_design/... File ash/common/material_design/material_design_controller.h (right): https://codereview.chromium.org/2174643002/diff/1/ash/common/material_design/... ash/common/material_design/material_design_controller.h:47: static bool IsTooltipMaterial(); Since you're in the process of making the top chrome and other system tooltips conform to MD style, I'd vote for just landing this as being on by default. I think it's the right thing to do (and would simplify this CL). If this does land behind the flag, though, I'd suggest renaming this to something like IsShelfTooltipMaterial() to clarify that they are for the shelf app icons.
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
https://codereview.chromium.org/2174643002/diff/1/ash/common/material_design/... File ash/common/material_design/material_design_controller.h (right): https://codereview.chromium.org/2174643002/diff/1/ash/common/material_design/... ash/common/material_design/material_design_controller.h:47: static bool IsTooltipMaterial(); On 2016/07/22 17:24:31, tdanderson wrote: > Since you're in the process of making the top chrome and other system tooltips > conform to MD style, I'd vote for just landing this as being on by default. I > think it's the right thing to do (and would simplify this CL). > > If this does land behind the flag, though, I'd suggest renaming this to > something like IsShelfTooltipMaterial() to clarify that they are for the shelf > app icons. Done. https://codereview.chromium.org/2174643002/diff/1/ash/shelf/shelf_tooltip_man... File ash/shelf/shelf_tooltip_manager.cc (right): https://codereview.chromium.org/2174643002/diff/1/ash/shelf/shelf_tooltip_man... ash/shelf/shelf_tooltip_manager.cc:59: set_shadow(material ? views::BubbleBorder::NO_ASSETS_TOOLTIP On 2016/07/22 16:51:06, msw wrote: > driveby: Can you use the existing NO_ASSETS setting with different (smaller) > margins? Avoiding a new bubble setting would be nice. views::BubbleBorder::NO_ASSETS results in huge vertical margin that cannot be reduced below 17. I could plumb through some way to adjust internal margins in the BubbleBorder but I don't think this is any more elegant. Am I missing something?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
varkha@chromium.org changed reviewers: + jamescook@chromium.org
+jamescook@ for OWNERS in ash/shelf.
msw@chromium.org changed reviewers: + mgiuca@chromium.org
https://codereview.chromium.org/2174643002/diff/1/ash/shelf/shelf_tooltip_man... File ash/shelf/shelf_tooltip_manager.cc (right): https://codereview.chromium.org/2174643002/diff/1/ash/shelf/shelf_tooltip_man... ash/shelf/shelf_tooltip_manager.cc:59: set_shadow(material ? views::BubbleBorder::NO_ASSETS_TOOLTIP On 2016/07/22 22:03:56, varkha wrote: > On 2016/07/22 16:51:06, msw wrote: > > driveby: Can you use the existing NO_ASSETS setting with different (smaller) > > margins? Avoiding a new bubble setting would be nice. > > views::BubbleBorder::NO_ASSETS results in huge vertical margin that cannot be > reduced below 17. I could plumb through some way to adjust internal margins in > the BubbleBorder but I don't think this is any more elegant. Am I missing > something? There's only one user; SubtleNotificationView. Perhaps we can reduce the 17px |border_interior_thickness| used by NO_ASSETS, and increase the internal padding/margin of the SubtleNotificationView. +mgiuca
https://codereview.chromium.org/2174643002/diff/1/ash/shelf/shelf_tooltip_man... File ash/shelf/shelf_tooltip_manager.cc (right): https://codereview.chromium.org/2174643002/diff/1/ash/shelf/shelf_tooltip_man... ash/shelf/shelf_tooltip_manager.cc:59: set_shadow(material ? views::BubbleBorder::NO_ASSETS_TOOLTIP On 2016/07/22 22:14:52, msw wrote: > On 2016/07/22 22:03:56, varkha wrote: > > On 2016/07/22 16:51:06, msw wrote: > > > driveby: Can you use the existing NO_ASSETS setting with different (smaller) > > > margins? Avoiding a new bubble setting would be nice. > > > > views::BubbleBorder::NO_ASSETS results in huge vertical margin that cannot be > > reduced below 17. I could plumb through some way to adjust internal margins in > > the BubbleBorder but I don't think this is any more elegant. Am I missing > > something? > > There's only one user; SubtleNotificationView. Perhaps we can reduce the 17px > |border_interior_thickness| used by NO_ASSETS, and increase the internal > padding/margin of the SubtleNotificationView. +mgiuca Oops, there are multiple users; but the same thought holds. I suppose it's nbd if we need to add another bubble border type just to reduce insets; it just seems silly.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
varkha@, it sounds like msw and tdanderson had some questions... can you ping me for OWNERS again after you've worked things out with them?
I have added methods to allow customizing the interior border thickness. This seems the simplest and safest solution given how intertwined the margins, the borders and the contents are in those bubbles. I have also addressed tdanderson@'s comments. PTAL. https://codereview.chromium.org/2174643002/diff/1/ash/shelf/shelf_tooltip_man... File ash/shelf/shelf_tooltip_manager.cc (right): https://codereview.chromium.org/2174643002/diff/1/ash/shelf/shelf_tooltip_man... ash/shelf/shelf_tooltip_manager.cc:59: set_shadow(material ? views::BubbleBorder::NO_ASSETS_TOOLTIP On 2016/07/22 22:16:25, msw wrote: > On 2016/07/22 22:14:52, msw wrote: > > On 2016/07/22 22:03:56, varkha wrote: > > > On 2016/07/22 16:51:06, msw wrote: > > > > driveby: Can you use the existing NO_ASSETS setting with different > (smaller) > > > > margins? Avoiding a new bubble setting would be nice. > > > > > > views::BubbleBorder::NO_ASSETS results in huge vertical margin that cannot > be > > > reduced below 17. I could plumb through some way to adjust internal margins > in > > > the BubbleBorder but I don't think this is any more elegant. Am I missing > > > something? > > > > There's only one user; SubtleNotificationView. Perhaps we can reduce the 17px > > |border_interior_thickness| used by NO_ASSETS, and increase the internal > > padding/margin of the SubtleNotificationView. +mgiuca > > Oops, there are multiple users; but the same thought holds. I suppose it's nbd > if we need to add another bubble border type just to reduce insets; it just > seems silly. Yes, the margins that are settable with set_margin and the internal bubble border border_thickness combine in fairly complex ways. Margins affect the internal layout (define where the content appears) while the border thickness doesn't. Also the overall height H can be defined by the border_thickness b (H >= 2*b) but can be also defined by the content c and margins m (insets are affected by margins but I am not sure if there are NO_ASSET cases with additional insets from title, close button and such). So H >= max((c + m) , 2*b). Setting margins therefore affects the height in combination with the content which can be different while border thickness imposes a minimum regardless of contents. There is probably some way to untangle this but with more than one call site and multiple scenarios affecting it I can never be sure that I am not breaking some positioning or layout on some platform. So I would actually prefer plumbing through access to adjust border thickness at runtime or alternatively add a new preset which seems like a legitimate template. On the other hand I think I need to increase the border thickness to 12 to make the ash shelf tooltip be same height regardless of content.
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ash/shelf LGTM with nit https://codereview.chromium.org/2174643002/diff/40001/ash/shelf/shelf_tooltip... File ash/shelf/shelf_tooltip_manager.cc (right): https://codereview.chromium.org/2174643002/diff/40001/ash/shelf/shelf_tooltip... ash/shelf/shelf_tooltip_manager.cc:47: // bubble and an additional 1px padding for the shelf item views. I like that you write detailed documentation for layout constants. https://codereview.chromium.org/2174643002/diff/40001/ash/shelf/shelf_tooltip... ash/shelf/shelf_tooltip_manager.cc:64: gfx::Insets insets = nit: Just gfx::Insets insets(value, value); https://codereview.chromium.org/2174643002/diff/40001/ash/shelf/shelf_tooltip... ash/shelf/shelf_tooltip_manager.cc:87: theme->GetSystemColor(ui::NativeTheme::kColorId_TooltipText)); Just for my knowledge, is this the usual way of getting MD colors (e.g. via GetSystemColor -> GetAuraColor in ui/native_theme/common_theme.cc)?
bubble lgtm; thanks for the more generic solution there. https://codereview.chromium.org/2174643002/diff/40001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_border.h (right): https://codereview.chromium.org/2174643002/diff/40001/ui/views/bubble/bubble_... ui/views/bubble/bubble_border.h:209: // Needs to be invoked once |arrow_paint_type_| has been set. nit: "May be invoked after |arrow_paint_type_| has been set." or "May only be".
https://codereview.chromium.org/2174643002/diff/40001/ash/shelf/shelf_tooltip... File ash/shelf/shelf_tooltip_manager.cc (right): https://codereview.chromium.org/2174643002/diff/40001/ash/shelf/shelf_tooltip... ash/shelf/shelf_tooltip_manager.cc:47: // bubble and an additional 1px padding for the shelf item views. On 2016/07/25 17:01:08, James Cook wrote: > I like that you write detailed documentation for layout constants. Acknowledged. https://codereview.chromium.org/2174643002/diff/40001/ash/shelf/shelf_tooltip... ash/shelf/shelf_tooltip_manager.cc:64: gfx::Insets insets = On 2016/07/25 17:01:08, James Cook wrote: > nit: Just gfx::Insets insets(value, value); Done. https://codereview.chromium.org/2174643002/diff/40001/ash/shelf/shelf_tooltip... ash/shelf/shelf_tooltip_manager.cc:87: theme->GetSystemColor(ui::NativeTheme::kColorId_TooltipText)); On 2016/07/25 17:01:08, James Cook wrote: > Just for my knowledge, is this the usual way of getting MD colors (e.g. via > GetSystemColor -> GetAuraColor in ui/native_theme/common_theme.cc)? In this case we want the color to match the top chrome and this is what the top chrome was using for tooltips. https://codereview.chromium.org/2174643002/diff/40001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_border.h (right): https://codereview.chromium.org/2174643002/diff/40001/ui/views/bubble/bubble_... ui/views/bubble/bubble_border.h:209: // Needs to be invoked once |arrow_paint_type_| has been set. On 2016/07/25 18:11:56, msw wrote: > nit: "May be invoked after |arrow_paint_type_| has been set." or "May only be". Done.
The CQ bit was checked by varkha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamescook@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/2174643002/#ps60001 (title: "[ash-md] Updates ash shelf tooltips to MD (nits)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [ash-md] Updates ash shelf tooltips to MD Makes shelf tooltip appear as in MD spec. - rounded corners with a radius of 2px - height = 24px - margins of 8px - offset from the shelf 8px - black background at 80% opacity - white text color at 87% opacity Note: Sizing of the shelf tooltips (and other chrome bubbles) is broken because of http://crbug.com/630444. This CL was debugged with a local rollback of https://codereview.chromium.org/2148963002. BUG=595011 ========== to ========== [ash-md] Updates ash shelf tooltips to MD Makes shelf tooltip appear as in MD spec. - rounded corners with a radius of 2px - height = 24px - margins of 8px - offset from the shelf 8px - black background at 80% opacity - white text color at 87% opacity Note: Sizing of the shelf tooltips (and other chrome bubbles) is broken because of http://crbug.com/630444. This CL was debugged with a local rollback of https://codereview.chromium.org/2148963002. BUG=595011 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [ash-md] Updates ash shelf tooltips to MD Makes shelf tooltip appear as in MD spec. - rounded corners with a radius of 2px - height = 24px - margins of 8px - offset from the shelf 8px - black background at 80% opacity - white text color at 87% opacity Note: Sizing of the shelf tooltips (and other chrome bubbles) is broken because of http://crbug.com/630444. This CL was debugged with a local rollback of https://codereview.chromium.org/2148963002. BUG=595011 ========== to ========== [ash-md] Updates ash shelf tooltips to MD Makes shelf tooltip appear as in MD spec. - rounded corners with a radius of 2px - height = 24px - margins of 8px - offset from the shelf 8px - black background at 80% opacity - white text color at 87% opacity Note: Sizing of the shelf tooltips (and other chrome bubbles) is broken because of http://crbug.com/630444. This CL was debugged with a local rollback of https://codereview.chromium.org/2148963002. BUG=595011 Committed: https://crrev.com/4196fbdf697ccefa236d9b331698cdd34f8f1e18 Cr-Commit-Position: refs/heads/master@{#407898} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/4196fbdf697ccefa236d9b331698cdd34f8f1e18 Cr-Commit-Position: refs/heads/master@{#407898} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
