726071 - get rid nsAccUtils::GetPositionAndSizeForXULSelectControlItem

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Make nsAccUtils::GetPositionAndSizeForXULSelectControlItem to take nsAccessible* instead of nsIContent*. Having an accessible you can get content and document.

http://mxr.mozilla.org/mozilla-central/ident?i=GetPositionAndSizeForXULSelectControlItem&filter=

Comment 1

[alexander :surkov (:asurkov)]

should be fixed the same way as bug 726069 comment #2

Comment 2

[Mark Capella [:capella]]

Is this change going to be like the original approach Jignesh Kakadiya took to bug#726069? Or is there more to it, such as the final changes we did:

1) legalize: ... then you can remove ...
2) add a test for ... in attributes/test_obj_group.xul
3) add a test for ... attributes/test_obj_group.html

Comment 3

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #2)
> Is this change going to be like the original approach Jignesh Kakadiya took
> to bug#726069? Or is there more to it, such as the final changes we did:
> 
> 1) legalize: ... then you can remove ...
> 2) add a test for ... in attributes/test_obj_group.xul
> 3) add a test for ... attributes/test_obj_group.html

a final approach sure

Comment 4

[Mark Capella [:capella]]

Hmmmm ... nsAccUtils::GetPositionAndSizeForXULSelectControlItem is consumed by nsXULRadioButtonAccessible::GetPositionAndSizeInternal, nsXULListitemAccessible::GetPositionAndSizeInternal, and by nsXULTabAccessible::GetPositionAndSizeInternal.

The only problem with removing those blocks of code like we did in bug726069 is that the xul:listbox tests fail now with (wrong group position / size of the group, no expected attributes).

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #4)

> The only problem with removing those blocks of code like we did in bug726069
> is that the xul:listbox tests fail now with (wrong group position / size of
> the group, no expected attributes).

do you need a help to figure why? please attach the patch

Comment 6

[Mark Capella [:capella]]

Attachment: Patch (v1)

Maybe a small bit of help ... :)

I'm reading through role.h but this might take awhile !

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #6)
> Created attachment 605063 [details] [diff] [review]
> Patch (v1)
> 
> Maybe a small bit of help ... :)

small bit of help is you need to debug it :) that's the best way to figure out what's going wrong.

Comment 8

[Mark Capella [:capella]]

Do you mean through through reading the code and analyzing ? That's where I am now ... if you know a way to use Win Dev debugger or such please share that.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #8)
> Do you mean through through reading the code and analyzing ? That's where I
> am now ... if you know a way to use Win Dev debugger or such please share
> that.

yep, real debugger. I use VC 10, just run debug Firefox version and user Tools->Attach To Process. Open source file and set breakpoint. Then load your test.

Anyway, you need to check nsXULListitemAccessible::NativeRole() to see if GroupInfo handles XUL listitem roles well. As far as I can see it doesn't know about RICH_OPTION, that's probably cause of failure you see.

Comment 10

[Mark Capella [:capella]]

Attachment: Patch (v2)

Ok, nsXULListitemAccessible::NativeRole() indeed returns a RICH_OPTION which I've added to AccGroupInfo class, and the tests pass again. If this is right, I'll have to wonder why it also could return a CHECKBUTTON that AccGroupInfo doesn't know about.

I'm also looking into the below for debugging tips and will check into IRQ for further help. (Cool stuff I've been looking for)

https://developer.mozilla.org/en/Debugging_Mozilla_on_Windows_FAQ
https://developer.mozilla.org/en/Building_Firefox_with_Debug_Symbols

Comment 11

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #10)
> Created attachment 605187 [details] [diff] [review]
> Patch (v2)
> 
> Ok, nsXULListitemAccessible::NativeRole() indeed returns a RICH_OPTION which
> I've added to AccGroupInfo class, and the tests pass again. If this is
> right, I'll have to wonder why it also could return a CHECKBUTTON that
> AccGroupInfo doesn't know about.

