27011 2009-07-06 15:49:54 -0700 [Gtk] Implement support for get_index_in_parent 2009-11-02 22:04:00 -0800 1 1 1 Unclassified WebKit Accessibility 528+ (Nightly build) PC Linux RESOLVED FIXED Gtk P2 Normal --- 25531 0 jdiggs webkit-unassigned apinheiro walker.willie xan.lopez oldest_to_newest 130043 0 jdiggs 2009-07-06 15:49:54 -0700 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 130453 1 32456 xan.lopez 2009-07-08 10:26:43 -0700 Created attachment 32456 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). 130473 2 32463 jdiggs 2009-07-08 11:42:52 -0700 Created attachment 32463 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. 130481 3 jdiggs 2009-07-08 12:07:45 -0700 (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! 130751 4 32456 jmalonzo 2009-07-09 15:30:04 -0700 Comment on attachment 32456 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. 131125 5 xan.lopez 2009-07-11 12:17:04 -0700 (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? 131149 6 jdiggs 2009-07-11 16:22:15 -0700 (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. 157873 7 jdiggs 2009-10-26 12:41:28 -0700 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. 157880 8 41884 xan.lopez 2009-10-26 13:02:50 -0700 Created attachment 41884 getindexinparent.patch New version using parentObjectUnignored as proposed by Joannie. 157953 9 41884 gustavo 2009-10-26 15:05:14 -0700 Comment on attachment 41884 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. 157965 10 jdiggs 2009-10-26 15:28:17 -0700 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.) 158117 11 xan.lopez 2009-10-27 01:28:40 -0700 (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! 158119 12 xan.lopez 2009-10-27 01:34:37 -0700 (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? ;) 158127 13 41884 xan.lopez 2009-10-27 02:26:13 -0700 Comment on attachment 41884 getindexinparent.patch Landed in r50134, leaving open to deal with the critical warning stuff. 158169 14 jdiggs 2009-10-27 06:29:13 -0700 (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). 159988 15 jdiggs 2009-11-02 22:04:00 -0800 I issue per bug. Spun off bug 31044 for the assertion. Closing this one. 32456 2009-07-08 10:26:43 -0700 2009-10-26 13:02:50 -0700 getindexinparent.patch getindexinparent.patch text/plain 2368 xan.lopez RnJvbSBkNjI2NWU4NDRhYjRkZjY1YWMxYjAxZDZjMDZkOTY5Y2UyMzgwMTdhIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBYYW4gTG9wZXogPHhsb3BlekBpZ2FsaWEuY29tPgpEYXRlOiBX ZWQsIDggSnVsIDIwMDkgMjA6MTk6NDggKzAzMDAKU3ViamVjdDogW1BBVENIXSAyMDA5LTA3LTA4 ICBYYW4gTG9wZXogIDx4bG9wZXpAaWdhbGlhLmNvbT4KCiAgICAgICAgUmV2aWV3ZWQgYnkgTk9C T0RZIChPT1BTISkuCgogICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD0yNzAxMQogICAgICAgIFtHdGtdIEltcGxlbWVudCBzdXBwb3J0IGZvciBnZXRfaW5kZXhf aW5fcGFyZW50CgogICAgICAgIEltcGxlbWVudCBhdGtfb2JqZWN0X2dldF9pbmRleF9pbl9wYXJl bnQuCgogICAgICAgICogYWNjZXNzaWJpbGl0eS9ndGsvQWNjZXNzaWJpbGl0eU9iamVjdFdyYXBw ZXJBdGsuY3BwOgogICAgICAgICh3ZWJraXRfYWNjZXNzaWJsZV9nZXRfaW5kZXhfaW5fcGFyZW50 KToKLS0tCiBXZWJDb3JlL0NoYW5nZUxvZyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICB8ICAgMTIgKysrKysrKysrKysrCiAuLi4vZ3RrL0FjY2Vzc2liaWxpdHlPYmplY3RXcmFwcGVy QXRrLmNwcCAgICAgICAgICB8ICAgMTQgKysrKysrKysrKysrLS0KIDIgZmlsZXMgY2hhbmdlZCwg MjQgaW5zZXJ0aW9ucygrKSwgMiBkZWxldGlvbnMoLSkKCmRpZmYgLS1naXQgYS9XZWJDb3JlL0No YW5nZUxvZyBiL1dlYkNvcmUvQ2hhbmdlTG9nCmluZGV4IDNkN2E4MTYuLjNmMjM1NWYgMTAwNjQ0 Ci0tLSBhL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0yLDYg KzIsMTggQEAKIAogICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KIAorICAgICAg ICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjcwMTEKKyAgICAgICAg W0d0a10gSW1wbGVtZW50IHN1cHBvcnQgZm9yIGdldF9pbmRleF9pbl9wYXJlbnQKKworICAgICAg ICBJbXBsZW1lbnQgYXRrX29iamVjdF9nZXRfaW5kZXhfaW5fcGFyZW50LgorCisgICAgICAgICog YWNjZXNzaWJpbGl0eS9ndGsvQWNjZXNzaWJpbGl0eU9iamVjdFdyYXBwZXJBdGsuY3BwOgorICAg ICAgICAod2Via2l0X2FjY2Vzc2libGVfZ2V0X2luZGV4X2luX3BhcmVudCk6CisKKzIwMDktMDct MDggIFhhbiBMb3BleiAgPHhsb3BlekBpZ2FsaWEuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5 IE5PQk9EWSAoT09QUyEpLgorCiAgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3df YnVnLmNnaT9pZD0yNTQxNQogICAgICAgICBbR1RLXVtBVEtdIFBsZWFzZSBpbXBsZW1lbnQgc3Vw cG9ydCBmb3IgZ2V0X3RleHRfYXRfb2Zmc2V0CiAKZGlmZiAtLWdpdCBhL1dlYkNvcmUvYWNjZXNz aWJpbGl0eS9ndGsvQWNjZXNzaWJpbGl0eU9iamVjdFdyYXBwZXJBdGsuY3BwIGIvV2ViQ29yZS9h Y2Nlc3NpYmlsaXR5L2d0ay9BY2Nlc3NpYmlsaXR5T2JqZWN0V3JhcHBlckF0ay5jcHAKaW5kZXgg YzJhOTgyOC4uMWY2NDRjZCAxMDA2NDQKLS0tIGEvV2ViQ29yZS9hY2Nlc3NpYmlsaXR5L2d0ay9B Y2Nlc3NpYmlsaXR5T2JqZWN0V3JhcHBlckF0ay5jcHAKKysrIGIvV2ViQ29yZS9hY2Nlc3NpYmls aXR5L2d0ay9BY2Nlc3NpYmlsaXR5T2JqZWN0V3JhcHBlckF0ay5jcHAKQEAgLTE2Nyw4ICsxNjcs MTggQEAgc3RhdGljIEF0a09iamVjdCogd2Via2l0X2FjY2Vzc2libGVfcmVmX2NoaWxkKEF0a09i amVjdCogb2JqZWN0LCBnaW50IGluZGV4KQogCiBzdGF0aWMgZ2ludCB3ZWJraXRfYWNjZXNzaWJs ZV9nZXRfaW5kZXhfaW5fcGFyZW50KEF0a09iamVjdCogb2JqZWN0KQogewotICAgIC8vIEZJWE1F OiBUaGlzIG5lZWRzIHRvIGJlIGltcGxlbWVudGVkLgotICAgIG5vdEltcGxlbWVudGVkKCk7Cisg ICAgQWNjZXNzaWJpbGl0eU9iamVjdCogY29yZU9iamVjdCA9IGNvcmUob2JqZWN0KTsKKyAgICBB Y2Nlc3NpYmlsaXR5T2JqZWN0KiBwYXJlbnQgPSBjb3JlT2JqZWN0LT5wYXJlbnRPYmplY3QoKTsK KworICAgIGdfcmV0dXJuX3ZhbF9pZl9mYWlsIChwYXJlbnQsIDApOworCisgICAgQWNjZXNzaWJp bGl0eU9iamVjdDo6QWNjZXNzaWJpbGl0eUNoaWxkcmVuVmVjdG9yIGNoaWxkcmVuID0gcGFyZW50 LT5jaGlsZHJlbigpOworICAgIHVuc2lnbmVkIGNvdW50ID0gY2hpbGRyZW4uc2l6ZSgpOworICAg IGZvciAodW5zaWduZWQgayA9IDA7IGsgPCBjb3VudDsgKytrKSB7CisgICAgICAgIGlmIChjaGls ZHJlbltrXSA9PSBjb3JlT2JqZWN0KQorICAgICAgICAgICAgcmV0dXJuIGs7CisgICAgfQorCiAg ICAgcmV0dXJuIDA7CiB9CiAKLS0gCjEuNi4wLjYKCg== 32463 2009-07-08 11:42:52 -0700 2009-07-08 11:42:52 -0700 test case indexInParent.html text/html 426 jdiggs PGh0bWw+CjxoZWFkPgo8dGl0bGU+VGVzdDwvdGl0bGU+CjwvaGVhZD4KPGJvZHk+CjxwPkZpcnN0 IHBhcmFncmFwaDwvcD4KPHA+U2Vjb25kIHBhcmFncmFwaDwvcD4KPG9sPgo8bGk+Rmlyc3QgaXRl bTwvbGk+CjxsaT48cD5UaGlzIGl0ZW0gaXMgY29tcG9zZWQgb2YgdGhyZWUgcGFyYWdyYXBocy4g VGhpcyBvbmUuPC9wPgogICAgPHA+QW5kIHRoaXMgb25lLjwvcD4KICAgIDxwPkFuZCB0aGlzIG9u ZSwgd2hpY2ggaGFzIGEgY291cGxlIG9mIDxhIGhyZWY9ImZvbyI+bGlua3M8L2E+IGluIDxhIGhy ZWY9ImJhciI+aXQ8L2E+LjwvcD4KPC9saT4KPGxpPlRoaXJkIGl0ZW08L2xpPgo8bGk+PGEgaHJl Zj0iZm9vYmFyIj5Gb3VydGggaXRlbSBpcyBhIGxpbms8L2E+PC9saT4KPGxpPkZpZnRoIGl0ZW08 L2xpPgo8L29sPgo8L2JvZHk+CjwvaHRtbD4K 41884 2009-10-26 13:02:50 -0700 2009-10-27 02:26:13 -0700 getindexinparent.patch getindexinparent.patch text/plain 2293 xan.lopez RnJvbSBkNjE4NzA0MmZhYTgwZGNjOGE0NmYyOWVhMWQ3NDE2ZTFiNWZkYzljIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBYYW4gTG9wZXogPHhsb3BlekBpZ2FsaWEuY29tPgpEYXRlOiBN b24sIDI2IE9jdCAyMDA5IDIxOjU4OjM2ICswMjAwClN1YmplY3Q6IFtQQVRDSF0gMjAwOS0xMC0y NiAgWGFuIExvcGV6ICA8eGxvcGV6QGlnYWxpYS5jb20+CgogICAgICAgIFJldmlld2VkIGJ5IE5P Qk9EWSAoT09QUyEpLgoKICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9MjcwMTEKICAgICAgICBbR3RrXSBJbXBsZW1lbnQgc3VwcG9ydCBmb3IgZ2V0X2luZGV4 X2luX3BhcmVudAoKICAgICAgICBJbXBsZW1lbnQgYXRrX29iamVjdF9nZXRfaW5kZXhfaW5fcGFy ZW50LgoKICAgICAgICAqIGFjY2Vzc2liaWxpdHkvZ3RrL0FjY2Vzc2liaWxpdHlPYmplY3RXcmFw cGVyQXRrLmNwcDoKICAgICAgICAod2Via2l0X2FjY2Vzc2libGVfZ2V0X2luZGV4X2luX3BhcmVu dCk6Ci0tLQogV2ViQ29yZS9DaGFuZ2VMb2cgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgfCAgIDEyICsrKysrKysrKysrKwogLi4uL2d0ay9BY2Nlc3NpYmlsaXR5T2JqZWN0V3JhcHBl ckF0ay5jcHAgICAgICAgICAgfCAgIDE0ICsrKysrKysrKysrKy0tCiAyIGZpbGVzIGNoYW5nZWQs IDI0IGluc2VydGlvbnMoKyksIDIgZGVsZXRpb25zKC0pCgpkaWZmIC0tZ2l0IGEvV2ViQ29yZS9D aGFuZ2VMb2cgYi9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCBmY2Q5NDkzLi45ODAyMmE3IDEwMDY0 NAotLS0gYS9XZWJDb3JlL0NoYW5nZUxvZworKysgYi9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwz ICsxLDE1IEBACisyMDA5LTEwLTI2ICBYYW4gTG9wZXogIDx4bG9wZXpAaWdhbGlhLmNvbT4KKwor ICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBodHRwczovL2J1 Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjcwMTEKKyAgICAgICAgW0d0a10gSW1wbGVt ZW50IHN1cHBvcnQgZm9yIGdldF9pbmRleF9pbl9wYXJlbnQKKworICAgICAgICBJbXBsZW1lbnQg YXRrX29iamVjdF9nZXRfaW5kZXhfaW5fcGFyZW50LgorCisgICAgICAgICogYWNjZXNzaWJpbGl0 eS9ndGsvQWNjZXNzaWJpbGl0eU9iamVjdFdyYXBwZXJBdGsuY3BwOgorICAgICAgICAod2Via2l0 X2FjY2Vzc2libGVfZ2V0X2luZGV4X2luX3BhcmVudCk6CisKIDIwMDktMTAtMjYgIE1pa2UgQmVs c2hlICA8bWlrZUBiZWxzaGUuY29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5IERhcmluIEZpc2hl ci4KZGlmZiAtLWdpdCBhL1dlYkNvcmUvYWNjZXNzaWJpbGl0eS9ndGsvQWNjZXNzaWJpbGl0eU9i amVjdFdyYXBwZXJBdGsuY3BwIGIvV2ViQ29yZS9hY2Nlc3NpYmlsaXR5L2d0ay9BY2Nlc3NpYmls aXR5T2JqZWN0V3JhcHBlckF0ay5jcHAKaW5kZXggZTI4MjRkZC4uYzg2NTFkMCAxMDA2NDQKLS0t IGEvV2ViQ29yZS9hY2Nlc3NpYmlsaXR5L2d0ay9BY2Nlc3NpYmlsaXR5T2JqZWN0V3JhcHBlckF0 ay5jcHAKKysrIGIvV2ViQ29yZS9hY2Nlc3NpYmlsaXR5L2d0ay9BY2Nlc3NpYmlsaXR5T2JqZWN0 V3JhcHBlckF0ay5jcHAKQEAgLTIxOSw4ICsyMTksMTggQEAgc3RhdGljIEF0a09iamVjdCogd2Vi a2l0X2FjY2Vzc2libGVfcmVmX2NoaWxkKEF0a09iamVjdCogb2JqZWN0LCBnaW50IGluZGV4KQog CiBzdGF0aWMgZ2ludCB3ZWJraXRfYWNjZXNzaWJsZV9nZXRfaW5kZXhfaW5fcGFyZW50KEF0a09i amVjdCogb2JqZWN0KQogewotICAgIC8vIEZJWE1FOiBUaGlzIG5lZWRzIHRvIGJlIGltcGxlbWVu dGVkLgotICAgIG5vdEltcGxlbWVudGVkKCk7CisgICAgQWNjZXNzaWJpbGl0eU9iamVjdCogY29y ZU9iamVjdCA9IGNvcmUob2JqZWN0KTsKKyAgICBBY2Nlc3NpYmlsaXR5T2JqZWN0KiBwYXJlbnQg PSBjb3JlT2JqZWN0LT5wYXJlbnRPYmplY3RVbmlnbm9yZWQoKTsKKworICAgIGdfcmV0dXJuX3Zh bF9pZl9mYWlsIChwYXJlbnQsIDApOworCisgICAgQWNjZXNzaWJpbGl0eU9iamVjdDo6QWNjZXNz aWJpbGl0eUNoaWxkcmVuVmVjdG9yIGNoaWxkcmVuID0gcGFyZW50LT5jaGlsZHJlbigpOworICAg IHVuc2lnbmVkIGNvdW50ID0gY2hpbGRyZW4uc2l6ZSgpOworICAgIGZvciAodW5zaWduZWQgayA9 IDA7IGsgPCBjb3VudDsgKytrKSB7CisgICAgICAgIGlmIChjaGlsZHJlbltrXSA9PSBjb3JlT2Jq ZWN0KQorICAgICAgICAgICAgcmV0dXJuIGs7CisgICAgfQorCiAgICAgcmV0dXJuIDA7CiB9CiAK LS0gCjEuNi41LjEKCg==