Add contextmenu attribute for HTMLElement.

Add contextmenu attribute for HTMLElement.

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.

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

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

[pals, 2014-08-17 17:18:13]

Please review.

https://codereview.chromium.org/464903002/diff/1/LayoutTests/fast/dom/HTMLEle...
File LayoutTests/fast/dom/HTMLElement/contextmenu.html (right):

https://codereview.chromium.org/464903002/diff/1/LayoutTests/fast/dom/HTMLEle...
LayoutTests/fast/dom/HTMLElement/contextmenu.html:56: container5.contextMenu =
menu3;
Specification
http://www.whatwg.org/specs/web-apps/current-work/#reflect
under section
"On setting, if the given element has an id attribute, and has the same home
subtree as the element of the attribute being set, and the given element is the
FIRST ELEMENT in that home subtree whose ID is the value of that id attribute,
then the content attribute must be set to the value of that id attribute.
Otherwise, the content attribute must be set to the empty string."

https://codereview.chromium.org/464903002/diff/1/LayoutTests/fast/dom/HTMLEle...
LayoutTests/fast/dom/HTMLElement/contextmenu.html:87:
shouldBeNull("container9.contextMenu");
type of menu should be "popup" for contextmenu attribute.

https://codereview.chromium.org/464903002/diff/1/Source/core/html/HTMLElement...
File Source/core/html/HTMLElement.cpp (right):

https://codereview.chromium.org/464903002/diff/1/Source/core/html/HTMLElement...
Source/core/html/HTMLElement.cpp:913: return isHTMLMenuElement(element) ?
toHTMLMenuElement(element) : nullptr;
Specification
http://www.whatwg.org/specs/web-apps/current-work/#reflect
under section
"If a reflecting IDL attribute has the type HTMLElement ..."

https://codereview.chromium.org/464903002/diff/1/Source/core/html/HTMLElement...
Source/core/html/HTMLElement.cpp:924: if (!contextMenuId.isNull() &&
equalIgnoringCase(contextMenuType, "popup") &&  contextMenu->inDocument())
http://www.whatwg.org/specs/web-apps/current-work/#attr-contextmenu
"The value must be the ID of a menu element in the same home subtree whose type
attribute is in the popup menu state."

https://codereview.chromium.org/464903002/diff/1/Source/core/html/HTMLElement...
Source/core/html/HTMLElement.cpp:927: setAttribute(contextmenuAttr, "");
Specification
http://www.whatwg.org/specs/web-apps/current-work/#reflect
under section
"On setting, if the given element has an id attribute, and has the same home
subtree as the element of the attribute being set, and the given element is the
first element in that home subtree whose ID is the value of that id attribute,
then the content attribute must be set to the value of that id attribute.
Otherwise, the content attribute must be set to the empty string."

[tkent, 2014-08-18 02:24:53]

https://codereview.chromium.org/464903002/diff/1/LayoutTests/fast/dom/HTMLEle...
File LayoutTests/fast/dom/HTMLElement/contextmenu-expected.txt (right):

https://codereview.chromium.org/464903002/diff/1/LayoutTests/fast/dom/HTMLEle...
LayoutTests/fast/dom/HTMLElement/contextmenu-expected.txt:21: FAIL
container5.contextMenu.label should be menu 6. Was menu 5.
Failing.

https://codereview.chromium.org/464903002/diff/1/LayoutTests/fast/dom/HTMLEle...
LayoutTests/fast/dom/HTMLElement/contextmenu-expected.txt:29: FAIL
container9.contextMenu should be null. Was [object HTMLMenuElement].
Failing.

https://codereview.chromium.org/464903002/diff/1/LayoutTests/fast/dom/HTMLEle...
File LayoutTests/fast/dom/HTMLElement/contextmenu.html (right):

