Issue 12041085: Fix menu corners: the menu background was being painted as a rect when in fact,

Issue 12041085: Fix menu corners: the menu background was being painted as a rect when in fact, (Closed)

Created:
7 years, 11 months ago by varunjain

Modified:
7 years, 10 months ago

Reviewers:
sky

CC:
chromium-reviews, tfarina

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Fix menu corners: the menu background was being painted as a rect when in fact, the border is a rounded rect. In this CL, I change the menu background painting code to paint a rounded rect instead. I also had to make the hosting widget transparent to avoid showing black background around the rounded corners. One more fix was needed: a selected item in a menu is indicated by painting a highlight colored rectangle background for that item. Due to this, selecting the topmost or the bottom-most item messes up the roundedness of the menu background. I fix this by simply adding a padding inside the menu so that the highlighted background of the item does not overlap the rounded corners of the menu. BUG=171703 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179267

Patch Set 1 #

Patch Set 2 : patch #

Total comments: 2

Patch Set 3 : patch #

Total comments: 8

Patch Set 4 : patch #

Total comments: 14

Patch Set 5 : patch #

Patch Set 6 : patch #

Patch Set 7 : patch #

Patch Set 8 : patch #

Created: 7 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+85 lines, -49 lines)			Patch
M	ui/native_theme/native_theme.h	View	1 2 3	2 chunks	+5 lines, -0 lines	0 comments	Download
M	ui/native_theme/native_theme_aura.h	View	1 2 3	1 chunk	+4 lines, -2 lines	0 comments	Download
M	ui/native_theme/native_theme_aura.cc	View	1 2 3	2 chunks	+23 lines, -3 lines	0 comments	Download
M	ui/native_theme/native_theme_base.h	View	1 2 3	1 chunk	+4 lines, -2 lines	0 comments	Download
M	ui/native_theme/native_theme_base.cc	View	1 2 3	2 chunks	+5 lines, -3 lines	0 comments	Download
M	ui/views/controls/menu/menu_config.h	View	1 2 3 4	1 chunk	+3 lines, -0 lines	0 comments	Download
M	ui/views/controls/menu/menu_config.cc	View	1 2 3	1 chunk	+2 lines, -1 line	0 comments	Download
M	ui/views/controls/menu/menu_config_views.cc	View	1 2 3 4 5 6	2 chunks	+9 lines, -0 lines	0 comments	Download
M	ui/views/controls/menu/menu_host.cc	View	1 2 3 4 5 6	1 chunk	+8 lines, -7 lines	0 comments	Download
M	ui/views/controls/menu/menu_scroll_view_container.cc	View	1 2 3 4 5 6 7	3 chunks	+15 lines, -10 lines	0 comments	Download
M	ui/views/round_rect_painter.h	View	1 2 3 4 5	1 chunk	+2 lines, -4 lines	0 comments	Download
M	ui/views/round_rect_painter.cc	View	1 2 3 4 5	2 chunks	+5 lines, -17 lines	0 comments	Download

Messages

Total messages: 16 (0 generated)

Expand Messages | Collapse Messages

sky

Hold off on this until we hear back from Sabestian. Round rects are problematic on ...

7 years, 11 months ago (2013-01-24 22:49:44 UTC) #2

varunjain

Changed according to Sebastien's feedback. PTAL. https://codereview.chromium.org/12041085/diff/2001/ui/views/controls/menu/menu_scroll_view_container.cc File ui/views/controls/menu/menu_scroll_view_container.cc (right): https://codereview.chromium.org/12041085/diff/2001/ui/views/controls/menu/menu_scroll_view_container.cc#newcode263 ui/views/controls/menu/menu_scroll_view_container.cc:263: On 2013/01/24 22:49:44, ...

7 years, 11 months ago (2013-01-25 15:58:07 UTC) #4

sky

https://codereview.chromium.org/12041085/diff/8001/ui/native_theme/native_theme_aura.cc File ui/native_theme/native_theme_aura.cc (right): https://codereview.chromium.org/12041085/diff/8001/ui/native_theme/native_theme_aura.cc#newcode17 ui/native_theme/native_theme_aura.cc:17: #include "ui/views/controls/menu/menu_config.h" ui shouldn't depend upon views like this. ...

7 years, 11 months ago (2013-01-25 16:10:51 UTC) #5

varunjain

7 years, 11 months ago (2013-01-25 18:50:40 UTC) #6

sky

https://codereview.chromium.org/12041085/diff/10003/ui/views/controls/menu/menu_config.h File ui/views/controls/menu/menu_config.h (right): https://codereview.chromium.org/12041085/diff/10003/ui/views/controls/menu/menu_config.h#newcode131 ui/views/controls/menu/menu_config.h:131: int corner_radius; Add description. https://codereview.chromium.org/12041085/diff/10003/ui/views/controls/menu/menu_host.cc File ui/views/controls/menu/menu_host.cc (right): https://codereview.chromium.org/12041085/diff/10003/ui/views/controls/menu/menu_host.cc#newcode40 ...

