Bug 35986 - VoiceOver can navigate to hidden content in widget (WAI-ARIA)
Summary: VoiceOver can navigate to hidden content in widget (WAI-ARIA)
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-03-10 14:32 PST by chris fleizach
Modified: 2010-03-11 12:37 PST (History)
6 users (show)

See Also:


Attachments
Patch (20.63 KB, patch)
2010-03-10 14:37 PST, 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-03-10 14:32:06 PST
VoiceOver can navigate to hidden content because aria-hidden is not respected on elements don't use AccessibilityRenderObject::accessibilityIsIgnored
Comment 1 chris fleizach 2010-03-10 14:37:22 PST
Created attachment 50439 [details]
Patch
Comment 2 Beth Dakin 2010-03-10 14:51:23 PST
Comment on attachment 50439 [details]
Patch

Generally, I think this patch is good, so r=me. However, I don't like having two functions called accessibilityIsIgnored() and accessibilityIsIgnoredCommon(). I wish the names of these functions made the differences between the two more obvious. Maybe we could think of better names. Chris, could you explain what you think the difference is between the two, and perhaps that will inspire better names?
Comment 3 chris fleizach 2010-03-10 15:17:52 PST
so AccessibilityRenderObject ::accessibilityIsIgnored() has all kinds of logic in it to deal with the various roles that AccessibilityRenderObject comprises (web areas, text fields, links, images, etc). that logic would not work and be wasted if it was called by AccessibilityTable, which has its own logic to determine ignoring.

however, both those cases need to know some common things that would apply to any elements. namely, aria-hidden, presentational status and whether the platform says an element should be ignored.

so there's "common" ignored logic and then "AccessibilityRenderObject" specific ignored logic.

i had the hardest time with the naming too... so i don't have any great ideas. here are some that came to mind

accessibilityIgnoredBase
baselineAccessibilityIgnored
accessibillityIgnoredBaseline
Comment 4 Beth Dakin 2010-03-10 16:15:18 PST
accessibilityIgnoredBase

I think I like this one best.
Comment 5 chris fleizach 2010-03-10 16:36:37 PST
http://trac.webkit.org/changeset/55818
Comment 7 Beth Dakin 2010-03-10 23:22:23 PST
Looks like this caused a test failure on Leopard, Chris :-(
Comment 8 chris fleizach 2010-03-11 06:41:41 PST
i had fixed those test failures by the time of your comment i'm pretty sure. things should be ok by now
Comment 9 Gustavo Noronha (kov) 2010-03-11 11:19:24 PST
This change makes two regressions tests fail on GTK+:

accessibility/aria-hidden-with-elements.html
platform/gtk/accessibility/table-hierarchy.html

The first one looks like a new test, so failing it may not be bad, but the second one seems to be a real regression.
Comment 10 chris fleizach 2010-03-11 11:21:12 PST
it does look like a real regression. i'll take a close look at the diffs and see if i mismanaged a merge
Comment 11 chris fleizach 2010-03-11 11:27:17 PST
yea it looks like i made AccessibilityTableRow return the wrong thing on GTK. i'll fix this post-haste
Comment 12 chris fleizach 2010-03-11 11:58:44 PST
problem is that accessibilityIsIgnoredBase() no longer accounts for when the platform says to include the object. will make a new bug to change this behavior
Comment 13 chris fleizach 2010-03-11 11:59:38 PST
https://bugs.webkit.org/show_bug.cgi?id=36025
Comment 14 Gustavo Noronha (kov) 2010-03-11 12:35:16 PST
(In reply to comment #13)
> https://bugs.webkit.org/show_bug.cgi?id=36025

Thanks! Is this something you will be fixing soon, you think? If so, I think skipping the test for now, and unskipping it then is OK, but we should not leave this regression as is if it takes a while to correct, since we are planning a release in a few days, and should consider applying the usual policy of reverting on regressions.
Comment 15 chris fleizach 2010-03-11 12:37:44 PST
i'm uploading a patch now