Fixing IA2 role for input type attribute in button and checkbox state with menu and aria role menu …

Fixing IA2 role for input type attribute in button and checkbox state with menu and aria role menu as parent

This chromium side CL depends on following blink side CLs
https://codereview.chromium.org/589383004/
https://codereview.chromium.org/629173002/
https://codereview.chromium.org/641053003/
https://codereview.chromium.org/649073002/

BUG=169574
Committed: https://crrev.com/f1b5973d881211bbe02f67dbaad419829718a41e
Cr-Commit-Position: refs/heads/master@{#299675}

[shreeramk, 2014-10-08 17:53:33]

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

[shreeramk, 2014-10-08 17:53:34]

For input type attribute in button state with menu as parent- MenuItemRole is
send from blink side.

Corresponding chromium side code is
case ui::AX_ROLE_MENU_ITEM:
      ia_role_ = ROLE_SYSTEM_MENUITEM;
      break;

For input type attribute in checkbox state with menu as parent,
CheckBoxMenuItemRole is sent from blink side.

Corresponding chromium side code is
case ui::AX_ROLE_CHECK_BOX_MENU_ITEM:
      ia_role_ = ROLE_SYSTEM_CHECKBUTTON;
      ia2_role_ = IA2_ROLE_CHECK_MENU_ITEM;
      ia2_state_ |= IA2_STATE_CHECKABLE;
      break;

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

https://codereview.chromium.org/634533002/diff/80001/content/test/data/access...
content/test/data/accessibility/input-button-in-menu-expected-win.txt:6:
ROLE_SYSTEM_MENUITEM name='Button in element with menu role' FOCUSABLE
Similarly here as well.

Instead of exposing as ROLE_SYSTEM_MENUITEM, win_chromium_rel is exposing it as
ROLE_SYSTEM_PUSHBUTTON.

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

https://codereview.chromium.org/634533002/diff/80001/content/test/data/access...
content/test/data/accessibility/input-checkbox-in-menu-expected-win.txt:5:
IA2_ROLE_CHECK_MENU_ITEM FOCUSABLE IA2_STATE_CHECKABLE checkable:true
I am not able to figure out the reason that why the win_chromium_rel bot is
giving error.

Instead of IA2_ROLE_CHECK_MENU_ITEM, its giving
ROLE_SYSTEM_CHECKBUTTON(ia_role).

Any inputs??

[shreeramk, 2014-10-08 17:55:15]

On 2014/10/08 17:53:34, shreeram.k wrote:
> For input type attribute in button state with menu as parent- MenuItemRole is
> send from blink side.
> 
> Corresponding chromium side code is
> case ui::AX_ROLE_MENU_ITEM:
>       ia_role_ = ROLE_SYSTEM_MENUITEM;
>       break;
> 
> For input type attribute in checkbox state with menu as parent,
> CheckBoxMenuItemRole is sent from blink side.
> 
> Corresponding chromium side code is
> case ui::AX_ROLE_CHECK_BOX_MENU_ITEM:
>       ia_role_ = ROLE_SYSTEM_CHECKBUTTON;
>       ia2_role_ = IA2_ROLE_CHECK_MENU_ITEM;
>       ia2_state_ |= IA2_STATE_CHECKABLE;
>       break;
> 
>
https://codereview.chromium.org/634533002/diff/80001/content/test/data/access...
> File content/test/data/accessibility/input-button-in-menu-expected-win.txt
> (left):
> 
>
https://codereview.chromium.org/634533002/diff/80001/content/test/data/access...
> content/test/data/accessibility/input-button-in-menu-expected-win.txt:6:
> ROLE_SYSTEM_MENUITEM name='Button in element with menu role' FOCUSABLE
> Similarly here as well.
> 
> Instead of exposing as ROLE_SYSTEM_MENUITEM, win_chromium_rel is exposing it
as
> ROLE_SYSTEM_PUSHBUTTON.
> 
>
https://codereview.chromium.org/634533002/diff/80001/content/test/data/access...
> File content/test/data/accessibility/input-checkbox-in-menu-expected-win.txt
> (right):
> 
>
https://codereview.chromium.org/634533002/diff/80001/content/test/data/access...
> content/test/data/accessibility/input-checkbox-in-menu-expected-win.txt:5:
> IA2_ROLE_CHECK_MENU_ITEM FOCUSABLE IA2_STATE_CHECKABLE checkable:true
> I am not able to figure out the reason that why the win_chromium_rel bot is
> giving error.
> 
> Instead of IA2_ROLE_CHECK_MENU_ITEM, its giving
> ROLE_SYSTEM_CHECKBUTTON(ia_role).
> 
> Any inputs??

This is working fine on my windows local system. I am getting this error only on
try bot.

[dmazzoni, 2014-10-08 18:55:59]

I have no idea why you'd be getting different results locally. I added a
bunch of other windows trybots to see if there was an anomaly with the
trybots or locally.

On Wed, Oct 8, 2014 at 10:55 AM, <shreeram.k@samsung.com> wrote:

> On 2014/10/08 17:53:34, shreeram.k wrote:
>
>> For input type attribute in button state with menu as parent-
>> MenuItemRole is
>> send from blink side.
>>
>
>  Corresponding chromium side code is
>> case ui::AX_ROLE_MENU_ITEM:
>>        ia_role_ = ROLE_SYSTEM_MENUITEM;
>>        break;
>>
>
>  For input type attribute in checkbox state with menu as parent,
>> CheckBoxMenuItemRole is sent from blink side.
>>
>
>  Corresponding chromium side code is
>> case ui::AX_ROLE_CHECK_BOX_MENU_ITEM:
>>        ia_role_ = ROLE_SYSTEM_CHECKBUTTON;
>>        ia2_role_ = IA2_ROLE_CHECK_MENU_ITEM;
>>        ia2_state_ |= IA2_STATE_CHECKABLE;
>>        break;
>>
>
>
> https://codereview.chromium.org/634533002/diff/80001/content/test/data/
> accessibility/input-button-in-menu-expected-win.txt
>
>> File content/test/data/accessibility/input-button-in-
>> menu-expected-win.txt
>> (left):
>>
>
>
> https://codereview.chromium.org/634533002/diff/80001/content/test/data/
> accessibility/input-button-in-menu-expected-win.txt#oldcode6
>
>> content/test/data/accessibility/input-button-in-menu-expected-win.txt:6:
>> ROLE_SYSTEM_MENUITEM name='Button in element with menu role' FOCUSABLE
>> Similarly here as well.
>>
>
>  Instead of exposing as ROLE_SYSTEM_MENUITEM, win_chromium_rel is exposing
>> it
>>
> as
>
>> ROLE_SYSTEM_PUSHBUTTON.
>>
>
>
> https://codereview.chromium.org/634533002/diff/80001/content/test/data/
> accessibility/input-checkbox-in-menu-expected-win.txt
>
>> File content/test/data/accessibility/input-checkbox-
>> in-menu-expected-win.txt
>> (right):
>>
>
>
> https://codereview.chromium.org/634533002/diff/80001/content/test/data/
> accessibility/input-checkbox-in-menu-expected-win.txt#newcode5
>
>> content/test/data/accessibility/input-checkbox-
>> in-menu-expected-win.txt:5:
>> IA2_ROLE_CHECK_MENU_ITEM FOCUSABLE IA2_STATE_CHECKABLE checkable:true
>> I am not able to figure out the reason that why the win_chromium_rel bot
>> is
>> giving error.
>>
>
>  Instead of IA2_ROLE_CHECK_MENU_ITEM, its giving
>> ROLE_SYSTEM_CHECKBUTTON(ia_role).
>>
>
>  Any inputs??
>>
>
> This is working fine on my windows local system. I am getting this error
> only on
> try bot.
>
> https://codereview.chromium.org/634533002/
>
> 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.

[dmazzoni, 2014-10-08 18:59:15]

Oh, wait - this depends on a Blink change, right? You need to wait for the Blink
change to "roll" into Chromium. It's usually no more than a day or two behind.

https://codereview.chromium.org/634533002/diff/80001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_android.cc (right):

https://codereview.chromium.org/634533002/diff/80001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_android.cc:100: GetRole() ==
ui::AX_ROLE_CHECK_BOX_MENU_ITEM ||
Any reason not to do RADIO_MENU_ITEM in the same change? They're so similar, it
seems like it'd be easier to handle both at once.

[shreeramk, 2014-10-08 19:38:24]

On 2014/10/08 18:59:15, dmazzoni wrote:
> Oh, wait - this depends on a Blink change, right? You need to wait for the
Blink
> change to "roll" into Chromium. It's usually no more than a day or two behind.
> 

Maybe that could be the reason.

https://codereview.chromium.org/634533002/diff/80001/content/browser/accessib...
> File content/browser/accessibility/browser_accessibility_android.cc (right):
> 
>
https://codereview.chromium.org/634533002/diff/80001/content/browser/accessib...
> content/browser/accessibility/browser_accessibility_android.cc:100: GetRole()
==
> ui::AX_ROLE_CHECK_BOX_MENU_ITEM ||
> Any reason not to do RADIO_MENU_ITEM in the same change? They're so similar,
it
> seems like it'd be easier to handle both at once.

[shreeramk, 2014-10-08 19:38:52]

https://codereview.chromium.org/634533002/diff/80001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_android.cc (right):

https://codereview.chromium.org/634533002/diff/80001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_android.cc:100: GetRole() ==
ui::AX_ROLE_CHECK_BOX_MENU_ITEM ||
On 2014/10/08 18:59:15, dmazzoni wrote:
> Any reason not to do RADIO_MENU_ITEM in the same change? They're so similar,
it
> seems like it'd be easier to handle both at once.

Because for radio with menu as parent, MenuItemRole is returned from blink, if I
add  AX_ROLE_MENU_ITEM, then it would be applied on all where the role is
AX_ROLE_MENU_ITEM. 

The only solution for this would be introduce a new role in blink side for radio
with menu as parent.

[dmazzoni, 2014-10-08 19:40:42]

The try run shows you exactly what Blink revision it used at the top of the
log - confirm it there.

Locally, look at DEPS and .DEPS.git to see what Blink revision you're
compiling against.

On Wed, Oct 8, 2014 at 12:38 PM, <shreeram.k@samsung.com> wrote:

> On 2014/10/08 18:59:15, dmazzoni wrote:
>
>> Oh, wait - this depends on a Blink change, right? You need to wait for the
>>
> Blink
>
>> change to "roll" into Chromium. It's usually no more than a day or two
>> behind.
>>
>
>
> Maybe that could be the reason.
>
>
> https://codereview.chromium.org/634533002/diff/80001/
> content/browser/accessibility/browser_accessibility_android.cc
>
>> File content/browser/accessibility/browser_accessibility_android.cc
>> (right):
>>
>
>
> https://codereview.chromium.org/634533002/diff/80001/
> content/browser/accessibility/browser_accessibility_android.cc#newcode100
>
>> content/browser/accessibility/browser_accessibility_android.cc:100:
>> GetRole()
>>
> ==
>
>> ui::AX_ROLE_CHECK_BOX_MENU_ITEM ||
>> Any reason not to do RADIO_MENU_ITEM in the same change? They're so
>> similar,
>>
> it
>
>> seems like it'd be easier to handle both at once.
>>
>
>
>
> https://codereview.chromium.org/634533002/
>

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

[dmazzoni, 2014-10-08 19:41:40]

https://codereview.chromium.org/634533002/diff/80001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_android.cc (right):

https://codereview.chromium.org/634533002/diff/80001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_android.cc:100: GetRole() ==
ui::AX_ROLE_CHECK_BOX_MENU_ITEM ||
On 2014/10/08 19:38:51, shreeram.k wrote:
> On 2014/10/08 18:59:15, dmazzoni wrote:
> > Any reason not to do RADIO_MENU_ITEM in the same change? They're so similar,
> it
> > seems like it'd be easier to handle both at once.
> 
> Because for radio with menu as parent, MenuItemRole is returned from blink, if
I
> add  AX_ROLE_MENU_ITEM, then it would be applied on all where the role is
> AX_ROLE_MENU_ITEM. 
> 
> The only solution for this would be introduce a new role in blink side for
radio
> with menu as parent.

Ah, OK - can you do that in a separate change?

[shreeramk, 2014-10-08 19:46:07]

On 2014/10/08 19:41:40, dmazzoni wrote:
>
https://codereview.chromium.org/634533002/diff/80001/content/browser/accessib...
> File content/browser/accessibility/browser_accessibility_android.cc (right):
> 
>
https://codereview.chromium.org/634533002/diff/80001/content/browser/accessib...
> content/browser/accessibility/browser_accessibility_android.cc:100: GetRole()
==
> ui::AX_ROLE_CHECK_BOX_MENU_ITEM ||
> On 2014/10/08 19:38:51, shreeram.k wrote:
> > On 2014/10/08 18:59:15, dmazzoni wrote:
> > > Any reason not to do RADIO_MENU_ITEM in the same change? They're so
similar,
> > it
> > > seems like it'd be easier to handle both at once.
> > 
> > Because for radio with menu as parent, MenuItemRole is returned from blink,
if
> I
> > add  AX_ROLE_MENU_ITEM, then it would be applied on all where the role is
> > AX_ROLE_MENU_ITEM. 
> > 
> > The only solution for this would be introduce a new role in blink side for
> radio
> > with menu as parent.
> 
> Ah, OK - can you do that in a separate change?

Yes I ll do that in a separate change.

[shreeramk, 2014-10-09 08:27:50]

On 2014/10/08 19:46:07, shreeram.k wrote:
> On 2014/10/08 19:41:40, dmazzoni wrote:
> >
>
https://codereview.chromium.org/634533002/diff/80001/content/browser/accessib...
> > File content/browser/accessibility/browser_accessibility_android.cc (right):
> > 
> >
>
https://codereview.chromium.org/634533002/diff/80001/content/browser/accessib...
> > content/browser/accessibility/browser_accessibility_android.cc:100:
GetRole()
> ==
> > ui::AX_ROLE_CHECK_BOX_MENU_ITEM ||
> > On 2014/10/08 19:38:51, shreeram.k wrote:
> > > On 2014/10/08 18:59:15, dmazzoni wrote:
> > > > Any reason not to do RADIO_MENU_ITEM in the same change? They're so
> similar,
> > > it
> > > > seems like it'd be easier to handle both at once.
> > > 
> > > Because for radio with menu as parent, MenuItemRole is returned from
blink,
> if
> > I
> > > add  AX_ROLE_MENU_ITEM, then it would be applied on all where the role is
> > > AX_ROLE_MENU_ITEM. 
> > > 
> > > The only solution for this would be introduce a new role in blink side for
> > radio
> > > with menu as parent.
> > 
> > Ah, OK - can you do that in a separate change?
> 
> Yes I ll do that in a separate change.

I think again I/we mistaken something. Why for button in menuitem do we need
checkable attribute?
We don't need any checkable attribute for button.

[dmazzoni, 2014-10-09 15:19:09]

On 2014/10/09 08:27:50, shreeram.k wrote:
> I think again I/we mistaken something. Why for button in menuitem do we need
> checkable attribute?
> We don't need any checkable attribute for button.

You're right, we don't. If I said otherwise I must have been mixed up and
thought I was looking at the checkbox in menu example.

[shreeramk, 2014-10-09 16:29:48]

I found out why win_* bots were failing.

Actually in this CL https://codereview.chromium.org/629173002/ I forgot to add
the condition in AXRenderObject.cpp when aria role menu is parent for input type
button and checkbox. Hence its failing for the case when aria role menu is
parent for button and checkbox

I have uploaded a blink side cl here https://codereview.chromium.org/641053003/

I am extremely sorry about this mistake.

Actually I use window system to verify my CLs. And linux system to upload them.
As due to company policy restriction I am not able to upload patch from Windows.
I have raised a request for proxy exemption to upload files(CLs) from Windows
system.

Thanks :)

