WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
39324
AX: WebKit doesn't call [super -accessibilityAttributeValue:attribute forParameter:] when it encounters a parameterized attribute that it doesn't handle.
https://bugs.webkit.org/show_bug.cgi?id=39324
Summary
AX: WebKit doesn't call [super -accessibilityAttributeValue:attribute forPara...
chris fleizach
Reported
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.
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
chris fleizach
Comment 1
2010-05-18 15:46:02 PDT
Created
attachment 56417
[details]
patch
Darin Adler
Comment 2
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.
chris fleizach
Comment 3
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)
WebKit Commit Bot
Comment 4
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
chris fleizach
Comment 5
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
chris fleizach
Comment 6
2010-05-27 09:45:17 PDT
it looks like on tiger this idiom doesn't work as documented
chris fleizach
Comment 7
2010-05-27 09:46:32 PDT
and on leopard it looks like it crashes. whereas on tiger it times out a few tests
chris fleizach
Comment 8
2010-05-27 09:54:09 PDT
working on fix for tiger and leopard...
chris fleizach
Comment 9
2010-05-27 10:00:19 PDT
conferring with appkit that this should not work in leopard and before, but will work going forward
WebKit Review Bot
Comment 10
2010-05-27 10:25:14 PDT
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
Comment 11
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)
chris fleizach
Comment 12
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
chris fleizach
Comment 13
2010-05-27 16:05:02 PDT
Created
attachment 57279
[details]
Patch
chris fleizach
Comment 14
2010-05-28 09:22:10 PDT
Comment on
attachment 57279
[details]
Patch i'll break this up into smaller patches
chris fleizach
Comment 15
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
chris fleizach
Comment 16
2010-05-28 11:16:04 PDT
Created
attachment 57347
[details]
Patch
Beth Dakin
Comment 17
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 ;-)
chris fleizach
Comment 18
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?
Beth Dakin
Comment 19
2010-06-01 11:32:41 PDT
I think the comment is fine in one place :-)
chris fleizach
Comment 20
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
chris fleizach
Comment 21
2010-06-01 12:10:27 PDT
http://trac.webkit.org/changeset/60496
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug