New issue
Advanced search Search tips

Issue 860673 link

Starred by 8 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug



Sign in to add a comment

MacViews: sort out context menus and menu whitespace

Project Member Reported by ellyjo...@chromium.org, Jul 6

Issue description

The current menu styling is:

* Items are 32pt high, except containers, which are 40pt high
* Menus have 8pt of padding at the edges and around separators

The larger menus make it harder to mouse between items and the new top padding means the top item is no longer under the cursor (or nearly so) on context menus.

From discussion with pkasting@ (CCed), we tried this type of menu styling before and saw regressions in some key metrics - perhaps pkasting@ can add some of that detail here.
 
Issue 860350 has been merged into this issue.
A related issue: the spacing between the check and the menu item is very small ( issue 832257 ).
Cc: bettes@chromium.org ellyjo...@chromium.org
 Issue 832257  has been merged into this issue.
Labels: -M-69 Group-Menus
Labels: M-69
Cc: ftirelo@chromium.org
Fabio is moving our menu paddings to 4dp across all platforms. 

Current Windows: 3dp
Current MacViews: 8dp
Current Cocoa: 4dp

4dp should be a suitable cross-platform solution, regardless of the outcome of this bug.
Project Member

Comment 7 by bugdroid1@chromium.org, Jul 12

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b56c4e40b65ee14134dd2e61834f260ca9c1715e

commit b56c4e40b65ee14134dd2e61834f260ca9c1715e
Author: Elly Fong-Jones <ellyjones@chromium.org>
Date: Thu Jul 12 12:36:53 2018

macviews: switch back to Cocoa menus for Views context menus

This change:
1) Reverts erikchen@'s 79c8511fb8801f7fc23c4cbe81311b6065daef15, which removed
   the Cocoa menu runner code;
2) Wires the Cocoa menu runner code back up in MenuRunnerImplInterface::Create;
3) Fixes a latent bug in Textfield that could cause it to delete its context
   menu while handling a context menu activation - this is fine with Views menus
   but forbidden with Cocoa menus.

Bug: 860673
Change-Id: If110f09379c4661a4f61a8b0257cf5214503e6a9
Reviewed-on: https://chromium-review.googlesource.com/1131564
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574538}
[modify] https://crrev.com/b56c4e40b65ee14134dd2e61834f260ca9c1715e/ui/views/BUILD.gn
[modify] https://crrev.com/b56c4e40b65ee14134dd2e61834f260ca9c1715e/ui/views/controls/menu/menu_runner_cocoa_unittest.mm
[modify] https://crrev.com/b56c4e40b65ee14134dd2e61834f260ca9c1715e/ui/views/controls/menu/menu_runner_impl.cc
[add] https://crrev.com/b56c4e40b65ee14134dd2e61834f260ca9c1715e/ui/views/controls/menu/menu_runner_impl_cocoa.h
[add] https://crrev.com/b56c4e40b65ee14134dd2e61834f260ca9c1715e/ui/views/controls/menu/menu_runner_impl_cocoa.mm
[modify] https://crrev.com/b56c4e40b65ee14134dd2e61834f260ca9c1715e/ui/views/controls/textfield/textfield.cc
[modify] https://crrev.com/b56c4e40b65ee14134dd2e61834f260ca9c1715e/ui/views/controls/textfield/textfield_unittest.cc

On #6: I wonder if there are other padding sizes that should be changed along with top/bottom padding, to make the experience consistent.

Some examples:
 - submenu_horizontal_inset, defined as 0 on Mac and 3 on other platforms;
 - item_bottom_margin, defined as 3 on all platforms;
 - Several separator heights (I don't fully understand all of them, but they have very distinct values on each platform).

I have a draft CL (crrev.com/c/1135186), but I'd like to sort this out first. In addition, this seems risky to land so close to M69 branch cut. WDYT?
Project Member

Comment 9 by bugdroid1@chromium.org, Jul 12

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c7a09eb8d51c7e5d4d6ae68b4de0c8d3d9fcbabb

commit c7a09eb8d51c7e5d4d6ae68b4de0c8d3d9fcbabb
Author: Elly Fong-Jones <ellyjones@chromium.org>
Date: Thu Jul 12 15:10:07 2018

macviews: use cocoa context menus for web content

Bug: 860673
Change-Id: I459eaf26c3fffb6c87ddbe2b8cd28db15d7cc90f
Reviewed-on: https://chromium-review.googlesource.com/1134539
Reviewed-by: Leonard Grey <lgrey@chromium.org>
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574562}
[modify] https://crrev.com/c7a09eb8d51c7e5d4d6ae68b4de0c8d3d9fcbabb/chrome/browser/ui/cocoa/tab_contents/chrome_web_contents_view_delegate_mac.mm