[dmazzoni, 2014-10-10 05:43:21]

No problem about the mistake, but I just realized that there's a checkbox menu
item, and also a menu item checkbox. Is that on purpose?

https://codereview.chromium.org/634533002/diff/100001/chrome/common/extension...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/634533002/diff/100001/chrome/common/extension...
chrome/common/extensions/api/automation.idl:67: checkBoxMenuItem,
wait, how is this different than menuItemCheckBox?

[shreeramk, 2014-10-10 05:55:12]

On 2014/10/10 05:43:21, dmazzoni wrote:
> No problem about the mistake, but I just realized that there's a checkbox menu
> item, and also a menu item checkbox. Is that on purpose?
> 
>
https://codereview.chromium.org/634533002/diff/100001/chrome/common/extension...
> File chrome/common/extensions/api/automation.idl (right):
> 
>
https://codereview.chromium.org/634533002/diff/100001/chrome/common/extension...
> chrome/common/extensions/api/automation.idl:67: checkBoxMenuItem,
> wait, how is this different than menuItemCheckBox?

Yes its on purpose.
I introduce two roles because mac(AX) spec differs for aria role
menuitemcheckbox and input type checkbox with menu as parent.

Input type checkbox with menu as parent-

AX: ?

For aria role menuitemcheckbox:

