Bug 139581 - AX: [ATK] Use ATK_RELATION_MEMBER_OF to implement linkedUIElements for radio button group members.
Summary: AX: [ATK] Use ATK_RELATION_MEMBER_OF to implement linkedUIElements for radio ...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-12-12 03:04 PST by Andrzej Badowski
Modified: 2017-03-11 10:48 PST (History)
7 users (show)

See Also:


Attachments
proposed patch (11.73 KB, patch)
2014-12-13 04:50 PST, Andrzej Badowski
no flags Details | Formatted Diff | Diff
proposed patch 2 (11.92 KB, patch)
2014-12-29 04:17 PST, Andrzej Badowski
no flags Details | Formatted Diff | Diff
proposed patch 2 (11.92 KB, patch)
2014-12-29 05:46 PST, Andrzej Badowski
no flags Details | Formatted Diff | Diff
proposed patch 2 (11.92 KB, patch)
2014-12-30 02:02 PST, Andrzej Badowski
cfleizach: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrzej Badowski 2014-12-12 03:04:36 PST
I would like to be discussed the use of the ATK_RELATION_MEMBER_OF in order to expose relationship AccessibilityRenderObject::linkedUIElements.
Comment 1 Andrzej Badowski 2014-12-13 04:50:34 PST
Created attachment 243255 [details]
proposed patch
Comment 2 Joanmarie Diggs 2014-12-15 01:19:39 PST
It looks to me like ATK_RELATION_MEMBER_OF is applicable for a group of radio buttons because those radio buttons are all a member of a group. However, I question (quite a bit) its applicability for an internal anchor connection.
Comment 3 Andrzej Badowski 2014-12-16 01:01:13 PST
(In reply to comment #2)
> It looks to me like ATK_RELATION_MEMBER_OF is applicable for a group of
> radio buttons because those radio buttons are all a member of a group.
> However, I question (quite a bit) its applicability for an internal anchor
> connection.

Thank you for your feedback. I understand your concerns about internal anchor link, I had it as well. but: 
1. There is no other relationship ATK more appropriate to do so. 
2. The linkedUIElements treats both objects together.
Comment 4 chris fleizach 2014-12-16 11:38:25 PST
Comment on attachment 243255 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=243255&action=review

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1046
> +    return roleValue() == RadioButtonRole || (isLink() && internalLinkElement());

how come isLink gets this attribute?
i don't think there are any tests which test the isLink part
Comment 5 Joanmarie Diggs 2014-12-16 13:21:02 PST
(In reply to comment #3)
> (In reply to comment #2)
> > It looks to me like ATK_RELATION_MEMBER_OF is applicable for a group of
> > radio buttons because those radio buttons are all a member of a group.
> > However, I question (quite a bit) its applicability for an internal anchor
> > connection.
> 
> Thank you for your feedback. I understand your concerns about internal
> anchor link, I had it as well. but: 
> 1. There is no other relationship ATK more appropriate to do so. 

Assuming there is a use case for that, then the correct solution would be to file a bug against ATK requesting a new AtkRelation type; not to lump non-members together with members. 

> 2. The linkedUIElements treats both objects together.

Just because a particular toolkit or engine does something does not make it appropriate for all platforms.

Taking the above into account, for GTK please do NOT include internal anchors in the member-of AtkRelation type.
Comment 6 Joanmarie Diggs 2014-12-16 13:21:28 PST
(In reply to comment #3)
> (In reply to comment #2)
> > It looks to me like ATK_RELATION_MEMBER_OF is applicable for a group of
> > radio buttons because those radio buttons are all a member of a group.
> > However, I question (quite a bit) its applicability for an internal anchor
> > connection.
> 
> Thank you for your feedback. I understand your concerns about internal
> anchor link, I had it as well. but: 
> 1. There is no other relationship ATK more appropriate to do so. 

Assuming there is a use case for that, then the correct solution would be to file a bug against ATK requesting a new AtkRelation type; not to lump non-members together with members. 

> 2. The linkedUIElements treats both objects together.

Just because a particular toolkit or engine does something does not make it appropriate for all platforms.

Taking the above into account, for GTK please do NOT include internal anchors in the member-of AtkRelation type.
Comment 7 Alejandro Piñeiro 2014-12-17 04:27:35 PST
(In reply to comment #3)
> (In reply to comment #2)
> > It looks to me like ATK_RELATION_MEMBER_OF is applicable for a group of
> > radio buttons because those radio buttons are all a member of a group.
> > However, I question (quite a bit) its applicability for an internal anchor
> > connection.
> 
> Thank you for your feedback. I understand your concerns about internal
> anchor link, I had it as well. but: 
> 1. There is no other relationship ATK more appropriate to do so. 

First, as Joanmarie said, if you think that an appropiate relationship is missing, then the way to go is asking for a new ATK relation. Relationships has meaning, that different ATs use in order to know how to present information to the users. ATK_RELATION_MEMBER has a meaning, and it is not to include anchors.

Second, I really don't think that a relationship is needed here. Relations, at least in ATK, are used to relate two (or more) different objects, because is a convenient way to get one based on the other. But for the case of links/anchors, that is not needed because ATK API for links already contemplate this:

https://developer.gnome.org/atk/unstable/AtkHyperlink.html#atk-hyperlink-get-object

So you can already get the anchors from an Hypertext document without a relationship.

> 2. The linkedUIElements treats both objects together.

I know that one of the objectives of the test is also detecting things that are still not implemented on the ports, but in this case, seems that is more like that the current test infrastructure is forcing a specific accessibility implementation, that is not one of the objectives. As I said, ATK hyperlink/hypertext API already provides to ATs the way to get the individual anchors, without the need of adding a relationship. Adding the relationship sounds like adding something that ATs doesn't need, so it is just complicating the implementation (imho).
Comment 8 Andrzej Badowski 2014-12-17 05:59:03 PST
(In reply to comment #7)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > It looks to me like ATK_RELATION_MEMBER_OF is applicable for a group of
> > > radio buttons because those radio buttons are all a member of a group.
> > > However, I question (quite a bit) its applicability for an internal anchor
> > > connection.
> > 
> > Thank you for your feedback. I understand your concerns about internal
> > anchor link, I had it as well. but: 
> > 1. There is no other relationship ATK more appropriate to do so. 
> 
> First, as Joanmarie said, if you think that an appropiate relationship is
> missing, then the way to go is asking for a new ATK relation. Relationships
> has meaning, that different ATs use in order to know how to present
> information to the users. ATK_RELATION_MEMBER has a meaning, and it is not
> to include anchors.
> 
> Second, I really don't think that a relationship is needed here. Relations,
> at least in ATK, are used to relate two (or more) different objects, because
> is a convenient way to get one based on the other. But for the case of
> links/anchors, that is not needed because ATK API for links already
> contemplate this:
> 
> https://developer.gnome.org/atk/unstable/AtkHyperlink.html#atk-hyperlink-get-
> object
> 
> So you can already get the anchors from an Hypertext document without a
> relationship.
> 
> > 2. The linkedUIElements treats both objects together.
> 
> I know that one of the objectives of the test is also detecting things that
> are still not implemented on the ports, but in this case, seems that is more
> like that the current test infrastructure is forcing a specific
> accessibility implementation, that is not one of the objectives. As I said,
> ATK hyperlink/hypertext API already provides to ATs the way to get the
> individual anchors, without the need of adding a relationship. Adding the
> relationship sounds like adding something that ATs doesn't need, so it is
> just complicating the implementation (imho).
Thank you for checking and tips on how to solve the problem.
Comment 9 Andrzej Badowski 2014-12-17 06:00:16 PST
Comment on attachment 243255 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=243255&action=review

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1046
>> +    return roleValue() == RadioButtonRole || (isLink() && internalLinkElement());
> 
> how come isLink gets this attribute?
> i don't think there are any tests which test the isLink part

You're right. I should prepare a suitable test.
Comment 10 Radar WebKit Bug Importer 2014-12-17 11:22:38 PST
<rdar://problem/19281517>
Comment 11 Andrzej Badowski 2014-12-29 04:17:50 PST
Created attachment 243792 [details]
proposed patch 2

This patch takes into account negative positions of debaters as to the ATK_RELATION_MEMBER_OF applicability for the internal link.
In the current version ATK_RELATION_MEMBER_OF is restricted to the radio button group.
Comment 12 Andrzej Badowski 2014-12-29 05:46:23 PST
Created attachment 243794 [details]
proposed patch 2
Comment 13 Andrzej Badowski 2014-12-30 02:02:25 PST
Created attachment 243819 [details]
proposed patch 2

attaching another patch 2 due to  efl-WK2 build errors not associated with the patch
Comment 14 Andrzej Badowski 2015-01-09 01:40:01 PST
The function mentioned by Alejandro (atk-hyperlink-get-object) returns an object that is a visual representation of internal link (text or picture) rather than the object pointed to by the link. Hence, I would like to temporarily suspend this type of relationship and reduce the patch to a group of radiobuttons.
Getting to the object pointed to by the link requires several steps and I'm going to deal with this in the future.
What do you think about this, Joanie ?
Comment 15 chris fleizach 2016-08-17 08:59:39 PDT
this patch is almost two years old. I don't know why its languished so long, but marking as r- so it can at least be re-based