https://codereview.chromium.org/464903002/diff/1/LayoutTests/fast/dom/HTMLEle...
LayoutTests/fast/dom/HTMLElement/contextmenu.html:56: container5.contextMenu =
menu3;
On 2014/08/17 17:18:12, OOO_13th-18thAug wrote:
> Specification
> http://www.whatwg.org/specs/web-apps/current-work/#reflect
> under section
> "On setting, if the given element has an id attribute, and has the same home
> subtree as the element of the attribute being set, and the given element is
the
> FIRST ELEMENT in that home subtree whose ID is the value of that id attribute,
> then the content attribute must be set to the value of that id attribute.
> Otherwise, the content attribute must be set to the empty string."

Please write this paragraph as a code comment of HTMLElement::setContextMenu.

|menu3| is NOT the first element. The first element with id=menu5 is |menu2|. 
So |container5.contextMenu| should be null.

https://codereview.chromium.org/464903002/diff/1/Source/core/html/HTMLElement...
File Source/core/html/HTMLElement.cpp (right):

https://codereview.chromium.org/464903002/diff/1/Source/core/html/HTMLElement...
Source/core/html/HTMLElement.cpp:913: return isHTMLMenuElement(element) ?
toHTMLMenuElement(element) : nullptr;
On 2014/08/17 17:18:13, OOO_13th-18thAug wrote:

> http://www.whatwg.org/specs/web-apps/current-work/#reflect

Off topic: Please refer to multipage version URL.  The single page version is
too slow.

We should have a comment that ignoring type attribute is intentional according
to the standard.

https://codereview.chromium.org/464903002/diff/1/Source/core/html/HTMLElement...
Source/core/html/HTMLElement.cpp:924: if (!contextMenuId.isNull() &&
equalIgnoringCase(contextMenuType, "popup") &&  contextMenu->inDocument())
On 2014/08/17 17:18:13, OOO_13th-18thAug wrote:
> http://www.whatwg.org/specs/web-apps/current-work/#attr-contextmenu
> "The value must be the ID of a menu element in the same home subtree whose
type
> attribute is in the popup menu state."

You misread the specification.  This paragraph is for web authors.  This
paragraph doesn't say UA should reject IDs of <menu type=popup> elements.
According to the reflect behavior specification, we should accept any <menu>
elements.

https://codereview.chromium.org/464903002/diff/1/Source/core/html/HTMLElement...
Source/core/html/HTMLElement.cpp:927: setAttribute(contextmenuAttr, "");
On 2014/08/17 17:18:13, OOO_13th-18thAug wrote:
> Specification
> http://www.whatwg.org/specs/web-apps/current-work/#reflect
> under section
> "On setting, if the given element has an id attribute, and has the same home
> subtree as the element of the attribute being set, and the given element is
the
> first element in that home subtree whose ID is the value of that id attribute,
> then the content attribute must be set to the value of that id attribute.
> Otherwise, the content attribute must be set to the empty string."

"same home subtree" means we need to check treeScope identity in Blink because
the HTML specification doesn't care about Shadow DOM.

There is no code to check "the first element"

[tkent, 2014-08-18 02:27:27]

https://codereview.chromium.org/464903002/diff/1/Source/core/html/HTMLElement...
File Source/core/html/HTMLElement.cpp (right):

https://codereview.chromium.org/464903002/diff/1/Source/core/html/HTMLElement...
Source/core/html/HTMLElement.cpp:924: if (!contextMenuId.isNull() &&
equalIgnoringCase(contextMenuType, "popup") &&  contextMenu->inDocument())
On 2014/08/18 02:24:52, tkent wrote:
> On 2014/08/17 17:18:13, OOO_13th-18thAug wrote:
> > http://www.whatwg.org/specs/web-apps/current-work/#attr-contextmenu
> > "The value must be the ID of a menu element in the same home subtree whose
> type
> > attribute is in the popup menu state."
> 
> You misread the specification.  This paragraph is for web authors.  This
> paragraph doesn't say UA should reject IDs of <menu type=popup> elements.
> According to the reflect behavior specification, we should accept any <menu>
> elements.

