Bug 39324 - AX: WebKit doesn't call [super -accessibilityAttributeValue:attribute forParameter:] when it encounters a parameterized attribute that it doesn't handle.
Summary: AX: WebKit doesn't call [super -accessibilityAttributeValue:attribute forPara...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: chris fleizach
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-18 15:27 PDT by chris fleizach
Modified: 2010-06-01 12:10 PDT (History)
5 users (show)

See Also:


Attachments
patch (1.96 KB, patch)
2010-05-18 15:46 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
Patch (46.75 KB, patch)
2010-05-27 16:05 PDT, chris fleizach
cfleizach: review-
Details | Formatted Diff | Diff
Patch (2.22 KB, patch)
2010-05-28 11:16 PDT, chris fleizach
bdakin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chris fleizach 2010-05-18 15:27:01 PDT
WebKit doesn't call [super -accessibilityAttributeValue:attribute forParameter:] when it encounters a parameterized attribute that it doesn't handle.
Comment 1 chris fleizach 2010-05-18 15:46:02 PDT
Created attachment 56417 [details]
patch
Comment 2 Darin Adler 2010-05-18 16:02:15 PDT
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.
Comment 3 chris fleizach 2010-05-18 16:04:34 PDT
(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)
Comment 4 WebKit Commit Bot 2010-05-21 07:10:25 PDT
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
Comment 5 chris fleizach 2010-05-27 09:22:27 PDT
If this causes failure on leopard, I'll have to see whats the backtrace. 
http://trac.webkit.org/changeset/60307
Comment 6 chris fleizach 2010-05-27 09:45:17 PDT
it looks like on tiger this idiom doesn't work as documented
Comment 7 chris fleizach 2010-05-27 09:46:32 PDT
and on leopard it looks like it crashes. whereas on tiger it times out a few tests
Comment 8 chris fleizach 2010-05-27 09:54:09 PDT
working on fix for tiger and leopard...
Comment 9 chris fleizach 2010-05-27 10:00:19 PDT
conferring with appkit that this should not work in leopard and before, but will work going forward
Comment 11 chris fleizach 2010-05-27 10:26:49 PDT
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)
Comment 12 chris fleizach 2010-05-27 12:19:23 PDT
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
Comment 13 chris fleizach 2010-05-27 16:05:02 PDT
Created attachment 57279 [details]
Patch
Comment 14 chris fleizach 2010-05-28 09:22:10 PDT
Comment on attachment 57279 [details]
Patch

i'll break this up into smaller patches
Comment 15 chris fleizach 2010-05-28 11:12:33 PDT
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
Comment 16 chris fleizach 2010-05-28 11:16:04 PDT
Created attachment 57347 [details]
Patch
Comment 17 Beth Dakin 2010-06-01 11:25:30 PDT
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 ;-)
Comment 18 chris fleizach 2010-06-01 11:27:45 PDT
(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?
Comment 19 Beth Dakin 2010-06-01 11:32:41 PDT
I think the comment is fine in one place :-)
Comment 20 chris fleizach 2010-06-01 11:34:17 PDT
(In reply to comment #19)
> I think the comment is fine in one place :-)

thanx. will make the change
Comment 21 chris fleizach 2010-06-01 12:10:27 PDT
http://trac.webkit.org/changeset/60496