Closed
Bug 726069
Opened 12 years ago
Closed 12 years ago
get rid nsAccUtils::GetPositionAndSizeForXULContainerItem
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
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)
10.56 KB,
patch
|
hub
:
review+
surkov
:
feedback+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
Attachment #597355 -
Flags: review?(surkov.alexander)
Reporter | ||
Comment 2•12 years ago
|
||
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)
Comment 3•12 years ago
|
||
> sounds good?
Yes, I can give a try.
Reporter | ||
Comment 4•12 years ago
|
||
(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
Reporter | ||
Updated•12 years ago
|
Assignee: nobody → jigneshhk1992
Reporter | ||
Updated•12 years ago
|
Summary: nsAccUtils::GetPositionAndSizeForXULContainerItem should use nsDocAccessible::GetAccessible → get rid nsAccUtils::GetPositionAndSizeForXULContainerItem
Updated•12 years ago
|
Assignee: jigneshhk1992 → markcapella
Assignee | ||
Comment 5•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Comment 6•12 years ago
|
||
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)
Reporter | ||
Comment 7•12 years ago
|
||
(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
Assignee | ||
Comment 8•12 years ago
|
||
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)
Reporter | ||
Comment 9•12 years ago
|
||
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+
Reporter | ||
Updated•12 years ago
|
Attachment #597355 -
Attachment is obsolete: true
Comment 10•12 years ago
|
||
Comment on attachment 604928 [details] [diff] [review] Patch (v3) Review of attachment 604928 [details] [diff] [review]: ----------------------------------------------------------------- looks good
Attachment #604928 -
Flags: review?(hub) → review+
Reporter | ||
Comment 11•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/53fb368c8fa0
Comment 12•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/53fb368c8fa0
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in
before you can comment on or make changes to this bug.
Description
•