ftirelo@: I think it's fine to land such changes close to the cut - MenuConfig is a pretty "safe" class to change. Do you want to own this bug now that my changes have landed?
Labels: ReleaseBlock-Stable
ellyjones@: my concern is more about unexpected side effects we may have. I recently needed to revert a pretty safe CL that changed link colors to GB600, because it broke contrast ratio on the bookmarks bar (https://chromium-review.googlesource.com/c/chromium/src/+/1106457)

I will send the CL for review, but I will not be able to own this bug, since I don't have much context of the main issue here (specially that it's RBS now).
Project Member

Comment 13 by bugdroid1@chromium.org, Jul 12

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/29b678961513d03f4065f12061cabac15698b381

commit 29b678961513d03f4065f12061cabac15698b381
Author: Fabio Tirelo <ftirelo@chromium.org>
Date: Thu Jul 12 22:19:09 2018

[Menu config] Change vertical border padding to 4 on all platforms

Bug: 860673
Change-Id: I64299a1c24247b00ee389c29751da14ef95e1230
Reviewed-on: https://chromium-review.googlesource.com/1135186
Reviewed-by: Michael Wasserman <msw@chromium.org>
Commit-Queue: Fabio Tirelo <ftirelo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574750}
[modify] https://crrev.com/29b678961513d03f4065f12061cabac15698b381/ui/views/controls/menu/menu_config.cc
[modify] https://crrev.com/29b678961513d03f4065f12061cabac15698b381/ui/views/controls/menu/menu_config_mac.mm

The bookmarks bar context menu is using the MacViews menu and the spacing issue between the checkmark and text has not been addressed.
Status: Started (was: Assigned)
#14: Indeed. I didn't know about the bookmarks bar context menu - that's pretty puzzling, and I suspect it's because the bookmarks bar menus are "nested". The checkmark spacing is still to be looked at.
Labels: -Pri-1 -ReleaseBlock-Stable Pri-2
Remaining work is not release blocking.
New design spec: 

* change items from 32pt --> 28pt
* change padding at the edges/around separators from 8pt --> 4pt (might already be true via Fabio)
Screen Shot 2018-07-18 at 10.35.24 PM.png
155 KB View Download
Mac look with the new spacings - I think it's really nice :)

<https://chromium-review.googlesource.com/c/chromium/src/+/1148467>
Screen Shot 2018-07-24 at 11.16.43 AM.png
38.8 KB View Download
Project Member

Comment 20 by bugdroid1@chromium.org, Jul 25

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d3cf1351bd82d7d3fdd9745592ad8c127275bef8

commit d3cf1351bd82d7d3fdd9745592ad8c127275bef8
Author: Elly Fong-Jones <ellyjones@chromium.org>
Date: Wed Jul 25 15:52:30 2018

macviews: use new menu spacings

Latest revisions from UX in comment 17 on the linked bug:

Text items reduced from 32 to 28
Container items reduced from 48 to 40 (not specced, but not doing this made
them look very weird)
Separator spacing reduced from 8pt to 4pt

Bug: 860673
Change-Id: Iad700a22eb9aad43f2c328314a6bffa885843194
Reviewed-on: https://chromium-review.googlesource.com/1148467
Commit-Queue: Sidney San Martín <sdy@chromium.org>
Reviewed-by: Sidney San Martín <sdy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577908}
[modify] https://crrev.com/d3cf1351bd82d7d3fdd9745592ad8c127275bef8/ui/views/controls/menu/menu_config_mac.mm

