Closed Bug 726069 Opened 8 years ago Closed 8 years ago

get rid nsAccUtils::GetPositionAndSizeForXULContainerItem

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: surkov, Assigned: capella)

References

Details

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

Attachments

(1 file, 2 obsolete files)

Move nsAccUtils::GetPositionAndSizeForXULContainerItem logic into nsXULMenuitemAccessible::GetPositionAndSizeInternal which is unique consumer. Do mDoc->GetAccessible(). Remove nsAccUtils method.

See http://mxr.mozilla.org/mozilla-central/ident?i=GetPositionAndSizeForXULContainerItem&filter=
Attached patch Patch (obsolete) — Splinter Review
Attachment #597355 - Flags: review?(surkov.alexander)
Comment on attachment 597355 [details] [diff] [review]
Patch

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

sorry to say when you've got a patch but there's nicer and performant way:
1) legalize: COMBOBOX_OPTION and PARENT_MENUITEM in AccGroupInfo class and then you can remove GetLevelInternal and GetPositionAndSizeInternal from nsXULMenuitemAccessible
2) add a test for XUL menulist in attributes/test_obj_group.xul
3) add a test for ARIA role="combobox" in attributes/test_obj_group.html

sounds good?
Attachment #597355 - Flags: review?(surkov.alexander)
> sounds good?

Yes, I can give a try.
(In reply to Jignesh Kakadiya [:jhk] from comment #3)
> > sounds good?
> 
> Yes, I can give a try.

thank you, please don't hesitate to ask questions
Assignee: nobody → jigneshhk1992
Summary: nsAccUtils::GetPositionAndSizeForXULContainerItem should use nsDocAccessible::GetAccessible → get rid nsAccUtils::GetPositionAndSizeForXULContainerItem
Assignee: jigneshhk1992 → markcapella
Attached patch Patch (v2) (obsolete) — Splinter Review
I spoke with Jignesh Kakadiya through email, who said I could finish this for him.

This attached patch starts from his original work, and adds the changes requested in comment #2, and some simple tests also as requested in comment #2. 

Note that changes to existing TestGroupAttr testcases /menu_item/ in test_obj_group.xul were required as a result of the code changes ...
Attachment #604641 - Flags: feedback?(surkov.alexander)
Status: NEW → ASSIGNED
Comment on attachment 604641 [details] [diff] [review]
Patch (v2)

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

otherwise is good

::: accessible/src/base/AccGroupInfo.h
@@ +66,5 @@
>          role != mozilla::a11y::roles::OPTION &&
>          role != mozilla::a11y::roles::LISTITEM &&
>          role != mozilla::a11y::roles::MENUITEM &&
> +        role != mozilla::a11y::roles::COMBOBOX_OPTION &&
> +        role != mozilla::a11y::roles::PARENT_MENUITEM &&

I think you should tweak AccGroupInfo::BaseRole because parent menu item is a menuitem that contains menu.

::: accessible/src/xul/nsXULMenuAccessible.cpp
@@ -307,5 @@
>  
> -PRInt32
> -nsXULMenuitemAccessible::GetLevelInternal()
> -{
> -  return nsAccUtils::GetLevelForXULContainerItem(mContent);

I'd suggest to deal with level stuffs in follow up (in general level isn't handled by AccGroupinfo, it's calculated in nsAccessible::GetLevelInternal())

::: accessible/tests/mochitest/attributes/test_obj_group.xul
@@ +35,5 @@
>        {
>          testGroupAttrs("menu_item1.1", 1, 1);
> +        testGroupAttrs("menu_item1.2", 1, 2);
> +        testGroupAttrs("menu_item1.4", 2, 2);
> +        testGroupAttrs("menu_item2", 1, 1);

that's why you it didn't pass for you. You shouldn't change existing test until you have clever idea why you need to change them

@@ +60,5 @@
>  
>        this.finalCheck = function openSubMenu_finalCheck()
>        {
> +        testGroupAttrs("menu_item2.1", 1, 2);
> +        testGroupAttrs("menu_item2.2", 2, 2);

and since level doesn't work you just removed tests
Attachment #604641 - Flags: feedback?(surkov.alexander)
(In reply to alexander :surkov from comment #2)
> 1) legalize: COMBOBOX_OPTION and PARENT_MENUITEM in AccGroupInfo class and
> then you can remove GetLevelInternal and GetPositionAndSizeInternal from
> nsXULMenuitemAccessible

yep, you cannot simply remove GetLevelInternal
Attached patch Patch (v3)Splinter Review
Tweaking BaseRole() and restoring GetLevelInternal() does indeed allow the original tests to pass, as well as our new ones.
Attachment #604641 - Attachment is obsolete: true
Attachment #604928 - Flags: feedback?(surkov.alexander)
Comment on attachment 604928 [details] [diff] [review]
Patch (v3)

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

nice
Attachment #604928 - Flags: review?(hub)
Attachment #604928 - Flags: feedback?(surkov.alexander)
Attachment #604928 - Flags: feedback+
Attachment #597355 - Attachment is obsolete: true
Comment on attachment 604928 [details] [diff] [review]
Patch (v3)

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

looks good
Attachment #604928 - Flags: review?(hub) → review+
https://hg.mozilla.org/mozilla-central/rev/53fb368c8fa0
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.