Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10829283:
Adjust menu item sizes for touch / non-touch according to spec. (Closed)
|
Created:
8 years, 4 months ago by flackr Modified:
8 years, 4 months ago CC:
chromium-reviews, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdjust menu item sizes for touch / non-touch according to spec.
BUG=139822
TEST=Menu item sizes are the same size and match the spec except for touch layout separators.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151479
Patch Set 1 #
Total comments: 2
Patch Set 2 : Only change for normal layout mode. #
Total comments: 2
Messages
Total messages: 12 (0 generated)
PTAL, see comment in bug / CL about separator size. These should have padding since this is used for context menus as well. Thanks!
https://chromiumcodereview.appspot.com/10829283/diff/1/ui/views/controls/menu... File ui/views/controls/menu/menu_config.cc (left): https://chromiumcodereview.appspot.com/10829283/diff/1/ui/views/controls/menu... ui/views/controls/menu/menu_config.cc:45: if (ui::GetDisplayLayout() == ui::LAYOUT_TOUCH) { This change effects windows too. Do we care?
https://chromiumcodereview.appspot.com/10829283/diff/1/ui/views/controls/menu... File ui/views/controls/menu/menu_config.cc (left): https://chromiumcodereview.appspot.com/10829283/diff/1/ui/views/controls/menu... ui/views/controls/menu/menu_config.cc:45: if (ui::GetDisplayLayout() == ui::LAYOUT_TOUCH) { For Windows: I am under the impression that we want to use the same layout now everywhere. The only difference for Windows is that we have "real buttons" in the wrench menu.
On 2012/08/10 22:57:48, Mr4D wrote: > https://chromiumcodereview.appspot.com/10829283/diff/1/ui/views/controls/menu... > File ui/views/controls/menu/menu_config.cc (left): > > https://chromiumcodereview.appspot.com/10829283/diff/1/ui/views/controls/menu... > ui/views/controls/menu/menu_config.cc:45: if (ui::GetDisplayLayout() == > ui::LAYOUT_TOUCH) { > For Windows: I am under the impression that we want to use the same layout now > everywhere. The only difference for Windows is that we have "real buttons" in > the wrench menu. Can someone verify that with Gideon (pm for win8).
On 2012/08/10 23:11:58, sky wrote: > On 2012/08/10 22:57:48, Mr4D wrote: > > > https://chromiumcodereview.appspot.com/10829283/diff/1/ui/views/controls/menu... > > File ui/views/controls/menu/menu_config.cc (left): > > > > > https://chromiumcodereview.appspot.com/10829283/diff/1/ui/views/controls/menu... > > ui/views/controls/menu/menu_config.cc:45: if (ui::GetDisplayLayout() == > > ui::LAYOUT_TOUCH) { > > For Windows: I am under the impression that we want to use the same layout now > > everywhere. The only difference for Windows is that we have "real buttons" in > > the wrench menu. > > Can someone verify that with Gideon (pm for win8). Verified with Gideon on bug tracker.
On 2012/08/13 16:03:28, flackr wrote: > On 2012/08/10 23:11:58, sky wrote: > > On 2012/08/10 22:57:48, Mr4D wrote: > > > > > > https://chromiumcodereview.appspot.com/10829283/diff/1/ui/views/controls/menu... > > > File ui/views/controls/menu/menu_config.cc (left): > > > > > > > > > https://chromiumcodereview.appspot.com/10829283/diff/1/ui/views/controls/menu... > > > ui/views/controls/menu/menu_config.cc:45: if (ui::GetDisplayLayout() == > > > ui::LAYOUT_TOUCH) { > > > For Windows: I am under the impression that we want to use the same layout > now > > > everywhere. The only difference for Windows is that we have "real buttons" > in > > > the wrench menu. > > > > Can someone verify that with Gideon (pm for win8). > > Verified with Gideon on bug tracker. I'm going to check how this looks on windows though since the touch optimized menu (at least on chromeos) does not match the spec due to the 1px tall separators and may look awkward.
On 2012/08/13 16:05:38, flackr wrote: > On 2012/08/13 16:03:28, flackr wrote: > > On 2012/08/10 23:11:58, sky wrote: > > > On 2012/08/10 22:57:48, Mr4D wrote: > > > > > > > > > > https://chromiumcodereview.appspot.com/10829283/diff/1/ui/views/controls/menu... > > > > File ui/views/controls/menu/menu_config.cc (left): > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/10829283/diff/1/ui/views/controls/menu... > > > > ui/views/controls/menu/menu_config.cc:45: if (ui::GetDisplayLayout() == > > > > ui::LAYOUT_TOUCH) { > > > > For Windows: I am under the impression that we want to use the same layout > > now > > > > everywhere. The only difference for Windows is that we have "real buttons" > > in > > > > the wrench menu. > > > > > > Can someone verify that with Gideon (pm for win8). > > > > Verified with Gideon on bug tracker. > > I'm going to check how this looks on windows though since the touch optimized > menu (at least on chromeos) does not match the spec due to the 1px tall > separators and may look awkward. Scott, I've posted screenshots in the bug I think we should leave the touch layout menu height alone until the menu separator issue is fixed. Thanks for pointing this out, I didn't think we were running LAYOUT_TOUCH on windows yet. Can you review / approve the menu_config_views changes? Thanks!
LGTM
See my comment. https://chromiumcodereview.appspot.com/10829283/diff/6002/ui/views/controls/m... File ui/views/controls/menu/menu_config_views.cc (right): https://chromiumcodereview.appspot.com/10829283/diff/6002/ui/views/controls/m... ui/views/controls/menu/menu_config_views.cc:36: // padding to implement full-height buttons. Since the upper / lower button line *is* the separator line there is no way to draw the button if the separator has a non zero padding.
https://chromiumcodereview.appspot.com/10829283/diff/6002/ui/views/controls/m... File ui/views/controls/menu/menu_config_views.cc (right): https://chromiumcodereview.appspot.com/10829283/diff/6002/ui/views/controls/m... ui/views/controls/menu/menu_config_views.cc:36: // padding to implement full-height buttons. On 2012/08/13 21:04:11, Mr4D wrote: > Since the upper / lower button line *is* the separator line there is no way to > draw the button if the separator has a non zero padding. I understand the difficulties involved but the metrics are wrong if we change the general separator height. We could change the separator class to allow removing the padding from the top / bottom / both for the special cases where this is needed in the wrench menu or change these menu items to have a special separator as part of the Edit / Zoom menu items.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/flackr@chromium.org/10829283/6002
Change committed as 151479 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
