Bug 39324 – AX: WebKit doesn't call [super -accessibilityAttributeValue:attribute forParameter:] when it encounters a parameterized attribute that it doesn't handle.

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

[chris fleizach]

WebKit doesn't call [super -accessibilityAttributeValue:attribute forParameter:] when it encounters a parameterized attribute that it doesn't handle.

[chris fleizach]

Created attachment 56417 [details]
patch

[Darin Adler]

Comment on attachment 56417 [details]
patch

> -        // got a parameter of a type we never use
> -        // NOTE: No ASSERT_NOT_REACHED because this can happen accidentally
> -        // while using accesstool (e.g.), forcing you to start over
> -        return nil;
> +        // Default to super if the parameter type is unknown.
> +        return [super accessibilityAttributeValue:attribute forParameter:parameter];

Is this right? It's a parameter we support, but a type we don't. I don't think calling super makes a lot of sense in this case, but I guess it's harmless.

> -    return nil;
> +    // Default to super if parameter is not supported.
> +    return [super accessibilityAttributeValue:attribute forParameter:parameter];

This seems clearly right.

[chris fleizach]

(In reply to comment #2)
> (From update of attachment 56417 [details])
> > -        // got a parameter of a type we never use
> > -        // NOTE: No ASSERT_NOT_REACHED because this can happen accidentally
> > -        // while using accesstool (e.g.), forcing you to start over
> > -        return nil;
> > +        // Default to super if the parameter type is unknown.
> > +        return [super accessibilityAttributeValue:attribute forParameter:parameter];
> 
> Is this right? It's a parameter we support, but a type we don't. I don't think calling super makes a lot of sense in this case, but I guess it's harmless.

At this point, it's still not clear if its a parameter we support. its only clear that its an unsupported type (but it might be an type supported by super)

[WebKit Commit Bot]

Comment on attachment 56417 [details]
patch

Rejecting patch 56417 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing/iframes', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found
Testing 18369 test cases.
accessibility/aria-tables.html -> crashed

Exiting early after 1 failures. 20 tests run.
171.06s total testing time

19 test cases (95%) succeeded
1 test case (5%) crashed

Full output: http://webkit-commit-queue.appspot.com/results/2276405

[chris fleizach]

If this causes failure on leopard, I'll have to see whats the backtrace. 
http://trac.webkit.org/changeset/60307

[chris fleizach]

it looks like on tiger this idiom doesn't work as documented

[chris fleizach]

and on leopard it looks like it crashes. whereas on tiger it times out a few tests

[chris fleizach]

working on fix for tiger and leopard...

[chris fleizach]

conferring with appkit that this should not work in leopard and before, but will work going forward

[WebKit Review Bot]

http://trac.webkit.org/changeset/60307 might have broken Tiger Intel Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/60305
http://trac.webkit.org/changeset/60306
http://trac.webkit.org/changeset/60307
http://trac.webkit.org/changeset/60308

[chris fleizach]

rolling out
http://trac.webkit.org/changeset/60310
until i have a better fix (which required more code than i thought, so i will open for review again)

[chris fleizach]

here's the deal

 we want to pass unknown attributes to super. 

However, if an unsupported attribute is passed in it raises an accessibility exception. Normally those are caught by the AX runtime so that it can inform the AX client (like VoiceOver). However, for these failing tests, DRT is the AX client, but its not catching those innocuous errors. 

DRT needs to catch those errors

[chris fleizach]

Created attachment 57279 [details]
Patch

[chris fleizach]

Comment on attachment 57279 [details]
Patch

i'll break this up into smaller patches

[chris fleizach]

with 
https://bugs.webkit.org/show_bug.cgi?id=39880
and
https://bugs.webkit.org/show_bug.cgi?id=39881

fixed, this patch should work

[chris fleizach]

Created attachment 57347 [details]
Patch

[Beth Dakin]

Comment on attachment 57347 [details]
Patch

I am going to r+ this, but do you think there is any particular reason why this should be a goto instead of a regular function call? I shy away from using gotos since there aren't a lot of them in the code, but since we don't have an official policy, r+ anyway. I would like it if you would answer my question though ;-)

[chris fleizach]

(In reply to comment #17)
> (From update of attachment 57347 [details])
> I am going to r+ this, but do you think there is any particular reason why this should be a goto instead of a regular function call? I shy away from using gotos since there aren't a lot of them in the code, but since we don't have an official policy, r+ anyway. I would like it if you would answer my question though ;-)

using the goto allowed be to consolidate the comment. i saw a few other uses of goto, so i figured it was acceptable. i'm generally against them as well, but it looked ok here. what do you think about the comment... is it ok just having it at the bottom, despite [super] being called in two places?

[Beth Dakin]

I think the comment is fine in one place :-)

[chris fleizach]

(In reply to comment #19)
> I think the comment is fine in one place :-)

thanx. will make the change

[chris fleizach]

http://trac.webkit.org/changeset/60496