then you should add it (if other checkbutton roles are ok with that), but it sounds like checkbuttons and richoptions can be mixed. You'd need to handle this.

Comment 12

[Mark Capella [:capella]]

Comment on attachment 605187 [details] [diff] [review]
Patch (v2)

I think I'll wait to see about CHECKBUTTON since it's not an issue right now. Adding the RICH_OPTION fixed up the immediate situation.

Comment 13

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #12)
> Comment on attachment 605187 [details] [diff] [review]
> Patch (v2)
> 
> I think I'll wait to see about CHECKBUTTON since it's not an issue right
> now.

obviously that'll be a regression, just add a test for <listitem type="checkbox"/> and compare results.

Comment 14

[Mark Capella [:capella]]

Yah - that falls down ... let me patch it and repost ...

Comment 15

[Mark Capella [:capella]]

Attachment: Patch (v3)

Patched to include "checkbox" ...

Comment 16

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #15)
> Created attachment 605283 [details] [diff] [review]
> Patch (v3)
> 
> Patched to include "checkbox" ...

here's where interesting happens, we don't expose group information for checkbox until it's a part of XUL listbox

Comment 17

[alexander :surkov (:asurkov)]

Comment on attachment 605283 [details] [diff] [review]
Patch (v3)

Review of attachment 605283 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/base/AccGroupInfo.h
@@ +74,5 @@
>          role != mozilla::a11y::roles::RADIO_MENU_ITEM &&
>          role != mozilla::a11y::roles::RADIOBUTTON &&
>          role != mozilla::a11y::roles::PAGETAB)
>        return nsnull;
>  

I'd say to make that checkbutton case working properly then you need to change IsConceptualParent and move IsConceptualParent check from AccGroupInfo constructor to CreateGroupInfo. Also you need to tweak BaseRole to put checkbutton listitem into the group of ordinal listitems.

::: accessible/tests/mochitest/attributes/test_obj_group.xul
@@ +149,5 @@
>      <listitem label="item2" id="item2"/>
>    </listbox>
>  
> +  <listbox>
> +    <listitem label="item3" id="item3" type="checkbox"/>

add it to listbox above and see it has separate group. I don't think it should.

Comment 18

[Mark Capella [:capella]]

Hmmm ... leaving tests alone, but changing listbox, all still passes ok...

      testGroupAttrs("item1", 1, 2);
      testGroupAttrs("item2", 2, 2);
      testGroupAttrs("item3", 1, 1);
  <listbox>
    <listitem label="item1" id="item1"/>
    <listitem label="item2" id="item2"/>
    <listitem label="item3" id="item3" type="checkbox"/>
  </listbox>

Comment 19

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #18)
> Hmmm ... leaving tests alone, but changing listbox, all still passes ok...
> 
>       testGroupAttrs("item1", 1, 2);
>       testGroupAttrs("item2", 2, 2);
>       testGroupAttrs("item3", 1, 1);

of course :) but this one doesn't 
testGroupAttrs("item1", 1, 3);
testGroupAttrs("item2", 2, 3);
testGroupAttrs("item3", 3, 3);

Comment 20

[Mark Capella [:capella]]

Attachment: Patch (v4)

This is far as I got today. All works except for tests listitem7/listitem8 where they are mixed together in one listbox.

I spent what little time I had today trying to get the dang debugger to work with the mochitests without success, so tomorrow/later I'll try to make sense of the suggestions you posted.

--mark

Comment 21

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #20)
> This is far as I got today. All works except for tests listitem7/listitem8
> where they are mixed together in one listbox.

it doesn't work because BaseRole fix is incorrect, you treat checkbutton as menuitem while you you should treat it as listitem or something.

> I spent what little time I had today trying to get the dang debugger to work
> with the mochitests without success, so tomorrow/later I'll try to make
> sense of the suggestions you posted.

ok, cool

Comment 22

[Mark Capella [:capella]]

