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

Issue 743160 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: ----
Pri: 3
Type: Feature

Blocked on:
issue 750802
issue 750864


Sign in to add a comment

Check context menu item visibility in UI code

Project Member Reported by catmulli...@chromium.org, Jul 14 2017 
sky@ and tapted@, what are thoughts on changing the UI code to support checking context menu item visibility? 

Background:

The Extensions team is adding support for a visibility property in chrome.contextMenus items. See  crbug.com/463476  for details.

Currently, the context menu UI code* does not check menu items' visibilities and displays whatever is in the context menu model. This is a problem because the context menu model may include hidden items, and the UI code should not display them.

We should change the UI code to check context menu item visibility before displaying items. The implementation of the change would be handled by the extensions team and be passed to a UI OWNER for review.

*Mac: chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm
Linux, Windows: chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.cc

Comment 1 by tapted@chromium.org, Jul 17 2017 

does ExtensionContextMenuModel just need to override ui::SimpleMenuModel::IsCommandIdVisible() ?

The other plumbing seems to be in place already:

There is ui::MenuModel has IsVisible(), which is wrapped in an adapter and checked by views::MenuItemView::AddMenuItemAt(..) at https://cs.chromium.org/chromium/src/ui/views/controls/menu/menu_item_view.cc?sq=package:chromium&l=260

and by menu_controller.mm at -[MenuController validateUserInterfaceItem] - https://cs.chromium.org/chromium/src/ui/base/cocoa/menu_controller.mm?type=cs&q=validateUserInterfaceItem&l=194

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

Blockedon: 750864

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

Blockedon: 750802

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

does ExtensionContextMenuModel just need to override ui::SimpleMenuModel::IsCommandIdVisible() ?

> Yes, this has been implemented.

The other plumbing seems to be in place already:

> True. Looking into this, the proper visibility checks are in place.

I've run some test cases on the visibility checks in the UI code on Mac and Linux. There are a couple UI bugs with respect to item visibility. I have filed these bugs(see comments 2 and 3).

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

Owner: catmulli...@chromium.org
Status: Started (was: Available)

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

Status: Fixed (was: Started)
►

Sign in to add a comment