Input type in radio state with menu as parent should be exposed similar to ARIA role menuitemradio

Input type in radio state with menu as parent should be exposed similar to ARIA role menuitemradio

For input type attribute in radio state with menu as parent is '?'.
A '?' in a cell indicates the data has yet to be provided.

ARIA role menuitemradio: A checkable menuitem in a set of elements with role menuitemradio,
only one of which can be checked at a time.

The behavior of input type in radio state with menu as parent seems similar with ARIA role menuitemradio.

A spec bug has been filed for this https://www.w3.org/Bugs/Public/show_bug.cgi?id=27041

This CL depends on https://codereview.chromium.org/651893002/

This CL also depends on https://codereview.chromium.org/634533002/ for fix related to android.view.MenuItem

BUG=422879
Committed: https://crrev.com/0ca4b4577d582d392920e225f76c28ec50691cbf
Cr-Commit-Position: refs/heads/master@{#309438}

[shreeramk, 2014-10-15 14:51:38]

shreeram.k@samsung.com changed reviewers:
+ dmazzoni@chromium.org, mkwst@chromium.org

[shreeramk, 2014-10-15 14:51:39]

Please review.

Thanks :)

[dmazzoni, 2014-10-15 23:25:15]

https://codereview.chromium.org/652103002/diff/80001/content/test/data/access...
File content/test/data/accessibility/input-radio-expected-win.txt (right):

https://codereview.chromium.org/652103002/diff/80001/content/test/data/access...
content/test/data/accessibility/input-radio-expected-win.txt:3:
ROLE_SYSTEM_RADIOBUTTON MIXED FOCUSABLE IA2_STATE_CHECKABLE xml-roles:radio
checkable:true
Why is this mixed?

https://codereview.chromium.org/652103002/diff/80001/content/test/data/access...
File content/test/data/accessibility/input-radio-in-menu-expected-win.txt
(right):

https://codereview.chromium.org/652103002/diff/80001/content/test/data/access...
content/test/data/accessibility/input-radio-in-menu-expected-win.txt:3:
IA2_ROLE_RADIO_MENU_ITEM MIXED FOCUSABLE checkable:true
Same - why mixed?

[shreeramk, 2014-10-16 05:24:39]

It doesn't happening in chrome 38. It seems to be a regression of some CL.


On 2014/10/15 23:25:15, dmazzoni wrote:
>
https://codereview.chromium.org/652103002/diff/80001/content/test/data/access...
> File content/test/data/accessibility/input-radio-expected-win.txt (right):
> 
>
https://codereview.chromium.org/652103002/diff/80001/content/test/data/access...
> content/test/data/accessibility/input-radio-expected-win.txt:3:
> ROLE_SYSTEM_RADIOBUTTON MIXED FOCUSABLE IA2_STATE_CHECKABLE xml-roles:radio
> checkable:true
> Why is this mixed?
> 
>
https://codereview.chromium.org/652103002/diff/80001/content/test/data/access...
> File content/test/data/accessibility/input-radio-in-menu-expected-win.txt
> (right):
> 
>
https://codereview.chromium.org/652103002/diff/80001/content/test/data/access...
> content/test/data/accessibility/input-radio-in-menu-expected-win.txt:3:
> IA2_ROLE_RADIO_MENU_ITEM MIXED FOCUSABLE checkable:true
> Same - why mixed?

[dmazzoni, 2014-10-16 06:15:45]

It doesn't make sense - can you debug and figure out where the MIXED is coming
from?

[shreeramk, 2014-10-16 06:41:21]

On 2014/10/16 06:15:45, dmazzoni wrote:
> It doesn't make sense - can you debug and figure out where the MIXED is coming
> from?

Yes. I have given a debug build. I ll try to find out.

[shreeramk, 2014-10-27 16:06:51]

I ll update this issue once I ll be back to office. Sorry for the delay. I am
OOO since 19th Oct'14

[shreeramk, 2014-11-05 13:50:53]

On 2014/10/27 16:06:51, shreeramk wrote:
> I ll update this issue once I ll be back to office. Sorry for the delay. I am
> OOO since 19th Oct'14