Attachment: Patch (v5)

Oh! I wasn't sure what you meant by ordinal listitems ..

Regarding your further suggestions, I've moved IsConceptualParent check from AccGroupInfo constructor to CreateGroupInfo, and made a few changes to  IsConceptualParent. (My latest train-of-thought is attached.)

I must be wrong with either one or both of the changes, as there seems to be no effect on the testing. Listitem 7 and 8 still do not combine properly.

Comment 23

[alexander :surkov (:asurkov)]

you know you could introduce CHECK_LISTITEM role, it would simplify things much

Comment 24

[Mark Capella [:capella]]

A whole new role? Are you sure I could handle that? :)

Comment 25

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #24)
> A whole new role? 

yeah, sometimes we do that if that allows to keep things simpler. See we have CHECK_MENU_ITEM roles for example so it makes sense to do the same for listitem.

> Are you sure I could handle that? :)

this bug stopped to be a good first bug but I think you can handle it.

Comment 26

[Mark Capella [:capella]]

I'm laughing ... glad you noticed the "stopped being a good first bug" portion... If you have the patience, I have the time and interest.

"Add a new role item to role.h" seems like the obvious first step ... then maybe clone "menuitemcheckbox" in nsARIAMap.cpp into "listitemcheckbox"? Then something else for CHECK_LISTITEM to match CHECK_MENU_ITEM in accgroupinfo.h? Will I need to put the IsConceptualParent check back into the constructor where it started?

I may ask a lot of questions.

Comment 27

[Trevor Saunders (:tbsaunde)]

