[Gtk] Implement support for get_index_in_parent

• Component: Accessibility
• Status: RESOLVED FIXED

[Joanmarie Diggs]

Steps to reproduce:

1. Launch Epiphany and navigate to google.com

2. In Accerciser's tree on the left, select/highlight the accessible link for "Maps"

3. In Accerciser's iPython Console, type:

    acc.getIndexInParent()

Expected results: 3

Actual results: 0

Note that this bug is not limited to Google or links or whathaveyou. That's merely an example. getIndexInParent seems to always return 0 for descendants of the document frame.

http://library.gnome.org/devel/atk/stable/AtkObject.html#atk-object-get-index-in-parent

[Xan Lopez]

Created attachment 32456 [details]
getindexinparent.patch

This implements the function, but the particular testcase you mention does not work because the object hierarchy is not quite right in Google's case (the Map object has a dummy 'label' parent).

[Joanmarie Diggs]

Created attachment 32463 [details]
test case

Thanks for looking at this so quickly!

I'll look/test more closely later tonight (after the DayJob), but from some quick testing I found at least one oddity:

1. Open the attached in Epiphany.

2. In Accerciser, expand the accessible associated with the list and highlight the first list item.

3. acc.getIndexInParent()

4. Choose the next list item.

5. acc.getIndexInParent()

Rinse and repeat for each immediate child of the list. Here's what I get:

1st child: 0
2nd child: 0 <-- should be 1
3rd child: 1 <-- should be 2
4th child: 2 <-- should be 3
5th child: 4
6th child: 5
7th child: 6

And, yes, given that there are 5 list items rather than 7, it would seem I've found another bug. :-) I'll file it shortly. But given the rendering of the list as having 7 children, I'd expect those children to have the indexes indicated above.

[Joanmarie Diggs]

(In reply to comment #1)
> Created an attachment (id=32456) [details]
> getindexinparent.patch
> 
> This implements the function, but the particular testcase you mention does not
> work because the object hierarchy is not quite right in Google's case (the Map
> object has a dummy 'label' parent).

Actually, my test case does work with your patch. :-) Ignore the "label" graphic in Accerciser. The Role column says it's a link. If I highlight the accessible of ROLE_LINKN for "Maps" (not the accessible text object which is the child of that link), acc.getIndexInParent() returns 3 as expected. Yea!

I did find another oddity on Google though. If you look at the hierarchy in Accerciser, you should see:

document frame
  panel
      text
      link 
      link
      link
      link
      link
      link
      link
  panel
  panel
  panel
  panel
  panel
  panel
  panel