The change was introduced between Win-292908-chrome-win32 and
Win-292909-chrome-win32.

Win-292908-chrome-win32
{"v8_revision": 23571, "chromium_revision": 292908, "webkit_revision": 181213}

Win-292909-chrome-win32
{"v8_revision": 23571, "chromium_revision": 292909, "webkit_revision": 181229}

I downloaded the versions from here
http://commondatastorage.googleapis.com/chromium-browser-snapshots/index.html...
and
http://commondatastorage.googleapis.com/chromium-browser-snapshots/index.html...

Could you please how to see what all CLs went between these two versions??

[dmazzoni, 2014-11-05 16:37:46]

On 2014/11/05 13:50:53, shreeramk wrote:
> On 2014/10/27 16:06:51, shreeramk wrote:
> > I ll update this issue once I ll be back to office. Sorry for the delay. I
am
> > OOO since 19th Oct'14
> 
> The change was introduced between Win-292908-chrome-win32 and
> Win-292909-chrome-win32.
> 
> Win-292908-chrome-win32
> {"v8_revision": 23571, "chromium_revision": 292908, "webkit_revision": 181213}
> 
> Win-292909-chrome-win32
> {"v8_revision": 23571, "chromium_revision": 292909, "webkit_revision": 181229}
> 
> I downloaded the versions from here
>
http://commondatastorage.googleapis.com/chromium-browser-snapshots/index.html...
> and
>
http://commondatastorage.googleapis.com/chromium-browser-snapshots/index.html...
> 
> Could you please how to see what all CLs went between these two versions??

Here's the URL that shows you WebKit/Blink changes between 181213 and 181229:

http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog_blink.html?u...

You can find lots of useful urls for things like this in tools/bisect-builds.py

[shreeramk, 2014-11-05 18:19:27]

On 2014/11/05 16:37:46, dmazzoni wrote:
> On 2014/11/05 13:50:53, shreeramk wrote:
> > On 2014/10/27 16:06:51, shreeramk wrote:
> > > I ll update this issue once I ll be back to office. Sorry for the delay. I
> am
> > > OOO since 19th Oct'14
> > 
> > The change was introduced between Win-292908-chrome-win32 and
> > Win-292909-chrome-win32.
> > 
> > Win-292908-chrome-win32
> > {"v8_revision": 23571, "chromium_revision": 292908, "webkit_revision":
181213}
> > 
> > Win-292909-chrome-win32
> > {"v8_revision": 23571, "chromium_revision": 292909, "webkit_revision":
181229}
> > 
> > I downloaded the versions from here
> >
>
http://commondatastorage.googleapis.com/chromium-browser-snapshots/index.html...
> > and
> >
>
http://commondatastorage.googleapis.com/chromium-browser-snapshots/index.html...
> > 
> > Could you please how to see what all CLs went between these two versions??
> 
> Here's the URL that shows you WebKit/Blink changes between 181213 and 181229:
> 
>
http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog_blink.html?u...
> 
> You can find lots of useful urls for things like this in
tools/bisect-builds.py


I checked the cls between those two revisions. Only one change in blink side
related to input type in radio button
https://codereview.chromium.org/394433002

To get all chromium side CLs between chromium revision 292908 and 292909, I need
to use this url
http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog.html Right?

But its not showing any details when I am using this 2292908:292909 in the SVN
revision range. Do I have to use the same revision range as used for
blink(181213:181229)??

[dmazzoni, 2014-11-06 07:10:51]

On Wed, Nov 5, 2014 at 10:19 AM, <shreeram.k@samsung.com> wrote:

> To get all chromium side CLs between chromium revision 292908 and 292909,
> I need
> to use this url
> http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog.html
> Right?
>
> But its not showing any details when I am using this 2292908:292909 in the
> SVN
> revision range. Do I have to use the same revision range as used for
> blink(181213:181229)??
>

SVN Is dead now, you have to use Git.

If you look in tools/bisect-builds.py, you'll see it uses this url now:

https://chromium.googlesource.com/chromium/src/+log/%s..%s?pretty=full