reject IDs of <menu type=popup> -> reject IDs of *no* <menu type=popup>

[pals, 2014-08-18 14:23:25]

Fixed the failing tests. Also added test for a shadow dom. PTAL.

https://codereview.chromium.org/464903002/diff/1/LayoutTests/fast/dom/HTMLEle...
File LayoutTests/fast/dom/HTMLElement/contextmenu-expected.txt (right):

https://codereview.chromium.org/464903002/diff/1/LayoutTests/fast/dom/HTMLEle...
LayoutTests/fast/dom/HTMLElement/contextmenu-expected.txt:21: FAIL
container5.contextMenu.label should be menu 6. Was menu 5.
On 2014/08/18 02:24:52, tkent wrote:
> Failing.

Done.

https://codereview.chromium.org/464903002/diff/1/LayoutTests/fast/dom/HTMLEle...
LayoutTests/fast/dom/HTMLElement/contextmenu-expected.txt:29: FAIL
container9.contextMenu should be null. Was [object HTMLMenuElement].
On 2014/08/18 02:24:52, tkent wrote:
> Failing.

Done.

https://codereview.chromium.org/464903002/diff/1/LayoutTests/fast/dom/HTMLEle...
File LayoutTests/fast/dom/HTMLElement/contextmenu.html (right):

https://codereview.chromium.org/464903002/diff/1/LayoutTests/fast/dom/HTMLEle...
LayoutTests/fast/dom/HTMLElement/contextmenu.html:56: container5.contextMenu =
menu3;
On 2014/08/18 02:24:52, tkent wrote:
> On 2014/08/17 17:18:12, OOO_13th-18thAug wrote:
> > Specification
> > http://www.whatwg.org/specs/web-apps/current-work/#reflect
> > under section
> > "On setting, if the given element has an id attribute, and has the same home
> > subtree as the element of the attribute being set, and the given element is
> the
> > FIRST ELEMENT in that home subtree whose ID is the value of that id
attribute,
> > then the content attribute must be set to the value of that id attribute.
> > Otherwise, the content attribute must be set to the empty string."
> 
> Please write this paragraph as a code comment of HTMLElement::setContextMenu.
> 
> |menu3| is NOT the first element. The first element with id=menu5 is |menu2|. 
> So |container5.contextMenu| should be null.

Done.

https://codereview.chromium.org/464903002/diff/1/Source/core/html/HTMLElement...
File Source/core/html/HTMLElement.cpp (right):

https://codereview.chromium.org/464903002/diff/1/Source/core/html/HTMLElement...
Source/core/html/HTMLElement.cpp:913: return isHTMLMenuElement(element) ?
toHTMLMenuElement(element) : nullptr;
On 2014/08/18 02:24:52, tkent wrote:
> On 2014/08/17 17:18:13, OOO_13th-18thAug wrote:
> 
> > http://www.whatwg.org/specs/web-apps/current-work/#reflect
> 
> Off topic: Please refer to multipage version URL.  The single page version is
> too slow.
> 
> We should have a comment that ignoring type attribute is intentional according
> to the standard.

Done.

https://codereview.chromium.org/464903002/diff/1/Source/core/html/HTMLElement...
Source/core/html/HTMLElement.cpp:924: if (!contextMenuId.isNull() &&
equalIgnoringCase(contextMenuType, "popup") &&  contextMenu->inDocument())
On 2014/08/18 02:27:27, tkent wrote:
> On 2014/08/18 02:24:52, tkent wrote:
> > On 2014/08/17 17:18:13, OOO_13th-18thAug wrote:
> > > http://www.whatwg.org/specs/web-apps/current-work/#attr-contextmenu
> > > "The value must be the ID of a menu element in the same home subtree whose
> > type
> > > attribute is in the popup menu state."
> > 
> > You misread the specification.  This paragraph is for web authors.  This
> > paragraph doesn't say UA should reject IDs of <menu type=popup> elements.
> > According to the reflect behavior specification, we should accept any <menu>
> > elements.
> 
> reject IDs of <menu type=popup> -> reject IDs of *no* <menu type=popup>