AX: 
AXRole:AXMenuItem
AXSubRole: <nil>
AXRoleDescription:'menu item'
If aria-checked (state)="true": AXMenuItemMarkChar ✓
Otherwise: AXMenuItemMarkChar <nil>

[shreeramk, 2014-10-13 06:29:56]

Sorry try bots will fail since one of the dependent blink side CL is yet to
merge.

[dmazzoni, 2014-10-13 07:54:46]

https://codereview.chromium.org/634533002/diff/190001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/634533002/diff/190001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_win.cc:3625: case
ui::AX_ROLE_MENU_ITEM_RADIO:
A radio should be checkable too. The html spec says "?" but the ARIA spec for
menuitemradio is clear.

https://codereview.chromium.org/634533002/diff/190001/content/test/data/acces...
File content/test/data/accessibility/aria-menuitemradio-expected-android.txt
(right):

https://codereview.chromium.org/634533002/diff/190001/content/test/data/acces...
content/test/data/accessibility/aria-menuitemradio-expected-android.txt:3:
android.widget.RadioButton" checkable clickable name='Menu item 1'
Extra quote?

[shreeramk, 2014-10-13 08:31:45]

On 2014/10/13 07:54:46, dmazzoni wrote:
>
https://codereview.chromium.org/634533002/diff/190001/content/browser/accessi...
> File content/browser/accessibility/browser_accessibility_win.cc (right):
> 
>
https://codereview.chromium.org/634533002/diff/190001/content/browser/accessi...
> content/browser/accessibility/browser_accessibility_win.cc:3625: case
> ui::AX_ROLE_MENU_ITEM_RADIO:
> A radio should be checkable too. The html spec says "?" but the ARIA spec for
> menuitemradio is clear.

