New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 750802 link

Starred by 1 user
Status: Fixed
Owner:
█ catmulli...@chromium.org
Last visit > 30 days ago
Closed: Aug 2017
Cc:
rdevlin....@chromium.org
tapted@chromium.org
sky@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 3
Type: Bug

Blocking:
issue 743160



Sign in to add a comment

Context menu submenu items are not hidden properly in UI

Project Member Reported by catmulli...@chromium.org, Jul 31 2017 
In a context menu, if a submenu item is set to be hidden, the context menu UI does not properly hide it.

On Mac, the parent submenu item remains visible along with submenu arrow. It appears that validateUserInterfaceItem (https://cs.chromium.org/chromium/src/ui/base/cocoa/menu_controller.mm?type=cs&q=validateUserInterfaceItem&l=180) is never passed items of type ui::MenuModel::TYPE_SUBMENU.

On Linux, the parent submenu item disappears, but the slot containing the item still remains along with two separators.

See images attached to visualize the bug.
Mac_context_submenu_hidden_bug.png
266 KB View Download

Comment 1 by catmulli...@chromium.org, Jul 31 2017

Linux_context_sumenu_hidden_bug.png
102 KB View Download

Comment 2 by catmulli...@chromium.org, Jul 31 2017

Blocking: 743160
Cc: tapted@chromium.org rdevlin....@chromium.org sky@chromium.org
Components: UI
Labels: OS-Linux OS-Mac OS-Windows

Comment 3 by catmulli...@chromium.org, Aug 15 2017 

 Issue 750864  has been merged into this issue.

Comment 4 by catmulli...@chromium.org, Aug 15 2017

Owner: catmulli...@chromium.org
Status: Started (was: Untriaged)
The following are screenshots of the fixes. 

Note that the bug on Linux, wherein the parent submenu item disappears, but the slot containing the item still remains along with two separators is not a ui/views/* bug. The bug is rooted in the visibility logic.
Mac_fix_vis_prop_bug_hidden_submenu_parent.png
219 KB View Download
Mac_fix_vis_prop_bug_hidden_children.png
275 KB View Download
Linux_fix_vis_prop_bug_hidden_children.png
79.2 KB View Download
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 22 2017 

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

commit 3f8c6daa6103d70811de2aad16fd3d939d42871e
Author: Catherine Mullings <catmullings@chromium.org>
Date: Tue Aug 22 18:12:11 2017

Fix UI bugs when hiding context menu submenus and their children

This CL fixes a bug on Mac so that when a context menu submenu is
hidden, it does not appear in the context menu. Related on Mac, this CL
fixes the case when a context menu submenu's children are hidden, so
that the context menu submenu contains the empty menu item, "(empty)".

The second bug indicated above is fixed on Linux as well in this bug.

Note that this patch depends on the on-going implementation being
reviewed here: https://chromium-review.googlesource.com/c/553578

Bug:  750802 
Change-Id: Ic8c7c0a1c8da11adb3fc5dfb271f26b28c35a41c
Reviewed-on: https://chromium-review.googlesource.com/616100
Commit-Queue: catmullings <catmullings@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496370}
[modify] https://crrev.com/3f8c6daa6103d70811de2aad16fd3d939d42871e/ui/base/cocoa/menu_controller.mm
[modify] https://crrev.com/3f8c6daa6103d70811de2aad16fd3d939d42871e/ui/base/cocoa/menu_controller_unittest.mm
[modify] https://crrev.com/3f8c6daa6103d70811de2aad16fd3d939d42871e/ui/views/controls/menu/menu_item_view.cc
[modify] https://crrev.com/3f8c6daa6103d70811de2aad16fd3d939d42871e/ui/views/controls/menu/menu_item_view.h
[modify] https://crrev.com/3f8c6daa6103d70811de2aad16fd3d939d42871e/ui/views/controls/menu/menu_item_view_unittest.cc
[modify] https://crrev.com/3f8c6daa6103d70811de2aad16fd3d939d42871e/ui/views/controls/menu/submenu_view.cc
[modify] https://crrev.com/3f8c6daa6103d70811de2aad16fd3d939d42871e/ui/views/controls/menu/submenu_view.h

Comment 6 by catmulli...@chromium.org, Aug 22 2017

Status: Fixed (was: Started)
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 22 2017 

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

commit 9de3fd9b60657b115b9311b318285eb53dfc226b
Author: Dave Schuyler <dschuyler@chromium.org>
Date: Tue Aug 22 18:50:53 2017

Revert "Fix UI bugs when hiding context menu submenus and their children"

This reverts commit 3f8c6daa6103d70811de2aad16fd3d939d42871e.

Reason for revert: <INSERT REASONING HERE>

Note from Catherine Mullings:

Btw, this other CL (https://chromium-review.googlesource.com/c/chromium/src/+/616100/) depends on 553578. If you revert 553578, you'll need to revert 616100 too

(and I've reverted 553578).


Original change's description:
> Fix UI bugs when hiding context menu submenus and their children
> 
> This CL fixes a bug on Mac so that when a context menu submenu is
> hidden, it does not appear in the context menu. Related on Mac, this CL
> fixes the case when a context menu submenu's children are hidden, so
> that the context menu submenu contains the empty menu item, "(empty)".
> 
> The second bug indicated above is fixed on Linux as well in this bug.
> 
> Note that this patch depends on the on-going implementation being
> reviewed here: https://chromium-review.googlesource.com/c/553578
> 
> Bug:  750802 
> Change-Id: Ic8c7c0a1c8da11adb3fc5dfb271f26b28c35a41c
> Reviewed-on: https://chromium-review.googlesource.com/616100
> Commit-Queue: catmullings <catmullings@chromium.org>
> Reviewed-by: Scott Violet <sky@chromium.org>
> Reviewed-by: Trent Apted <tapted@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#496370}

TBR=sky@chromium.org,tapted@chromium.org,rdevlin.cronin@chromium.org,catmullings@chromium.org

Change-Id: Id431bc9898a3a1cdabb829ad7168d81c211383b1
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  750802 
Reviewed-on: https://chromium-review.googlesource.com/627016
Reviewed-by: Dave Schuyler <dschuyler@chromium.org>
Commit-Queue: Dave Schuyler <dschuyler@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496387}
[modify] https://crrev.com/9de3fd9b60657b115b9311b318285eb53dfc226b/ui/base/cocoa/menu_controller.mm
[modify] https://crrev.com/9de3fd9b60657b115b9311b318285eb53dfc226b/ui/base/cocoa/menu_controller_unittest.mm
[modify] https://crrev.com/9de3fd9b60657b115b9311b318285eb53dfc226b/ui/views/controls/menu/menu_item_view.cc
[modify] https://crrev.com/9de3fd9b60657b115b9311b318285eb53dfc226b/ui/views/controls/menu/menu_item_view.h
[modify] https://crrev.com/9de3fd9b60657b115b9311b318285eb53dfc226b/ui/views/controls/menu/menu_item_view_unittest.cc
[modify] https://crrev.com/9de3fd9b60657b115b9311b318285eb53dfc226b/ui/views/controls/menu/submenu_view.cc
[modify] https://crrev.com/9de3fd9b60657b115b9311b318285eb53dfc226b/ui/views/controls/menu/submenu_view.h
Project Member

Comment 8 by bugdroid1@chromium.org, Aug 22 2017 

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

commit 4a2a453e32fd4e8c9790389552e7ed5dc26c757b
Author: catmullings <catmullings@chromium.org>
Date: Tue Aug 22 22:35:34 2017

Revert "Revert "Fix UI bugs when hiding context menu submenus and their children""

This reverts commit 9de3fd9b60657b115b9311b318285eb53dfc226b.

Reason for revert: 

Given https://chromium-review.googlesource.com/c/chromium/src/+/627259, it is safe to revert this revert of 616100. For details see revert reason in CL 627259.

Original change's description:
> Revert "Fix UI bugs when hiding context menu submenus and their children"
> 
> This reverts commit 3f8c6daa6103d70811de2aad16fd3d939d42871e.
> 
> Reason for revert: <INSERT REASONING HERE>
> 
> Note from Catherine Mullings:
> 
> Btw, this other CL (https://chromium-review.googlesource.com/c/chromium/src/+/616100/) depends on 553578. If you revert 553578, you'll need to revert 616100 too
> 
> (and I've reverted 553578).
> 
> 
> Original change's description:
> > Fix UI bugs when hiding context menu submenus and their children
> > 
> > This CL fixes a bug on Mac so that when a context menu submenu is
> > hidden, it does not appear in the context menu. Related on Mac, this CL
> > fixes the case when a context menu submenu's children are hidden, so
> > that the context menu submenu contains the empty menu item, "(empty)".
> > 
> > The second bug indicated above is fixed on Linux as well in this bug.
> > 
> > Note that this patch depends on the on-going implementation being
> > reviewed here: https://chromium-review.googlesource.com/c/553578
> > 
> > Bug:  750802 
> > Change-Id: Ic8c7c0a1c8da11adb3fc5dfb271f26b28c35a41c
> > Reviewed-on: https://chromium-review.googlesource.com/616100
> > Commit-Queue: catmullings <catmullings@chromium.org>
> > Reviewed-by: Scott Violet <sky@chromium.org>
> > Reviewed-by: Trent Apted <tapted@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#496370}
> 
> TBR=sky@chromium.org,tapted@chromium.org,rdevlin.cronin@chromium.org,catmullings@chromium.org
> 
> Change-Id: Id431bc9898a3a1cdabb829ad7168d81c211383b1
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug:  750802 
> Reviewed-on: https://chromium-review.googlesource.com/627016
> Reviewed-by: Dave Schuyler <dschuyler@chromium.org>
> Commit-Queue: Dave Schuyler <dschuyler@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#496387}

TBR=sky@chromium.org,tapted@chromium.org,rdevlin.cronin@chromium.org,dschuyler@chromium.org,catmullings@chromium.org

Change-Id: I24310cb1211e12c92f5207e5d8f895e31581a3d6
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  750802 
Reviewed-on: https://chromium-review.googlesource.com/627186
Reviewed-by: Dave Schuyler <dschuyler@chromium.org>
Commit-Queue: catmullings <catmullings@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496478}
[modify] https://crrev.com/4a2a453e32fd4e8c9790389552e7ed5dc26c757b/ui/base/cocoa/menu_controller.mm
[modify] https://crrev.com/4a2a453e32fd4e8c9790389552e7ed5dc26c757b/ui/base/cocoa/menu_controller_unittest.mm
[modify] https://crrev.com/4a2a453e32fd4e8c9790389552e7ed5dc26c757b/ui/views/controls/menu/menu_item_view.cc
[modify] https://crrev.com/4a2a453e32fd4e8c9790389552e7ed5dc26c757b/ui/views/controls/menu/menu_item_view.h
[modify] https://crrev.com/4a2a453e32fd4e8c9790389552e7ed5dc26c757b/ui/views/controls/menu/menu_item_view_unittest.cc
[modify] https://crrev.com/4a2a453e32fd4e8c9790389552e7ed5dc26c757b/ui/views/controls/menu/submenu_view.cc
[modify] https://crrev.com/4a2a453e32fd4e8c9790389552e7ed5dc26c757b/ui/views/controls/menu/submenu_view.h
►

Sign in to add a comment