Last Comment Bug 726069 - get rid nsAccUtils::GetPositionAndSizeForXULContainerItem
: get rid nsAccUtils::GetPositionAndSizeForXULContainerItem
Status: RESOLVED FIXED
[good first bug][mentor=surkov.alexan...
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Mark Capella [:capella]
:
: alexander :surkov
Mentors:
Depends on:
Blocks: 725572
  Show dependency treegraph
 
Reported: 2012-02-10 10:29 PST by alexander :surkov
Modified: 2012-03-13 04:48 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (6.37 KB, patch)
2012-02-15 02:59 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review
Patch (v2) (11.43 KB, patch)
2012-03-10 06:23 PST, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v3) (10.56 KB, patch)
2012-03-12 07:29 PDT, Mark Capella [:capella]
hub: review+
surkov.alexander: feedback+
Details | Diff | Splinter Review

Description alexander :surkov 2012-02-10 10:29:20 PST
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=
Comment 1 Jignesh Kakadiya [:jhk] 2012-02-15 02:59:50 PST
Created attachment 597355 [details] [diff] [review]
Patch
Comment 2 alexander :surkov 2012-02-15 03:26:56 PST
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?
Comment 3 Jignesh Kakadiya [:jhk] 2012-02-15 03:49:52 PST
> sounds good?

Yes, I can give a try.
Comment 4 alexander :surkov 2012-02-15 04:01:55 PST
(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
Comment 5 Mark Capella [:capella] 2012-03-10 06:23:43 PST
Created attachment 604641 [details] [diff] [review]
Patch (v2)

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 ...
Comment 6 alexander :surkov 2012-03-10 07:13:10 PST
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
Comment 7 alexander :surkov 2012-03-10 07:15:55 PST
(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
Comment 8 Mark Capella [:capella] 2012-03-12 07:29:39 PDT
Created attachment 604928 [details] [diff] [review]
Patch (v3)

Tweaking BaseRole() and restoring GetLevelInternal() does indeed allow the original tests to pass, as well as our new ones.
Comment 9 alexander :surkov 2012-03-12 07:34:39 PDT
Comment on attachment 604928 [details] [diff] [review]
Patch (v3)

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

nice
Comment 10 Hubert Figuiere [:hub] 2012-03-12 10:52:31 PDT
Comment on attachment 604928 [details] [diff] [review]
Patch (v3)

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

looks good
Comment 12 Marco Bonardo [::mak] 2012-03-13 04:48:47 PDT
https://hg.mozilla.org/mozilla-central/rev/53fb368c8fa0

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