Allow the bottom section of the App Menu to be a different color |
|||||
Issue descriptionThe Chrome Enterprise team intends to implement a UX proposal [1] to better show users that Chrome is managed by their administrator. For the desktop implementation, one thing that stands out is the change to the app menu. In the mocks, the last menu-item has a different background color (grey) than the others. It doesn't look like there exists a way to do this right now. There are two parts to implementing this: 1. Let AppMenu set a background for MenuItemView and separators However, our menu-item is the last one in the app menu. When menus are using BubbleBorder, this leaves white padding for the rounded corner. This leads us to: 2. Let MenuModel set a background for the bottom padding space in a BubbleBorder I made a quick-and-dirty patch [2][3] to show (2) what it could look like. Does this approach (in 2 parts) make sense? Or is there a cleaner way to have an element use a different-colored background (and matching the padding in BubbleBorder)? [1] https://docs.google.com/presentation/d/1Q6Pn0YB1UgH1QKiSMQadTjStMMXKVoH60nb9QN3ee1o/edit#slide=id.g3d9e99adf5_1_0 [2] https://pastebin.com/7X645sCN [3] https://screenshot.googleplex.com/h5yds59rsod
,
Sep 28
Thanks for the tip, msw@, I hadn't realized we had similar UI implemented already. It looks like the code for BubbleFrameView::SetFootnoteView() doesn't handle corner radius super well, though. It looks OK right now, because most(all?) BubbleFrameViews have a small corner radius. But it becomes obvious when corner radius is upped, especially on darker backgrounds. See [1]. The footnote just draws a rectangle as its background, and the rectangle can spill outside of the rounded corners. Because app menu has a bigger corner radius, it looks more obvious [2]. So anyways, I think I can implement a new HalfRoundedRectBackground class instead of the current SolidBackground class it's using. This would avoid spilling outside of the rounded corners, and should solve my problem. If that's the path I end up going, I could also update BubbleFrameView to use that new class to draw its footer's background. Two birds with one stone, and all. [1] https://screenshot.googleplex.com/u0YphPBhykR [2] https://screenshot.googleplex.com/jAn85pv4FKT
,
Sep 28
I'm not sure exactly how, but I thought bubbles/menus could clip their content to the rounded rect border. It's probably worth looking into that option first (via canvas clipping, window frame skpaths, ???) Otherwise, something like HalfRoundedRectBackground might be reasonable.
,
Sep 28
Check out BubbleFrameView::GetClientMask and GetWindowMask. I wonder why those aren't working as-is for the bubble you tested.
,
Oct 1
GetClientMask() applies to the Client View of the frame, not the whole Frame View. At least, that's what it looks like from playing around with it a bit [1]. So, it's not what we need here. GetWindowMask() should apply to the whole frame, but doesn't clip when there's a shadow, to leave space for it [1]. This makes it unreliable. I tried forcing the clipping in GetWindowMask() to see what would happen, and the result isn't super pretty [3]... The shadow disappears because the mask applies to _everything_. Also, GetWindowMask() doesn't seem to get called on Linux for some reason. Not sure what's up with that one. The HalfRoundedRectBackground solution doesn't seem to need that much code, and looks fine [4]. I've been playing around a lot with the code, so I have a lot of code cleanup to do. Anyways, I should be able to make a CL for this soon. [1] https://screenshot.googleplex.com/0RPiyibOMTc [2] https://cs.chromium.org/chromium/src/ui/views/bubble/bubble_frame_view.cc?l=212&rcl=49993008676b4624600f439cbb91eb4fbc8b15b3 [3] https://screenshot.googleplex.com/pTbQXUdhJS3 [4] https://screenshot.googleplex.com/Waai7M4Wpoj
,
Oct 2
Sounds reasonable, thanks for taking a closer look.
,
Oct 9
,
Oct 9
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/db163611c6f71edee205d1a210464949f6f53f42 commit db163611c6f71edee205d1a210464949f6f53f42 Author: Nicolas Ouellet-payeur <nicolaso@chromium.org> Date: Tue Oct 09 18:23:10 2018 Add footnote support to top-level menus Adds a function to |MenuDelegate| that can be overloaded to create a footnote View for a top-level menu. The footnote view is rendered at the bottom of the menu, with a different background color, similar to |BubbleFrameView::SetFootnoteView()|. Bug: 889229 Change-Id: I9afc1b3c65e9c48f670102f61b888c736ccaa45c Reviewed-on: https://chromium-review.googlesource.com/c/1258045 Reviewed-by: Michael Wasserman <msw@chromium.org> Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org> Cr-Commit-Position: refs/heads/master@{#597999} [modify] https://crrev.com/db163611c6f71edee205d1a210464949f6f53f42/ui/views/BUILD.gn [modify] https://crrev.com/db163611c6f71edee205d1a210464949f6f53f42/ui/views/bubble/bubble_frame_view.cc [modify] https://crrev.com/db163611c6f71edee205d1a210464949f6f53f42/ui/views/bubble/bubble_frame_view.h [modify] https://crrev.com/db163611c6f71edee205d1a210464949f6f53f42/ui/views/bubble/bubble_frame_view_unittest.cc [add] https://crrev.com/db163611c6f71edee205d1a210464949f6f53f42/ui/views/bubble/footnote_container_view.cc [add] https://crrev.com/db163611c6f71edee205d1a210464949f6f53f42/ui/views/bubble/footnote_container_view.h [modify] https://crrev.com/db163611c6f71edee205d1a210464949f6f53f42/ui/views/controls/menu/menu_delegate.cc [modify] https://crrev.com/db163611c6f71edee205d1a210464949f6f53f42/ui/views/controls/menu/menu_delegate.h [add] https://crrev.com/db163611c6f71edee205d1a210464949f6f53f42/ui/views/controls/menu/menu_footnote_unittest.cc [modify] https://crrev.com/db163611c6f71edee205d1a210464949f6f53f42/ui/views/controls/menu/menu_scroll_view_container.cc [modify] https://crrev.com/db163611c6f71edee205d1a210464949f6f53f42/ui/views/controls/menu/menu_scroll_view_container.h [modify] https://crrev.com/db163611c6f71edee205d1a210464949f6f53f42/ui/views/controls/menu/submenu_view.cc
,
Oct 9
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8b3faf7c7be02e55d5ab94e8e9454859601fbd4f commit 8b3faf7c7be02e55d5ab94e8e9454859601fbd4f Author: Nicolas Ouellet-payeur <nicolaso@chromium.org> Date: Tue Oct 09 20:57:10 2018 Refactor menu footnote unittests Bug: 889229 Change-Id: I7b2a0f0c64b136f7210fda4c6ead654558de1e56 Reviewed-on: https://chromium-review.googlesource.com/c/1271780 Reviewed-by: Michael Wasserman <msw@chromium.org> Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org> Cr-Commit-Position: refs/heads/master@{#598076} [modify] https://crrev.com/8b3faf7c7be02e55d5ab94e8e9454859601fbd4f/ui/views/controls/menu/menu_footnote_unittest.cc
,
Oct 9
,
Oct 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d615e4c1f1fec57bb2e1c58e519f6382bbc552fe commit d615e4c1f1fec57bb2e1c58e519f6382bbc552fe Author: Nicolas Ouellet-Payeur <nicolaso@chromium.org> Date: Thu Oct 11 19:13:55 2018 Revert "Refactor menu footnote unittests" This reverts commit 8b3faf7c7be02e55d5ab94e8e9454859601fbd4f. Reason for revert: crbug.com/894193 Original change's description: > Refactor menu footnote unittests > > Bug: 889229 > Change-Id: I7b2a0f0c64b136f7210fda4c6ead654558de1e56 > Reviewed-on: https://chromium-review.googlesource.com/c/1271780 > Reviewed-by: Michael Wasserman <msw@chromium.org> > Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org> > Cr-Commit-Position: refs/heads/master@{#598076} TBR=msw@chromium.org,nicolaso@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 889229 Change-Id: Idc3a12270c7f11b84cfc3f6437a85f3472d0f405 Reviewed-on: https://chromium-review.googlesource.com/c/1276545 Reviewed-by: Avi Drissman <avi@chromium.org> Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org> Cr-Commit-Position: refs/heads/master@{#598880} [modify] https://crrev.com/d615e4c1f1fec57bb2e1c58e519f6382bbc552fe/ui/views/controls/menu/menu_footnote_unittest.cc
,
Oct 11
Looks like this needs a little more work on Mac because of bug 894193 . Looking into it.
,
Oct 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1ac58b88f929a017ef1886ff030e84ddfe7cdcac commit 1ac58b88f929a017ef1886ff030e84ddfe7cdcac Author: Nicolas Ouellet-Payeur <nicolaso@chromium.org> Date: Thu Oct 11 22:27:31 2018 Revert "Add footnote support to top-level menus" This reverts commit db163611c6f71edee205d1a210464949f6f53f42. Reason for revert: crbug.com/894193 Original change's description: > Add footnote support to top-level menus > > Adds a function to |MenuDelegate| that can be overloaded to create a > footnote View for a top-level menu. > > The footnote view is rendered at the bottom of the menu, with a > different background color, similar to > |BubbleFrameView::SetFootnoteView()|. > > Bug: 889229 > Change-Id: I9afc1b3c65e9c48f670102f61b888c736ccaa45c > Reviewed-on: https://chromium-review.googlesource.com/c/1258045 > Reviewed-by: Michael Wasserman <msw@chromium.org> > Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org> > Cr-Commit-Position: refs/heads/master@{#597999} TBR=msw@chromium.org,nicolaso@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 889229 Change-Id: I84c7cc30b80bef800accbe7c1c5995105f545009 Reviewed-on: https://chromium-review.googlesource.com/c/1277951 Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org> Commit-Queue: Michael Wasserman <msw@chromium.org> Reviewed-by: Michael Wasserman <msw@chromium.org> Cr-Commit-Position: refs/heads/master@{#598982} [modify] https://crrev.com/1ac58b88f929a017ef1886ff030e84ddfe7cdcac/ui/views/BUILD.gn [modify] https://crrev.com/1ac58b88f929a017ef1886ff030e84ddfe7cdcac/ui/views/bubble/bubble_frame_view.cc [modify] https://crrev.com/1ac58b88f929a017ef1886ff030e84ddfe7cdcac/ui/views/bubble/bubble_frame_view.h [modify] https://crrev.com/1ac58b88f929a017ef1886ff030e84ddfe7cdcac/ui/views/bubble/bubble_frame_view_unittest.cc [delete] https://crrev.com/5b2786cb7ab7b3956f48876c1d4b34cc4e971dfb/ui/views/bubble/footnote_container_view.cc [delete] https://crrev.com/5b2786cb7ab7b3956f48876c1d4b34cc4e971dfb/ui/views/bubble/footnote_container_view.h [modify] https://crrev.com/1ac58b88f929a017ef1886ff030e84ddfe7cdcac/ui/views/controls/menu/menu_delegate.cc [modify] https://crrev.com/1ac58b88f929a017ef1886ff030e84ddfe7cdcac/ui/views/controls/menu/menu_delegate.h [delete] https://crrev.com/5b2786cb7ab7b3956f48876c1d4b34cc4e971dfb/ui/views/controls/menu/menu_footnote_unittest.cc [modify] https://crrev.com/1ac58b88f929a017ef1886ff030e84ddfe7cdcac/ui/views/controls/menu/menu_scroll_view_container.cc [modify] https://crrev.com/1ac58b88f929a017ef1886ff030e84ddfe7cdcac/ui/views/controls/menu/menu_scroll_view_container.h [modify] https://crrev.com/1ac58b88f929a017ef1886ff030e84ddfe7cdcac/ui/views/controls/menu/submenu_view.cc
,
Oct 16
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1e0704e6fc64ffadb72ba89035447ba1fc690426 commit 1e0704e6fc64ffadb72ba89035447ba1fc690426 Author: Nicolas Ouellet-Payeur <nicolaso@chromium.org> Date: Tue Oct 16 06:02:50 2018 Reland: Add footnote support to top-level menus Adds a function to |MenuDelegate| that can be overloaded to create a footnote View for a top-level menu. The footnote view is rendered at the bottom of the menu, with a different background color, similar to |BubbleFrameView::SetFootnoteView()|. Bug: 889229 , 894193 Change-Id: Icb67740fddfe03a7a2d9bae7c70b383cfe6a9845 Reviewed-on: https://chromium-review.googlesource.com/c/1277573 Reviewed-by: Michael Wasserman <msw@chromium.org> Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org> Cr-Commit-Position: refs/heads/master@{#599881} [modify] https://crrev.com/1e0704e6fc64ffadb72ba89035447ba1fc690426/ui/views/BUILD.gn [modify] https://crrev.com/1e0704e6fc64ffadb72ba89035447ba1fc690426/ui/views/bubble/bubble_frame_view.cc [modify] https://crrev.com/1e0704e6fc64ffadb72ba89035447ba1fc690426/ui/views/bubble/bubble_frame_view.h [modify] https://crrev.com/1e0704e6fc64ffadb72ba89035447ba1fc690426/ui/views/bubble/bubble_frame_view_unittest.cc [add] https://crrev.com/1e0704e6fc64ffadb72ba89035447ba1fc690426/ui/views/bubble/footnote_container_view.cc [add] https://crrev.com/1e0704e6fc64ffadb72ba89035447ba1fc690426/ui/views/bubble/footnote_container_view.h [modify] https://crrev.com/1e0704e6fc64ffadb72ba89035447ba1fc690426/ui/views/controls/menu/menu_delegate.cc [modify] https://crrev.com/1e0704e6fc64ffadb72ba89035447ba1fc690426/ui/views/controls/menu/menu_delegate.h [add] https://crrev.com/1e0704e6fc64ffadb72ba89035447ba1fc690426/ui/views/controls/menu/menu_footnote_unittest.cc [modify] https://crrev.com/1e0704e6fc64ffadb72ba89035447ba1fc690426/ui/views/controls/menu/menu_scroll_view_container.cc [modify] https://crrev.com/1e0704e6fc64ffadb72ba89035447ba1fc690426/ui/views/controls/menu/menu_scroll_view_container.h [modify] https://crrev.com/1e0704e6fc64ffadb72ba89035447ba1fc690426/ui/views/controls/menu/submenu_view.cc
,
Oct 26
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by msw@chromium.org
, Sep 25