7 years, 11 months ago (2013-01-25 20:30:24 UTC) #7

varunjain

7 years, 11 months ago (2013-01-25 22:57:29 UTC) #8

sky

https://codereview.chromium.org/12041085/diff/10003/ui/views/controls/menu/menu_host.cc File ui/views/controls/menu/menu_host.cc (right): https://codereview.chromium.org/12041085/diff/10003/ui/views/controls/menu/menu_host.cc#newcode40 ui/views/controls/menu/menu_host.cc:40: const MenuConfig& menu_config = submenu_->GetMenuItem()->GetMenuConfig(); On 2013/01/25 22:57:29, varunjain ...

7 years, 11 months ago (2013-01-25 23:43:18 UTC) #9

varunjain

7 years, 10 months ago (2013-01-28 20:58:01 UTC) #10

https://codereview.chromium.org/12041085/diff/10003/ui/views/controls/menu/me...
File ui/views/controls/menu/menu_host.cc (right):

https://codereview.chromium.org/12041085/diff/10003/ui/views/controls/menu/me...
ui/views/controls/menu/menu_host.cc:40: const MenuConfig& menu_config =
submenu_->GetMenuItem()->GetMenuConfig();
On 2013/01/25 23:43:18, sky wrote:
> On 2013/01/25 22:57:29, varunjain wrote:
> > On 2013/01/25 20:30:24, sky wrote:
> > > Can you make sure the MenuController is correctly set when you invoke
this?
> By
> > > that I mean make sure MenuItemView::GetMenuConfig() has a MenuController.
> > 
> > I think it does not matter if the controller exists or not. We are just
> > interested in the corner_radius here, so even if the controller does not
> exist,
> > we get the corner_radius from the default MenuConfig::instance().
> 
> If the code falls back to MenuConfig::instance() then it may not be using the
> right instance (because of the theme). That's why I mentioned earlier you made
> need to make this take a NativeTheme.

my point is that the default value of corner radius is independent of native
theme. if controller does not yet exist, we want to use the default value of
corner_radius, which is what we get from MenuConfig::instance(). I am not sure
what you mean by "it may not be using the right instance". For the case when
controller is not initialized, on WIN, MenuConfig::instance() will give the
instance() defined in menu_config_win and on CHROMEOS, the one defined in
menu_config_views. Isnt that what we want?

