MacViews: sort out context menus and menu whitespace |
||||||||||||||||
Issue descriptionThe 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.
,
Jul 6
A related issue: the spacing between the check and the menu item is very small ( issue 832257 ).
,
Jul 6
,
Jul 12
,
Jul 12
,
Jul 12
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.
,
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
,
Jul 12
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?
,
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
,
Jul 12
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?
,
Jul 12
,
Jul 12
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).
,
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
,
Jul 16
The bookmarks bar context menu is using the MacViews menu and the spacing issue between the checkmark and text has not been addressed.
,
Jul 17
#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.
,
Jul 17
Remaining work is not release blocking.
,
Jul 19
New design spec: * change items from 32pt --> 28pt * change padding at the edges/around separators from 8pt --> 4pt (might already be true via Fabio)
,
Jul 24
Mac look with the new spacings - I think it's really nice :) <https://chromium-review.googlesource.com/c/chromium/src/+/1148467>
,
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
,
Jul 25
This one's fixed in trunk now, but requesting M69 merge to avoid extra user-visible UI changes :)
,
Jul 26
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...!!
,
Jul 26
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
,
Jul 26
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.
,
Jul 26
#24: Hm, yeah. That's probably because we needed the vertical margin to avoid the corner radius.
,
Jul 26
ellyjones@, Is cl listed at #20 good to merge to M69? Just checking back based on comments #24 and #25.
,
Jul 26
#26: Yes, #20 is fine to merge - #24 is an extremely minor visual glitch.
,
Jul 26
Approving merge to M69 branch 3497 based on comment #27. Please merge. Thank you.
,
Jul 26
Merge complete: <https://chromium-review.googlesource.com/c/chromium/src/+/1151687>
,
Jul 26
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
,
Jul 26
I think the checkmark issue still needs to be addressed?
,
Jul 26
#31: The current look of checkmarks is WAI, but I'll follow up with bettes@ to see how we feel about it in use.
,
Jul 26
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.
,
Aug 1
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...!!
,
Dec 2
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...
,
Dec 3
Yeah - I will experiment with making them vertically centered. I got a signoff from bettes@ :)
,
Jan 15
Any update?
,
Jan 15
Unfortunately not - I missed M73 with this probably :( |
||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||
Comment 1 by ellyjo...@chromium.org
, Jul 6