(I expanded the first panel just to be sure we're on the same page so to speak.)

Looking strictly at the panels/immediate children of the document frame, try acc.getIndexInParent()

I get:
1st child: 0
2nd child: 1
3rd child: 0
4th child: 0
5th child: 1
6th child: 2
7th child: 4
8th child: 5

So independent of the list rendering issue I reported in my previous comment, I think there may be something not quite right about your patch. Sorry!

[Jan Alonzo]

Comment on attachment 32456 [details]
getindexinparent.patch

> -    // FIXME: This needs to be implemented.
> -    notImplemented();
> +    AccessibilityObject* coreObject = core(object);
> +    AccessibilityObject* parent = coreObject->parentObject();
> +
> +    g_return_val_if_fail (parent, 0);

Just a nit: this needs to be in "if" since we're dealing with WebCore objects.

I'll clear the review flag for now until we sort out Joanmarie's findings.

[Xan Lopez]

(In reply to comment #4)
> (From update of attachment 32456 [details])
> > -    // FIXME: This needs to be implemented.
> > -    notImplemented();
> > +    AccessibilityObject* coreObject = core(object);
> > +    AccessibilityObject* parent = coreObject->parentObject();
> > +
> > +    g_return_val_if_fail (parent, 0);
> 
> Just a nit: this needs to be in "if" since we're dealing with WebCore objects.

I don't understand what you mean here.

> 
> I'll clear the review flag for now until we sort out Joanmarie's findings.

Joannie, do you think it's better to land this as it is and fix things incrementally or would be this patch worse than no implementation?

[Joanmarie Diggs]

(In reply to comment #5)
> Joannie, do you think it's better to land this as it is and fix things
> incrementally or would be this patch worse than no implementation?

It's up to you. If you'd prefer to check in what you have, I'd be happy to open a new bug for the two issues I found.

From my perspective, since I cannot rely upon the return value, I am not going to implement anything in Orca that requires it.

[Joanmarie Diggs]

After staring at/working on hierarchy issues all this weekend, it dawned on me what the problem might be:

> +    AccessibilityObject* coreObject = core(object);
> +    AccessibilityObject* parent = coreObject->parentObject();

The parentObject() is from the dom. Instead use parentObjectUnignored().

I made the change and tested quite a bit. It works as expected.

[Xan Lopez]

Created attachment 41884 [details]
getindexinparent.patch

New version using parentObjectUnignored as proposed by Joannie.

[Gustavo Noronha (kov)]

Comment on attachment 41884 [details]
getindexinparent.patch


> +    g_return_val_if_fail (parent, 0);

Space before ( here.

> +    AccessibilityObject::AccessibilityChildrenVector children = parent->children();
> +    unsigned count = children.size();
> +    for (unsigned k = 0; k < count; ++k) {
> +        if (children[k] == coreObject)
> +            return k;
> +    }

k? i sounds more idiomatic ;D also, why not use children.size() directly instead of creating a variable?

Except for the nits, I see no obvious problem, and trust Joanmarie's judgement.

[Joanmarie Diggs]

Just noticed this:

** (GtkLauncher:3481): CRITICAL **: gint webkit_accessible_get_index_in_parent(AtkObject*): assertion `parent' failed

Web views claim to be orphans. Not a big deal, but we should address it. In separate patch of course. :-P

(I hacked around this same issue in webkit_accessible_get_parent.)

[Xan Lopez]

(In reply to comment #9)
> (From update of attachment 41884 [details])
> 
> > +    g_return_val_if_fail (parent, 0);
> 
> Space before ( here.

Right.

> 
> > +    AccessibilityObject::AccessibilityChildrenVector children = parent->children();
> > +    unsigned count = children.size();
> > +    for (unsigned k = 0; k < count; ++k) {
> > +        if (children[k] == coreObject)
> > +            return k;
> > +    }
> 
> k? i sounds more idiomatic ;D also, why not use children.size() directly
> instead of creating a variable?

Just a micro-optimization to not evaluate children.size() multiple times.

> 
> Except for the nits, I see no obvious problem, and trust Joanmarie's judgement.

Thanks!

[Xan Lopez]

(In reply to comment #10)
> Just noticed this:
> 
> ** (GtkLauncher:3481): CRITICAL **: gint
> webkit_accessible_get_index_in_parent(AtkObject*): assertion `parent' failed
> 
> Web views claim to be orphans. Not a big deal, but we should address it. In
> separate patch of course. :-P
> 
> (I hacked around this same issue in webkit_accessible_get_parent.)

Hrm. I also see get_parent is using parentObject instead of parentObjectUnignored? ;)

[Xan Lopez]

Comment on attachment 41884 [details]
getindexinparent.patch

Landed in r50134, leaving open to deal with the critical warning stuff.

[Joanmarie Diggs]

(In reply to comment #12)
> (In reply to comment #10)
> > Just noticed this:
> > 
> > ** (GtkLauncher:3481): CRITICAL **: gint
> > webkit_accessible_get_index_in_parent(AtkObject*): assertion `parent' failed
> > 
> > Web views claim to be orphans. Not a big deal, but we should address it. In
> > separate patch of course. :-P
> > 
> > (I hacked around this same issue in webkit_accessible_get_parent.)
> 
> Hrm. I also see get_parent is using parentObject instead of
> parentObjectUnignored? ;)

Heh. Good point. Prior to this weekend's excursion into Hierarchy Hell, I didn't fully get what the unignored bit was all about.

Bug 30817 opened to evaluate that situation (and possibly remove the hack of calling the parent class).

[Joanmarie Diggs]

I issue per bug. Spun off bug 31044 for the assertion. Closing this one.