Bug 104662 – [GTK][Mac] Share accessibility LayoutTest

• Status: UNCONFIRMED
• Component: Accessibility

[Mateusz Leszko]

Created attachment 178779 [details]
list of duplications

I am planing to add some test in EFL, but they suppose to be duplicated, already.

Some test are common in those ports. I propose to share them and skip if they're not supported on others.
Duplicated files in attachment.

[Mateusz Leszko]

Other proposition is to create/rename little revolutionary directory:
LayoutTests/platform/atk/
and place there tests from GTK.

Some test will need to be changed like
platform/gtk/accessibility/object-attributes.html
where we have line: "toolkit:WebKitGtk". It has to be changed to WebKitAtk or something like that.

[Alejandro Piñeiro]

(In reply to comment #1)
> Other proposition is to create/rename little revolutionary directory:
> LayoutTests/platform/atk/
> and place there tests from GTK.
> 
> Some test will need to be changed like
> platform/gtk/accessibility/object-attributes.html
> where we have line: "toolkit:WebKitGtk". It has to be changed to WebKitAtk or something like that.

CCing Joanmarie. That attribute was specifically requested by Orca, the GNOME Screen reader. I'm pretty sure that in this case, just renaming the attribute name would lead to regressions in the behaviour of Orca with WebkitGTK based browsers. Joanmarie can provide more information/proposals here.

[Joanmarie Diggs]

(In reply to comment #1)
> Other proposition is to create/rename little revolutionary directory:
> LayoutTests/platform/atk/
> and place there tests from GTK.
> 
> Some test will need to be changed like
> platform/gtk/accessibility/object-attributes.html
> where we have line: "toolkit:WebKitGtk". It has to be changed to WebKitAtk or something like that.

It will need to be conditionalized (toolkit:WebKitGtk and toolkit:WebKitEfl are AtkObject attributes)

[Mateusz Leszko]

> It will need to be conditionalized (toolkit:WebKitGtk and toolkit:WebKitEfl are AtkObject attributes)
Yes, It can be changed in WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:
attributeSet = addToAtkAttributeSet(attributeSet, "toolkit", "WebKitGtk");

[Joanmarie Diggs]

(In reply to comment #4)
> > It will need to be conditionalized (toolkit:WebKitGtk and toolkit:WebKitEfl are AtkObject attributes)
> Yes, It can be changed in WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:
> attributeSet = addToAtkAttributeSet(attributeSet, "toolkit", "WebKitGtk");

The code currently distinguishes between WebKitGtk versus WebKitEfl. Unless we know for a fact that this distinction will never become relevant, we can conditionalize the test (i.e. via testRunner.platformName) rather than change the code and report a different toolkit to ATs. Especially given the fact that the Layout tests don't check the AtkObject attributes all that often.

[Mateusz Leszko]

(In reply to comment #5)
> The code currently distinguishes between WebKitGtk versus WebKitEfl. Unless we know for a fact that this distinction will never become relevant, we can conditionalize the test (i.e. via testRunner.platformName) rather than change the code and report a different toolkit to ATs. Especially given the fact that the Layout tests don't check the AtkObject attributes all that often.

Yes, you are right. It can be usefull attribute for AT.
What do you think about common directory - atk?

[Joanmarie Diggs]

(In reply to comment #6)
> (In reply to comment #5)
> > The code currently distinguishes between WebKitGtk versus WebKitEfl. Unless we know for a fact that this distinction will never become relevant, we can conditionalize the test (i.e. via testRunner.platformName) rather than change the code and report a different toolkit to ATs. Especially given the fact that the Layout tests don't check the AtkObject attributes all that often.
> 
> Yes, you are right. It can be usefull attribute for AT.
> What do you think about common directory - atk?

That worked nicely for sharing the accessibility support and I assume should work equally nicely for sharing accessibility tests, with the every-once-in-a-while conditionalization I mentioned.

[Mateusz Leszko]

Created attachment 184194 [details]
patch proposition

I've moved some test from GTk and Mac to new catalog LayoutTests/accessibility/atk
and edited TestExpectations in mac, gtk and efl.

Please make opinion about that move.

[Mateusz Leszko]

Created attachment 184195 [details]
patch proposition

[Mario Sanchez Prada]

(In reply to comment #8)
> Created an attachment (id=184194) [details]
> patch proposition
> 
> I've moved some test from GTk and Mac to new catalog LayoutTests/accessibility/atk
> and edited TestExpectations in mac, gtk and efl.
> 
> Please make opinion about that move.

What is the reason for that move? I mean, are you sure you won't break other platforms (e.g. mac, chromium) by doing that move?

I agree with the renaming of gtk/ -> atk/, but I think moving many tests from LayoutTests/platform/[gtk|mac]/accessibility to LayoutTests/accessibility could bring undesired results (and in any case, they probably should go in a separate patch)

Besides, I don't see in the new patch any usage of testRunner.platformName. Perhaps you just don't need it after all?

My 2 cents

[Mateusz Leszko]

(In reply to comment #10)
> What is the reason for that move? I mean, are you sure you won't break other platforms (e.g. mac, chromium) by doing that move?
> 

No, I am not sure for mac. It has to be tested first.

> I agree with the renaming of gtk/ -> atk/, but I think moving many tests from LayoutTests/platform/[gtk|mac]/accessibility to LayoutTests/accessibility could bring undesired results (and in any case, they probably should go in a separate patch)
> 
> Besides, I don't see in the new patch any usage of testRunner.platformName. Perhaps you just don't need it after all?
> 

Yes it is needed. This is just proposition patch. I wanted to verify what do you think. After that platformName has to be added.

[chris fleizach]

Comment on attachment 184195 [details]
patch proposition

I don't think you should make a subfolder for a platform inside accessibility. that's what the platform folders are for.

[Mario Sanchez Prada]

(In reply to comment #12)
> (From update of attachment 184195 [details])
> I don't think you should make a subfolder for a platform inside accessibility. that's what the platform folders are for.

Ah! wait... I thought the change you were proposing was to rename platform/gtk to platform/atk (which was the original proposal in comment #1).

So, I agree with Chris on this.

[Mateusz Leszko]

But those are common test. Shouldn't we put them on common folder?

[Mario Sanchez Prada]

(In reply to comment #14)
> But those are common test. Shouldn't we put them on common folder?

The problem is that they are platform specific tests, and so they should live under LayoutTests/platform/<platform>, not under LayoutTests/accessibility/<platform>.

However, I've just realized of another problem with having a LayoutTests/platform/atk directory is that "atk" is not a valid platform name when it comes to tests (see "Platform options" section in the output of 'run-webkit-tests --help') and so I don't think this will magically work for GTK and EFL unless you also patch the run-webkit-tests script.

I guess all you would need to make it work that way would be to add the LayoutTests/platform/atk directory in _search_paths() both in layout_tests/port/gtk.py and layout_tests/port/efl.py, but you'd better double check that in case I' missing something.

[Mateusz Leszko]

(In reply to comment #15)
> (In reply to comment #14)
> > But those are common test. Shouldn't we put them on common folder?
> 
> The problem is that they are platform specific tests, and so they should live under LayoutTests/platform/<platform>, not under LayoutTests/accessibility/<platform>.
> 

We want to put test based on ATK accessibility in LayoutTests/accessibility/atk. I got this idea with consultation with Grzegorz Czajkowski and Krzysztof Czech. 

In b104926, Gyuyoung Kim seems likes this idea. Also Alejandro Piñeiro suggested me to put them in common directory(not exactly this).

I think putting them to LayoutTests/platform/atk can be too big thing. Cause atk is not platform name. Can you change your mind and agree to put them in accessibility/atk?

Alternatively maybe you have different idea where, and how put test for EFL, and not duplicate those from others platforms.

[chris fleizach]

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > But those are common test. Shouldn't we put them on common folder?
> > 
> > The problem is that they are platform specific tests, and so they should live under LayoutTests/platform/<platform>, not under LayoutTests/accessibility/<platform>.
> > 
> 
> We want to put test based on ATK accessibility in LayoutTests/accessibility/atk. I got this idea with consultation with Grzegorz Czajkowski and Krzysztof Czech. 
> 
> In b104926, Gyuyoung Kim seems likes this idea. Also Alejandro Piñeiro suggested me to put them in common directory(not exactly this).
> 
> I think putting them to LayoutTests/platform/atk can be too big thing. Cause atk is not platform name. Can you change your mind and agree to put them in accessibility/atk?
> 
> Alternatively maybe you have different idea where, and how put test for EFL, and not duplicate those from others platforms.

is accessibility the only thing that atk platforms share? it seems like there could be an atk folder in LayoutTests/platform that will hold these types. then ATK type platform would run the tests in their own platform and in Atk

[Mateusz Leszko]

(In reply to comment #17)
> is accessibility the only thing that atk platforms share? it seems like
> there could be an atk folder in LayoutTests/platform that will hold these
> types. then ATK type platform would run the tests in their own platform and
> in Atk
Yes, ATK is just accessibility thing. It is possible to create
LayoutTests/platform/atk dir and put test there, but atk is not a platform, and some of us may not like it.

[Mario Sanchez Prada]

(In reply to comment #18)
> (In reply to comment #17)
> > is accessibility the only thing that atk platforms share? it seems like
> > there could be an atk folder in LayoutTests/platform that will hold these
> > types. then ATK type platform would run the tests in their own platform and
> > in Atk
> Yes, ATK is just accessibility thing. It is possible to create
> LayoutTests/platform/atk dir and put test there, but atk is not a platform, 
> and some of us may not like it.

I agree (see comment #15) that atk is not really a platform, and that certainly makes it quite weird if we add a platform/atk directory (we can't do run-webkit-tests --atk after all, neither it would make sense IMHO)

In the other hand, however, atk is certainly not a WebKit feature either but a specific set of interfaces that are only used in some ports, and so I can't avoid feeling like and accessibility/atk directory (even if you make sure it's skipped in every platform but GTK anr EFL) is also weird.

Still I see the obvious gain of reusing the tests for the two platforms using the same a11y code, so it's clear it would be good if we found a solution here...

I personally think that this is a very peculiar case because accessibility itself is different in some ways to some other stuff in WebKit like rendering or multimedia for instance, regardless of the specific differences between platforms in those cases (specific mm backends or graphics libraries). At least in those cases the basic structure of the tests are normally the same (a DOM tree is the same DOM tree regardless of the platform).

However, in accessibility that's not always true: different ports offer different views of the accessibility *hierarchy* from WebCore, by means of the platform-specific a11y hierarchy (e.g. more or less "flattened") represented by all those wrapper objects. And so far this was not a problem because only GTK used that different structure, so it was easy to just put the tests expecting a different a11y hierarchy in the platform/gtk directory. But now the EFL port is also interested in that hierarchy, which is no longer a one-platform-specific thing, yet not a global thing to be considered either.

But still, at least in my opinion, is something leaning more towards the platform-specific worlds under LayoutTests/platform/ than towards the more global world that is just under LayoutTests/, so that's why I proposed that change in the first place: to create a platform/atk directory (yes, even if it's not a "true platform") and modify _search_paths() both in layout_tests/port/gtk.py and layout_tests/port/efl.py so the gtk and efl ports will run those tests, in a similar way to how other directories as gtk-wk1 and gtk-wk2 are loaded for GTK (even if gtk-wk1 and gtk-wk1 are not really "platforms"), or efl-wk1 and efl-wk2 for EFL.

Still, I know the simil is not 100% accurate either and beware I'm not really happy either about the platform/atk thing, but I think that's probably a better solution than just creating a LayoutTests/accessibility/atk directory just for the sake of sharing that stuff between these two platforms.

Anyway, this is not a strong opinion anyway. I'm just trying to be constructive and expose the reasoning behind my suggestion so you better understand why I'm proposing that. But I'm of course open to the other option you proposed (or any other) as long as we all are reasonably happy with the result.

Hope this helps.

[Mateusz Leszko]

Ok. I will put a patch soon with common folder like LayoutTests/platform/atk
will see how community will see that.