(In reply to Mark Capella [:capella] from comment #26)
> I'm laughing ... glad you noticed the "stopped being a good first bug"
> portion... If you have the patience, I have the time and interest.

I think I can help you with this part.

> "Add a new role item to role.h" seems like the obvious first step ... then

yeah, and a couple other places, each of the atk, msaa and mac directories in accessible/src/ have a nsRoleMap.h header that you should add to as well as a list of string roles in nsAccessibilityService.cpp

> maybe clone "menuitemcheckbox" in nsARIAMap.cpp into "listitemcheckbox"?

no, those are for  ARIA roles which are a different type of role than the ones in Role.h

> Then something else for CHECK_LISTITEM to match CHECK_MENU_ITEM in
> accgroupinfo.h? Will I need to put the IsConceptualParent check back into
> the constructor where it started?

well, you should then simplify things as you can as Alex suggests, I haven't read the patch / understood exactly what the issue is yet, so I'm not sure what exactly you need to do after add the role.
> 
> I may ask a lot of questions.

Comment 28

[Mark Capella [:capella]]

Hey Trevor! Thanks for your input, along with Alex.

For role.h I added a new (next to last) entry for CHECK_LIST_ITEM value 125, and bumped LAST_ENTRY to value 126.

> atk, msaa and mac directories in accessible/src/ have a nsRoleMap.h header

For mac nsRoleMap.h I added another NSAccessibilityMenuItemRole item as the (next to last) entry versus creating some new item, as it seemed like a generic role used by others in the file.

For msaa nsRoleMap.h I cloned the CHECK_MENU_ITEM entry { ROLE_SYSTEM_MENUITEM, IA2_ROLE_CHECK_MENU_ITEM } to the (next to last) position. Should I instead create a new IA2_ROLE_CHECK_LIST_ITEM portion of the entry, and then maybe add to AccessibleRole.idl?

For atk nsRoleMap.h I created a new ATK_ROLE_CHECK_LIST_ITEM entry in the appropriate place ... does this force a change into ATKOBJECT.h to match ATK_ROLE_CHECK_MENU_ITEM ?

> list of string roles in nsAccessibilityService.cpp

I hope you meant the .h file... where I added a CHECK_LIST_ITEM to the appropriate / last place in the list ...

Thanks again for the attention :p

Comment 29

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #28)
> Hey Trevor! Thanks for your input, along with Alex.
> 
> For role.h I added a new (next to last) entry for CHECK_LIST_ITEM value 125,
> and bumped LAST_ENTRY to value 126.

it's worth to name it CHECK_RICH_OPTION.

> > atk, msaa and mac directories in accessible/src/ have a nsRoleMap.h header
> 
> For mac nsRoleMap.h I added another NSAccessibilityMenuItemRole item as the
> (next to last) entry versus creating some new item, as it seemed like a
> generic role used by others in the file.
> 
> For msaa nsRoleMap.h I cloned the CHECK_MENU_ITEM entry {
> ROLE_SYSTEM_MENUITEM, IA2_ROLE_CHECK_MENU_ITEM } to the (next to last)
> position. Should I instead create a new IA2_ROLE_CHECK_LIST_ITEM portion of
> the entry, and then maybe add to AccessibleRole.idl?

please add it

Comment 30

[Mark Capella [:capella]]

Attachment: Patch (v6)

Ok, I've made the changes for the original bug request. I also added the new role now named "CHECK_RICH_ITEM" and updated the various related files, and made the changes to support a checklist listitem.

Also please note, the IsConceptualParent() check was moved during all this from the AccGroupInfo() constructor to the CreateGroupInfo() routine. I'm not sure if this was strictly required, but it doesn't seem to have hurt anything, so I left it here.

Please let me know if there is anything further required. -- mark

Comment 31

[alexander :surkov (:asurkov)]

Comment on attachment 606237 [details] [diff] [review]
Patch (v6)

Review of attachment 606237 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/atk/nsRoleMap.h
@@ +169,5 @@
>      ATK_ROLE_TABLE_CELL,          // roles::GRID_CELL            121
>      ATK_ROLE_PANEL,               // roles::EMBEDDED_OBJECT      122
>      ATK_ROLE_SECTION,             // roles::NOTE                 123
>      ATK_ROLE_PANEL,               // roles::FIGURE               124
> +    ATK_ROLE_CHECK_RICH_ITEM,     // roles::CHECK_RICH_ITEM      125

name it CHECK_RICH_OPTION to keep closer to RICH_OPTION

::: accessible/src/base/AccGroupInfo.cpp
@@ -166,5 @@
> -
> -  // Previous sibling of parent group is a tree item, this is the
> -  // conceptual tree item parent.
> -  if (parentPrevSiblingRole == roles::OUTLINEITEM)
> -    mParent = parentPrevSibling;

having new role it doesn't sound like you need to move parent code

::: accessible/src/base/AccGroupInfo.h
@@ +130,5 @@
>          aRole == mozilla::a11y::roles::PARENT_MENUITEM ||
>          aRole == mozilla::a11y::roles::RADIO_MENU_ITEM)
>        return mozilla::a11y::roles::MENUITEM;
> +
> +    if (aRole == mozilla::a11y::roles::CHECKBUTTON ||

wrong

@@ +132,5 @@
>        return mozilla::a11y::roles::MENUITEM;
> +
> +    if (aRole == mozilla::a11y::roles::CHECKBUTTON ||
> +        aRole == mozilla::a11y::roles::RICH_OPTION)
> +      return mozilla::a11y::roles::CHECK_RICH_ITEM;

return RICH_OPTION and you don't need to check RICH_OPTION

::: accessible/src/mac/nsRoleMap.h
@@ +166,5 @@
>    NSAccessibilityGroupRole,                     // ROLE_GRID_CELL
>    NSAccessibilityGroupRole,                     // ROLE_EMBEDDED_OBJECT
>    NSAccessibilityGroupRole,                     // ROLE_NOTE
>    NSAccessibilityGroupRole,                     // ROLE_FIGURE
> +  NSAccessibilityMenuItemRole,                  // ROLE_CHECK_RICH_ITEM

I wonder if checkbutton is mapped to menuitem, sounds like no

::: accessible/src/msaa/nsRoleMap.h
@@ +449,5 @@
>    // roles::FIGURE
>    { ROLE_SYSTEM_GROUPING, ROLE_SYSTEM_GROUPING },
>  
> +  // roles::CHECK_RICH_ITEM
> +  { ROLE_SYSTEM_MENUITEM, IA2_ROLE_CHECK_RICH_ITEM },

same

::: other-licenses/ia2/AccessibleRole.idl
@@ +253,5 @@
>    */
> +  IA2_ROLE_VIEW_PORT,
> +
> +  /// Used for check buttons that are rich items.
> +  IA2_ROLE_CHECK_RICH_ITEM

we're not allowed to change IA2 standard on our own :)

Comment 32

[Mark Capella [:capella]]

Let's see,

> name it CHECK_RICH_OPTION to keep closer to RICH_OPTION
> having new role it doesn't sound like you need to move parent code

I created CHECK_RICH_ITEM by accident / moving too fast, will rename to CHECK_RICH_OPTION, and will put the IsConceptualParent() check back into the constructor.

Please clarify the rest:

>> +    if (aRole == mozilla::a11y::roles::CHECKBUTTON ||
>> +        aRole == mozilla::a11y::roles::RICH_OPTION)
>> +      return mozilla::a11y::roles::CHECK_RICH_ITEM;
> return RICH_OPTION and you don't need to check RICH_OPTION

I'll simply remove the second line of the IF statement there ...

>>    NSAccessibilityGroupRole,                     // ROLE_FIGURE
>> +  NSAccessibilityMenuItemRole,                  // ROLE_CHECK_RICH_ITEM
>I wonder if checkbutton is mapped to menuitem, sounds like no

Do you prefer that I set the entry NSAccessibilityMenuItemRole to be like the one above it? ie: NSAccessibilityGroupRole?

>>    { ROLE_SYSTEM_GROUPING, ROLE_SYSTEM_GROUPING },
>>  
>> +  // roles::CHECK_RICH_ITEM
>> +  { ROLE_SYSTEM_MENUITEM, IA2_ROLE_CHECK_RICH_ITEM },
>same

And then you'd prefer that I set the entry as "ROLE_SYSTEM_GROUPING, ROLE_SYSTEM_GROUPING"?

>we're not allowed to change IA2 standard on our own :)

Then I'm not sure what you wanted added to the AccessibleRole.idl in comment #29 above ...

Comment 33

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #32)

> >> +    if (aRole == mozilla::a11y::roles::CHECKBUTTON ||
> >> +        aRole == mozilla::a11y::roles::RICH_OPTION)
> >> +      return mozilla::a11y::roles::CHECK_RICH_ITEM;
> > return RICH_OPTION and you don't need to check RICH_OPTION
> 
> I'll simply remove the second line of the IF statement there ...

yes and please fix CHECKBUTTON thing

> >>    NSAccessibilityGroupRole,                     // ROLE_FIGURE
> >> +  NSAccessibilityMenuItemRole,                  // ROLE_CHECK_RICH_ITEM
> >I wonder if checkbutton is mapped to menuitem, sounds like no
> 
> Do you prefer that I set the entry NSAccessibilityMenuItemRole to be like
> the one above it? ie: NSAccessibilityGroupRole?

use the role used for CHECKBUTTON

> >>    { ROLE_SYSTEM_GROUPING, ROLE_SYSTEM_GROUPING },
> >>  
> >> +  // roles::CHECK_RICH_ITEM
> >> +  { ROLE_SYSTEM_MENUITEM, IA2_ROLE_CHECK_RICH_ITEM },
> >same
> 
> And then you'd prefer that I set the entry as "ROLE_SYSTEM_GROUPING,
> ROLE_SYSTEM_GROUPING"?

same

> >we're not allowed to change IA2 standard on our own :)
> 
> Then I'm not sure what you wanted added to the AccessibleRole.idl in comment
> #29 above ...

nsIAccessibleRole.idl located in accessible/public folder

Comment 34

[Mark Capella [:capella]]

>> +    if (aRole == mozilla::a11y::roles::CHECKBUTTON ||
>> +        aRole == mozilla::a11y::roles::RICH_OPTION)
>> +      return mozilla::a11y::roles::CHECK_RICH_ITEM;
>yes and please fix CHECKBUTTON thing

So just drop the whole IF statement? Or is there something here I'm missing?

Comment 35

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #34)
> >> +    if (aRole == mozilla::a11y::roles::CHECKBUTTON ||
> >> +        aRole == mozilla::a11y::roles::RICH_OPTION)
> >> +      return mozilla::a11y::roles::CHECK_RICH_ITEM;
> >yes and please fix CHECKBUTTON thing
> 
> So just drop the whole IF statement? Or is there something here I'm missing?

Nah, you should treat CHECK_RICH_OPTION as RICH_OPTION when you calculate group information.

Comment 36

[Mark Capella [:capella]]

Attachment: Patch (v7)

I can see a way to change the following that doesn't cause the tests to fail..

>> +    if (aRole == mozilla::a11y::roles::CHECKBUTTON ||
>> +        aRole == mozilla::a11y::roles::RICH_OPTION)
>> +      return mozilla::a11y::roles::CHECK_RICH_ITEM;

Comment 37

[Mark Capella [:capella]]

I meant "I can't" ... the above works, nothing else seems to...

Comment 38

[alexander :surkov (:asurkov)]

Comment on attachment 606393 [details] [diff] [review]
Patch (v7)

Review of attachment 606393 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/base/AccGroupInfo.h
@@ +67,5 @@
>          role != mozilla::a11y::roles::LISTITEM &&
>          role != mozilla::a11y::roles::MENUITEM &&
>          role != mozilla::a11y::roles::COMBOBOX_OPTION &&
> +        role != mozilla::a11y::roles::RICH_OPTION &&
> +        role != mozilla::a11y::roles::CHECKBUTTON &&

you shouldn't calculate group info for checkbutton

@@ +91,5 @@
>          aRole == mozilla::a11y::roles::PARENT_MENUITEM ||
>          aRole == mozilla::a11y::roles::RADIO_MENU_ITEM)
>        return mozilla::a11y::roles::MENUITEM;
> +
> +    if (aRole == mozilla::a11y::roles::CHECKBUTTON ||

why do you treat checkboxes as listitems?

@@ +93,5 @@
>        return mozilla::a11y::roles::MENUITEM;
> +
> +    if (aRole == mozilla::a11y::roles::CHECKBUTTON ||
> +        aRole == mozilla::a11y::roles::RICH_OPTION)
> +      return mozilla::a11y::roles::CHECK_RICH_OPTION;

cast check_rich_option to rich_option, i.e. vice versa you do

Comment 39

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #37)
> I meant "I can't" ... the above works, nothing else seems to...

sorry, what doesn't work?

Comment 40

[Mark Capella [:capella]]

Attachment: Patch (v8)

Honestly, I'm confused. What I had seemed to work. Is the attached as you now request?

Comment 41

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #40)
> Created attachment 606437 [details] [diff] [review]
> Patch (v8)
> 
> Honestly, I'm confused. What I had seemed to work. Is the attached as you
> now request?

yep, that looks right. If mochitests pass then let's ask Trevor for review.

Comment 42

[Mark Capella [:capella]]

Attachment produces the following:

INFO | automation.py | Application pid: 4044
Server listening on port 4443 with cert pgo server certificate
TEST-INFO | unknown test url | must wait for load
TEST-PASS | unknown test url | Wrong group position (posinset) for  'listitem1'  - 1 should equal 1
TEST-UNEXPECTED-FAIL | unknown test url | Wrong size of the group (setsize) for  'listitem1'  - got 2, expected 4
TEST-UNEXPECTED-FAIL | unknown test url | Attribute 'setsize' has wrong value for  'listitem1'  - got 2, expected 4
TEST-PASS | unknown test url | Attribute 'posinset' has wrong value for  'listitem1'  - 1 should equal 1
TEST-UNEXPECTED-FAIL | unknown test url | Wrong group position (posinset) for  'listitem2'  - got 0, expected 2
TEST-UNEXPECTED-FAIL | unknown test url | Wrong size of the group (setsize) for  'listitem2'  - got 0, expected 4
TEST-UNEXPECTED-FAIL | unknown test url | There is no expected attribute 'posinset'  for  'listitem2'
TEST-UNEXPECTED-FAIL | unknown test url | There is no expected attribute 'setsize'  for  'listitem2'
TEST-UNEXPECTED-FAIL | unknown test url | Wrong group position (posinset) for  'listitem3'  - got 0, expected 3
TEST-UNEXPECTED-FAIL | unknown test url | Wrong size of the group (setsize) for  'listitem3'  - got 0, expected 4
TEST-UNEXPECTED-FAIL | unknown test url | There is no expected attribute 'posinset'  for  'listitem3'
TEST-UNEXPECTED-FAIL | unknown test url | There is no expected attribute 'setsize'  for  'listitem3'
TEST-UNEXPECTED-FAIL | unknown test url | Wrong group position (posinset) for  'listitem4'  - got 2, expected 4
TEST-UNEXPECTED-FAIL | unknown test url | Wrong size of the group (setsize) for  'listitem4'  - got 2, expected 4
TEST-UNEXPECTED-FAIL | unknown test url | Attribute 'setsize' has wrong value for  'listitem4'  - got 2, expected 4
TEST-UNEXPECTED-FAIL | unknown test url | Attribute 'posinset' has wrong value for  'listitem4'  - got 2, expected 4

Comment 43

[Mark Capella [:capella]]

(Attachment (v7) produces passing results, attachment (v8) does not...)

Comment 44

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #43)
> (Attachment (v7) produces passing results, attachment (v8) does not...)

because you forgot to fix XUL listitem accessible to use CHECK_RICH_OPTION role

Comment 45

[Mark Capella [:capella]]

Attachment: Patch (v9)

Change builds ok, all mochitests pass :)

