Bug 56855 – [GTK] Provide a way in DRT to check the platform name

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Mario Sanchez Prada]

Currently, we're having a situation with how DRT outputs rolenames in GTK, since it's not always the same way that Mac does it, and this results in the need of keeping different expected files for accessibility layout tests just because that.

For instance, for a given a11y object exposing a link, we have this:

  - Mac's DRT: "AXRole: AXLink"
  - GTK's DRT: "AXRole: link" 

Thus, it would be great if we could make GTK's DRT to output the same string than Mac's, specially now that we're working on improving the WAI-ARIA support in WebKitGTK, easing unskipping already existing ones, instead of having to develop new ones for GTK only.

Adding to CC people that I think might be interested on this.

[Mario Sanchez Prada]

Just realized that this perhaps is not a good / feasible proposal since there's not a 1:1 match between WebCore roles and Mac role names (it's almost there, but it's not 100% accurate), nor between Mac and ATK role names, so it's gonna be difficult to reach a point whether we can use the same role names as the expected results for both the Mac and GTK platforms.

One option could be then, instead of trying to print in GTK the same output than for Mac, to try to reach some consensus and make the two platforms print platform-independent rolenames as expected results in the tests (maybe those from WebCore) through the role() method in the DRT, so both platforms would expect the same thing. 

However, I wonder if it makes sense printing rolenames that does not exist in one platform: e.g., printing 'AXGroup' in GTK when there's no such ATK_ROLE_GROUP role, or printing 'AXDocument' when it seems there's not a 'NSAccessibilityDocumentRole' in Mac (talking from my vast ignorance in the Mac platform)...

Perhaps at the end it will be better to just have platform-dependant tests for a11y stuff as, after all, a11y implementations are quite different between the two platforms...

I'll keep the bug open, though, so the debate keeps open too... this could be an interesting topic to talk about next month in person in the WK contributors meeting anyway :-)

[chris fleizach]

