Implement checked attribute for menuitem.

Implement checked attribute for menuitem.

The checked attribute is a boolean attribute that, if present,
indicates that the command is selected. The attribute must be
omitted unless the type attribute is in either the Checkbox state
or the Radio state.

If the type attribute is in the Checkbox state
If the element has a checked attribute, the UA must remove that
attribute. Otherwise, the UA must add a checked attribute, with the
literal value "checked".

Specification
https://html.spec.whatwg.org/multipage/forms.html#attr-menuitem-checked

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

[pals, 2014-11-28 07:47:22]

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

[pals, 2014-11-28 07:54:21]

I have tested the activation behavior manually which works as expected with
http://jsfiddle.net/oec7vv4r/2/show/. PTAL.

[tkent, 2014-11-28 08:17:24]

We need a way to test activation behavior.  Adding something to testRunner?

https://codereview.chromium.org/766863002/diff/1/Source/core/page/CustomConte...
File Source/core/page/CustomContextMenuProvider.cpp (right):

https://codereview.chromium.org/766863002/diff/1/Source/core/page/CustomConte...
Source/core/page/CustomContextMenuProvider.cpp:50:
element->dispatchEvent(click.release());
According to the specification, we should not dispatch a click event for
type=checkbox.

https://codereview.chromium.org/766863002/diff/1/Source/core/page/CustomConte...
Source/core/page/CustomContextMenuProvider.cpp:92:
contextMenu.appendItem(ContextMenuItem(CheckableActionType,
static_cast<ContextMenuAction>(ContextMenuItemBaseCustomTag + m_menuItems.size()
- 1), labelString, !menuItem->fastHasAttribute(disabledAttr),
menuItem->fastHasAttribute(checkedAttr)));
Some arguments are not simple, but duplicated with the appendItem() call in the
else block.  I recommend to add local variables for them.  e.g.

ContextMenuAction action = static_cast<ContextMenuAction>(...);
bool enabled = menuItem->fastHasAttribute(disabledAttr);
if (equalIgnoringCase(menuItem->fastGetAttribute(typeAttr), "checkbox"))
    contextMenu.appendItem(..., action, labelString, enabled, ...)
else
    contextMenu.appendItem(..., action, labelString, enabled, ...)

[pals, 2014-11-28 09:12:00]

Once I found a way to test activation behavior.
https://codereview.chromium.org/243403006/#ps300001

Can you please check if we can test in the same way?

https://codereview.chromium.org/766863002/diff/1/Source/core/page/CustomConte...
File Source/core/page/CustomContextMenuProvider.cpp (right):

https://codereview.chromium.org/766863002/diff/1/Source/core/page/CustomConte...
Source/core/page/CustomContextMenuProvider.cpp:50:
element->dispatchEvent(click.release());
On 2014/11/28 08:17:24, tkent wrote:
> According to the specification, we should not dispatch a click event for
> type=checkbox.

If we don't dispatch a click, then how the onclick handler will be called for
the menuitem? I'm not sure how the app use <menuitem type="checkbox"> in that
case? Firefox sends click for type checkbox. Please check
http://jsfiddle.net/oec7vv4r/2/show/ on firefox after changing menu
type="context".

https://codereview.chromium.org/766863002/diff/1/Source/core/page/CustomConte...
Source/core/page/CustomContextMenuProvider.cpp:92:
contextMenu.appendItem(ContextMenuItem(CheckableActionType,
static_cast<ContextMenuAction>(ContextMenuItemBaseCustomTag + m_menuItems.size()
- 1), labelString, !menuItem->fastHasAttribute(disabledAttr),
menuItem->fastHasAttribute(checkedAttr)));
On 2014/11/28 08:17:24, tkent wrote:
> Some arguments are not simple, but duplicated with the appendItem() call in
the
> else block.  I recommend to add local variables for them.  e.g.
> 
> ContextMenuAction action = static_cast<ContextMenuAction>(...);
> bool enabled = menuItem->fastHasAttribute(disabledAttr);
> if (equalIgnoringCase(menuItem->fastGetAttribute(typeAttr), "checkbox"))
>     contextMenu.appendItem(..., action, labelString, enabled, ...)
> else
>     contextMenu.appendItem(..., action, labelString, enabled, ...)