Comment 46

[alexander :surkov (:asurkov)]

Comment on attachment 607494 [details] [diff] [review]
Patch (v9)

Review of attachment 607494 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/mac/nsRoleMap.h
@@ +166,5 @@
>    NSAccessibilityGroupRole,                     // ROLE_GRID_CELL
>    NSAccessibilityGroupRole,                     // ROLE_EMBEDDED_OBJECT
>    NSAccessibilityGroupRole,                     // ROLE_NOTE
>    NSAccessibilityGroupRole,                     // ROLE_FIGURE
> +  NSAccessibilityCheckBoxRole,                  // ROLE_CHECK_RICH_OPTION

would you mind to file a bug to change these comments to roles::ROLENAME?

Comment 47

[alexander :surkov (:asurkov)]

Trevor, ping for feedback

Comment 48

[Trevor Saunders (:tbsaunde)]

Comment on attachment 607494 [details] [diff] [review]
Patch (v9)

missed this somehow :(

Comment 49

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c2e0e02113b

Comment 50

[Ed Morley [:emorley]]

Please can you set the target milestone when landing on inbound, along the lines of http://blog.bonardo.net/2012/03/23/how-you-can-help-mozilla-inbound-sheriffs-when-pushing :-)

https://hg.mozilla.org/mozilla-central/rev/6c2e0e02113b

Comment 51

[alexander :surkov (:asurkov)]

(In reply to Ed Morley [:edmorley] from comment #50)
> Please can you set the target milestone when landing on inbound, along the
> lines of
> http://blog.bonardo.net/2012/03/23/how-you-can-help-mozilla-inbound-sheriffs-
> when-pushing :-)

ok