VoiceOver can navigate to hidden content because aria-hidden is not respected on elements don't use AccessibilityRenderObject::accessibilityIsIgnored
Created attachment 50439 [details]
Comment on attachment 50439 [details]
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?
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
I think I like this one best.
This added a failure on Leopard:
Looks like this caused a test failure on Leopard, Chris :-(
i had fixed those test failures by the time of your comment i'm pretty sure. things should be ok by now
This change makes two regressions tests fail on GTK+:
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.
it does look like a real regression. i'll take a close look at the diffs and see if i mismanaged a merge
yea it looks like i made AccessibilityTableRow return the wrong thing on GTK. i'll fix this post-haste
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
(In reply to comment #13)
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.
i'm uploading a patch now