Done.

https://codereview.chromium.org/464903002/diff/1/Source/core/html/HTMLElement...
Source/core/html/HTMLElement.cpp:927: setAttribute(contextmenuAttr, "");
On 2014/08/18 02:24:52, tkent wrote:
> On 2014/08/17 17:18:13, OOO_13th-18thAug wrote:
> > Specification
> > http://www.whatwg.org/specs/web-apps/current-work/#reflect
> > under section
> > "On setting, if the given element has an id attribute, and has the same home
> > subtree as the element of the attribute being set, and the given element is
> the
> > first element in that home subtree whose ID is the value of that id
attribute,
> > then the content attribute must be set to the value of that id attribute.
> > Otherwise, the content attribute must be set to the empty string."
> 
> "same home subtree" means we need to check treeScope identity in Blink because
> the HTML specification doesn't care about Shadow DOM.
> 
> There is no code to check "the first element"

Done. I was doing inDocument() check. Also added a test with shadow dom.

[tkent, 2014-08-19 01:14:42]

https://codereview.chromium.org/464903002/diff/40001/LayoutTests/fast/dom/HTM...
File LayoutTests/fast/dom/HTMLElement/contextmenu.html (right):

https://codereview.chromium.org/464903002/diff/40001/LayoutTests/fast/dom/HTM...
LayoutTests/fast/dom/HTMLElement/contextmenu.html:8: description('Tests the
contextmenu attribute.');
Inconsistent quote characters.
You use single-quotes in this line, and use double-quotes in many other lines.

https://codereview.chromium.org/464903002/diff/40001/LayoutTests/fast/dom/HTM...
LayoutTests/fast/dom/HTMLElement/contextmenu.html:19: 
Need more getter test cases.

- Pointing an ID of non-<menu> element
- Pointing <menu> with non-popup type.
- Pointing an ID which multiple elements have

https://codereview.chromium.org/464903002/diff/40001/LayoutTests/fast/dom/HTM...
LayoutTests/fast/dom/HTMLElement/contextmenu.html:41:
shouldBeNull("container4.contextMenu");
Also, check container4.hasAttribute('contextmenu') value and
container4.getAttribute('contextmenu') value.

https://codereview.chromium.org/464903002/diff/40001/LayoutTests/fast/dom/HTM...
LayoutTests/fast/dom/HTMLElement/contextmenu.html:57:
shouldBeNull("container5.contextMenu");
Additionally, we should test:
  container5.contextMenu = menu2;
  shouldBe("container5.contextMenu", "menu2");

https://codereview.chromium.org/464903002/diff/40001/LayoutTests/fast/dom/HTM...
LayoutTests/fast/dom/HTMLElement/contextmenu.html:60: document.write("<div
id='container6'></div>");
We should change the HTML to 
  <div id='container6' contextmenu='menu6'><menu id='menu6'></menu></div>

https://codereview.chromium.org/464903002/diff/40001/Source/core/html/HTMLEle...
File Source/core/html/HTMLElement.cpp (right):

https://codereview.chromium.org/464903002/diff/40001/Source/core/html/HTMLEle...
Source/core/html/HTMLElement.cpp:920: if (!contextMenu)
The specification isn't clear, but I think |element.contextMenu = null;| should
follow the 'Otherwise' case in the specification.

https://codereview.chromium.org/464903002/diff/40001/Source/core/html/HTMLEle...
Source/core/html/HTMLElement.cpp:931: if (!contextMenuId.isNull() &&
(contextMenu == treeScope().getElementById(contextMenuId)) &&
contextMenu->isInTreeScope())
The parentheses on the second condition is unnecessary.
The third condition is unnecessary because the second condition implies it.

if (!contextMenuId.isNull() && contextMenu ==
treeScope().getElementById(contextMenuId))
    setAttribute(...

[pals, 2014-08-19 10:12:32]

Added more tests. PTAL.

https://codereview.chromium.org/464903002/diff/40001/LayoutTests/fast/dom/HTM...
File LayoutTests/fast/dom/HTMLElement/contextmenu.html (right):

https://codereview.chromium.org/464903002/diff/40001/LayoutTests/fast/dom/HTM...
LayoutTests/fast/dom/HTMLElement/contextmenu.html:8: description('Tests the
contextmenu attribute.');
On 2014/08/19 01:14:41, tkent wrote:
> Inconsistent quote characters.
> You use single-quotes in this line, and use double-quotes in many other lines.

Done.

https://codereview.chromium.org/464903002/diff/40001/LayoutTests/fast/dom/HTM...
LayoutTests/fast/dom/HTMLElement/contextmenu.html:19: 
On 2014/08/19 01:14:41, tkent wrote:
> Need more getter test cases.
> 
> - Pointing an ID of non-<menu> element
> - Pointing <menu> with non-popup type.
> - Pointing an ID which multiple elements have

Done.

https://codereview.chromium.org/464903002/diff/40001/LayoutTests/fast/dom/HTM...
LayoutTests/fast/dom/HTMLElement/contextmenu.html:41:
shouldBeNull("container4.contextMenu");
On 2014/08/19 01:14:41, tkent wrote:
> Also, check container4.hasAttribute('contextmenu') value and
> container4.getAttribute('contextmenu') value.

Done.

https://codereview.chromium.org/464903002/diff/40001/LayoutTests/fast/dom/HTM...
LayoutTests/fast/dom/HTMLElement/contextmenu.html:57:
shouldBeNull("container5.contextMenu");
On 2014/08/19 01:14:41, tkent wrote:
> Additionally, we should test:
>   container5.contextMenu = menu2;
>   shouldBe("container5.contextMenu", "menu2");

Done.

https://codereview.chromium.org/464903002/diff/40001/LayoutTests/fast/dom/HTM...
LayoutTests/fast/dom/HTMLElement/contextmenu.html:60: document.write("<div
id='container6'></div>");
On 2014/08/19 01:14:41, tkent wrote:
> We should change the HTML to 
>   <div id='container6' contextmenu='menu6'><menu id='menu6'></menu></div>

Done.

https://codereview.chromium.org/464903002/diff/40001/Source/core/html/HTMLEle...
File Source/core/html/HTMLElement.cpp (right):

https://codereview.chromium.org/464903002/diff/40001/Source/core/html/HTMLEle...
Source/core/html/HTMLElement.cpp:920: if (!contextMenu)
On 2014/08/19 01:14:41, tkent wrote:
> The specification isn't clear, but I think |element.contextMenu = null;|
should
> follow the 'Otherwise' case in the specification.

Done.

https://codereview.chromium.org/464903002/diff/40001/Source/core/html/HTMLEle...
Source/core/html/HTMLElement.cpp:931: if (!contextMenuId.isNull() &&
(contextMenu == treeScope().getElementById(contextMenuId)) &&
contextMenu->isInTreeScope())
On 2014/08/19 01:14:41, tkent wrote:
> The parentheses on the second condition is unnecessary.
> The third condition is unnecessary because the second condition implies it.
> 
> if (!contextMenuId.isNull() && contextMenu ==
> treeScope().getElementById(contextMenuId))
>     setAttribute(...

Done.

[tkent, 2014-08-19 12:38:35]

The CQ bit was checked by tkent@chromium.org

[tkent, 2014-08-19 12:38:35]

lgtm

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

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

[commit-bot: I haz the power, 2014-08-19 13:44:03]

Committed patchset #3 (80001) as 180551