Note: There are a few cases of duplicates in user autocompletion which are being worked on.

get rid nsAccUtils::GetPositionAndSizeForXULSelectControlItem

RESOLVED FIXED in mozilla14

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: surkov, Assigned: capella)

Tracking

unspecified
mozilla14
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=surkov.alexander@gmail.com][lang=c++])

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

6 years ago
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=
(Reporter)

Comment 1

6 years ago
should be fixed the same way as bug 726069 comment #2
Summary: nsAccUtils::GetPositionAndSizeForXULSelectControlItem should use nsDocAccessible::GetAccessible → get rid nsAccUtils::GetPositionAndSizeForXULSelectControlItem
(Assignee)

Comment 2

5 years ago
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
(Reporter)

Comment 3

5 years ago
(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
(Assignee)

Updated

5 years ago
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
(Assignee)

Comment 4

5 years ago
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).
(Reporter)

Comment 5

5 years ago
(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
(Assignee)

Comment 6

5 years ago
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 !
(Reporter)

Comment 7

5 years ago
(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.
(Assignee)

Comment 8

5 years ago
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.
(Reporter)

Comment 9

5 years ago
(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.
(Assignee)

Comment 10

5 years ago
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
Attachment #605063 - Attachment is obsolete: true
(Reporter)

Comment 11

5 years ago
(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.
(Assignee)

Comment 12

5 years ago
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.
Attachment #605187 - Flags: review?(surkov.alexander)
(Reporter)

Comment 13

5 years ago
(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.
(Assignee)

Comment 14

5 years ago
Yah - that falls down ... let me patch it and repost ...
(Assignee)

Comment 15

5 years ago
Created attachment 605283 [details] [diff] [review]
Patch (v3)

Patched to include "checkbox" ...
Attachment #605187 - Attachment is obsolete: true
Attachment #605187 - Flags: review?(surkov.alexander)
Attachment #605283 - Flags: review?(surkov.alexander)
(Reporter)

Comment 16

5 years ago
(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
(Reporter)

Comment 17

5 years ago
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.
Attachment #605283 - Flags: review?(surkov.alexander)
(Assignee)

Comment 18

5 years ago
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>
(Reporter)

Comment 19

5 years ago
(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);
(Assignee)

Comment 20

5 years ago
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
(Reporter)

Comment 21

5 years ago
(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
(Assignee)

Comment 22

5 years ago
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.
Attachment #605283 - Attachment is obsolete: true
Attachment #605672 - Attachment is obsolete: true
(Reporter)

Comment 23

5 years ago
you know you could introduce CHECK_LISTITEM role, it would simplify things much
(Assignee)

Comment 24

5 years ago
A whole new role? Are you sure I could handle that? :)
(Reporter)

Comment 25

5 years ago
(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.
(Assignee)

Comment 26

5 years ago
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.
(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.
(Assignee)

Comment 28

5 years ago
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
(Reporter)

Comment 29

5 years ago
(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
(Assignee)

Comment 30

5 years ago
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
Attachment #605838 - Attachment is obsolete: true
(Reporter)

Comment 31

5 years ago
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 :)
Attachment #606237 - Flags: review+
(Assignee)

Comment 32

5 years ago
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 ...
(Reporter)

Comment 33

5 years ago
(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
(Assignee)

Comment 34

5 years ago
>> +    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?
(Reporter)

Comment 35

5 years ago
(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.
(Assignee)

Comment 36

5 years ago
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;
(Assignee)

Comment 37

5 years ago
I meant "I can't" ... the above works, nothing else seems to...
(Reporter)

Comment 38

5 years ago
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
(Reporter)

Updated

5 years ago
Attachment #606237 - Attachment is obsolete: true
(Reporter)

Comment 39

5 years ago
(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?
(Assignee)

Comment 40

5 years ago
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?
(Reporter)

Comment 41

5 years ago
(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.
(Reporter)

Updated

5 years ago
Attachment #606393 - Attachment is obsolete: true
(Assignee)

Comment 42

5 years ago
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
(Assignee)

Comment 43

5 years ago
(Attachment (v7) produces passing results, attachment (v8) does not...)
(Reporter)

Comment 44

5 years ago
(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
(Assignee)

Comment 45

5 years ago
Created attachment 607494 [details] [diff] [review]
Patch (v9)

Change builds ok, all mochitests pass :)
Attachment #606437 - Attachment is obsolete: true
Attachment #607494 - Flags: feedback?(surkov.alexander)
(Reporter)

Comment 46

5 years ago
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?
Attachment #607494 - Flags: review+
Attachment #607494 - Flags: feedback?(trev.saunders)
Attachment #607494 - Flags: feedback?(surkov.alexander)
(Reporter)

Comment 47

5 years ago
Trevor, ping for feedback
Comment on attachment 607494 [details] [diff] [review]
Patch (v9)

missed this somehow :(
Attachment #607494 - Flags: feedback?(trev.saunders) → feedback+
(Reporter)

Comment 49

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c2e0e02113b
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
(Reporter)

Comment 51

5 years ago
(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
(Reporter)

Updated

5 years ago
Flags: in-testsuite+
Depends on: 765172
You need to log in before you can comment on or make changes to this bug.