Bug 153696 – REGRESSION(r195463): [GTK] accessibility/roles-computedRoleString.html and accessibility/roles-exposed.html failing

• Component: Accessibility
• Status: RESOLVED FIXED

[Michael Catanzaro]

accessibility/roles-computedRoleString.html and accessibility/roles-exposed.html have been failing on the GTK port since r195463.

I bet they just need rebaselined, but AX folks please check.

[Radar WebKit Bug Importer]

<rdar://problem/24423458>

[Mario Sanchez Prada]

(In reply to comment #0)
> accessibility/roles-computedRoleString.html and
> accessibility/roles-exposed.html have been failing on the GTK port since
> r195463.
> 
> I bet they just need rebaselined, but AX folks please check.

Almost, with the exception of the <pre> elements that should still be mapped to ATK_ROLE_SECTION and not ATK_ROLE_PANEL (as they will be now that the core role is GroupRole).

I'll prepare a patch including the small change and the rebaseline needed

[Mario Sanchez Prada]

Actually, I think it's probably better to keep the newly added isStyleFormatGroup() function limited to the MAC plaftorm (where it was aimed for anyway), since that way we can keep checking against the PreRole without having to look for the tag name from the different places we're doing that now already.

Besides, making that code MAC-only will probably fix these tests too, maybe without needing to rebaseline them at all.

[Mario Sanchez Prada]

Created attachment 270382 [details]
Patch proposal

Ready. Chris, could you take a quick look to make sure the PLATFORM(MAC) guards make sense to you too?

Thanks

[Darin Adler]

Comment on attachment 270382 [details]
Patch proposal

What’s the real concept here? Is the notion of a style format group specific to Mac, as opposed to say, iOS? Is the issue really about something different from the Mac from every other platform?

[Michael Catanzaro]

Comment on attachment 270382 [details]
Patch proposal

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

> LayoutTests/platform/gtk/accessibility/roles-computedRoleString-expected.txt:51
> +FAIL: pre -> . Expected: group.

It's too confusing to have FAIL print in our expected results; we should do this less often. Can you try reworking the test to avoid this?

[chris fleizach]

(In reply to comment #5)
> Comment on attachment 270382 [details]
> Patch proposal
> 
> What’s the real concept here? Is the notion of a style format group specific
> to Mac, as opposed to say, iOS? Is the issue really about something
> different from the Mac from every other platform?

I think we can expose this on iOS too without problem. 

I guess GTK does not want this behavior though

[Mario Sanchez Prada]

(In reply to comment #5)
> Comment on attachment 270382 [details]
> Patch proposal
> 
> What’s the real concept here? Is the notion of a style format group specific
> to Mac, as opposed to say, iOS? Is the issue really about something
> different from the Mac from every other platform?

The main problem is that Mac (and iOS, I think) uses the role GroupRole and a specific subrole to expose the nature of certain element like <pre> to Assistive technologies but, in contrast, ATK does not have this concept of subrole, and will therefore expose all those elements as ATK_ROLE_PANEL (since that's that GroupRole maps into), which at least for <pre> elements is wrong (they should be exposed as ATK_ROLE_SECTION).

One solution, ATK-specific, is to have the ATK wrapper to check the tagName for GroupRole elements and then decide whether to expose them as ATK_ROLE_PANEL or ATK_ROLE_SECTION, but since the PreRole was still around and Chris's patch seemed to be Mac-specific in a way, I thought that making that distinction (exposing the role of those elements via GroupRole + Subrole) only on the Mac, would make sense, hence the patch.

But if that's unnacceptable, we could certainly rework the patch to make the distinction at the ATK level, that would be another option too.

(In reply to comment #7)
> (In reply to comment #5)
> > Comment on attachment 270382 [details]
> > Patch proposal
> > 
> > What’s the real concept here? Is the notion of a style format group specific
> > to Mac, as opposed to say, iOS? Is the issue really about something
> > different from the Mac from every other platform?
> 
> I think we can expose this on iOS too without problem. 
> 
> I guess GTK does not want this behavior though

As mentioned above, the two key concepts for the GTK port are that (1) we don't have subroles (so we can't expose things using a GrouRole + specific subrole) and that (2) we want to expose <pre> elements with role ATK_ROLE_SECTION (so we still need to distinguish one GroupRole elements from another).

That said, let me know if you prefer an ATK-specific solution and I'll rework the code to have the ATK wrapper check the tag name for GroupRole elements. But in that case, I think we should probably remove all trace of the PreRole definition and usage, since the newly added code basically prevents that role from ever being exposed, I believe. 

(In reply to comment #6)
> Comment on attachment 270382 [details]
> Patch proposal
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270382&action=review
> 
> > LayoutTests/platform/gtk/accessibility/roles-computedRoleString-expected.txt:51
> > +FAIL: pre -> . Expected: group.
> 
> It's too confusing to have FAIL print in our expected results; we should do
> this less often. Can you try reworking the test to avoid this?

I agree it's definitely confusing, but it's hard to rework since the data-role attribute states GroupRole, which is what correct for the Mac, so you can't really fix it for one platform without screwing things up the other. We could make these platform specific tests, but that would fork/duplicate so many role tests unnecessarily IMHO, so that's why I think that, in this particular case, a single FAIL expectation for the <pre> element does the job well enough.

Maybe Joanie has a different point of view, suggestion, though. Joanie, ideas?

[Mario Sanchez Prada]

(In reply to comment #6)
> Comment on attachment 270382 [details]
> Patch proposal
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270382&action=review
> 
> > LayoutTests/platform/gtk/accessibility/roles-computedRoleString-expected.txt:51
> > +FAIL: pre -> . Expected: group.
> 
> It's too confusing to have FAIL print in our expected results; we should do
> this less often. Can you try reworking the test to avoid this?

I spoke to Joanie over IRC and she agrees that the current situation is broken in ATK, but not only due to <pre> elements being exposed as ATK_ROLE_PANEL, but also due to inline elements that should be exposed as ATK_ROLE_STATIC being also exposed as ATK_ROLE_PANEL, because now they are all GroupRoles internally, plus a specific subrole that is not used in the ATK role.

So, we think that restoring the old behaviour is the way to go, and probably making the code Mac-specific is the way to go, since those changes do only make sense in a context where role+subrole are used.

Now about the test expectation: Joanie agrees on whatever the computed role string is printed out is not really relevant as long as that gets exposed as an ATK_ROLE_SECTION in the ATK world, so perhaps the right thing to do here is to adapt the WebKitGTK's WKTR so that it prints the right thing in there (after all, it was printing a '.' before).

If nobody has any objection, I'll probably work into a new patch in that direction (keeping isStyleFormatGroup() Mac-specific + adapting WKTR) changes tomorrow morning.

[Joanmarie Diggs]

So looking a bit more at this.... I'm not convinced that the answer is keeping this Mac-specific and adding platform guards.

The notion of a "style format group" is not necessarily a Mac thing. In fact, I could make a case for always wanting to expose the style format group elements in ATK. So if they start showing up in the ATK/AT-SPI2 tree, that's fine with me -- as long as they are mapped correctly, which is not the case currently.

On the Mac, style format group is apparently always exposed now and mapped to WebCore Accessibility's GroupRole.

In ATK, a style format group which is not a RenderBlock and which had some property necessitating exposure was mapped as InlineRole prior to this change. InlineRole in ATK maps to ATK_ROLE_STATIC. A format group which is a RenderBlock (which seems to be just the 'pre' element at the moment) can be exposed as GroupRole as long as it gets mapped as ATK_ROLE_SECTION. We already have this situation for required descendant elements within tables and lists with role="presentation"/"none". This is just another case IMHO.

So instead of always returning GroupRole if isStyleFormatGroup() is true, could you return InlineRole if it's a RenderInline and GroupRole if it's a RenderBlock and then adjust the platform-specific role mappings in the platform-specific code?

As for other comments which have been made:

While we don't have subroles (yet) in ATK, we do expose the element tag as an object attribute, so ATs can already customize handling of ATK_ROLE_STATIC objects that are style formatting groups.

As for the computed role test. Were it me, I wouldn't bother with it beyond accepting the new result. As I understand it, the results in that test correspond with the results you see in WebInspector and contain the ARIA role equivalent for the element; it's not a platform thang.

[Mario Sanchez Prada]

I'm terribly sorry about this. I haven't forgotten about it but simply did not find the right time yet to get into it yet, even though I kept a pinned tab open all these days thinking I could have done it sooner...

Truth is, I don't know when I'm going to be able to do it in the short term, so I'm mentioning this now explicitly in case someone else wants to pick it up, in which case I'd be more than happy to help reviewing it.

Of course, should nobody picked it up, I'd still keep it in my TODO and would try to get back to it when possible, ideally soon. No promises, though.

Thanks and apologies for setting the wrong expectations in the first place.

[Joanmarie Diggs]

Created attachment 275778 [details]
Patch

[Joanmarie Diggs]

Comment on attachment 275778 [details]
Patch

Chris: Please review when you have a chance. Should be a quick one. (And it will make it possible to unskip the roles-exposed.html test on my platform. That's a critical test to have working....)

Thanks!

[WebKit Commit Bot]

Comment on attachment 275778 [details]
Patch

Clearing flags on attachment: 275778

Committed r199110: <http://trac.webkit.org/changeset/199110>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.