Done.

[tkent, 2014-12-01 01:57:27]

https://codereview.chromium.org/766863002/diff/1/Source/core/page/CustomConte...
File Source/core/page/CustomContextMenuProvider.cpp (right):

https://codereview.chromium.org/766863002/diff/1/Source/core/page/CustomConte...
Source/core/page/CustomContextMenuProvider.cpp:50:
element->dispatchEvent(click.release());
On 2014/11/28 09:12:00, sanjoy_pal wrote:
> On 2014/11/28 08:17:24, tkent wrote:
> > According to the specification, we should not dispatch a click event for
> > type=checkbox.
> 
> If we don't dispatch a click, then how the onclick handler will be called for
> the menuitem? I'm not sure how the app use <menuitem type="checkbox"> in that
> case? Firefox sends click for type checkbox. Please check
> http://jsfiddle.net/oec7vv4r/2/show/ on firefox after changing menu
> type="context".

The standard says onclick handler is not called.  If you think this behavior is
weird, you need to discuss on the standard body.

[pals, 2014-12-02 06:34:56]

On 2014/12/01 01:57:27, tkent wrote:
>
https://codereview.chromium.org/766863002/diff/1/Source/core/page/CustomConte...
> File Source/core/page/CustomContextMenuProvider.cpp (right):
> 
>
https://codereview.chromium.org/766863002/diff/1/Source/core/page/CustomConte...
> Source/core/page/CustomContextMenuProvider.cpp:50:
> element->dispatchEvent(click.release());
> On 2014/11/28 09:12:00, sanjoy_pal wrote:
> > On 2014/11/28 08:17:24, tkent wrote:
> > > According to the specification, we should not dispatch a click event for
> > > type=checkbox.
> > 
> > If we don't dispatch a click, then how the onclick handler will be called
for
> > the menuitem? I'm not sure how the app use <menuitem type="checkbox"> in
that
> > case? Firefox sends click for type checkbox. Please check
> > http://jsfiddle.net/oec7vv4r/2/show/ on firefox after changing menu
> > type="context".
> 
> The standard says onclick handler is not called.  If you think this behavior
is
> weird, you need to discuss on the standard body.

"If the type attribute is in the Checkbox state
     If the element has a checked attribute, the UA must remove that attribute.
Otherwise, the UA must add a checked attribute, with the literal value
"checked"."

The above step here is actually post-activation-behavior. When a user agent is
to run post-click activation steps on an element, it must run the activation
behavior defined for that element, if any. 
https://html.spec.whatwg.org/multipage/interaction.html#activation-behavior

Below is the whatwg discussion link.
http://lists.w3.org/Archives/Public/public-whatwg-archive/2014Dec/0001.html

[tkent, 2014-12-03 00:18:21]

Thank you for the clarification.  So, the code change looks ok.
What about testing?

[pals, 2014-12-04 11:24:48]

On 2014/12/03 00:18:21, tkent wrote:
> Thank you for the clarification.  So, the code change looks ok.
> What about testing?

Added tests to verify activation behavior of context menu. PTAL.

[tkent, 2014-12-04 12:56:15]

Should HTMLMenuItemElement do activation behavior by click() IDL function?

[pals, 2014-12-04 14:19:25]

On 2014/12/04 12:56:15, tkent wrote:
> Should HTMLMenuItemElement do activation behavior by click() IDL function?

Do you mean element.click()?

[tkent, 2014-12-05 00:58:12]

On 2014/12/04 14:19:25, sanjoy_pal wrote:
> On 2014/12/04 12:56:15, tkent wrote:
> > Should HTMLMenuItemElement do activation behavior by click() IDL function?
> 
> Do you mean element.click()?