Yesterday I filed a spec bug for this too.
This will also require a blink side CL as to return the MenuItemRadioRole when
input type is radio and parent is menu.
So, it would be better to handle in a differnt CL.

>
https://codereview.chromium.org/634533002/diff/190001/content/test/data/acces...
> File content/test/data/accessibility/aria-menuitemradio-expected-android.txt
> (right):
> 
>
https://codereview.chromium.org/634533002/diff/190001/content/test/data/acces...
> content/test/data/accessibility/aria-menuitemradio-expected-android.txt:3:
> android.widget.RadioButton" checkable clickable name='Menu item 1'
> Extra quote?

Will fix this in the next patchset.

[shreeramk, 2014-10-13 09:05:09]

On 2014/10/13 08:31:45, shreeram.k wrote:
> On 2014/10/13 07:54:46, dmazzoni wrote:
> >
>
https://codereview.chromium.org/634533002/diff/190001/content/browser/accessi...
> > File content/browser/accessibility/browser_accessibility_win.cc (right):
> > 
> >
>
https://codereview.chromium.org/634533002/diff/190001/content/browser/accessi...
> > content/browser/accessibility/browser_accessibility_win.cc:3625: case
> > ui::AX_ROLE_MENU_ITEM_RADIO:
> > A radio should be checkable too. The html spec says "?" but the ARIA spec
for
> > menuitemradio is clear.
> 
> Yesterday I filed a spec bug for this too.
> This will also require a blink side CL as to return the MenuItemRadioRole when
> input type is radio and parent is menu.
> So, it would be better to handle in a differnt CL.

