Closed
Bug 443881
Opened 16 years ago
Closed 16 years ago
take into account separators in xul menus when group attributes are calculating
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
VERIFIED
FIXED
mozilla1.9.1a1
People
(Reporter: surkov, Assigned: surkov)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
9.84 KB,
patch
|
neil
:
superreview+
|
Details | Diff | Splinter Review |
take into account separators in xul menus when group attributes are calculating
Assignee | ||
Comment 1•16 years ago
|
||
Attachment #328499 -
Flags: superreview?(neil)
Attachment #328499 -
Flags: review?(marco.zehe)
Assignee | ||
Updated•16 years ago
|
Status: NEW → ASSIGNED
Comment 2•16 years ago
|
||
Comment on attachment 328499 [details] [diff] [review] patch r=me for the functionality part, and from what I know of the code, it also looks good. But we'll count on Neil's sr to catch anything obvious.
Attachment #328499 -
Flags: review?(marco.zehe) → review+
Comment 3•16 years ago
|
||
Comment on attachment 328499 [details] [diff] [review] patch >- nsCOMPtr<nsIDOMXULElement> currItem; >- container->GetItemAtIndex(index, getter_AddRefs(currItem)); >- nsCOMPtr<nsIDOMNode> currNode(do_QueryInterface(currItem)); >+ nsCOMPtr<nsIDOMXULElement> item; >+ container->GetItemAtIndex(index, getter_AddRefs(item)); >+ nsCOMPtr<nsIDOMNode> currNode(do_QueryInterface(item)); I'm not sure why you bothered with the rename. I feel sure I've said this before, but I feel you should be able to pass item to GetAccessibleFor without having to explicitly QI it to nsIDOMNode first. >+ PRUint32 itemRole = nsAccessible::Role(itemAcc); itemAcc could be null, is Role null-safe? >+ if (itemRole == nsIAccessibleRole::ROLE_SEPARATOR) { >+ // It's not our group. Continue the searching. I'd say // We need to start a new group. >+ continue; > } >+ >+ if (itemAcc && !(nsAccessible::State(itemAcc) & >+ nsIAccessibleStates::STATE_INVISIBLE)) { I'm tempted to suggest using } else if instead of continue; } if although there's not much difference in readability. I actually think it would be more efficient to calculate the position in set by starting at the child index and counting back down to 0 or a separator.
Assignee | ||
Comment 4•16 years ago
|
||
Attachment #328499 -
Attachment is obsolete: true
Attachment #328618 -
Flags: superreview?(neil)
Attachment #328499 -
Flags: superreview?(neil)
Assignee | ||
Comment 5•16 years ago
|
||
(In reply to comment #3) > (From update of attachment 328499 [details] [diff] [review]) > >- nsCOMPtr<nsIDOMXULElement> currItem; > >- container->GetItemAtIndex(index, getter_AddRefs(currItem)); > >- nsCOMPtr<nsIDOMNode> currNode(do_QueryInterface(currItem)); > >+ nsCOMPtr<nsIDOMXULElement> item; > >+ container->GetItemAtIndex(index, getter_AddRefs(item)); > >+ nsCOMPtr<nsIDOMNode> currNode(do_QueryInterface(item)); > I'm not sure why you bothered with the rename. Just below I use "itemAcc", "itemRole". Those I think are more consistent with "item" rather than with "currItem" another comments are addressed in the new patch.
Comment 6•16 years ago
|
||
Comment on attachment 328618 [details] [diff] [review] patch2 >+ break; // We reached the begin of our group. Nit: beginning
Attachment #328618 -
Flags: superreview?(neil) → superreview+
Assignee | ||
Comment 7•16 years ago
|
||
checked in http://hg.mozilla.org/mozilla-central/index.cgi/rev/38ce8d8d0167
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•16 years ago
|
Flags: in-testsuite+
Comment 8•16 years ago
|
||
Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a1pre) Gecko/2008071003 Minefield/3.1a1pre.
Status: RESOLVED → VERIFIED
Updated•16 years ago
|
Target Milestone: --- → mozilla1.9.1a1
You need to log in
before you can comment on or make changes to this bug.
Description
•