Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2555373004:
Apply new MD shadows to CrOS tray bubbles. (Closed)
|
Created:
4 years ago by Evan Stade Modified:
4 years ago CC:
chromium-reviews, groby+bubble_chromium.org, sadrul, yusukes+watch_chromium.org, Peter Beverloo, tfarina, mlamouri+watch-notifications_chromium.org, shuchen+watch_chromium.org, nona+watch_chromium.org, msw+watch_chromium.org, oshima+watch_chromium.org, kalyank, hcarmona+bubble_chromium.org, rouslan+bubble_chromium.org, awdf+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionApply new MD shadows to CrOS tray bubbles.
* affects system menu, message center, palette bubble, ime bubble, etc.
* WM shadow aperture calculation is adjusted to work better for small
windows (like the message center when it's empty). There's a TODO
here which will have to wait for Sebastien to get back so he can give
feedback on what to do in this case.
* fix alignment of NO_ASSETS bubble --- don't account for
non-existent stroke.
* existing BubbleBorder tests applied to more different kinds of bubble
BUG=608852
Committed: https://crrev.com/ff9d99415ba8fd50e6b02d17a4540a30fafafe54
Cr-Commit-Position: refs/heads/master@{#438196}
Patch Set 1 #Patch Set 2 : . #
Total comments: 2
Patch Set 3 : leave activation alone #
Total comments: 10
Patch Set 4 : comment #
Total comments: 5
Patch Set 5 : fix test for mac #
Messages
Total messages: 58 (23 generated)
Description was changed from ========== Apply new MD shadows to CrOS tray bubbles. * affects system menu, message center, palette bubble, ime bubble, etc. * system menu joins the rest of the bubbles as being activatable (little to no effective behavioral difference, just aesthetics) * alignment of NO_ASSETS bubble is fixed --- don't account for non-existent stroke. * existing BubbleBorder tests applied to more different kinds of bubble BUG=608852 ========== to ========== Apply new MD shadows to CrOS tray bubbles. * affects system menu, message center, palette bubble, ime bubble, etc. * system menu joins the rest of the bubbles as being activatable (little to no effective behavioral difference, just aesthetics) * WM shadow aperture calculation is adjusted to work better for small windows (like the message center when it's empty). There's a TODO here which will have to wait for Sebastien to get back so he can give feedback on what to do in this case. * fix alignment of NO_ASSETS bubble --- don't account for non-existent stroke. * existing BubbleBorder tests applied to more different kinds of bubble BUG=608852 ==========
estade@chromium.org changed reviewers: + jamescook@chromium.org, msw@chromium.org
The CQ bit was checked by estade@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...
jamescook, ptal ash/ and shadow.cc msw, ptal ui/views/bubble/
https://codereview.chromium.org/2555373004/diff/20001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_border_unittest.cc (right): https://codereview.chromium.org/2555373004/diff/20001/ui/views/bubble/bubble_... ui/views/bubble/bubble_border_unittest.cc:330: const gfx::Size kContentSize(500, 600); summary of changes to this test (since it's hard to tell with all the reformatting): 1. this size is larger (so we don't run into minimum size issues for big shadows) 2. there's a loop that iterates over all shadow types 3. stroke is only used for asset-based shadows 4. kHeightDifference check updated to work for big shadow
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
ui/views/bubble lgtm with a nit; I hope the activation change is okay. https://codereview.chromium.org/2555373004/diff/20001/ui/views/bubble/tray_bu... File ui/views/bubble/tray_bubble_view.cc (right): https://codereview.chromium.org/2555373004/diff/20001/ui/views/bubble/tray_bu... ui/views/bubble/tray_bubble_view.cc:205: set_can_activate(true); nit: this is true by default; remove the explicit init
estade@chromium.org changed reviewers: + oshima@chromium.org
estade@chromium.org changed reviewers: - oshima@chromium.org
On 2016/12/08 22:09:46, msw wrote: > ui/views/bubble lgtm with a nit; I hope the activation change is okay. Looks like I shouldn't be making that change as it was changed intentionally recently -- crbug.com/120680 I can work around this but it does seem buggy that it's different between the system menu and other tray bubbles. > > https://codereview.chromium.org/2555373004/diff/20001/ui/views/bubble/tray_bu... > File ui/views/bubble/tray_bubble_view.cc (right): > > https://codereview.chromium.org/2555373004/diff/20001/ui/views/bubble/tray_bu... > ui/views/bubble/tray_bubble_view.cc:205: set_can_activate(true); > nit: this is true by default; remove the explicit init
(so please hold off on reviewing for now)
On 2016/12/09 16:40:13, Evan Stade wrote: > (so please hold off on reviewing for now) You might want to talk to oshima@ about that CL. I think it was for ARC++. Deactivated ARC++ windows stop updating/painting IIRC.
On 2016/12/09 17:00:45, James Cook wrote: > On 2016/12/09 16:40:13, Evan Stade wrote: > > (so please hold off on reviewing for now) > > You might want to talk to oshima@ about that CL. I think it was for ARC++. > Deactivated ARC++ windows stop updating/painting IIRC. yea, I'll just leave it alone for now. The weird thing is that all the other tray bubbles still steal activation (seems like the TODO(oshima) suggests they should be updated). PTAL latest patchset. It's odd that we'll be applying STYLE_ACTIVE to bubbles which are technically not active, and in the future I suspect there will be bubbles that want a different style, but for now this works. I've added a TODO to move elevation logic into shadow_controller, but that's tied up with improving the animations (the existing animations don't look great any more; they kind of flash with the new shadow appearance).
Description was changed from ========== Apply new MD shadows to CrOS tray bubbles. * affects system menu, message center, palette bubble, ime bubble, etc. * system menu joins the rest of the bubbles as being activatable (little to no effective behavioral difference, just aesthetics) * WM shadow aperture calculation is adjusted to work better for small windows (like the message center when it's empty). There's a TODO here which will have to wait for Sebastien to get back so he can give feedback on what to do in this case. * fix alignment of NO_ASSETS bubble --- don't account for non-existent stroke. * existing BubbleBorder tests applied to more different kinds of bubble BUG=608852 ========== to ========== Apply new MD shadows to CrOS tray bubbles. * affects system menu, message center, palette bubble, ime bubble, etc. * WM shadow aperture calculation is adjusted to work better for small windows (like the message center when it's empty). There's a TODO here which will have to wait for Sebastien to get back so he can give feedback on what to do in this case. * fix alignment of NO_ASSETS bubble --- don't account for non-existent stroke. * existing BubbleBorder tests applied to more different kinds of bubble BUG=608852 ==========
The CQ bit was checked by estade@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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Are the shadow changes tied to a flag, or are they on by default for M57? When you say the existing animations don't look great, is this CL going to mess up our dev channel animation appearance, or is this going to get sorted out before users ever see it? https://codereview.chromium.org/2555373004/diff/40001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_border.cc (right): https://codereview.chromium.org/2555373004/diff/40001/ui/views/bubble/bubble_... ui/views/bubble/bubble_border.cc:258: const int stroke_width = shadow_ == NO_ASSETS ? 0 : kStroke; nit: Can you add a comment in the enum saying what NO_ASSETS means? (I think I know, but it would be easier if there were docs.) https://codereview.chromium.org/2555373004/diff/40001/ui/wm/core/shadow.cc File ui/wm/core/shadow.cc (right): https://codereview.chromium.org/2555373004/diff/40001/ui/wm/core/shadow.cc#ne... ui/wm/core/shadow.cc:198: // by adjusting the shadow for very small windows, or other means. Aside: I vaguely recall from long ago that we decided not to worry about very small windows (like 20x20 or whatever) since they don't occur in practice. I'm not sure how small is "very small" for the purposes of this code. https://codereview.chromium.org/2555373004/diff/40001/ui/wm/core/shadow_contr... File ui/wm/core/shadow_controller.cc (right): https://codereview.chromium.org/2555373004/diff/40001/ui/wm/core/shadow_contr... ui/wm/core/shadow_controller.cc:58: case ui::wm::WINDOW_TYPE_POPUP: From the code this looks like it will apply large shadows to non-bubble windows (like any Widget of type frameless, and drag widgets too: https://cs.chromium.org/chromium/src/ui/views/widget/widget_aura_utils.cc?l=2... That seems undesirable to me, even in the short term. https://codereview.chromium.org/2555373004/diff/40001/ui/wm/core/shadow_contr... ui/wm/core/shadow_controller.cc:205: GetShadowStyleForWindow(lost_active) == Shadow::STYLE_INACTIVE) { Shouldn't you be checking if the window that lost activation was previously active? Or always updating the style?
On 2016/12/09 19:40:13, James Cook wrote: > Are the shadow changes tied to a flag, or are they on by default for M57? > > When you say the existing animations don't look great, is this CL going to mess > up our dev channel animation appearance, or is this going to get sorted out > before users ever see it? No, it's not behind a flag, and this CL doesn't mess up the existing animations --- they didn't look amazing even before this change. But they also don't look glaringly awful. They just look a little worse with the new style of shadow than they did with the old shadows. I'm not sure if dev channel users will ever see this because dev is only cut once a week and currently dev is still on 56. In any case, here's a fairly simple CL to make the animations look much better (imo): https://codereview.chromium.org/2562143002/ https://codereview.chromium.org/2555373004/diff/40001/ui/wm/core/shadow.cc File ui/wm/core/shadow.cc (right): https://codereview.chromium.org/2555373004/diff/40001/ui/wm/core/shadow.cc#ne... ui/wm/core/shadow.cc:198: // by adjusting the shadow for very small windows, or other means. On 2016/12/09 19:40:13, James Cook wrote: > Aside: I vaguely recall from long ago that we decided not to worry about very > small windows (like 20x20 or whatever) since they don't occur in practice. I'm > not sure how small is "very small" for the purposes of this code. I ran into this problem for the message center bubble when I have no messages (at which point it's only 50px tall). It's not super high priority imo because it is pretty rare, as you say --- you can't make a browser window short enough to see it. That said, I might not want to launch this to stable like this, but if we don't have a great solution by then we can always just revert the shadow change only for the notification bubble. https://codereview.chromium.org/2555373004/diff/40001/ui/wm/core/shadow_contr... File ui/wm/core/shadow_controller.cc (right): https://codereview.chromium.org/2555373004/diff/40001/ui/wm/core/shadow_contr... ui/wm/core/shadow_controller.cc:58: case ui::wm::WINDOW_TYPE_POPUP: On 2016/12/09 19:40:13, James Cook wrote: > From the code this looks like it will apply large shadows to non-bubble windows > (like any Widget of type frameless, and drag widgets too: > > https://cs.chromium.org/chromium/src/ui/views/widget/widget_aura_utils.cc?l=2... > > That seems undesirable to me, even in the short term. I'll look into it. I had hoped that other popups weren't getting WM shadows. https://codereview.chromium.org/2555373004/diff/40001/ui/wm/core/shadow_contr... ui/wm/core/shadow_controller.cc:205: GetShadowStyleForWindow(lost_active) == Shadow::STYLE_INACTIVE) { On 2016/12/09 19:40:13, James Cook wrote: > Shouldn't you be checking if the window that lost activation was previously > active? Or always updating the style? I don't think this change affects the behavior at all, does it?
https://codereview.chromium.org/2555373004/diff/40001/ui/wm/core/shadow_contr... File ui/wm/core/shadow_controller.cc (right): https://codereview.chromium.org/2555373004/diff/40001/ui/wm/core/shadow_contr... ui/wm/core/shadow_controller.cc:58: case ui::wm::WINDOW_TYPE_POPUP: On 2016/12/09 21:09:18, Evan Stade wrote: > On 2016/12/09 19:40:13, James Cook wrote: > > From the code this looks like it will apply large shadows to non-bubble > windows > > (like any Widget of type frameless, and drag widgets too: > > > > > https://cs.chromium.org/chromium/src/ui/views/widget/widget_aura_utils.cc?l=2... > > > > That seems undesirable to me, even in the short term. > > I'll look into it. I had hoped that other popups weren't getting WM shadows. Do you have an example of a drag widget? When I drag a bookmark bar button or something inside a webpage, I don't see a shadow of any kind. When I drag a window around, I assume that's still using the same window, which is active.
On 2016/12/09 21:20:32, Evan Stade wrote: > https://codereview.chromium.org/2555373004/diff/40001/ui/wm/core/shadow_contr... > File ui/wm/core/shadow_controller.cc (right): > > https://codereview.chromium.org/2555373004/diff/40001/ui/wm/core/shadow_contr... > ui/wm/core/shadow_controller.cc:58: case ui::wm::WINDOW_TYPE_POPUP: > On 2016/12/09 21:09:18, Evan Stade wrote: > > On 2016/12/09 19:40:13, James Cook wrote: > > > From the code this looks like it will apply large shadows to non-bubble > > windows > > > (like any Widget of type frameless, and drag widgets too: > > > > > > > > > https://cs.chromium.org/chromium/src/ui/views/widget/widget_aura_utils.cc?l=2... > > > > > > That seems undesirable to me, even in the short term. > > > > I'll look into it. I had hoped that other popups weren't getting WM shadows. > > Do you have an example of a drag widget? When I drag a bookmark bar button or > something inside a webpage, I don't see a shadow of any kind. When I drag a > window around, I assume that's still using the same window, which is active. I'll look at the CL again later today, but for drag widgets I would try: * Dragging text and images between browser windows * Dragging text out of a views text view (omnibox) * Dragging out shelf icons Window dragging is different. Also, are you saying the empty notification center window looks weird with this CL, or does your code deal with that? Also also, can you upload a screenshot of the output of this CL somewhere?
On 2016/12/09 22:39:57, James Cook wrote: > I'll look at the CL again later today, but for drag widgets I would try: > * Dragging text and images between browser windows > * Dragging text out of a views text view (omnibox) > * Dragging out shelf icons Best I can tell, none of these actually set a shadow in the widget initparams, and shadow controller doesn't give them one by default. To get the tray bubble view to have this shadow I had to change the Widget::InitParams. > > Window dragging is different. DragWindowController does seem to do something relevant and is used when you're dragging a window across displays. In that case, active seems warranted. It looks fine when I test. > > Also, are you saying the empty notification center window looks weird with this > CL, or does your code deal with that? > > Also also, can you upload a screenshot of the output of this CL somewhere? Here's a screenshot of the system menu and the message center. Can you notice the problem with the latter? https://screenshot.googleplex.com/pipkpaTK7FC.png https://codereview.chromium.org/2555373004/diff/40001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_border.cc (right): https://codereview.chromium.org/2555373004/diff/40001/ui/views/bubble/bubble_... ui/views/bubble/bubble_border.cc:258: const int stroke_width = shadow_ == NO_ASSETS ? 0 : kStroke; On 2016/12/09 19:40:13, James Cook wrote: > nit: Can you add a comment in the enum saying what NO_ASSETS means? (I think I > know, but it would be easier if there were docs.) Done.
On 2016/12/09 23:06:16, Evan Stade wrote: > On 2016/12/09 22:39:57, James Cook wrote: > > I'll look at the CL again later today, but for drag widgets I would try: > > * Dragging text and images between browser windows > > * Dragging text out of a views text view (omnibox) > > * Dragging out shelf icons > > Best I can tell, none of these actually set a shadow in the widget initparams, > and shadow controller doesn't give them one by default. To get the tray bubble > view to have this shadow I had to change the Widget::InitParams. > > > > > Window dragging is different. > > DragWindowController does seem to do something relevant and is used when you're > dragging a window across displays. In that case, active seems warranted. It > looks fine when I test. > > > > > Also, are you saying the empty notification center window looks weird with > this > > CL, or does your code deal with that? > > > > Also also, can you upload a screenshot of the output of this CL somewhere? > > Here's a screenshot of the system menu and the message center. Can you notice > the problem with the latter? https://screenshot.googleplex.com/pipkpaTK7FC.png > > https://codereview.chromium.org/2555373004/diff/40001/ui/views/bubble/bubble_... > File ui/views/bubble/bubble_border.cc (right): > > https://codereview.chromium.org/2555373004/diff/40001/ui/views/bubble/bubble_... > ui/views/bubble/bubble_border.cc:258: const int stroke_width = shadow_ == > NO_ASSETS ? 0 : kStroke; > On 2016/12/09 19:40:13, James Cook wrote: > > nit: Can you add a comment in the enum saying what NO_ASSETS means? (I think > I > > know, but it would be easier if there were docs.) > > Done. Can you take another screenshot on a lighter, non-uniform background? I have a hard time seeing gray shadows on a gray background.
On 2016/12/10 00:00:55, James Cook wrote: > On 2016/12/09 23:06:16, Evan Stade wrote: > > On 2016/12/09 22:39:57, James Cook wrote: > > > I'll look at the CL again later today, but for drag widgets I would try: > > > * Dragging text and images between browser windows > > > * Dragging text out of a views text view (omnibox) > > > * Dragging out shelf icons > > > > Best I can tell, none of these actually set a shadow in the widget initparams, > > and shadow controller doesn't give them one by default. To get the tray bubble > > view to have this shadow I had to change the Widget::InitParams. > > > > > > > > Window dragging is different. > > > > DragWindowController does seem to do something relevant and is used when > you're > > dragging a window across displays. In that case, active seems warranted. It > > looks fine when I test. > > > > > > > > Also, are you saying the empty notification center window looks weird with > > this > > > CL, or does your code deal with that? > > > > > > Also also, can you upload a screenshot of the output of this CL somewhere? > > > > Here's a screenshot of the system menu and the message center. Can you notice > > the problem with the latter? https://screenshot.googleplex.com/pipkpaTK7FC.png > > > > > https://codereview.chromium.org/2555373004/diff/40001/ui/views/bubble/bubble_... > > File ui/views/bubble/bubble_border.cc (right): > > > > > https://codereview.chromium.org/2555373004/diff/40001/ui/views/bubble/bubble_... > > ui/views/bubble/bubble_border.cc:258: const int stroke_width = shadow_ == > > NO_ASSETS ? 0 : kStroke; > > On 2016/12/09 19:40:13, James Cook wrote: > > > nit: Can you add a comment in the enum saying what NO_ASSETS means? (I > think > > I > > > know, but it would be easier if there were docs.) > > > > Done. > > Can you take another screenshot on a lighter, non-uniform background? I have a > hard time seeing gray shadows on a gray background. here it is on a white bg: https://screenshot.googleplex.com/sMyZxrBYQvY.png non-uniformity actually makes it harder to see the shadow. And pretty much all potential bg images I found online make it harder to see them. In fact I suspect Sebastien will have to darken them or they'll be more or less invisible for most people.
https://codereview.chromium.org/2555373004/diff/40001/ui/wm/core/shadow_contr... File ui/wm/core/shadow_controller.cc (right): https://codereview.chromium.org/2555373004/diff/40001/ui/wm/core/shadow_contr... ui/wm/core/shadow_controller.cc:205: GetShadowStyleForWindow(lost_active) == Shadow::STYLE_INACTIVE) { On 2016/12/09 21:09:18, Evan Stade wrote: > On 2016/12/09 19:40:13, James Cook wrote: > > Shouldn't you be checking if the window that lost activation was previously > > active? Or always updating the style? > > I don't think this change affects the behavior at all, does it? What if |lost_active| had shadow style Shadow::STYLE_ACTIVE? You should reset the shadow style to inactive, right? https://codereview.chromium.org/2555373004/diff/60001/ui/wm/core/shadow_contr... File ui/wm/core/shadow_controller.cc (right): https://codereview.chromium.org/2555373004/diff/60001/ui/wm/core/shadow_contr... ui/wm/core/shadow_controller.cc:58: return Shadow::STYLE_ACTIVE; This still concerns me. You're relying on the fact that no one happens to turn on shadows for popup windows right now. How does the code work today? Visually in ToT it looks to me like system tray bubbles have the same big shadows as active windows.
On 2016/12/10 00:18:06, Evan Stade wrote: > here it is on a white bg: https://screenshot.googleplex.com/sMyZxrBYQvY.png FYI, the no notifications bubble is too short for that shadow; there's a bad horizontal line.
On 2016/12/10 00:20:07, msw wrote: > On 2016/12/10 00:18:06, Evan Stade wrote: > > here it is on a white bg: https://screenshot.googleplex.com/sMyZxrBYQvY.png > FYI, the no notifications bubble is too short for that shadow; there's a bad > horizontal line. Yep, I see the line too. Also, the top-left corner looks a little odd, like it's not perfectly round.
On 2016/12/10 00:20:07, msw wrote: > On 2016/12/10 00:18:06, Evan Stade wrote: > > here it is on a white bg: https://screenshot.googleplex.com/sMyZxrBYQvY.png > FYI, the no notifications bubble is too short for that shadow; there's a bad > horizontal line. Indeed, though that horizontal line is all but invisible on anything but a white bg. See discussion above and code TODO about potential mitigations (pending Sebastien's feedback).
On 2016/12/10 00:22:23, James Cook wrote: > Also, the top-left corner looks a little odd, like it's not perfectly round. Which bubble's top left corner? They both look normal to me even zoomed in.
On 2016/12/10 00:24:41, Evan Stade wrote: > On 2016/12/10 00:22:23, James Cook wrote: > > Also, the top-left corner looks a little odd, like it's not perfectly round. > > Which bubble's top left corner? They both look normal to me even zoomed in. Top-left corner of both. (That's probably not caused by this CL, but there's something there. It's not perfectly round.)
On 2016/12/10 00:31:18, James Cook wrote: > On 2016/12/10 00:24:41, Evan Stade wrote: > > On 2016/12/10 00:22:23, James Cook wrote: > > > Also, the top-left corner looks a little odd, like it's not perfectly round. > > > > Which bubble's top left corner? They both look normal to me even zoomed in. > > Top-left corner of both. > > (That's probably not caused by this CL, but there's something there. It's not > perfectly round.) I'm still not sure I see it. At 100% with 2px of rounding, it just doesn't look that round, but I think it's doing the right thing. At 200% with 4px of rounding it would look smoother. How do you want to proceed with this CL? Would you rather see the notification center left alone for now?
On 2016/12/12 17:08:34, Evan Stade wrote: > On 2016/12/10 00:31:18, James Cook wrote: > > On 2016/12/10 00:24:41, Evan Stade wrote: > > > On 2016/12/10 00:22:23, James Cook wrote: > > > > Also, the top-left corner looks a little odd, like it's not perfectly > round. > > > > > > Which bubble's top left corner? They both look normal to me even zoomed in. > > > > Top-left corner of both. > > > > (That's probably not caused by this CL, but there's something there. It's not > > perfectly round.) > > I'm still not sure I see it. At 100% with 2px of rounding, it just doesn't look > that round, but I think it's doing the right thing. At 200% with 4px of rounding > it would look smoother. > > How do you want to proceed with this CL? Would you rather see the notification > center left alone for now? I think the top-left corner is acceptable. I don't think the horizontal line in the message center is acceptable. It would be nice to find a way to fix that. My biggest concern, however, is applying large shadows to all popups. See my question in shadow_controller.cc.
https://codereview.chromium.org/2555373004/diff/60001/ui/wm/core/shadow_contr... File ui/wm/core/shadow_controller.cc (right): https://codereview.chromium.org/2555373004/diff/60001/ui/wm/core/shadow_contr... ui/wm/core/shadow_controller.cc:58: return Shadow::STYLE_ACTIVE; On 2016/12/10 00:19:16, James Cook wrote: > This still concerns me. You're relying on the fact that no one happens to turn > on shadows for popup windows right now. I don't think that's an unreasonable assumption. It wouldn't make sense to just flip them on for popups with or without this patch. You're right, we need to eventually address it (as per the TODO), but as a part of future work. > > How does the code work today? Visually in ToT it looks to me like system tray > bubbles have the same big shadows as active windows. yes, that is accurate. Spec[1]. This is category 5 (it's ambiguous whether that applies to just the system menu or all tray bubbles but I assume the latter as interaction patterns are the same), and it matches category 1 (active application frame). [1] https://folio.googleplex.com/andromeda/Chrome%20OS/System%20Components/Specs%...
jamescook@chromium.org changed reviewers: + sky@chromium.org
+sky -- do you have thoughts on ui/wm/core/shadow_controller.cc line 58? https://codereview.chromium.org/2555373004/diff/60001/ui/wm/core/shadow_contr... File ui/wm/core/shadow_controller.cc (right): https://codereview.chromium.org/2555373004/diff/60001/ui/wm/core/shadow_contr... ui/wm/core/shadow_controller.cc:58: return Shadow::STYLE_ACTIVE; On 2016/12/12 17:51:20, Evan Stade wrote: > On 2016/12/10 00:19:16, James Cook wrote: > > This still concerns me. You're relying on the fact that no one happens to turn > > on shadows for popup windows right now. > > I don't think that's an unreasonable assumption. It wouldn't make sense to just > flip them on for popups with or without this patch. You're right, we need to > eventually address it (as per the TODO), but as a part of future work. > > > > > How does the code work today? Visually in ToT it looks to me like system tray > > bubbles have the same big shadows as active windows. > > yes, that is accurate. Spec[1]. This is category 5 (it's ambiguous whether that > applies to just the system menu or all tray bubbles but I assume the latter as > interaction patterns are the same), and it matches category 1 (active > application frame). > > [1] > https://folio.googleplex.com/andromeda/Chrome%20OS/System%20Components/Specs%... I meant, how does the code work today to paint the system tray bubbles with large shadows? Apparently they are not active, otherwise you wouldn't need this line and could rely on 61 below.
https://codereview.chromium.org/2555373004/diff/60001/ui/wm/core/shadow_contr... File ui/wm/core/shadow_controller.cc (right): https://codereview.chromium.org/2555373004/diff/60001/ui/wm/core/shadow_contr... ui/wm/core/shadow_controller.cc:58: return Shadow::STYLE_ACTIVE; On 2016/12/12 18:33:07, James Cook wrote: > On 2016/12/12 17:51:20, Evan Stade wrote: > > On 2016/12/10 00:19:16, James Cook wrote: > > > This still concerns me. You're relying on the fact that no one happens to > turn > > > on shadows for popup windows right now. > > > > I don't think that's an unreasonable assumption. It wouldn't make sense to > just > > flip them on for popups with or without this patch. You're right, we need to > > eventually address it (as per the TODO), but as a part of future work. > > > > > > > > How does the code work today? Visually in ToT it looks to me like system > tray > > > bubbles have the same big shadows as active windows. > > > > yes, that is accurate. Spec[1]. This is category 5 (it's ambiguous whether > that > > applies to just the system menu or all tray bubbles but I assume the latter as > > interaction patterns are the same), and it matches category 1 (active > > application frame). > > > > [1] > > > https://folio.googleplex.com/andromeda/Chrome%20OS/System%20Components/Specs%... > > I meant, how does the code work today to paint the system tray bubbles with > large shadows? Apparently they are not active, otherwise you wouldn't need this > line and could rely on 61 below. Bubbles are mapped to ui::wm::WINDOW_TYPE_POPUP, which by default shouldn't have a shadow. Is someone explicitly calling SetShadowType()?
https://codereview.chromium.org/2555373004/diff/60001/ui/wm/core/shadow_contr... File ui/wm/core/shadow_controller.cc (right): https://codereview.chromium.org/2555373004/diff/60001/ui/wm/core/shadow_contr... ui/wm/core/shadow_controller.cc:58: return Shadow::STYLE_ACTIVE; On 2016/12/12 18:33:07, James Cook wrote: > On 2016/12/12 17:51:20, Evan Stade wrote: > > On 2016/12/10 00:19:16, James Cook wrote: > > > This still concerns me. You're relying on the fact that no one happens to > turn > > > on shadows for popup windows right now. > > > > I don't think that's an unreasonable assumption. It wouldn't make sense to > just > > flip them on for popups with or without this patch. You're right, we need to > > eventually address it (as per the TODO), but as a part of future work. > > > > > > > > How does the code work today? Visually in ToT it looks to me like system > tray > > > bubbles have the same big shadows as active windows. > > > > yes, that is accurate. Spec[1]. This is category 5 (it's ambiguous whether > that > > applies to just the system menu or all tray bubbles but I assume the latter as > > interaction patterns are the same), and it matches category 1 (active > > application frame). > > > > [1] > > > https://folio.googleplex.com/andromeda/Chrome%20OS/System%20Components/Specs%... > > I meant, how does the code work today to paint the system tray bubbles with > large shadows? Apparently they are not active, otherwise you wouldn't need this > line and could rely on 61 below. They don't get wm shadows at all currently. With this patch they do because of changes in TrayBubbleView.
On 2016/12/12 21:28:40, Evan Stade wrote: > https://codereview.chromium.org/2555373004/diff/60001/ui/wm/core/shadow_contr... > File ui/wm/core/shadow_controller.cc (right): > > https://codereview.chromium.org/2555373004/diff/60001/ui/wm/core/shadow_contr... > ui/wm/core/shadow_controller.cc:58: return Shadow::STYLE_ACTIVE; > On 2016/12/12 18:33:07, James Cook wrote: > > On 2016/12/12 17:51:20, Evan Stade wrote: > > > On 2016/12/10 00:19:16, James Cook wrote: > > > > This still concerns me. You're relying on the fact that no one happens to > > turn > > > > on shadows for popup windows right now. > > > > > > I don't think that's an unreasonable assumption. It wouldn't make sense to > > just > > > flip them on for popups with or without this patch. You're right, we need to > > > eventually address it (as per the TODO), but as a part of future work. > > > > > > > > > > > How does the code work today? Visually in ToT it looks to me like system > > tray > > > > bubbles have the same big shadows as active windows. > > > > > > yes, that is accurate. Spec[1]. This is category 5 (it's ambiguous whether > > that > > > applies to just the system menu or all tray bubbles but I assume the latter > as > > > interaction patterns are the same), and it matches category 1 (active > > > application frame). > > > > > > [1] > > > > > > https://folio.googleplex.com/andromeda/Chrome%20OS/System%20Components/Specs%... > > > > I meant, how does the code work today to paint the system tray bubbles with > > large shadows? Apparently they are not active, otherwise you wouldn't need > this > > line and could rely on 61 below. > > They don't get wm shadows at all currently. With this patch they do because of > changes in TrayBubbleView. It ToT today I see shadows for the system tray bubble. https://screenshot.googleplex.com/kHAAfQCiBx0
On 2016/12/12 22:35:26, James Cook wrote: > On 2016/12/12 21:28:40, Evan Stade wrote: > > > https://codereview.chromium.org/2555373004/diff/60001/ui/wm/core/shadow_contr... > > File ui/wm/core/shadow_controller.cc (right): > > > > > https://codereview.chromium.org/2555373004/diff/60001/ui/wm/core/shadow_contr... > > ui/wm/core/shadow_controller.cc:58: return Shadow::STYLE_ACTIVE; > > On 2016/12/12 18:33:07, James Cook wrote: > > > On 2016/12/12 17:51:20, Evan Stade wrote: > > > > On 2016/12/10 00:19:16, James Cook wrote: > > > > > This still concerns me. You're relying on the fact that no one happens > to > > > turn > > > > > on shadows for popup windows right now. > > > > > > > > I don't think that's an unreasonable assumption. It wouldn't make sense to > > > just > > > > flip them on for popups with or without this patch. You're right, we need > to > > > > eventually address it (as per the TODO), but as a part of future work. > > > > > > > > > > > > > > How does the code work today? Visually in ToT it looks to me like system > > > tray > > > > > bubbles have the same big shadows as active windows. > > > > > > > > yes, that is accurate. Spec[1]. This is category 5 (it's ambiguous whether > > > that > > > > applies to just the system menu or all tray bubbles but I assume the > latter > > as > > > > interaction patterns are the same), and it matches category 1 (active > > > > application frame). > > > > > > > > [1] > > > > > > > > > > https://folio.googleplex.com/andromeda/Chrome%20OS/System%20Components/Specs%... > > > > > > I meant, how does the code work today to paint the system tray bubbles with > > > large shadows? Apparently they are not active, otherwise you wouldn't need > > this > > > line and could rely on 61 below. > > > > They don't get wm shadows at all currently. With this patch they do because of > > changes in TrayBubbleView. > > It ToT today I see shadows for the system tray bubble. > https://screenshot.googleplex.com/kHAAfQCiBx0 That's not from wm::Shadow. That's the bubble border, which is something I've disabled in this patch by using the NO_ASSETS BubbleBorder.
On 2016/12/12 22:35:26, James Cook wrote: > On 2016/12/12 21:28:40, Evan Stade wrote: > > > https://codereview.chromium.org/2555373004/diff/60001/ui/wm/core/shadow_contr... > > File ui/wm/core/shadow_controller.cc (right): > > > > > https://codereview.chromium.org/2555373004/diff/60001/ui/wm/core/shadow_contr... > > ui/wm/core/shadow_controller.cc:58: return Shadow::STYLE_ACTIVE; > > On 2016/12/12 18:33:07, James Cook wrote: > > > On 2016/12/12 17:51:20, Evan Stade wrote: > > > > On 2016/12/10 00:19:16, James Cook wrote: > > > > > This still concerns me. You're relying on the fact that no one happens > to > > > turn > > > > > on shadows for popup windows right now. > > > > > > > > I don't think that's an unreasonable assumption. It wouldn't make sense to > > > just > > > > flip them on for popups with or without this patch. You're right, we need > to > > > > eventually address it (as per the TODO), but as a part of future work. > > > > > > > > > > > > > > How does the code work today? Visually in ToT it looks to me like system > > > tray > > > > > bubbles have the same big shadows as active windows. > > > > > > > > yes, that is accurate. Spec[1]. This is category 5 (it's ambiguous whether > > > that > > > > applies to just the system menu or all tray bubbles but I assume the > latter > > as > > > > interaction patterns are the same), and it matches category 1 (active > > > > application frame). > > > > > > > > [1] > > > > > > > > > > https://folio.googleplex.com/andromeda/Chrome%20OS/System%20Components/Specs%... > > > > > > I meant, how does the code work today to paint the system tray bubbles with > > > large shadows? Apparently they are not active, otherwise you wouldn't need > > this > > > line and could rely on 61 below. > > > > They don't get wm shadows at all currently. With this patch they do because of > > changes in TrayBubbleView. > > It ToT today I see shadows for the system tray bubble. > https://screenshot.googleplex.com/kHAAfQCiBx0 That's not from wm::Shadow. That's the bubble border, which is something I've disabled in this patch by using the NO_ASSETS BubbleBorder.
Ah, that makes more sense. LGTM.
The CQ bit was checked by estade@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...
On 2016/12/13 00:10:43, James Cook wrote: > Ah, that makes more sense. LGTM. sky, does this seem OK to you?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Yes, LGTM.
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, jamescook@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2555373004/#ps80001 (title: "fix test for mac")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481644652910230,
"parent_rev": "607a2a4487c9c43f129fe123a78db238fbb3855b", "commit_rev":
"11fd06a29111ddc3f9c32f761dd9c813f40b5a41"}
Message was sent while issue was closed.
Description was changed from ========== Apply new MD shadows to CrOS tray bubbles. * affects system menu, message center, palette bubble, ime bubble, etc. * WM shadow aperture calculation is adjusted to work better for small windows (like the message center when it's empty). There's a TODO here which will have to wait for Sebastien to get back so he can give feedback on what to do in this case. * fix alignment of NO_ASSETS bubble --- don't account for non-existent stroke. * existing BubbleBorder tests applied to more different kinds of bubble BUG=608852 ========== to ========== Apply new MD shadows to CrOS tray bubbles. * affects system menu, message center, palette bubble, ime bubble, etc. * WM shadow aperture calculation is adjusted to work better for small windows (like the message center when it's empty). There's a TODO here which will have to wait for Sebastien to get back so he can give feedback on what to do in this case. * fix alignment of NO_ASSETS bubble --- don't account for non-existent stroke. * existing BubbleBorder tests applied to more different kinds of bubble BUG=608852 Review-Url: https://codereview.chromium.org/2555373004 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Apply new MD shadows to CrOS tray bubbles. * affects system menu, message center, palette bubble, ime bubble, etc. * WM shadow aperture calculation is adjusted to work better for small windows (like the message center when it's empty). There's a TODO here which will have to wait for Sebastien to get back so he can give feedback on what to do in this case. * fix alignment of NO_ASSETS bubble --- don't account for non-existent stroke. * existing BubbleBorder tests applied to more different kinds of bubble BUG=608852 Review-Url: https://codereview.chromium.org/2555373004 ========== to ========== Apply new MD shadows to CrOS tray bubbles. * affects system menu, message center, palette bubble, ime bubble, etc. * WM shadow aperture calculation is adjusted to work better for small windows (like the message center when it's empty). There's a TODO here which will have to wait for Sebastien to get back so he can give feedback on what to do in this case. * fix alignment of NO_ASSETS bubble --- don't account for non-existent stroke. * existing BubbleBorder tests applied to more different kinds of bubble BUG=608852 Committed: https://crrev.com/ff9d99415ba8fd50e6b02d17a4540a30fafafe54 Cr-Commit-Position: refs/heads/master@{#438196} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ff9d99415ba8fd50e6b02d17a4540a30fafafe54 Cr-Commit-Position: refs/heads/master@{#438196} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