By the way, this is already "checkable" in the code.

if (ia_role_ == ROLE_SYSTEM_CHECKBUTTON ||
      ia_role_ == ROLE_SYSTEM_RADIOBUTTON ||
      ia2_role_ == IA2_ROLE_CHECK_MENU_ITEM ||
      ia2_role_ == IA2_ROLE_RADIO_MENU_ITEM ||
      ia2_role_ == IA2_ROLE_TOGGLE_BUTTON) {
    ia2_attributes_.push_back(L"checkable:true");
  }

You are talking about IA2_STATE_CHECKABLE??

[shreeramk, 2014-10-13 18:07:12]

PTAL!

I think now its working fine.


Thanks :)

[Mike West, 2014-10-14 11:17:48]

mkwst@chromium.org changed required reviewers:
+ dmazzoni@chromium.org

[Mike West, 2014-10-14 11:17:49]

Looks reasonable % nits. Dominic needs to sign off on it, however.

https://codereview.chromium.org/634533002/diff/290001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/634533002/diff/290001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_win.cc:3627: ia2_role_ =
IA2_ROLE_RADIO_MENU_ITEM;
Aren't radio buttons checkable?

https://codereview.chromium.org/634533002/diff/290001/content/test/data/acces...
File content/test/data/accessibility/input-button-in-menu-expected-android.txt
(right):

