New issue
Advanced search Search tips

Issue 868037 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 8
Cc:
Components:
EstimatedDays: ----
NextAction: 2018-08-07
OS: Mac
Pri: 2
Type: Bug-Regression



Sign in to add a comment

[MacViews] White space in menus could be (2-3px?) vertically larger, so that the corner are not overlapped by the hover

Project Member Reported by meh...@chromium.org, Jul 26

Issue description

Chrome Version: Canary 70.0.3503.0
OS: macOS 10.13.6

What steps will reproduce the problem?
(1) Click on the Hamburger Menu, so that the Menu appears
(2) Hover over History, so the the sub menu appears
(3) Hover over History in the sub menu
(4) Take a look a the rounded corner

What is the expected result?
The white spaces could be (2-3px?) vertically larger, or the corners could be less round, so that the hover doesn't overlap the rounded corner.


What happens instead?
An overlapping is to see.

This is a regression from issue 860673. Marking as M-69, since the fix is also merged to it.

Thanks :)
Mehmet

 
actual_zoomed.png
17.9 KB View Download
expected_zoomed.png
15.4 KB View Download
Owner: ellyjo...@chromium.org
Status: Assigned (was: Untriaged)
bettes okay'ed adding the minimum height needed at the top and bottom to avoid the highlight overlap
Cc: nyerramilli@chromium.org rbasuvula@chromium.org ftirelo@chromium.org
 Issue 867874  has been merged into this issue.
Labels: RegressedIn-69 FoundIn-70 Target-69 FoundIn-69
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 6

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

commit 41f1bd9fcb37c0eec31bcf7c31d565615e0f408f
Author: Elly Fong-Jones <ellyjones@chromium.org>
Date: Mon Aug 06 15:01:09 2018

views: use corner radius for vertical margins on menus if set

If a menu has rounded corners, the vertical margin should always be exactly the
radius of that corner, to avoid visual glitches when the top or bottom items
are highlighted.

Bug:  868037 
Change-Id: Iab35de0b5be4152da1f24927c4a7f6bdef92a0d9
Reviewed-on: https://chromium-review.googlesource.com/1161101
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580870}
[modify] https://crrev.com/41f1bd9fcb37c0eec31bcf7c31d565615e0f408f/ui/views/controls/menu/menu_config.h
[modify] https://crrev.com/41f1bd9fcb37c0eec31bcf7c31d565615e0f408f/ui/views/controls/menu/menu_controller.cc
[modify] https://crrev.com/41f1bd9fcb37c0eec31bcf7c31d565615e0f408f/ui/views/controls/menu/menu_scroll_view_container.cc

Labels: Merge-Request-69
Requesting merge to M69.
Hello Elly,

thanks for fixing this. I took a look at latest Snapshot and I have a question regarding your change. 

The vertical height has only an effect, where the rounded corners are. Wouldn't it look nicer if the other white spaces could also have the same vertical space. This would match the behavior on MacOS Menus and would look consistent in the new menus.

WDYT?

Thanks for listening :)
actual.png
26.1 KB View Download
expected.png
28.4 KB View Download
macOS_Menus.png
17.7 KB View Download
#7: I don't have strong feelings one way or the other so I'm probably not going to change it further.
Okay, thanks for your feedback :)
NextAction: 2018-08-07
This is P2, is this really need a merge to M69?
If yes, pls update the bug with canary result tomorrow. 
The NextAction date has arrived: 2018-08-07
Labels: TE-Verified-M70 TE-Verified-70.0.3515.0
Able to reproduce the issue on Mac 10.13.3 using chrome reported version #70.0.3503.0 as per the merged  issue 867874 .

Verified the fix on Mac 10.13.3 using latest chrome version #70.0.3515.0 as per the merged  issue 867874 .
Attaching screen shot for reference.
Observed that focus highlight did not go out of the border of bookmarked link box.
Hence, the fix is working as expected. 
Adding the verified labels.

Thanks...!!
Screen Shot 2018-08-07 at 17.11.16.png
241 KB View Download
Screen Shot 2018-08-07 at 17.10.10.png
32.5 KB View Download
Labels: -Merge-Request-69
Hm. On reflection, I don't think this is serious enough to merit a merge to M69, so I'll untag the merge request.
Status: Fixed (was: Started)

Sign in to add a comment