Labels: Merge-Request-69
This one's fixed in trunk now, but requesting M69 merge to avoid extra user-visible UI changes :)
Labels: TE-Verified-M70 TE-Verified-70.0.3503.0
Verified the fix on Mac 10.13.1 using Chrome version #70.0.3503.0 as per comment#0.
Attaching screenshot for reference.
Observed that context menu white spaces were sorted.
Hence, the fix is working as expected. 
Adding the verified labels.
Note: Able to reproduce the issue on chrome version with out fix.

Thanks...!!



860673 CL Verification.png
66.5 KB View Download
Project Member

Comment 23 by sheriffbot@chromium.org, Jul 26

Labels: -Merge-Request-69 Merge-Review-69 Hotlist-Merge-Review
This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Hey Elly,

I found a bug with the new spacing. The hover is overlapping the rounded corners.
Please see the attached screencasts for reference.

Hope it is clear, what I mean.

Please let me know, if I should open a new report.

Thanks.
overlapping.mov
254 KB View Download
overlapping_2.mov
749 KB View Download
#24: Hm, yeah. That's probably because we needed the vertical margin to avoid the corner radius.
ellyjones@, Is cl listed at #20 good to merge to M69? Just checking back based on comments #24 and #25. 
#26: Yes, #20 is fine to merge - #24 is an extremely minor visual glitch.
Labels: -Merge-Review-69 Merge-Approved-69
Approving merge to M69 branch 3497 based on comment #27. Please merge. Thank you.
Status: Fixed (was: Started)
Merge complete: <https://chromium-review.googlesource.com/c/chromium/src/+/1151687>
Project Member

Comment 30 by bugdroid1@chromium.org, Jul 26

Labels: -merge-approved-69 merge-merged-3497
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d2d1726f6537fd557431f50130e96baa5201dac7

commit d2d1726f6537fd557431f50130e96baa5201dac7
Author: Elly Fong-Jones <ellyjones@chromium.org>
Date: Thu Jul 26 16:53:04 2018

macviews: use new menu spacings

Latest revisions from UX in comment 17 on the linked bug:

Text items reduced from 32 to 28
Container items reduced from 48 to 40 (not specced, but not doing this made
them look very weird)
Separator spacing reduced from 8pt to 4pt

Bug: 860673
Change-Id: Iad700a22eb9aad43f2c328314a6bffa885843194
Reviewed-on: https://chromium-review.googlesource.com/1148467
Commit-Queue: Sidney San Martín <sdy@chromium.org>
Reviewed-by: Sidney San Martín <sdy@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#577908}(cherry picked from commit d3cf1351bd82d7d3fdd9745592ad8c127275bef8)
Reviewed-on: https://chromium-review.googlesource.com/1151687
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#112}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/d2d1726f6537fd557431f50130e96baa5201dac7/ui/views/controls/menu/menu_config_mac.mm

I think the checkmark issue still needs to be addressed?
#31: The current look of checkmarks is WAI, but I'll follow up with bettes@ to see how we feel about it in use.
Okay, I just feel that since we are making everything more consistent with all platforms, the same should go for the checkmark spacing. The way it is on Windows looks much nicer IMO.
Labels: TE-Verified-69.0.3497.23 TE-Verified-M69
Verified the fix on Mac 10.13.1 using Chrome version #69.0.3497.23 as per comment#0.
Attaching screenshot for reference.
Observed that context menu white spaces were sorted.
Hence, the fix is working as expected. 
Adding the verified labels.
Note: Able to reproduce the issue on chrome version with out fix.

Thanks...!!
860673-3497 CL.png
64.7 KB View Download
bettes@chromium.org

I don't understand why the spacing between the text and checkmark is crowded only on Mac and not on any of the other platforms.

It looks much more balanced when there's more padding on the right. This has been bugging me for several months now...
Crowding.png
64.7 KB View Download
Labels: -M-69 -Target-69 M-73 Target-73
Status: Assigned (was: Fixed)
Yeah - I will experiment with making them vertically centered. I got a signoff from bettes@ :)

Comment 37 by jeffreyc...@gmail.com, Jan 15 (4 days ago)

Any update?

Comment 38 by ellyjo...@chromium.org, Jan 15 (4 days ago)

Labels: -M-73 -Target-73 M-74 Target-74
Unfortunately not - I missed M73 with this probably :(

Sign in to add a comment