Implement type=radio and radiogroup attribute for menuitem.

Implement type=radio and radiogroup attribute for menuitem.

The radiogroup attribute gives the name of the group of commands that will be toggled when the command itself is toggled, for commands whose type attribute has the value "radio". The scope of the name is the child list of the parent element. The attribute must be omitted unless the type attribute is in the Radio state. When specified, the attribute's value must be a non-empty string.

If the type attribute is in the Radio state
If the element has a parent, then the UA must walk the list of child nodes of that parent element, and for each node that is a menuitem element, if that element has a radiogroup attribute whose value exactly matches the current element's (treating missing radiogroup attributes as if they were the empty string), and has a checked attribute, must remove that attribute.

Then, the element's checked attribute must be set to the literal value "checked".

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

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

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

Patchset #2 (id:20001) has been deleted

[pals, 2014-12-09 11:40:10]

Patchset #2 (id:40001) has been deleted

[pals, 2014-12-09 11:44:01]

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

[pals, 2014-12-09 11:44:29]

Please review.

[tkent, 2014-12-09 12:43:43]

https://codereview.chromium.org/786753003/diff/60001/LayoutTests/fast/dom/HTM...
File LayoutTests/fast/dom/HTMLMenuElement/custom-context-menu.html (right):

https://codereview.chromium.org/786753003/diff/60001/LayoutTests/fast/dom/HTM...
LayoutTests/fast/dom/HTMLMenuElement/custom-context-menu.html:200: </script>
Needs more test scenarios.

* Clicking a menuitem which already has checked attribute.

* Case matching
   <menuitem type=radio radiogroup="g1">
   <menuitem type=radio radiogroup="G1">

* Whitespace matching
   <menuitem type=radio radiogroup="g1">
   <menuitem type=radio radiogroup=" g1 ">

* Different nesting level
   <menuitem type=radio radiogroup="g1">
   <menu>
      <menuitem type=radio radiogroup="g1">
      ...

* Empty radiogroup
    <menuitem type=radio radiogroup="">
    <menuitem type=radio>

* Type matching
   <menuitem type=radio radiogroup="g1">
   <menuitem type=checkbox radiogroup="g1">

https://codereview.chromium.org/786753003/diff/60001/Source/core/html/HTMLMen...
File Source/core/html/HTMLMenuItemElement.cpp (right):

