Last Comment Bug 726071 - get rid nsAccUtils::GetPositionAndSizeForXULSelectControlItem
: get rid nsAccUtils::GetPositionAndSizeForXULSelectControlItem
Status: RESOLVED FIXED
[good first bug][mentor=surkov.alexan...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Mark Capella [:capella]
:
Mentors:
Depends on: 765172
Blocks: 725572
  Show dependency treegraph
 
Reported: 2012-02-10 10:33 PST by alexander :surkov
Modified: 2012-06-19 11:03 PDT (History)
3 users (show)
surkov.alexander: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (8.11 KB, patch)
2012-03-12 12:59 PDT, Mark Capella [:capella]
no flags Details | Diff | Review
Patch (v2) (9.09 KB, patch)
2012-03-12 15:55 PDT, Mark Capella [:capella]
no flags Details | Diff | Review
Patch (v3) (10.52 KB, patch)
2012-03-12 19:54 PDT, Mark Capella [:capella]
no flags Details | Diff | Review
Patch (v4) (11.94 KB, patch)
2012-03-14 00:39 PDT, Mark Capella [:capella]
no flags Details | Diff | Review
Patch (v5) (16.26 KB, patch)
2012-03-14 10:42 PDT, Mark Capella [:capella]
no flags Details | Diff | Review
Patch (v6) (19.77 KB, patch)
2012-03-15 08:53 PDT, Mark Capella [:capella]
surkov.alexander: review+
Details | Diff | Review
Patch (v7) (16.38 KB, patch)
2012-03-15 16:08 PDT, Mark Capella [:capella]
no flags Details | Diff | Review
Patch (v8) (16.27 KB, patch)
2012-03-15 19:58 PDT, Mark Capella [:capella]
no flags Details | Diff | Review
Patch (v9) (16.72 KB, patch)
2012-03-20 02:52 PDT, Mark Capella [:capella]
surkov.alexander: review+
tbsaunde+mozbugs: feedback+
Details | Diff | Review

Description alexander :surkov 2012-02-10 10:33:40 PST
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 2012-02-15 05:43:47 PST
should be fixed the same way as bug 726069 comment #2
Comment 2 Mark Capella [:capella] 2012-03-12 07:52:00 PDT
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 2012-03-12 07:55:42 PDT
(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] 2012-03-12 12:48:36 PDT
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 2012-03-12 12:55:55 PDT
(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] 2012-03-12 12:59:01 PDT
Created attachment 605063 [details] [diff] [review]
Patch (v1)

Maybe a small bit of help ... :)

I'm reading through role.h but this might take awhile !
Comment 7 alexander :surkov 2012-03-12 13:02:22 PDT
(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] 2012-03-12 13:09:10 PDT
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 2012-03-12 13:16:12 PDT
(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] 2012-03-12 15:55:23 PDT
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.

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 2012-03-12 18:26:05 PDT
(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] 2012-03-12 18:34:52 PDT
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 2012-03-12 18:37:14 PDT
(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] 2012-03-12 18:47:04 PDT
Yah - that falls down ... let me patch it and repost ...
Comment 15 Mark Capella [:capella] 2012-03-12 19:54:51 PDT
Created attachment 605283 [details] [diff] [review]
Patch (v3)

Patched to include "checkbox" ...
Comment 16 alexander :surkov 2012-03-13 05:16:24 PDT
(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 2012-03-13 05:53:45 PDT
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] 2012-03-13 06:23:49 PDT
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 2012-03-13 06:27:06 PDT
(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] 2012-03-14 00:39:02 PDT
Created attachment 605672 [details] [diff] [review]
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 2012-03-14 05:50:02 PDT
(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] 2012-03-14 10:42:36 PDT
Created attachment 605838 [details] [diff] [review]
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 2012-03-14 11:04:51 PDT
you know you could introduce CHECK_LISTITEM role, it would simplify things much
Comment 24 Mark Capella [:capella] 2012-03-14 11:08:48 PDT
A whole new role? Are you sure I could handle that? :)
Comment 25 alexander :surkov 2012-03-14 11:23:37 PDT
(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] 2012-03-14 11:43:54 PDT
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) 2012-03-14 12:28:44 PDT
(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] 2012-03-14 15:09:34 PDT
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 2012-03-14 16:01:51 PDT
(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] 2012-03-15 08:53:48 PDT
Created attachment 606237 [details] [diff] [review]
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 2012-03-15 11:11:04 PDT
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] 2012-03-15 11:32:47 PDT
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 2012-03-15 11:41:58 PDT
(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] 2012-03-15 11:48:28 PDT
>> +    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 2012-03-15 13:13:49 PDT
(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] 2012-03-15 16:08:23 PDT
Created attachment 606393 [details] [diff] [review]
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] 2012-03-15 18:27:26 PDT
I meant "I can't" ... the above works, nothing else seems to...
Comment 38 alexander :surkov 2012-03-15 19:20:57 PDT
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 2012-03-15 19:22:04 PDT
(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] 2012-03-15 19:58:07 PDT
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?
Comment 41 alexander :surkov 2012-03-16 04:42:42 PDT
(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] 2012-03-16 05:46:28 PDT
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] 2012-03-16 05:55:57 PDT
(Attachment (v7) produces passing results, attachment (v8) does not...)
Comment 44 alexander :surkov 2012-03-20 01:42:11 PDT
(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] 2012-03-20 02:52:09 PDT
Created attachment 607494 [details] [diff] [review]
Patch (v9)

Change builds ok, all mochitests pass :)
Comment 46 alexander :surkov 2012-03-20 03:57:04 PDT
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 2012-03-23 06:25:06 PDT
Trevor, ping for feedback
Comment 48 Trevor Saunders (:tbsaunde) 2012-03-23 07:16:21 PDT
Comment on attachment 607494 [details] [diff] [review]
Patch (v9)

missed this somehow :(
Comment 50 Ed Morley [:emorley] 2012-03-24 13:40:22 PDT
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 2012-03-25 22:21:46 PDT
(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

Note You need to log in before you can comment on or make changes to this bug.