https://codereview.chromium.org/634533002/diff/290001/content/test/data/acces...
content/test/data/accessibility/input-button-in-menu-expected-android.txt:3:
android.view.View clickable focusable name='Button in menu element'
Is this accurate? "Button" seemed like a better representation.

[shreeramk, 2014-10-14 11:32:41]

https://codereview.chromium.org/634533002/diff/290001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/634533002/diff/290001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_win.cc:3627: ia2_role_ =
IA2_ROLE_RADIO_MENU_ITEM;
On 2014/10/14 11:17:48, Mike West wrote:
> Aren't radio buttons checkable?

This would be taken care in a differnt blink+chromium CL. Already blink CL for
that has been merged. But chromium side CL is pending on this CL because of
changes in browser_accessibility_android.cc.

Thats why I would like that this patch goes first since its also dependent on
many(4) CLs.

https://codereview.chromium.org/634533002/diff/290001/content/test/data/acces...
File content/test/data/accessibility/input-button-in-menu-expected-android.txt
(right):

https://codereview.chromium.org/634533002/diff/290001/content/test/data/acces...
content/test/data/accessibility/input-button-in-menu-expected-android.txt:3:
android.view.View clickable focusable name='Button in menu element'
On 2014/10/14 11:17:49, Mike West wrote:
> Is this accurate? "Button" seemed like a better representation.

For button with menu as parent, defined role is ROLE_SYSTEM_MENUITEM.

And for input type button, blink returns MenuItemRole(existing role in blink),
so for android adding that case here 

case ui::AX_ROLE_BUTTON:
case ui::AX_ROLE_MENU_BUTTON:
case ui::AX_ROLE_POP_UP_BUTTON:
    class_name = "android.widget.Button";
    break;

to get android.widget.Button will lead all menuitem types to
android.widget.Button

OR

MenuButtonRole can be used. which corresponds with AX_ROLE_MENU_BUTTON in
chromium side.

[dmazzoni, 2014-10-14 15:20:05]

lgtm

https://codereview.chromium.org/634533002/diff/290001/content/test/data/acces...
File content/test/data/accessibility/input-button-in-menu-expected-android.txt
(right):

https://codereview.chromium.org/634533002/diff/290001/content/test/data/acces...
content/test/data/accessibility/input-button-in-menu-expected-android.txt:3:
android.view.View clickable focusable name='Button in menu element'
On 2014/10/14 11:32:41, shreeram.k wrote:
> On 2014/10/14 11:17:49, Mike West wrote:
> > Is this accurate? "Button" seemed like a better representation.
> 
> For button with menu as parent, defined role is ROLE_SYSTEM_MENUITEM.
> 
> And for input type button, blink returns MenuItemRole(existing role in blink),
> so for android adding that case here 
> 
> case ui::AX_ROLE_BUTTON:
> case ui::AX_ROLE_MENU_BUTTON:
> case ui::AX_ROLE_POP_UP_BUTTON:
>     class_name = "android.widget.Button";
>     break;
> 
> to get android.widget.Button will lead all menuitem types to
> android.widget.Button
> 
> OR
> 
> MenuButtonRole can be used. which corresponds with AX_ROLE_MENU_BUTTON in
> chromium side.

You can use any class name from the Android widget hierarchy here -
http://developer.android.com/reference/android/view/MenuItem.html - so perhaps "
android.view.MenuItem"

Without testing on Android I don't know for sure what would be best, but I agree
with Mike that an interactive element should be a Button or MenuItem or some
interactive widget role, as opposed to just android.view.View.

[shreeramk, 2014-10-15 04:43:29]