(In reply to comment #1)

You have enumerated part of the problem. There's ultimately logic in the Mac wrapper and I imagine gtk as well that we want to test in many cases. Other ideas I have

Have a method like isMac() isGTK() so we can conditionalize in the test
Have another mapping method in DRT that took the platform output and converted to platform neutral role

> Just realized that this perhaps is not a good / feasible proposal since there's not a 1:1 match between WebCore roles and Mac role names (it's almost there, but it's not 100% accurate), nor between Mac and ATK role names, so it's gonna be difficult to reach a point whether we can use the same role names as the expected results for both the Mac and GTK platforms.
> 
> One option could be then, instead of trying to print in GTK the same output than for Mac, to try to reach some consensus and make the two platforms print platform-independent rolenames as expected results in the tests (maybe those from WebCore) through the role() method in the DRT, so both platforms would expect the same thing. 
> 
> However, I wonder if it makes sense printing rolenames that does not exist in one platform: e.g., printing 'AXGroup' in GTK when there's no such ATK_ROLE_GROUP role, or printing 'AXDocument' when it seems there's not a 'NSAccessibilityDocumentRole' in Mac (talking from my vast ignorance in the Mac platform)...
> 
> Perhaps at the end it will be better to just have platform-dependant tests for a11y stuff as, after all, a11y implementations are quite different between the two platforms...
> 
> I'll keep the bug open, though, so the debate keeps open too... this could be an interesting topic to talk about next month in person in the WK contributors meeting anyway :-)

[Mario Sanchez Prada]

(In reply to comment #2)
> (In reply to comment #1)
> 
> You have enumerated part of the problem. There's ultimately logic in the Mac 
> wrapper and I imagine gtk as well that we want to test in many cases. 

Yes, we have specific logic in the GTK wrapper to map those WebCore roles to ATK ones, and then we can get an string with the name for those ATK roles through atk_role_get_name(). However, in the mac platform I can see the mapping is more complex, since it not only maps webcore roles to mac roles, but also to subroles and role descriptions which is something we currently lack in the gtk platform because it's not something used in ATK/AT-SPI.

> Have a method like isMac() isGTK() so we can conditionalize in the test

You mean in DRT, right? so we can actually use that isMac() or isGTK() in the Javascript code when calling to the shouldBe() functions?

Still, we wouldn't be able to get rid of the platform-specific expected files, but at least we would need only one test (html) file.

> Have another mapping method in DRT that took the platform output and 
> converted to platform neutral role

That sounds good to me as well, although it would mean having to maintain an extra mapping in Mac and GTK's DRTs, unless we decide using one of the platform rolenames as the neutral ones, in which case I'd obviously choose Mac ones, since they're almost a 1:1 match with WebCore ones.

The benefit of that, in case it's actually feasible, would be that wouldn't require any change in Mac's code, nor in current layout tests, but just adding a mapping in GTK's DRT (not even in the wrapper, since we don't need that info in ATK/AT-SPI).

[chris fleizach]

(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > 
> > You have enumerated part of the problem. There's ultimately logic in the Mac 
> > wrapper and I imagine gtk as well that we want to test in many cases. 
> 
> Yes, we have specific logic in the GTK wrapper to map those WebCore roles to ATK ones, and then we can get an string with the name for those ATK roles through atk_role_get_name(). However, in the mac platform I can see the mapping is more complex, since it not only maps webcore roles to mac roles, but also to subroles and role descriptions which is something we currently lack in the gtk platform because it's not something used in ATK/AT-SPI.
> 

Yea, we'd need another step to convert from the platform name, back to a common name. That would be a mess to maintain though

> > Have a method like isMac() isGTK() so we can conditionalize in the test
> 
> You mean in DRT, right? so we can actually use that isMac() or isGTK() in the Javascript code when calling to the shouldBe() functions?
> 

yea that would mean just one test, maybe different expectations, but maybe not always.

> Still, we wouldn't be able to get rid of the platform-specific expected files, but at least we would need only one test (html) file.
> 

> > Have another mapping method in DRT that took the platform output and 
> > converted to platform neutral role
> 
> That sounds good to me as well, although it would mean having to maintain an extra mapping in Mac and GTK's DRTs, unless we decide using one of the platform rolenames as the neutral ones, in which case I'd obviously choose Mac ones, since they're almost a 1:1 match with WebCore ones.
> 
> The benefit of that, in case it's actually feasible, would be that wouldn't require any change in Mac's code, nor in current layout tests, but just adding a mapping in GTK's DRT (not even in the wrapper, since we don't need that info in ATK/AT-SPI).

[Mario Sanchez Prada]

(In reply to comment #4)
[...]
> > > Have a method like isMac() isGTK() so we can conditionalize in the test
> > 
> > You mean in DRT, right? so we can actually use that isMac() or isGTK() in the 
> > Javascript code when calling to the shouldBe() functions?
> > 
> 
> yea that would mean just one test, maybe different expectations, but maybe not 
> always.

Another thing I thought of while working on bug 57826 (and which I actually used in the currently proposed patch in there) is not to use role names at all in the tests, that is in the HTML files.

Instead, why not just dumping the role names directly in the output and let the actual check to be done in the expected files? At least that way we would only need one HTML file in some cases

[chris fleizach]

We can do that already if we don't want to use the shouldBe paradigm. The test won't be as pretty but some tests may fall into this
Category

(In reply to comment #5)
> (In reply to comment #4)
> [...]
> > > > Have a method like isMac() isGTK() so we can conditionalize in the test
> > > 
> > > You mean in DRT, right? so we can actually use that isMac() or isGTK() in the 
> > > Javascript code when calling to the shouldBe() functions?
> > > 
> > 
> > yea that would mean just one test, maybe different expectations, but maybe not 
> > always.
> 
> Another thing I thought of while working on bug 57826 (and which I actually used in the currently proposed patch in there) is not to use role names at all in the tests, that is in the HTML files.
> 
> Instead, why not just dumping the role names directly in the output and let the actual check to be done in the expected files? At least that way we would only need one HTML file in some cases

[Mario Sanchez Prada]

(In reply to comment #6)
> We can do that already if we don't want to use the shouldBe paradigm. The test won't be as pretty but some tests may fall into this
> Category

Yes, that's what I meant: use this kind of approach for platform-dependent code in the tests, so we just need to provide different expected files in these situations, leaving the html file untouched.

Obviously, there will be situations where having different tests will be mandatory (e.g. exposing different hierarchies, as it happens with tables), but for the rest of the cases I think this kind of writing the tests will be positive.

[Mario Sanchez Prada]

Created attachment 88630 [details]
Patch for fixing this (asking for opinion)

Hi again Chris,

Inspired by your suggestion of adding something like isMac()/isGTK() methods to DRT, I wrote the attached patch this morning, which would basically add a new property in the layoutController so we could easily check, from cross-platform layout tests, the name of the platform in use: "gtk", "mac", "wx"...

I found it specially useful when working on unskipping the accessibility/input-slider.html test, which has a part that doesn't apply to GTK (GTK won't expose objects of role SliderThumbRole), but the rest could be the same, providing we do not check rolenames in the test itself either, but just print them and check them in the expected files. See bug 58040 for more details.

Anyway, I'm not currently asking for review over this patch but more for an opinion (that's why I didn't mark bug 58040 as depending on this one). Well, and also to check through the EWS whether I broke or not compilation in other platforms, since I did changes for platforms other than GTK blindly.

If we agree it could be a good idea, then I would come with a proper patch, with a proper Changelog and perhaps with a new bug just for it (unless we found 'just ok' to use this one for that purpose, which I guess could be ok as well).

[chris fleizach]

I think this is a good idea, this is what I needed to do a number of times but had to resort to other games to make a test work on multiple platforms. 

Given that DRT for accessibility ultimately needs to interact with the platform specific AX API, it's helpful to make that distinction in the test.

Others may disagree, so I'm including Darin to offer his opinion

[Mario Sanchez Prada]

(In reply to comment #9)
> I think this is a good idea, this is what I needed to do a number of times but 
> had to resort to other games to make a test work on multiple platforms. 

Well, the idea was actually yours. No big surprise here :-)

> Given that DRT for accessibility ultimately needs to interact with the platform 
> specific AX API, it's helpful to make that distinction in the test.
>
> Others may disagree, so I'm including Darin to offer his opinion

Thank you for your support. Of course this won't be the silver bullet for all the cases (we'll still need to have platform specific tests anyway), but a very convenient tool IMHO for those cases were 95% of the test would be the same for different platforms, with just very small differences (e.g. objects not being exposed in a platform).

Giving the early feedback and re-reading the whole thread in this bug, I think it makes sense to change its name and provide a patch asking for review over it (so EWS will actually run, as a plus). Changing the name now, attaching the "reviewable patch" in some minutes.

Thanks

[Mario Sanchez Prada]

Created attachment 88635 [details]
Patch proposal

Attaching "reviewable" patch

[Adam Roben (:aroben)]

Comment on attachment 88635 [details]
Patch proposal

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

You should add a test that shows this is working. You should also explain why this is needed.

> Tools/DumpRenderTree/LayoutTestController.cpp:2093
> +    return JSValueMakeString(context, controller->platformName());

This is leaking the JSStringRef returned by platformName. platformName should return a JSRetainPtr<JSStringRef> instead.

[Mario Sanchez Prada]

Created attachment 95452 [details]
Patch proposal

(In reply to comment #12)
> (From update of attachment 88635 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88635&action=review
> 
> You should add a test that shows this is working. 

Well I haven't done it since it's a patch for a new functionality in DRT, not in WebKit, so I thought it wouldn't be needed since its functionality would be tested anytime it was used in a LayoutTest :-)

> You should also explain why this is needed.

See comment #8 and comment #10. It's all in there :-)

> > Tools/DumpRenderTree/LayoutTestController.cpp:2093
> > +    return JSValueMakeString(context, controller->platformName());
> 
> This is leaking the JSStringRef returned by platformName. platformName should return a JSRetainPtr<JSStringRef> instead.

Thanks for the catch. Attaching a new patch fixing these issues.

[chris fleizach]

(In reply to comment #13)
> Created an attachment (id=95452) [details]
> Patch proposal
> 
> (In reply to comment #12)
> > (From update of attachment 88635 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=88635&action=review
> > 
> > You should add a test that shows this is working. 
> 
> Well I haven't done it since it's a patch for a new functionality in DRT, not in WebKit, so I thought it wouldn't be needed since its functionality would be tested anytime it was used in a LayoutTest :-)
> 

I still think we should have a layout test to test this, otherwise we might find out that this doesn't work before we get to use it


> > You should also explain why this is needed.
> 
> See comment #8 and comment #10. It's all in there :-)
> 
> > > Tools/DumpRenderTree/LayoutTestController.cpp:2093
> > > +    return JSValueMakeString(context, controller->platformName());
> > 
> > This is leaking the JSStringRef returned by platformName. platformName should return a JSRetainPtr<JSStringRef> instead.
> 
> Thanks for the catch. Attaching a new patch fixing these issues.

[Mario Sanchez Prada]

(In reply to comment #14)
> [...]
> I still think we should have a layout test to test this, otherwise we might 
> find out that this doesn't work before we get to use it

Ok, although I must admit I do not have a clue about where under LayoutTests to place a test like this... any suggestion?

[chris fleizach]

(In reply to comment #15)
> (In reply to comment #14)
> > [...]
> > I still think we should have a layout test to test this, otherwise we might 
> > find out that this doesn't work before we get to use it
> 
> Ok, although I must admit I do not have a clue about where under LayoutTests to place a test like this... any suggestion?

I think sticking one in accessibility is fine

[Mario Sanchez Prada]

Created attachment 95894 [details]
Patch proposal + Layout test

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > [...]
> > > I still think we should have a layout test to test this, otherwise we might 
> > > find out that this doesn't work before we get to use it
> > 
> > Ok, although I must admit I do not have a clue about where under LayoutTests to place a test like this... any suggestion?
> 
> I think sticking one in accessibility is fine

Sounds good to me, specially taking into account that perhaps accessibility tests will be the only ones that will be using it, in the short term at least.

Actually, after thinking a little bit about this suggestion of putting it under accessibility/ I thought that it could be a good idea also, based on the same reasoning, to provide this feature only to the platforms that would be using it, that is, mac, win and gtk (after all, there's no point in adding code to platforms that never will use it). 

As a result, I dropped from this new patch all the code related to implementing this new function in chromium, qt and wx platforms, and secured the changes in the toplevel LayoutTestController.[cpp|h] files through #ifdef guards.

Also, provided expectations to other ports apart from Gtk, that is, win and mac.

[Mario Sanchez Prada]

Comment on attachment 95452 [details]
Patch proposal

Forgot to mark this as obsolete

[Mario Sanchez Prada]

Committed r88432: <http://trac.webkit.org/changeset/88432>