Right.  If so, CustomContextMenuProvider::contextMenuItemSelected should just
call element->click(), and HTMLMenuItem should implement the logic.

[pals, 2014-12-05 07:00:16]

On 2014/12/05 00:58:12, tkent wrote:
> On 2014/12/04 14:19:25, sanjoy_pal wrote:
> > On 2014/12/04 12:56:15, tkent wrote:
> > > Should HTMLMenuItemElement do activation behavior by click() IDL function?
> > 
> > Do you mean element.click()?
> 
> Right.  If so, CustomContextMenuProvider::contextMenuItemSelected should just
> call element->click(), and HTMLMenuItem should implement the logic.

As per specification
https://html.spec.whatwg.org/multipage/forms.html#menu-item-construct
the  relatedTarget attribute of that click event must be initialised to subject
(the element with contextmenu attribute), I am using Simulated click to do
click->setRelatedTarget(m_subjectElement.get());.

Moved the post activation behavior to HTMLMenuItemElement as suggested by you.

[tkent, 2014-12-05 07:33:07]

On 2014/12/05 07:00:16, sanjoy_pal wrote:
> > Right.  If so, CustomContextMenuProvider::contextMenuItemSelected should
just
> > call element->click(), and HTMLMenuItem should implement the logic.
> 
> As per specification
> https://html.spec.whatwg.org/multipage/forms.html#menu-item-construct
> the  relatedTarget attribute of that click event must be initialised to
subject
> (the element with contextmenu attribute), I am using Simulated click to do
> click->setRelatedTarget(m_subjectElement.get());.

I'm NOT asking about click() on the subject element.

<menu type=popup>
  <menuitem id=mi1 type=checkbox>
</menu>

document.getElementById('mi1').click();
// Should 'checked' attribute be added, or not?

[pals, 2014-12-05 08:56:07]

On 2014/12/05 07:33:07, tkent wrote:
> On 2014/12/05 07:00:16, sanjoy_pal wrote:
> > > Right.  If so, CustomContextMenuProvider::contextMenuItemSelected should
> just
> > > call element->click(), and HTMLMenuItem should implement the logic.
> > 
> > As per specification
> > https://html.spec.whatwg.org/multipage/forms.html#menu-item-construct
> > the  relatedTarget attribute of that click event must be initialised to
> subject
> > (the element with contextmenu attribute), I am using Simulated click to do
> > click->setRelatedTarget(m_subjectElement.get());.
> 
> I'm NOT asking about click() on the subject element.

I am referring to this part of code, where i set the relatedTarget of the click
event which is fired on HTMLMenuItemElement.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
 
> 
> <menu type=popup>
>   <menuitem id=mi1 type=checkbox>
> </menu>
> 
> document.getElementById('mi1').click();
> // Should 'checked' attribute be added, or not?

The 'checked' attribute should be added and this is happening with current
patchset.

[tkent, 2014-12-05 08:59:04]

On 2014/12/05 08:56:07, sanjoy_pal wrote:
> The 'checked' attribute should be added and this is happening with current
> patchset.

ok, please add a layout test for click().

[pals, 2014-12-05 09:40:35]

On 2014/12/05 08:59:04, tkent wrote:
> On 2014/12/05 08:56:07, sanjoy_pal wrote:
> > The 'checked' attribute should be added and this is happening with current
> > patchset.
> 
> ok, please add a layout test for click().

Added a layout test. Please review.

[tkent, 2014-12-05 10:07:21]

The CQ bit was checked by tkent@chromium.org

[tkent, 2014-12-05 10:07:22]

lgtm

[commit-bot: I haz the power, 2014-12-05 10:07:50]

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

[commit-bot: I haz the power, 2014-12-05 10:17:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-05 10:17:12]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/3...)

[pals, 2014-12-05 10:19:36]

The CQ bit was checked by sanjoy.pal@samsung.com

[commit-bot: I haz the power, 2014-12-05 10:19:55]

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

[commit-bot: I haz the power, 2014-12-05 11:35:30]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186584