https://codereview.chromium.org/12041085/diff/10003/ui/views/controls/menu/me...
ui/views/controls/menu/menu_host.cc:45: if
(ui::NativeTheme::IsNewMenuStyleEnabled() &&
On 2013/01/25 23:43:18, sky wrote:
> On 2013/01/25 22:57:29, varunjain wrote:
> > On 2013/01/25 20:30:24, sky wrote:
> > > I tend to think this code isn't needed at all. SetShape in
NativeWidgetAura
> > does
> > > nothing.
> > 
> > I think so too. But I am not very familiar with the new menu style code. If
> you
> > are sure, I can remove this, otherwise I would defer to yefim.
> 
> Nuke it.

Done.

varunjain

Hi Scott, Done according to your comment. PTAL. On 2013/01/28 20:58:01, varunjain wrote: > https://codereview.chromium.org/12041085/diff/10003/ui/views/controls/menu/menu_host.cc ...

7 years, 10 months ago (2013-01-28 22:36:34 UTC) #11

Hi Scott, Done according to your comment. PTAL.

On 2013/01/28 20:58:01, varunjain wrote:
>
https://codereview.chromium.org/12041085/diff/10003/ui/views/controls/menu/me...
> File ui/views/controls/menu/menu_host.cc (right):
> 
>
https://codereview.chromium.org/12041085/diff/10003/ui/views/controls/menu/me...
> ui/views/controls/menu/menu_host.cc:40: const MenuConfig& menu_config =
> submenu_->GetMenuItem()->GetMenuConfig();
> On 2013/01/25 23:43:18, sky wrote:
> > On 2013/01/25 22:57:29, varunjain wrote:
> > > On 2013/01/25 20:30:24, sky wrote:
> > > > Can you make sure the MenuController is correctly set when you invoke
> this?
> > By
> > > > that I mean make sure MenuItemView::GetMenuConfig() has a
MenuController.
> > > 
> > > I think it does not matter if the controller exists or not. We are just
> > > interested in the corner_radius here, so even if the controller does not
> > exist,
> > > we get the corner_radius from the default MenuConfig::instance().
> > 
> > If the code falls back to MenuConfig::instance() then it may not be using
the
> > right instance (because of the theme). That's why I mentioned earlier you
made
> > need to make this take a NativeTheme.
> 
> my point is that the default value of corner radius is independent of native
> theme. if controller does not yet exist, we want to use the default value of
> corner_radius, which is what we get from MenuConfig::instance(). I am not sure
> what you mean by "it may not be using the right instance". For the case when
> controller is not initialized, on WIN, MenuConfig::instance() will give the
> instance() defined in menu_config_win and on CHROMEOS, the one defined in
> menu_config_views. Isnt that what we want?
> 
>
https://codereview.chromium.org/12041085/diff/10003/ui/views/controls/menu/me...
> ui/views/controls/menu/menu_host.cc:45: if
> (ui::NativeTheme::IsNewMenuStyleEnabled() &&
> On 2013/01/25 23:43:18, sky wrote:
> > On 2013/01/25 22:57:29, varunjain wrote:
> > > On 2013/01/25 20:30:24, sky wrote:
> > > > I tend to think this code isn't needed at all. SetShape in
> NativeWidgetAura
> > > does
> > > > nothing.
> > > 
> > > I think so too. But I am not very familiar with the new menu style code.
If
> > you
> > > are sure, I can remove this, otherwise I would defer to yefim.
> > 
> > Nuke it.
> 
> Done.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varunjain@chromium.org/12041085/23015

7 years, 10 months ago (2013-01-28 23:00:59 UTC) #13

commit-bot: I haz the power

Failed to apply patch for ui/views/controls/menu/menu_scroll_view_container.cc: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...

7 years, 10 months ago (2013-01-28 23:01:01 UTC) #14

Failed to apply patch for ui/views/controls/menu/menu_scroll_view_container.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ui/views/controls/menu/menu_scroll_view_container.cc
  Hunk #2 FAILED at 177.
  Hunk #3 succeeded at 202 (offset -1 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
ui/views/controls/menu/menu_scroll_view_container.cc.rej

Patch:       ui/views/controls/menu/menu_scroll_view_container.cc
Index: ui/views/controls/menu/menu_scroll_view_container.cc
diff --git a/ui/views/controls/menu/menu_scroll_view_container.cc
b/ui/views/controls/menu/menu_scroll_view_container.cc
index
1dedb82790be9e412c1e2b30ac2a4768b0236a9d..887403fea87dfa3002840912ef3c23dcc0929f7e
100644
--- a/ui/views/controls/menu/menu_scroll_view_container.cc
+++ b/ui/views/controls/menu/menu_scroll_view_container.cc
@@ -26,6 +26,8 @@ namespace views {
 
 namespace {
 
+static const int kBorderPaddingDueToRoundedCorners = 1;
+
 // MenuScrollButton
------------------------------------------------------------
 
 // MenuScrollButton is used for the scroll buttons when not all menu items fit
@@ -175,21 +177,22 @@
MenuScrollViewContainer::MenuScrollViewContainer(SubmenuView* content_view)
   const MenuConfig& menu_config =
       content_view_->GetMenuItem()->GetMenuConfig();
 
+  int padding = menu_config.corner_radius > 0 ?
+        kBorderPaddingDueToRoundedCorners : 0;
+  int top = menu_config.menu_vertical_border_size + padding;
+  int left = menu_config.menu_horizontal_border_size + padding;
+  int bottom = menu_config.menu_vertical_border_size + padding;
+  int right = menu_config.menu_horizontal_border_size + padding;
+
   if (NativeTheme::IsNewMenuStyleEnabled()) {
     set_border(views::Border::CreateBorderPainter(
         new views::RoundRectPainter(
             ui::NativeTheme::instance()->GetSystemColor(
-                ui::NativeTheme::kColorId_MenuBorderColor)),
-        gfx::Insets(menu_config.menu_vertical_border_size,
-                    menu_config.menu_horizontal_border_size,
-                    menu_config.menu_vertical_border_size,
-                    menu_config.menu_horizontal_border_size)));
+                ui::NativeTheme::kColorId_MenuBorderColor),
+            menu_config.corner_radius),
+            gfx::Insets(top, left, bottom, right)));
   } else {
-    set_border(
-        Border::CreateEmptyBorder(menu_config.menu_vertical_border_size,
-                                  menu_config.menu_horizontal_border_size,
-                                  menu_config.menu_vertical_border_size,
-                                  menu_config.menu_horizontal_border_size));
+    set_border(Border::CreateEmptyBorder(top, left, bottom, right));
   }
 }
 
@@ -201,6 +204,8 @@ void MenuScrollViewContainer::OnPaintBackground(gfx::Canvas*
canvas) {
 
   gfx::Rect bounds(0, 0, width(), height());
   NativeTheme::ExtraParams extra;
+  const MenuConfig& menu_config =
content_view_->GetMenuItem()->GetMenuConfig();
+  extra.menu_background.corner_radius = menu_config.corner_radius;
   GetNativeTheme()->Paint(canvas->sk_canvas(),
       NativeTheme::kMenuPopupBackground, NativeTheme::kNormal, bounds, extra);
 }

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varunjain@chromium.org/12041085/26002

7 years, 10 months ago (2013-01-28 23:24:35 UTC) #15

Message was sent while issue was closed.

Change committed as 179267

Expand Messages | Collapse Messages