If an element A has a parent element, then A's assigned context menu is the assigned context menu o…

If an element A has a parent element, then A's assigned context menu is the assigned context menu of its parent element. Otherwise, A's assigned context menu is null.

Specification: https://html.spec.whatwg.org/multipage/forms.html#assigned-context-menu

"Each element has an assigned context menu, which can be null. If an element A has a contextmenu attribute, and there is an element with the ID given by A's contextmenu attribute's value in A's home subtree, and the first such element in tree order is a menu element whose type attribute is in the popup menu state, then A's assigned context menu is that element. Otherwise, if A has a parent element, then A's assigned context menu is the assigned context menu of its parent element. Otherwise, A's assigned context menu is null."

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

[pals, 2014-11-12 07:02:35]

sanjoy.pal@samsung.com changed reviewers:
+ tkent@chromium.org

[pals, 2014-11-12 07:03:33]

Please review.

[tkent, 2014-11-12 07:17:21]

https://codereview.chromium.org/721603002/diff/1/LayoutTests/fast/dom/HTMLMen...
File LayoutTests/fast/dom/HTMLMenuElement/custom-context-menu.html (left):

https://codereview.chromium.org/721603002/diff/1/LayoutTests/fast/dom/HTMLMen...
LayoutTests/fast/dom/HTMLMenuElement/custom-context-menu.html:11: <menuitem
label="Item1"></menuitem>
Please don't change lines unrelated to the subject of the CL.  It's noise for
code review.

https://codereview.chromium.org/721603002/diff/1/LayoutTests/fast/dom/HTMLMen...
File LayoutTests/fast/dom/HTMLMenuElement/custom-context-menu.html (right):

https://codereview.chromium.org/721603002/diff/1/LayoutTests/fast/dom/HTMLMen...
LayoutTests/fast/dom/HTMLMenuElement/custom-context-menu.html:153: var button5 =
document.getElementById("button_5");
Should have a test for button5.contextMenu IDL attribute value.

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

https://codereview.chromium.org/721603002/diff/1/Source/core/html/HTMLElement...
Source/core/html/HTMLElement.cpp:941: // If an element A has a parent element,
then A's assigned context menu is
You should not change this function.  The specification defines 'assigned
context menu'.  This function is 'contextMenu' IDL attribute.  They are
different.

https://codereview.chromium.org/721603002/diff/1/Source/core/html/HTMLElement...
Source/core/html/HTMLElement.cpp:945: return parentElement() ?
toHTMLElement(parentElement())->contextMenu() : nullptr;
This line has a bad cast.  parentElement might not be an HTMLElement.

[pals, 2014-11-12 09:30:05]

Added a new function assignedContextMenu(). PTAL.

https://codereview.chromium.org/721603002/diff/1/LayoutTests/fast/dom/HTMLMen...
File LayoutTests/fast/dom/HTMLMenuElement/custom-context-menu.html (left):

https://codereview.chromium.org/721603002/diff/1/LayoutTests/fast/dom/HTMLMen...
LayoutTests/fast/dom/HTMLMenuElement/custom-context-menu.html:11: <menuitem
label="Item1"></menuitem>
On 2014/11/12 07:17:21, tkent wrote:
> Please don't change lines unrelated to the subject of the CL.  It's noise for
> code review.
> 

Done.

https://codereview.chromium.org/721603002/diff/1/LayoutTests/fast/dom/HTMLMen...
File LayoutTests/fast/dom/HTMLMenuElement/custom-context-menu.html (right):

https://codereview.chromium.org/721603002/diff/1/LayoutTests/fast/dom/HTMLMen...
LayoutTests/fast/dom/HTMLMenuElement/custom-context-menu.html:153: var button5 =
document.getElementById("button_5");
On 2014/11/12 07:17:21, tkent wrote:
> Should have a test for button5.contextMenu IDL attribute value.

Done.

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

https://codereview.chromium.org/721603002/diff/1/Source/core/html/HTMLElement...
Source/core/html/HTMLElement.cpp:941: // If an element A has a parent element,
then A's assigned context menu is
On 2014/11/12 07:17:21, tkent wrote:
> You should not change this function.  The specification defines 'assigned
> context menu'.  This function is 'contextMenu' IDL attribute.  They are
> different.

Done.

https://codereview.chromium.org/721603002/diff/1/Source/core/html/HTMLElement...
Source/core/html/HTMLElement.cpp:945: return parentElement() ?
toHTMLElement(parentElement())->contextMenu() : nullptr;
On 2014/11/12 07:17:21, tkent wrote:
> This line has a bad cast.  parentElement might not be an HTMLElement.

Done.

[tkent, 2014-11-12 09:37:19]

Please put a short summary of the change at the beginning of the CL description.

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

https://codereview.chromium.org/721603002/diff/20001/Source/core/html/HTMLEle...
Source/core/html/HTMLElement.cpp:940: if (menu)
nit:
    if (HTMLMenuElement* menu = contextMenu())
        return menu;
is simpler.

https://codereview.chromium.org/721603002/diff/20001/Source/core/html/HTMLEle...
Source/core/html/HTMLElement.cpp:943: return parentElement() &&
parentElement()->isHTMLElement() ?
toHTMLElement(parentElement())->assignedContextMenu() :nullptr;
nit: need a space after :

[pals, 2014-11-12 09:43:59]

Fixed the comments and updated the description. Please review.

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

https://codereview.chromium.org/721603002/diff/20001/Source/core/html/HTMLEle...
Source/core/html/HTMLElement.cpp:940: if (menu)
On 2014/11/12 09:37:18, tkent wrote:
> nit:
>     if (HTMLMenuElement* menu = contextMenu())
>         return menu;
> is simpler.

Done.

https://codereview.chromium.org/721603002/diff/20001/Source/core/html/HTMLEle...
Source/core/html/HTMLElement.cpp:943: return parentElement() &&
parentElement()->isHTMLElement() ?
toHTMLElement(parentElement())->assignedContextMenu() :nullptr;
On 2014/11/12 09:37:18, tkent wrote:
> nit: need a space after :

Done.

[tkent, 2014-11-12 10:04:00]

The CQ bit was checked by tkent@chromium.org

[tkent, 2014-11-12 10:04:01]

lgtm

[commit-bot: I haz the power, 2014-11-12 10:04:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/721603002/40001

[commit-bot: I haz the power, 2014-11-12 11:22:57]

Committed patchset #3 (id:40001) as 185198