https://codereview.chromium.org/786753003/diff/60001/Source/core/html/HTMLMen...
Source/core/html/HTMLMenuItemElement.cpp:34: if
(isHTMLMenuItemElement(*nextElement) &&
nextElement->fastHasAttribute(checkedAttr) &&
equalIgnoringCase(nextElement->fastGetAttribute(radiogroupAttr), group)) {
Why equalsIgnoringCase?  The specification says "exactly matches"

[tkent, 2014-12-09 13:02:26]

https://codereview.chromium.org/786753003/diff/60001/Source/core/html/HTMLMen...
File Source/core/html/HTMLMenuItemElement.cpp (right):

https://codereview.chromium.org/786753003/diff/60001/Source/core/html/HTMLMen...
Source/core/html/HTMLMenuItemElement.cpp:33: while (nextElement) {
for (HTMLMenuItemElement& menuItem :
Traversal<HTMLMenuItemElement>::childrenOf(*parent)) {
    ....
}

[tkent, 2014-12-09 13:12:09]

https://codereview.chromium.org/786753003/diff/60001/LayoutTests/fast/dom/HTM...
File LayoutTests/fast/dom/HTMLMenuElement/custom-context-menu.html (right):

https://codereview.chromium.org/786753003/diff/60001/LayoutTests/fast/dom/HTM...
LayoutTests/fast/dom/HTMLMenuElement/custom-context-menu.html:200: </script>
On 2014/12/09 12:43:43, tkent wrote:
> Needs more test scenarios.
> 
> * Clicking a menuitem which already has checked attribute.
> 
> * Case matching
>    <menuitem type=radio radiogroup="g1">
>    <menuitem type=radio radiogroup="G1">
> 
> * Whitespace matching
>    <menuitem type=radio radiogroup="g1">
>    <menuitem type=radio radiogroup=" g1 ">
> 
> * Different nesting level
>    <menuitem type=radio radiogroup="g1">
>    <menu>
>       <menuitem type=radio radiogroup="g1">
>       ...
> 
> * Empty radiogroup
>     <menuitem type=radio radiogroup="">
>     <menuitem type=radio>
> 
> * Type matching
>    <menuitem type=radio radiogroup="g1">
>    <menuitem type=checkbox radiogroup="g1">
> 

oops, they should be in menuitem-click.html, not in this file.

[pals, 2014-12-09 14:48:49]

On 2014/12/09 12:43:43, tkent wrote:
>
https://codereview.chromium.org/786753003/diff/60001/LayoutTests/fast/dom/HTM...
> File LayoutTests/fast/dom/HTMLMenuElement/custom-context-menu.html (right):
> 
>
https://codereview.chromium.org/786753003/diff/60001/LayoutTests/fast/dom/HTM...
> LayoutTests/fast/dom/HTMLMenuElement/custom-context-menu.html:200: </script>
> Needs more test scenarios.
> 
> * Clicking a menuitem which already has checked attribute.
> 
> * Case matching
>    <menuitem type=radio radiogroup="g1">
>    <menuitem type=radio radiogroup="G1">
> 
> * Whitespace matching
>    <menuitem type=radio radiogroup="g1">
>    <menuitem type=radio radiogroup=" g1 ">
> 
> * Different nesting level
>    <menuitem id=mi1 type=radio radiogroup="g1" checked>
>    <menu>
>       <menuitem id=mi2 type=radio radiogroup="g1">
>       ...
<menu>
<menuitem id=mi1 type=radio radiogroup="g1" checked>
    <menu>
       <menuitem id=mi2 type=radio radiogroup="g1">

I am not sure of the behavior in this case. As per the specification we should
traverse through the children of the parent. In the above example, if mi2 is
clicked, mi2 gets checked. But, if mi2 is checked and mi1 clicked then we might
not be able to match radiogroup of mi1 and mi2. Probably I should ask this to
whatwg or am I missing something here.

> 
> * Empty radiogroup
>     <menuitem type=radio radiogroup="">
>     <menuitem type=radio>
> 
> * Type matching
>    <menuitem type=radio radiogroup="g1">
>    <menuitem type=checkbox radiogroup="g1">
> 
>
https://codereview.chromium.org/786753003/diff/60001/Source/core/html/HTMLMen...
> File Source/core/html/HTMLMenuItemElement.cpp (right):
> 
>
https://codereview.chromium.org/786753003/diff/60001/Source/core/html/HTMLMen...
> Source/core/html/HTMLMenuItemElement.cpp:34: if
> (isHTMLMenuItemElement(*nextElement) &&
> nextElement->fastHasAttribute(checkedAttr) &&
> equalIgnoringCase(nextElement->fastGetAttribute(radiogroupAttr), group)) {
> Why equalsIgnoringCase?  The specification says "exactly matches"

[tkent, 2014-12-09 21:26:17]

On 2014/12/09 14:48:49, sanjoy_pal wrote:
> <menu>
> <menuitem id=mi1 type=radio radiogroup="g1" checked>
>     <menu>
>        <menuitem id=mi2 type=radio radiogroup="g1">
> 
> I am not sure of the behavior in this case. As per the specification we should
> traverse through the children of the parent. In the above example, if mi2 is
> clicked, mi2 gets checked. But, if mi2 is checked and mi1 clicked then we
might
> not be able to match radiogroup of mi1 and mi2. Probably I should ask this to
> whatwg or am I missing something here.

The specification is clear about this.  mi1 and mi2 should be in different
groups, and they are unrelated.


* Type matching
   <menuitem type=radio radiogroup="g1">
   <menuitem type=checkbox radiogroup="g1" checked>

If we follow the specification, clicking the first menuitem should remove
|checked| attribute of the second menuitem though its type is checkbox.  This
might be a specification bug.

[pals, 2014-12-10 06:24:33]

On 2014/12/09 21:26:17, tkent wrote:
> On 2014/12/09 14:48:49, sanjoy_pal wrote:
> > <menu>
> > <menuitem id=mi1 type=radio radiogroup="g1" checked>
> >     <menu>
> >        <menuitem id=mi2 type=radio radiogroup="g1">
> > 
> > I am not sure of the behavior in this case. As per the specification we
should
> > traverse through the children of the parent. In the above example, if mi2 is
> > clicked, mi2 gets checked. But, if mi2 is checked and mi1 clicked then we
> might
> > not be able to match radiogroup of mi1 and mi2. Probably I should ask this
to
> > whatwg or am I missing something here.
> 
> The specification is clear about this.  mi1 and mi2 should be in different
> groups, and they are unrelated.
> 
> 
> * Type matching
>    <menuitem type=radio radiogroup="g1">
>    <menuitem type=checkbox radiogroup="g1" checked>
> 
> If we follow the specification, clicking the first menuitem should remove
> |checked| attribute of the second menuitem though its type is checkbox.  This
> might be a specification bug.

Ya. Added a check for that. 
Please take another look.

[tkent, 2014-12-10 06:50:49]

Please follow the specification.

https://codereview.chromium.org/786753003/diff/80001/Source/core/html/HTMLMen...
File Source/core/html/HTMLMenuItemElement.cpp (right):

https://codereview.chromium.org/786753003/diff/80001/Source/core/html/HTMLMen...
Source/core/html/HTMLMenuItemElement.cpp:31: String group =
fastHasAttribute(radiogroupAttr) ? fastGetAttribute(radiogroupAttr) : "";
This code searches the element for radiogroupAttr twice.
The return value should be |const AtomicString&|.

We should do just:

const AtomicString& group = fastGetAttribute(radiogroupAttr);

|group| is a null AtomicString if the element has no radiogroup attribute.

https://codereview.chromium.org/786753003/diff/80001/Source/core/html/HTMLMen...
Source/core/html/HTMLMenuItemElement.cpp:33: if
(equalIgnoringCase(menuItem.fastGetAttribute(typeAttr), "radio") &&
menuItem.fastHasAttribute(checkedAttr)) {
Don't check type=radio here.  The specification doesn't ask to check if
type=radio.

We prefer early continue.
    if (!menuItem.fastHasAttribute(checkedAttr))
        continue;

https://codereview.chromium.org/786753003/diff/80001/Source/core/html/HTMLMen...
Source/core/html/HTMLMenuItemElement.cpp:34: String groupAttr =
menuItem.fastHasAttribute(radiogroupAttr) ?
menuItem.fastGetAttribute(radiogroupAttr) : "";
Don't need to call fastHasAttribute like line. 31.

https://codereview.chromium.org/786753003/diff/80001/Source/core/html/HTMLMen...
Source/core/html/HTMLMenuItemElement.cpp:35: if (groupAttr.stripWhiteSpace() ==
group.stripWhiteSpace())
Don't use stripWhiteSpace().  The specification doesn't ask to do so.

equalIgnoringNullity(group.impl(), groupAttr.impl())

should be enough.

[pals, 2014-12-10 07:11:46]

Updated the code as per specification.

https://codereview.chromium.org/786753003/diff/80001/Source/core/html/HTMLMen...
File Source/core/html/HTMLMenuItemElement.cpp (right):

https://codereview.chromium.org/786753003/diff/80001/Source/core/html/HTMLMen...
Source/core/html/HTMLMenuItemElement.cpp:31: String group =
fastHasAttribute(radiogroupAttr) ? fastGetAttribute(radiogroupAttr) : "";
On 2014/12/10 06:50:49, tkent wrote:
> This code searches the element for radiogroupAttr twice.
> The return value should be |const AtomicString&|.
> 
> We should do just:
> 
> const AtomicString& group = fastGetAttribute(radiogroupAttr);
> 
> |group| is a null AtomicString if the element has no radiogroup attribute.

Done.

https://codereview.chromium.org/786753003/diff/80001/Source/core/html/HTMLMen...
Source/core/html/HTMLMenuItemElement.cpp:33: if
(equalIgnoringCase(menuItem.fastGetAttribute(typeAttr), "radio") &&
menuItem.fastHasAttribute(checkedAttr)) {
On 2014/12/10 06:50:49, tkent wrote:
> Don't check type=radio here.  The specification doesn't ask to check if
> type=radio.

But, this looks like a specification bug. I shall check with specification.

> 
> We prefer early continue.
>     if (!menuItem.fastHasAttribute(checkedAttr))
>         continue;

Done.

https://codereview.chromium.org/786753003/diff/80001/Source/core/html/HTMLMen...
Source/core/html/HTMLMenuItemElement.cpp:34: String groupAttr =
menuItem.fastHasAttribute(radiogroupAttr) ?
menuItem.fastGetAttribute(radiogroupAttr) : "";
On 2014/12/10 06:50:49, tkent wrote:
> Don't need to call fastHasAttribute like line. 31.
> 

Done.

https://codereview.chromium.org/786753003/diff/80001/Source/core/html/HTMLMen...
Source/core/html/HTMLMenuItemElement.cpp:35: if (groupAttr.stripWhiteSpace() ==
group.stripWhiteSpace())
On 2014/12/10 06:50:49, tkent wrote:
> Don't use stripWhiteSpace().  The specification doesn't ask to do so.
> 
> equalIgnoringNullity(group.impl(), groupAttr.impl())
> 
> should be enough.

Done.

[tkent, 2014-12-10 07:14:55]

The CQ bit was checked by tkent@chromium.org

[tkent, 2014-12-10 07:14:55]

lgtm

[commit-bot: I haz the power, 2014-12-10 07:16:18]

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

[commit-bot: I haz the power, 2014-12-10 08:21:44]

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

[commit-bot: I haz the power, 2014-12-10 08:21:45]

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-10 09:04:23]

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

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

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

[commit-bot: I haz the power, 2014-12-10 10:32:30]

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

[commit-bot: I haz the power, 2014-12-10 10:32:31]

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-10 12:33:48]

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

[commit-bot: I haz the power, 2014-12-10 12:33:59]

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

[pals, 2014-12-10 12:34:11]

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

[pals, 2014-12-10 13:14:14]

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

[commit-bot: I haz the power, 2014-12-10 13:15:18]

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

[commit-bot: I haz the power, 2014-12-10 14:31:14]

Committed patchset #5 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186848