Bug 157822 - AX: [ATK] Use WebCore Accessibility's AccessibilityText for AtkObject name and description
Summary: AX: [ATK] Use WebCore Accessibility's AccessibilityText for AtkObject name an...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Joanmarie Diggs (irc: joanie)
URL:
Keywords: InRadar
Depends on:
Blocks: 157187
  Show dependency treegraph
 
Reported: 2016-05-17 17:04 PDT by Joanmarie Diggs (irc: joanie)
Modified: 2016-05-18 00:11 PDT (History)
3 users (show)

See Also:


Attachments
Patch (78.92 KB, patch)
2016-05-17 18:10 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joanmarie Diggs (irc: joanie) 2016-05-17 17:04:53 PDT
WebKitGtk never switched fully over to use AccessibilityText for AtkObject name and description. As a result, we have code we don't need and which is in some cases incorrect with respect to the HTML AAM and/or AccName spec.

In addition, there are a number of places where we're taking the AXDescription as the AtkObject description. This would seem logical on the surface, but as outlined in the OR of bug 157187, it's wrong.

I'm making this bug a blocker to bug 157187 so we can fix the exposure and update the layout tests in a way that will hopefully be clear (enough) to verify the code change is not introducing any regressions. After that, we can (I will) come up with a proposal with respect to how we do cross-platform text alternative calculation layout tests.