On 2014/10/14 15:20:05, dmazzoni wrote:
> lgtm
> 
>
https://codereview.chromium.org/634533002/diff/290001/content/test/data/acces...
> File content/test/data/accessibility/input-button-in-menu-expected-android.txt
> (right):
> 
>
https://codereview.chromium.org/634533002/diff/290001/content/test/data/acces...
> content/test/data/accessibility/input-button-in-menu-expected-android.txt:3:
> android.view.View clickable focusable name='Button in menu element'
> On 2014/10/14 11:32:41, shreeram.k wrote:
> > On 2014/10/14 11:17:49, Mike West wrote:
> > > Is this accurate? "Button" seemed like a better representation.
> > 
> > For button with menu as parent, defined role is ROLE_SYSTEM_MENUITEM.
> > 
> > And for input type button, blink returns MenuItemRole(existing role in
blink),
> > so for android adding that case here 
> > 
> > case ui::AX_ROLE_BUTTON:
> > case ui::AX_ROLE_MENU_BUTTON:
> > case ui::AX_ROLE_POP_UP_BUTTON:
> >     class_name = "android.widget.Button";
> >     break;
> > 
> > to get android.widget.Button will lead all menuitem types to
> > android.widget.Button
> > 
> > OR
> > 
> > MenuButtonRole can be used. which corresponds with AX_ROLE_MENU_BUTTON in
> > chromium side.
> 
> You can use any class name from the Android widget hierarchy here -
> http://developer.android.com/reference/android/view/MenuItem.html - so perhaps
"
> android.view.MenuItem"
> 
> Without testing on Android I don't know for sure what would be best, but I
agree
> with Mike that an interactive element should be a Button or MenuItem or some
> interactive widget role, as opposed to just android.view.View.

Then I think for input type checkbox with menu as parent should also have
android.view.MenuItem since parent is menu hence the input type checkbox would
behave as menuitem.

Similarly for aria role menuitemcheckbox and menuitemradio.

Whats your view on this?

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

That sounds great to me - especially since Android does have a CHECKABLE
property that we're hopefully setting correctly.

On Tue, Oct 14, 2014 at 9:43 PM, <shreeram.k@samsung.com> wrote:

> On 2014/10/14 15:20:05, dmazzoni wrote:
>
>> lgtm
>>
>
>
> https://codereview.chromium.org/634533002/diff/290001/content/test/data/
> accessibility/input-button-in-menu-expected-android.txt
>
>> File content/test/data/accessibility/input-button-in-
>> menu-expected-android.txt
>> (right):
>>
>
>
> https://codereview.chromium.org/634533002/diff/290001/content/test/data/
> accessibility/input-button-in-menu-expected-android.txt#newcode3
>
>> content/test/data/accessibility/input-button-in-
>> menu-expected-android.txt:3:
>> android.view.View clickable focusable name='Button in menu element'
>> On 2014/10/14 11:32:41, shreeram.k wrote:
>> > On 2014/10/14 11:17:49, Mike West wrote:
>> > > Is this accurate? "Button" seemed like a better representation.
>> >
>> > For button with menu as parent, defined role is ROLE_SYSTEM_MENUITEM.
>> >
>> > And for input type button, blink returns MenuItemRole(existing role in
>>
> blink),
>
>> > so for android adding that case here
>> >
>> > case ui::AX_ROLE_BUTTON:
>> > case ui::AX_ROLE_MENU_BUTTON:
>> > case ui::AX_ROLE_POP_UP_BUTTON:
>> >     class_name = "android.widget.Button";
>> >     break;
>> >
>> > to get android.widget.Button will lead all menuitem types to
>> > android.widget.Button
>> >
>> > OR
>> >
>> > MenuButtonRole can be used. which corresponds with AX_ROLE_MENU_BUTTON
>> in
>> > chromium side.
>>
>
>  You can use any class name from the Android widget hierarchy here -
>> http://developer.android.com/reference/android/view/MenuItem.html - so
>> perhaps
>>
> "
>
>> android.view.MenuItem"
>>
>
>  Without testing on Android I don't know for sure what would be best, but I
>>
> agree
>
>> with Mike that an interactive element should be a Button or MenuItem or
>> some
>> interactive widget role, as opposed to just android.view.View.
>>
>
> Then I think for input type checkbox with menu as parent should also have
> android.view.MenuItem since parent is menu hence the input type checkbox
> would
> behave as menuitem.
>
> Similarly for aria role menuitemcheckbox and menuitemradio.
>
> Whats your view on this?
>
>
> https://codereview.chromium.org/634533002/
>
> 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-10-15 10:27:38]

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

[commit-bot: I haz the power, 2014-10-15 10:27:50]

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

[commit-bot: I haz the power, 2014-10-15 11:19:19]

Committed patchset #11 (id:400001)

[commit-bot: I haz the power, 2014-10-15 11:20:10]

Patchset 11 (id:??) landed as
https://crrev.com/f1b5973d881211bbe02f67dbaad419829718a41e
Cr-Commit-Position: refs/heads/master@{#299675}