Alternatively, it may be easiest to just do it locally. I'd just type "git
log", then search the buffer for 292909 and read the log messages there.

- Dominic

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[shreeramk, 2014-11-06 10:08:05]

I found this CL https://codereview.chromium.org/394433002 introduced that
'MIXED' bug.

I reverted this locally/manually and verified, its not happening.

[shreeramk, 2014-11-19 18:30:58]

On 2014/11/06 10:08:05, shreeramk wrote:
> I found this CL https://codereview.chromium.org/394433002 introduced that
> 'MIXED' bug.
> 
> I reverted this locally/manually and verified, its not happening.

@Dominic: This cl is pending because the changes introduced in here
https://codereview.chromium.org/394433002 has caused the side effect which is
introducing 'MIXED'. I have filed a bug(crbug.com/431784) for the same, but
there is no reply from the owner.

[dmazzoni, 2014-11-19 18:39:05]

>
> @Dominic: This cl is pending because the changes introduced in here
> https://codereview.chromium.org/394433002 has caused the side effect
> which is
> introducing 'MIXED'. I have filed a bug(crbug.com/431784) for the same,
> but
> there is no reply from the owner.
>

Can you debug it and figure out why it broke?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[shreeramk, 2014-11-25 13:11:18]

On 2014/11/19 18:39:05, dmazzoni wrote:
> >
> > @Dominic: This cl is pending because the changes introduced in here
> > https://codereview.chromium.org/394433002 has caused the side effect
> > which is
> > introducing 'MIXED'. I have filed a bug(crbug.com/431784) for the same,
> > but
> > there is no reply from the owner.
> >
> 
> Can you debug it and figure out why it broke?
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.


The reason why this is coming MIXED is, please check below link/file from that
CL:

https://codereview.chromium.org/394433002/patch/100001/110013

If input radio is checked it directly returns the pointer hence
shouldAppearIndeterminate() returns false.

If input type radio is not checked, it returns a nullptr, hence
shouldAppearIndeterminate() returns true.

bool RadioInputType::shouldAppearIndeterminate() const
 {
-  return false;
+  return !element().checkedRadioButtonForGroup();
 }


HTMLInputElement* HTMLInputElement::checkedRadioButtonForGroup()
{
    if (checked())
        return this;
    if (RadioButtonGroupScope* scope = radioButtonGroupScope())
        return scope->checkedButtonForGroup(name());
    return nullptr;
}

[dmazzoni, 2014-12-01 07:00:08]

I agree, that change looks wrong to me.

I think you should upload a change that reverts that one line return
!element().checkedRadioButtonForGroup() back to return false - and send it
to the original author for review, and cc me. Either he'll approve the fix,
or explain why he thinks it should be that way.

- Dominic

On Tue, Nov 25, 2014 at 5:11 AM, <shreeram.k@samsung.com> wrote:

> On 2014/11/19 18:39:05, dmazzoni wrote:
>
>> >
>> > @Dominic: This cl is pending because the changes introduced in here
>> > https://codereview.chromium.org/394433002 has caused the side effect
>> > which is
>> > introducing 'MIXED'. I have filed a bug(crbug.com/431784) for the same,
>> > but
>> > there is no reply from the owner.
>> >
>>
>
>  Can you debug it and figure out why it broke?
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
> The reason why this is coming MIXED is, please check below link/file from
> that
> CL:
>
> https://codereview.chromium.org/394433002/patch/100001/110013
>
> If input radio is checked it directly returns the pointer hence
> shouldAppearIndeterminate() returns false.
>
> If input type radio is not checked, it returns a nullptr, hence
> shouldAppearIndeterminate() returns true.
>
> bool RadioInputType::shouldAppearIndeterminate() const
>  {
> -  return false;
> +  return !element().checkedRadioButtonForGroup();
>  }
>
>
> HTMLInputElement* HTMLInputElement::checkedRadioButtonForGroup()
> {
>     if (checked())
>         return this;
>     if (RadioButtonGroupScope* scope = radioButtonGroupScope())
>         return scope->checkedButtonForGroup(name());
>     return nullptr;
> }
>
> https://codereview.chromium.org/652103002/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[shreeramk, 2014-12-02 05:37:17]