Patch will be forthcoming.
Comment 1 Joanmarie Diggs (irc: joanie) 2016-05-17 18:10:44 PDT
Created attachment 279191 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2016-05-17 18:12:21 PDT
<rdar://problem/26337017>
Comment 3 Radar WebKit Bug Importer 2016-05-17 18:12:26 PDT
<rdar://problem/26337023>
Comment 4 Joanmarie Diggs (irc: joanie) 2016-05-17 18:14:57 PDT
Chris: This is mostly ripping out code from my port to defer to WebCore Accessibility -- and adding a bunch of if/else statements to our shared layout tests to ask for AXTitle+AXDescription on my port and AXDescription+AXHelp on yours. Can you live with the latter if I promise to clean it up in bug 157187? (Please.) :)
Comment 5 Radar WebKit Bug Importer 2016-05-17 18:15:46 PDT
<rdar://problem/26337077>
Comment 6 chris fleizach 2016-05-17 18:46:15 PDT
So the idea is that we'll undo all the platform if else in the other patch?
Comment 7 Joanmarie Diggs (irc: joanie) 2016-05-17 19:40:40 PDT
(In reply to comment #6)
> So the idea is that we'll undo all the platform if else in the other patch?

Not "undo" in the literal sense; but the if/else stuff is ugly. Some possibilities that spring to mind:

1. Adding something(s) to WKTR like: AccessibilityUIElement::textAlternative()

2. Adding something(s) to accessibility-helper.js to get the correct information from each platform.

3. Change ATK's AccessibilityUIElement so that it converts everything to the property names WebCore Accessibility and AXAPI use.

Of the three approaches, I'm least jazzed by the third because when there is not a one-to-one correspondence between our platforms, you wind up adding hacks and workarounds to fake it. In the specific case of text alternatives, I don't think it would be too horrible:

* AccessibilityUIElement::helpText() would return the AtkObject description
* AccessibilityUIElement::description() would return the AtkObject name

But:

* AccessibilityUIElement::title() would return ???

Almost all of the time, the spec's accessible name is exposed via your AXDescription. But occasionally doing so is wrong; AXTitle should be used instead. But on my platform, that wrongness doesn't exist: The spec's accessible name is always exposed via the AtkObject name.

What I like about approaches 1 and/or 2 is that all platforms have text alternatives with associated specifications. You and I both need to expose the spec's accessible name and the spec's accessible description. What we don't have in common is the exact properties used. I think approaches 1 and/or 2 can solve that.

As an example there are the w3c-svg-*-calculation.html tests. But that was done in each test. Moving that functionality to accessibility-helper.js or AccessibilityUIElement::textAlternative() would make it possible to get all the if/else platformName junk out of all the tests.

Thoughts?
Comment 8 chris fleizach 2016-05-17 23:22:30 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > So the idea is that we'll undo all the platform if else in the other patch?
> 
> Not "undo" in the literal sense; but the if/else stuff is ugly. Some
> possibilities that spring to mind:
> 
> 1. Adding something(s) to WKTR like:
> AccessibilityUIElement::textAlternative()
> 
> 2. Adding something(s) to accessibility-helper.js to get the correct
> information from each platform.
> 
> 3. Change ATK's AccessibilityUIElement so that it converts everything to the
> property names WebCore Accessibility and AXAPI use.
> 
> Of the three approaches, I'm least jazzed by the third because when there is
> not a one-to-one correspondence between our platforms, you wind up adding
> hacks and workarounds to fake it. In the specific case of text alternatives,
> I don't think it would be too horrible:
> 
> * AccessibilityUIElement::helpText() would return the AtkObject description
> * AccessibilityUIElement::description() would return the AtkObject name
> 
> But:
> 
> * AccessibilityUIElement::title() would return ???
> 
> Almost all of the time, the spec's accessible name is exposed via your
> AXDescription. But occasionally doing so is wrong; AXTitle should be used
> instead. But on my platform, that wrongness doesn't exist: The spec's
> accessible name is always exposed via the AtkObject name.
> 
> What I like about approaches 1 and/or 2 is that all platforms have text
> alternatives with associated specifications. You and I both need to expose
> the spec's accessible name and the spec's accessible description. What we
> don't have in common is the exact properties used. I think approaches 1
> and/or 2 can solve that.
> 
> As an example there are the w3c-svg-*-calculation.html tests. But that was
> done in each test. Moving that functionality to accessibility-helper.js or
> AccessibilityUIElement::textAlternative() would make it possible to get all
> the if/else platformName junk out of all the tests.
> 
> Thoughts?

Probably having some helper methods in ax-utilities.js would be the best approach to writing cross platform tests
Comment 9 Joanmarie Diggs (irc: joanie) 2016-05-17 23:36:57 PDT
(In reply to comment #8)
> Probably having some helper methods in ax-utilities.js would be the best
> approach to writing cross platform tests

Ok, I'll give that a shot. Thanks!

Do you want that done as part of the fix for this bug, or done in bug 157187?
Comment 10 chris fleizach 2016-05-17 23:37:26 PDT
Comment on attachment 279191 [details]
Patch

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

> LayoutTests/accessibility/alt-tag-on-image-with-nonimage-role.html:29
> +        if (accessibilityController.platformName == "atk") {

We should probably have some helper method that can handle these platform differences seemlessly

like

accessibleNameShouldBe(group)

But we can probably live with this for now
Comment 11 chris fleizach 2016-05-17 23:37:50 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > Probably having some helper methods in ax-utilities.js would be the best
> > approach to writing cross platform tests
> 
> Ok, I'll give that a shot. Thanks!
> 
> Do you want that done as part of the fix for this bug, or done in bug 157187?

Let's do that in another bug so this one gets in
Comment 12 Joanmarie Diggs (irc: joanie) 2016-05-17 23:50:22 PDT
(In reply to comment #11)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > Probably having some helper methods in ax-utilities.js would be the best
> > > approach to writing cross platform tests
> > 
> > Ok, I'll give that a shot. Thanks!
> > 
> > Do you want that done as part of the fix for this bug, or done in bug 157187?
> 
> Let's do that in another bug so this one gets in

I appreciate it. I hadn't realized how many mappings were incorrect. :-/

I'll get started on the clean up tomorrow (assuming you're on the left coast).
Comment 13 WebKit Commit Bot 2016-05-18 00:11:12 PDT
Comment on attachment 279191 [details]
Patch

Clearing flags on attachment: 279191

Committed r201072: <http://trac.webkit.org/changeset/201072>
Comment 14 WebKit Commit Bot 2016-05-18 00:11:16 PDT
All reviewed patches have been landed.  Closing bug.