Closed Bug 761102 Opened 12 years ago Closed 12 years ago

focus may be missed when ARIA active-descendant is changed on active composite widget

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(1 file)

Attached patch patchSplinter Review
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #629732 - Flags: review?(trev.saunders)
Attachment #629732 - Flags: feedback?(dbolter)
Attachment #629732 - Flags: feedback?(dbolter) → feedback+
Comment on attachment 629732 [details] [diff] [review]
patch

>+  if (mRoleMapEntry && mRoleMapEntry->Is(nsGkAtoms::combobox)) {

I'm not really sure why that seems better than checking mRolemapEntry->role, but it seems fine.

>+    PRUint32 childCount = ChildCount();
>+    for (PRUint32 idx = 0; idx < childCount; idx++) {
>+      Accessible* child = mChildren.ElementAt(idx);
>+      if (child->Role() == roles::ENTRY)
>+        return FocusMgr()->HasDOMFocus(child->GetContent());

I'd say this is fine, but couldn't somebody do something crazy like

<div role="combobox">
<div><input></div>
<ul>
<li>blah</li>
</ul>
</div>

for which we'd need to search the subtree

nsGkAtoms::aria_activedescendant, id)) {
>-      nsIDocument* DOMDoc = aElm->OwnerDoc();
>-      dom::Element* activeDescendantElm = DOMDoc->GetElementById(id);
>+      dom::Element* activeDescendantElm = aElm->OwnerDoc()->GetElementById(id);

so it doesn't actually have to be a descendant?  or is this just faster because the doc has a fast mapping of id -> elem?

/test_focus_aria_activedescendant.html
>+++ b/accessible/tests/mochitest/events/test_focus_aria_activedescendant.html
>@@ -65,16 +65,20 @@ https://bugzilla.mozilla.org/show_bug.cg
> 
>     var gQueue = null;
>     function doTest()
>     {
>       gQueue = new eventQueue();
> 
>       gQueue.push(new synthFocus("container", new focusChecker("item1")));
>       gQueue.push(new changeARIAActiveDescendant("container", "item2"));
>+
>+      gQueue.push(new synthFocus("combobox_entry", new focusChecker("combobox_entry")));
>+      gQueue.push(new changeARIAActiveDescendant("combobox", "combobox_option2"));
>+
>       todo(false, "No focus for inserted element, bug 687011");
>       //gQueue.push(new insertItemNFocus("container", "item3"));
> 
>       gQueue.invoke(); // Will call SimpleTest.finish();
>     }
> 
>     SimpleTest.waitForExplicitFinish();
>     addA11yLoadEvent(doTest);
>@@ -82,19 +86,32 @@ https://bugzilla.mozilla.org/show_bug.cg
> </head>
> <body>
> 
>   <a target="_blank"
>      href="https://bugzilla.mozilla.org/show_bug.cgi?id=429547"
>      title="Support aria-activedescendant usage in nsIAccesible::TakeFocus()">
>     Mozilla Bug 429547
>   </a>
>+  <a target="_blank"
>+     href="https://bugzilla.mozilla.org/show_bug.cgi?id=761102"
>+     title="Focus may be missed when ARIA active-descendant is changed on active composite widget">
>+    Mozilla Bug 761102
>+  </a>
>   <p id="display"></p>
>   <div id="content" style="display: none"></div>
>   <pre id="test">
>   </pre>
> 
>   <div role="listbox" aria-activedescendant="item1" id="container" tabindex="1">
>     <div role="listitem" id="item1">item1</div>
>     <div role="listitem" id="item2">item2</div>
>   </div>
>+
>+  <div role="combobox" id="combobox">
>+    <input id="combobox_entry">
>+    <ul>
>+      <li role="option" id="combobox_option1">option1</li>
>+      <li role="option" id="combobox_option2">option2</li>
>+    </ul>
>+  </div>
> </body>
> </html>
Attachment #629732 - Flags: review?(trev.saunders) → review+
(In reply to Trevor Saunders (:tbsaunde) from comment #2)

> >+  if (mRoleMapEntry && mRoleMapEntry->Is(nsGkAtoms::combobox)) {
> 
> I'm not really sure why that seems better than checking mRolemapEntry->role,
> but it seems fine.

a bit faster, constants instead strings

> >+    PRUint32 childCount = ChildCount();
> >+    for (PRUint32 idx = 0; idx < childCount; idx++) {
> >+      Accessible* child = mChildren.ElementAt(idx);
> >+      if (child->Role() == roles::ENTRY)
> >+        return FocusMgr()->HasDOMFocus(child->GetContent());
> 
> I'd say this is fine, but couldn't somebody do something crazy like
> 
> <div role="combobox">
> <div><input></div>
> <ul>
> <li>blah</li>
> </ul>
> </div>
> 
> for which we'd need to search the subtree

the author can do anything, the spec says that example is possible but doesn't provide the exact rules. If somebody has a reason to do deep trees then we can fix it.

> nsGkAtoms::aria_activedescendant, id)) {
> >-      nsIDocument* DOMDoc = aElm->OwnerDoc();
> >-      dom::Element* activeDescendantElm = DOMDoc->GetElementById(id);
> >+      dom::Element* activeDescendantElm = aElm->OwnerDoc()->GetElementById(id);
> 
> so it doesn't actually have to be a descendant?  or is this just faster
> because the doc has a fast mapping of id -> elem?

iirc I filed bug about this but on the another hand the spec allows this iirc.
http://hg.mozilla.org/integration/mozilla-inbound/rev/4168b071661e
Flags: in-testsuite+
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/4168b071661e
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
(In reply to alexander :surkov from comment #3)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> 
> > >+  if (mRoleMapEntry && mRoleMapEntry->Is(nsGkAtoms::combobox)) {
> > 
> > I'm not really sure why that seems better than checking mRolemapEntry->role,
> > but it seems fine.
> 
> a bit faster, constants instead strings

I meant the a11y::Role which I believe would actually be epsilon faster since it would be a compare of 2 dwords not 2 qwords.
(In reply to Trevor Saunders (:tbsaunde) from comment #6)
> (In reply to alexander :surkov from comment #3)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > 
> > > >+  if (mRoleMapEntry && mRoleMapEntry->Is(nsGkAtoms::combobox)) {
> > > 
> > > I'm not really sure why that seems better than checking mRolemapEntry->role,
> > > but it seems fine.
> > 
> > a bit faster, constants instead strings
> 
> I meant the a11y::Role which I believe would actually be epsilon faster
> since it would be a compare of 2 dwords not 2 qwords.

ah, I see, I guess I wanted to operate on ARIA roles since in general ARIA role might be not 1 to 1 equivalence to accessible role. I don't have strong opinion on this though.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: