Add support to populate custom context menu items.

Add support to populate custom context menu items.

contextmenu attribute which enables web pages to specify custom
context menu along with or in place of default platform context
menu for a particular element. The value of the contextmenu
attribute must be the ID of menu element with type popup.
This CL enables ContextMenuController to populate custom context
menu items specified by contextmenu attribute on any element.

Specifications:
http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#context-menus

BUG=87553
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180896

[pals, 2014-08-20 12:43:33]

PTAL.

[tkent, 2014-08-21 01:34:39]

> Specifications:
> http://www.whatwg.org/specs/web-apps/current-work/#context-menus

Please do not refer to the single-page version.  We should refer to
multiple-page version.

https://codereview.chromium.org/492753002/diff/20001/LayoutTests/contextmenu/...
File LayoutTests/contextmenu/custom-context-menu.html (right):

https://codereview.chromium.org/492753002/diff/20001/LayoutTests/contextmenu/...
LayoutTests/contextmenu/custom-context-menu.html:33: if (window.eventSender) {
Show something if no eventSender.

https://codereview.chromium.org/492753002/diff/20001/LayoutTests/contextmenu/...
LayoutTests/contextmenu/custom-context-menu.html:35: var items =
eventSender.contextClick();
Let's make a function like contextClickAndGetMenuItems(element).

https://codereview.chromium.org/492753002/diff/20001/LayoutTests/contextmenu/...
LayoutTests/contextmenu/custom-context-menu.html:38:
eventSender.keyDown(String.fromCharCode(0x001B), null);
Is this necessary?

https://codereview.chromium.org/492753002/diff/20001/LayoutTests/contextmenu/...
LayoutTests/contextmenu/custom-context-menu.html:43:
shouldBeEqualToNumber((itemsWithCustomContextMenu.length - items.length), 9);
The first argument should be as string evaluated like other shouldBe functions.

https://codereview.chromium.org/492753002/diff/20001/Source/core/page/Context...
File Source/core/page/ContextMenuController.cpp (right):

https://codereview.chromium.org/492753002/diff/20001/Source/core/page/Context...
Source/core/page/ContextMenuController.cpp:89: void
ContextMenuController::populateCustomContextMenu(Event* event)
Event* -> Even&

https://codereview.chromium.org/492753002/diff/20001/Source/core/page/Context...
Source/core/page/ContextMenuController.cpp:95: if (node &&
node->isHTMLElement()) {
We prefer early return.

if (!node || !node->isHTMLElement())
    return;

https://codereview.chromium.org/492753002/diff/20001/Source/core/page/Context...
Source/core/page/ContextMenuController.cpp:100: RefPtrWillBeRawPtr<RelatedEvent>
relatedEvent = RelatedEvent::create(EventTypeNames::show, true, true, node);
Should we check type=popup?

https://codereview.chromium.org/492753002/diff/20001/Source/core/page/Context...
Source/core/page/ContextMenuController.cpp:102: return;
We need a layout test of canceling |show| event.

https://codereview.chromium.org/492753002/diff/20001/Source/core/page/Context...
Source/core/page/ContextMenuController.cpp:104: return;
Need a layout test for this too.

https://codereview.chromium.org/492753002/diff/20001/Source/core/page/Context...
File Source/core/page/ContextMenuControllerTest.cpp (right):

https://codereview.chromium.org/492753002/diff/20001/Source/core/page/Context...
Source/core/page/ContextMenuControllerTest.cpp:37:
TEST_F(ContextMenuControllerTest, TestCustomMenu)
A layout test can cover this.  We can remove this C++ test.

https://codereview.chromium.org/492753002/diff/20001/Source/core/page/CustomC...
File Source/core/page/CustomContextMenuProvider.cpp (right):

https://codereview.chromium.org/492753002/diff/20001/Source/core/page/CustomC...
Source/core/page/CustomContextMenuProvider.cpp:55: while (nextElement) {
The code doesn't match to the menu construction algorithm in the specification.

http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#constr...

- <hr>
- <menu> without label
- <menuitem> without label
- etc.

[pals, 2014-08-25 14:19:41]

Updated the menu builder algorithm as per specification except "If the child is
a menuitem element that defines a command". PTAL.

https://codereview.chromium.org/492753002/diff/20001/LayoutTests/contextmenu/...
File LayoutTests/contextmenu/custom-context-menu.html (right):

https://codereview.chromium.org/492753002/diff/20001/LayoutTests/contextmenu/...
LayoutTests/contextmenu/custom-context-menu.html:33: if (window.eventSender) {
On 2014/08/21 01:34:38, tkent wrote:
> Show something if no eventSender.

Done.

https://codereview.chromium.org/492753002/diff/20001/LayoutTests/contextmenu/...
LayoutTests/contextmenu/custom-context-menu.html:35: var items =
eventSender.contextClick();
On 2014/08/21 01:34:38, tkent wrote:
> Let's make a function like contextClickAndGetMenuItems(element).

Done.

https://codereview.chromium.org/492753002/diff/20001/LayoutTests/contextmenu/...
LayoutTests/contextmenu/custom-context-menu.html:38:
eventSender.keyDown(String.fromCharCode(0x001B), null);
On 2014/08/21 01:34:38, tkent wrote:
> Is this necessary?
Yes, otherwise next contextClick() does not invoke context menu.

https://codereview.chromium.org/492753002/diff/20001/LayoutTests/contextmenu/...
LayoutTests/contextmenu/custom-context-menu.html:43:
shouldBeEqualToNumber((itemsWithCustomContextMenu.length - items.length), 9);
On 2014/08/21 01:34:38, tkent wrote:
> The first argument should be as string evaluated like other shouldBe
functions.

Done.

https://codereview.chromium.org/492753002/diff/20001/Source/core/page/Context...
File Source/core/page/ContextMenuController.cpp (right):

https://codereview.chromium.org/492753002/diff/20001/Source/core/page/Context...
Source/core/page/ContextMenuController.cpp:89: void
ContextMenuController::populateCustomContextMenu(Event* event)
On 2014/08/21 01:34:39, tkent wrote:
> Event* -> Even&

Done.

https://codereview.chromium.org/492753002/diff/20001/Source/core/page/Context...
Source/core/page/ContextMenuController.cpp:95: if (node &&
node->isHTMLElement()) {
On 2014/08/21 01:34:39, tkent wrote:
> We prefer early return.
> 
> if (!node || !node->isHTMLElement())
>     return;

Done.

https://codereview.chromium.org/492753002/diff/20001/Source/core/page/Context...
Source/core/page/ContextMenuController.cpp:100: RefPtrWillBeRawPtr<RelatedEvent>
relatedEvent = RelatedEvent::create(EventTypeNames::show, true, true, node);
On 2014/08/21 01:34:38, tkent wrote:
> Should we check type=popup?

Done.

https://codereview.chromium.org/492753002/diff/20001/Source/core/page/Context...
Source/core/page/ContextMenuController.cpp:102: return;
On 2014/08/21 01:34:39, tkent wrote:
> We need a layout test of canceling |show| event.

Done.

https://codereview.chromium.org/492753002/diff/20001/Source/core/page/Context...
Source/core/page/ContextMenuController.cpp:104: return;
On 2014/08/21 01:34:39, tkent wrote:
> Need a layout test for this too.

Done.

https://codereview.chromium.org/492753002/diff/20001/Source/core/page/Context...
File Source/core/page/ContextMenuControllerTest.cpp (right):

https://codereview.chromium.org/492753002/diff/20001/Source/core/page/Context...
Source/core/page/ContextMenuControllerTest.cpp:37:
TEST_F(ContextMenuControllerTest, TestCustomMenu)
On 2014/08/21 01:34:39, tkent wrote:
> A layout test can cover this.  We can remove this C++ test.

Done.

https://codereview.chromium.org/492753002/diff/20001/Source/core/page/CustomC...
File Source/core/page/CustomContextMenuProvider.cpp (right):

https://codereview.chromium.org/492753002/diff/20001/Source/core/page/CustomC...
Source/core/page/CustomContextMenuProvider.cpp:55: while (nextElement) {
On 2014/08/21 01:34:39, tkent wrote:
> The code doesn't match to the menu construction algorithm in the
specification.
> 
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#constr...
> 
> - <hr>
> - <menu> without label
> - <menuitem> without label
> - etc.

Done.

[tkent, 2014-08-25 23:07:42]

https://codereview.chromium.org/492753002/diff/40001/LayoutTests/contextmenu/...
File LayoutTests/contextmenu/custom-context-menu.html (right):

https://codereview.chromium.org/492753002/diff/40001/LayoutTests/contextmenu/...
LayoutTests/contextmenu/custom-context-menu.html:1: <!DOCTYPE html>
Making LayoutTests/contextmenu/ only for this test doesn't sounds reasonable. 
Please move this to fast/dom/HTMLMenuElement/.

https://codereview.chromium.org/492753002/diff/40001/LayoutTests/contextmenu/...
LayoutTests/contextmenu/custom-context-menu.html:38: <!-- Test collapsing two or
more adjacent separators. -->
Is this comment valid?  Only single <hr> follows.

https://codereview.chromium.org/492753002/diff/40001/LayoutTests/contextmenu/...
LayoutTests/contextmenu/custom-context-menu.html:48: <!-- Test menuitem menu
without label. -->
without label -> with empty label?

https://codereview.chromium.org/492753002/diff/40001/LayoutTests/contextmenu/...
LayoutTests/contextmenu/custom-context-menu.html:50: <!-- Test menuitem menu
with empty label. -->
with empty label -> without label?

https://codereview.chromium.org/492753002/diff/40001/LayoutTests/contextmenu/...
LayoutTests/contextmenu/custom-context-menu.html:86: {
Position of the '{' is inconsistent in this file.

https://codereview.chromium.org/492753002/diff/40001/LayoutTests/contextmenu/...
LayoutTests/contextmenu/custom-context-menu.html:90: var items =
eventSender.contextClick();
return eventSender.contextClick();

https://codereview.chromium.org/492753002/diff/40001/LayoutTests/contextmenu/...
LayoutTests/contextmenu/custom-context-menu.html:99: parent =
event.target.parentNode;
event.target.remove();

https://codereview.chromium.org/492753002/diff/40001/LayoutTests/contextmenu/...
LayoutTests/contextmenu/custom-context-menu.html:108:
eventSender.keyDown(String.fromCharCode(0x001B), null);
eventSender.keyDown("escape");

https://codereview.chromium.org/492753002/diff/40001/Source/core/page/Context...
File Source/core/page/ContextMenuController.cpp (right):

https://codereview.chromium.org/492753002/diff/40001/Source/core/page/Context...
Source/core/page/ContextMenuController.cpp:33: #include
"core/events/EventDispatchMediator.h"
Looks unnecessary.

https://codereview.chromium.org/492753002/diff/40001/Source/core/page/Context...
Source/core/page/ContextMenuController.cpp:34: #include
"core/events/EventDispatcher.h"
Looks unnecessary.

https://codereview.chromium.org/492753002/diff/40001/Source/core/page/CustomC...
File Source/core/page/CustomContextMenuProvider.cpp (right):

https://codereview.chromium.org/492753002/diff/40001/Source/core/page/CustomC...
Source/core/page/CustomContextMenuProvider.cpp:58: // Collapse all sequences of
two or more adjacent separators in the menu or any submenus to a single
separator.
I recommend to wrap code comments in 80 columns.

https://codereview.chromium.org/492753002/diff/40001/Source/core/page/CustomC...
Source/core/page/CustomContextMenuProvider.cpp:70: if (labelString.isNull() ||
labelString.isEmpty())
isEmpty() is enough.  isEmpty() contains isNull().

https://codereview.chromium.org/492753002/diff/40001/Source/core/page/CustomC...
Source/core/page/CustomContextMenuProvider.cpp:87: if (labelString.isNull() ||
labelString.isEmpty()) {
This looks incorrect.

The following code is for the third bullet of step 2. So, the condition should
be isNull().

> If the child is a menu element with no label attribute
> Append a separator to the menu, then run the menu builder steps using this
child menu element for the same menu, then append another separator to the menu.

For isEmpty(), we need to do step 3, right?

> 3. Remove any submenu with no label, or whose label is the empty string, in
the menu or any submenus.

https://codereview.chromium.org/492753002/diff/40001/Source/core/page/CustomC...
Source/core/page/CustomContextMenuProvider.cpp:107: while
(contextMenu.items().last().type() == SeparatorType)
Need a test in a case of <menu type=popup> without any children.

[pals, 2014-08-26 07:13:23]

Please take a look.

https://codereview.chromium.org/492753002/diff/40001/LayoutTests/contextmenu/...
File LayoutTests/contextmenu/custom-context-menu.html (right):

https://codereview.chromium.org/492753002/diff/40001/LayoutTests/contextmenu/...
LayoutTests/contextmenu/custom-context-menu.html:1: <!DOCTYPE html>
On 2014/08/25 23:07:41, tkent wrote:
> Making LayoutTests/contextmenu/ only for this test doesn't sounds reasonable. 
> Please move this to fast/dom/HTMLMenuElement/.

Done.

https://codereview.chromium.org/492753002/diff/40001/LayoutTests/contextmenu/...
LayoutTests/contextmenu/custom-context-menu.html:38: <!-- Test collapsing two or
more adjacent separators. -->
On 2014/08/25 23:07:41, tkent wrote:
> Is this comment valid?  Only single <hr> follows.
> 
One more separator is added at the end of the above menu with empty label.

https://codereview.chromium.org/492753002/diff/40001/LayoutTests/contextmenu/...
LayoutTests/contextmenu/custom-context-menu.html:48: <!-- Test menuitem menu
without label. -->
On 2014/08/25 23:07:42, tkent wrote:
> without label -> with empty label?

Done.

https://codereview.chromium.org/492753002/diff/40001/LayoutTests/contextmenu/...
LayoutTests/contextmenu/custom-context-menu.html:50: <!-- Test menuitem menu
with empty label. -->
On 2014/08/25 23:07:41, tkent wrote:
> with empty label -> without label?

Done.

https://codereview.chromium.org/492753002/diff/40001/LayoutTests/contextmenu/...
LayoutTests/contextmenu/custom-context-menu.html:86: {
On 2014/08/25 23:07:41, tkent wrote:
> Position of the '{' is inconsistent in this file.

Done.

https://codereview.chromium.org/492753002/diff/40001/LayoutTests/contextmenu/...
LayoutTests/contextmenu/custom-context-menu.html:90: var items =
eventSender.contextClick();
On 2014/08/25 23:07:41, tkent wrote:
> return eventSender.contextClick();

Done.

https://codereview.chromium.org/492753002/diff/40001/LayoutTests/contextmenu/...
LayoutTests/contextmenu/custom-context-menu.html:99: parent =
event.target.parentNode;
On 2014/08/25 23:07:41, tkent wrote:
> event.target.remove();

Done.

https://codereview.chromium.org/492753002/diff/40001/LayoutTests/contextmenu/...
LayoutTests/contextmenu/custom-context-menu.html:108:
eventSender.keyDown(String.fromCharCode(0x001B), null);
On 2014/08/25 23:07:41, tkent wrote:
> eventSender.keyDown("escape");

Done.

https://codereview.chromium.org/492753002/diff/40001/Source/core/page/Context...
File Source/core/page/ContextMenuController.cpp (right):

https://codereview.chromium.org/492753002/diff/40001/Source/core/page/Context...
Source/core/page/ContextMenuController.cpp:33: #include
"core/events/EventDispatchMediator.h"
On 2014/08/25 23:07:42, tkent wrote:
> Looks unnecessary.

Done.

https://codereview.chromium.org/492753002/diff/40001/Source/core/page/Context...
Source/core/page/ContextMenuController.cpp:34: #include
"core/events/EventDispatcher.h"
On 2014/08/25 23:07:42, tkent wrote:
> Looks unnecessary.

Done.

https://codereview.chromium.org/492753002/diff/40001/Source/core/page/CustomC...
File Source/core/page/CustomContextMenuProvider.cpp (right):

https://codereview.chromium.org/492753002/diff/40001/Source/core/page/CustomC...
Source/core/page/CustomContextMenuProvider.cpp:58: // Collapse all sequences of
two or more adjacent separators in the menu or any submenus to a single
separator.
On 2014/08/25 23:07:42, tkent wrote:
> I recommend to wrap code comments in 80 columns.

Done.

https://codereview.chromium.org/492753002/diff/40001/Source/core/page/CustomC...
Source/core/page/CustomContextMenuProvider.cpp:70: if (labelString.isNull() ||
labelString.isEmpty())
On 2014/08/25 23:07:42, tkent wrote:
> isEmpty() is enough.  isEmpty() contains isNull().

Done.

https://codereview.chromium.org/492753002/diff/40001/Source/core/page/CustomC...
Source/core/page/CustomContextMenuProvider.cpp:87: if (labelString.isNull() ||
labelString.isEmpty()) {
On 2014/08/25 23:07:42, tkent wrote:
> This looks incorrect.
> 
> The following code is for the third bullet of step 2. So, the condition should
> be isNull().
> 
> > If the child is a menu element with no label attribute
> > Append a separator to the menu, then run the menu builder steps using this
> child menu element for the same menu, then append another separator to the
menu.
> 
> For isEmpty(), we need to do step 3, right?
> 
> > 3. Remove any submenu with no label, or whose label is the empty string, in
> the menu or any submenus.


My understanding from the above two statements is that the child menu (submenu)
with no label attribute should be flattened out inside two separator in the
parent menu, and child menu (submenu) should not be created. The code below
implements the same.


I think these two statements are somewhat contradictory. I asked for
clarification on whatwg@whatwg.org.

https://codereview.chromium.org/492753002/diff/40001/Source/core/page/CustomC...
Source/core/page/CustomContextMenuProvider.cpp:107: while
(contextMenu.items().last().type() == SeparatorType)
On 2014/08/25 23:07:42, tkent wrote:
> Need a test in a case of <menu type=popup> without any children.

Done.

[tkent, 2014-08-26 08:11:27]

https://codereview.chromium.org/492753002/diff/40001/Source/core/page/CustomC...
File Source/core/page/CustomContextMenuProvider.cpp (right):

https://codereview.chromium.org/492753002/diff/40001/Source/core/page/CustomC...
Source/core/page/CustomContextMenuProvider.cpp:87: if (labelString.isNull() ||
labelString.isEmpty()) {
On 2014/08/26 07:13:23, sanjoy_pal wrote:
> On 2014/08/25 23:07:42, tkent wrote:
> > This looks incorrect.
> > 
> > The following code is for the third bullet of step 2. So, the condition
should
> > be isNull().
> > 
> > > If the child is a menu element with no label attribute
> > > Append a separator to the menu, then run the menu builder steps using this
> > child menu element for the same menu, then append another separator to the
> menu.
> > 
> > For isEmpty(), we need to do step 3, right?
> > 
> > > 3. Remove any submenu with no label, or whose label is the empty string,
in
> > the menu or any submenus.
> 
> 
> My understanding from the above two statements is that the child menu
(submenu)
> with no label attribute should be flattened out inside two separator in the
> parent menu, and child menu (submenu) should not be created. The code below
> implements the same.
> 
> 
> I think these two statements are somewhat contradictory. I asked for
> clarification on mailto:whatwg@whatwg.org.

ok, let's revisit here after the clarification.

[tkent, 2014-08-26 08:12:03]

lgtm!

[tkent, 2014-08-26 08:12:06]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-08-26 08:12:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/492753002/60001

[commit-bot: I haz the power, 2014-08-26 10:47:55]

Committed patchset #3 (60001) as 180896