566260 - text accessibles shouldn't implement nsIAccessibleHyperLink

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

nsIAccessibleHyperText::GetLink() return embedded accessibles, however text accessibles implement nsIAccessibleHyperLink which is inconsistent. Also all nsIAccessibleHyperLink methods fails on text accessible currently.

Comment 1

[Marco Zehe (:MarcoZ)]

Comment on attachment 445645 [details] [diff] [review]
patch

r=me thanks!

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 445645 [details] [diff] [review]
patch

>diff --git a/accessible/src/atk/nsAccessibleWrap.cpp b/accessible/src/atk/nsAccessibleWrap.cpp
>--- a/accessible/src/atk/nsAccessibleWrap.cpp
>+++ b/accessible/src/atk/nsAccessibleWrap.cpp
>@@ -889,30 +889,27 @@ getChildCountCB(AtkObject *aAtkObj)
> AtkObject *
> refChildCB(AtkObject *aAtkObj, gint aChildIndex)
> {
>     // aChildIndex should not be less than zero
>     if (aChildIndex < 0) {
>       return nsnull;
>     }
> 
>-    // XXX Fix this so it is not O(n^2) to walk through the children!
>-    // Either we can cache the last accessed child so that we can just GetNextSibling()
>-    // or we should cache an array of children in each nsAccessible
>-    // (instead of mNextSibling on the children)
>     nsAccessibleWrap *accWrap = GetAccessibleWrap(aAtkObj);
>     if (!accWrap || nsAccUtils::MustPrune(accWrap)) {
>         return nsnull;
>     }
> 
>     nsCOMPtr<nsIAccessible> accChild;
>     nsCOMPtr<nsIAccessibleHyperText> hyperText;
>     accWrap->QueryInterface(NS_GET_IID(nsIAccessibleHyperText), getter_AddRefs(hyperText));
>     if (hyperText) {
>-        // If HyperText, then number of links matches number of children
>+        // If HyperText, then number of links matches number of children.

>+        // XXX Fix this so it is not O(n^2) to walk through the children!

I don't understand why this O(n^2) comment moved here?

>diff --git a/accessible/tests/mochitest/test_nsIAccessibleHyperLink.html b/accessible/tests/mochitest/test_nsIAccessibleHyperLink.html
>+++ b/accessible/tests/mochitest/test_nsIAccessibleHyperLink.html

>+      ok(!res, "Text accessible shouldn't implement nsIAccessibleHyperLink");

It is nice to see this test pass for this bug, but I'm not sure we need to continue testing this. What do you think?

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)

> >+        // If HyperText, then number of links matches number of children.
> 
> >+        // XXX Fix this so it is not O(n^2) to walk through the children!
> 
> I don't understand why this O(n^2) comment moved here?

GetLink() is O(n), they meant here 
for (var i = 0; i < count; i++)
  var child = parent.GetChildAt(i);

it's o(n^2)
 
> >+      ok(!res, "Text accessible shouldn't implement nsIAccessibleHyperLink");
> 
> It is nice to see this test pass for this bug, but I'm not sure we need to
> continue testing this. What do you think?

Why not? We test anything that can be broken.

Since fixed QueryInterface can be "fixed" or "cleaned up" easily in the future then it might make sense to have a test. At least it doesn't hit us.

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 445645 [details] [diff] [review]
patch

OK r=me thanks.

(In reply to comment #3)
> (In reply to comment #2)
> 
> > >+        // If HyperText, then number of links matches number of children.
> > 
> > >+        // XXX Fix this so it is not O(n^2) to walk through the children!
> > 
> > I don't understand why this O(n^2) comment moved here?
> 
> GetLink() is O(n), they meant here 
> for (var i = 0; i < count; i++)
>   var child = parent.GetChildAt(i);
> 
> it's o(n^2)

I see. Can you file a bug on nsHyperTextAccessible::GetLink and // XXX #### ?

> > >+      ok(!res, "Text accessible shouldn't implement nsIAccessibleHyperLink");
> > 
> > It is nice to see this test pass for this bug, but I'm not sure we need to
> > continue testing this. What do you think?
> 
> Why not? We test anything that can be broken.

Sure, but we should be aware of the costs. Time for a mochitest run slows us (everyone) down a little bit, so we should really be sure to test what is somewhat important ;)  I don't mind this test landing at least for now.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #4)

> Sure, but we should be aware of the costs. Time for a mochitest run slows us
> (everyone) down a little bit, so we should really be sure to test what is
> somewhat important ;)  I don't mind this test landing at least for now.

This makes sense, in general, not now perhaps, especially when we improve performance ;)

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> (From update of attachment 445645 [details] [diff] [review])

> I see. Can you file a bug on nsHyperTextAccessible::GetLink and // XXX #### ?

I think yes, however the problem is deeper, nsHyperTextAccessible is not performant.

Comment 7

[alexander :surkov (:asurkov)]

landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/31d0cc66956d (bug ref was added)