I was going through spec and found this about indeterminate attribute.

"The indeterminate IDL attribute must initially be set to false. On getting, it
must return the last value it was set to. On setting, it must be set to the new
value. It has no effect except for changing the appearance of checkbox
controls."

Link:
http://www.w3.org/html/wg/drafts/html/master/forms.html#dom-input-indeterminate

AX specs also says:
"Input (type attribute in the Checkbox state) - 	checkbox role, with the
aria-checked state set to "mixed" if the element's indeterminate IDL attribute
is true, or "true" if the element's checkedness is true, or "false" otherwise"

The spec doesn't say anything about radio button with indeterminate attribute.

http://rawgit.com/w3c/aria/master/html-aam/html-aam.html

I ll submit a patch reverting that one line change.

[shreeramk, 2014-12-05 04:59:28]

@Dominic:

I m not sure how come my changes are affecting aria-required test case i.e on
radiobutton.

It was not failing till Patch set 5. Bt started failing after patchset 7.

[shreeramk, 2014-12-05 11:48:41]

On 2014/12/05 04:59:28, shreeramk wrote:
> @Dominic:
> 
> I m not sure how come my changes are affecting aria-required test case i.e on
> radiobutton.
> 
> It was not failing till Patch set 5. Bt started failing after patchset 7.

I checked on canary build its exposing IA2_STATE_REQUIRED, but don't know why on
the bots its not exposing IA2_STATE_REQUIRED.

[shreeramk, 2014-12-17 10:18:29]

@Dominic: Please take a look.
Thanks!!

https://codereview.chromium.org/652103002/diff/200001/content/test/data/acces...
File content/test/data/accessibility/aria-required.html (right):

https://codereview.chromium.org/652103002/diff/200001/content/test/data/acces...
content/test/data/accessibility/aria-required.html:8: <div role="radiogroup"
aria-required="true">
This test was not correct. aria-required is used with aria role radiogroup, not
with aria role "radio".

[shreeramk, 2014-12-17 10:20:21]

According to specs, aria-required should be used with aria role radiogroup, not
with aria role "radio".

[shreeramk, 2014-12-19 04:08:43]

On 2014/12/17 10:20:21, shreeramk wrote:
> According to specs, aria-required should be used with aria role radiogroup,
not
> with aria role "radio".

@Dominic: PTAL. Thanks!!

[shreeramk, 2014-12-22 03:33:42]

@Dominic: PTAL! Thanks.

[dmazzoni, 2014-12-22 07:59:44]

lgtm

https://codereview.chromium.org/652103002/diff/200001/content/test/data/acces...
File content/test/data/accessibility/aria-required.html (right):

https://codereview.chromium.org/652103002/diff/200001/content/test/data/acces...
content/test/data/accessibility/aria-required.html:8: <div role="radiogroup"
aria-required="true">
On 2014/12/17 10:18:29, shreeramk wrote:
> This test was not correct. aria-required is used with aria role radiogroup,
not
> with aria role "radio".

Acknowledged.

[shreeramk, 2014-12-22 08:23:13]

The CQ bit was checked by shreeram.k@samsung.com

[commit-bot: I haz the power, 2014-12-22 08:23:34]

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

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

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

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

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[shreeramk, 2014-12-22 10:20:29]

The CQ bit was checked by shreeram.k@samsung.com

[commit-bot: I haz the power, 2014-12-22 10:21:06]

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

[commit-bot: I haz the power, 2014-12-22 12:23:56]

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

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

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[shreeramk, 2014-12-22 16:53:20]

The CQ bit was checked by shreeram.k@samsung.com

[commit-bot: I haz the power, 2014-12-22 16:53:59]

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

[commit-bot: I haz the power, 2014-12-22 17:50:16]

Committed patchset #13 (id:240001)

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

Patchset 13 (id:??) landed as
https://crrev.com/0ca4b4577d582d392920e225f76c28ec50691cbf
Cr-Commit-Position: refs/heads/master@{#309438}