Bug 52324 - REGRESSION: A problem with Voiceover and finding links
Summary: REGRESSION: A problem with Voiceover and finding links
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.6
: P1 Normal
Assignee: Nobody
URL: http://www.myponedesktop.com
Keywords: Regression
Depends on:
Blocks:
 
Reported: 2011-01-12 14:55 PST by Thomas Byskov Dalgaard
Modified: 2011-01-18 12:28 PST (History)
3 users (show)

See Also:


Attachments
Patch (9.71 KB, patch)
2011-01-14 16:00 PST, chris fleizach
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Byskov Dalgaard 2011-01-12 14:55:16 PST
A problem when using the latest version of Webkit with the Voiceover screenreader. If I go to the following URL and want to find  the link  "Run web client" I can not find it. However if I open the same page in Safari 5 (the latest version) the link is possible to find. Here is the steps to reproduce the error:
1. Go to the URL mentioned above.
2. Activate Voiceover by typing Command-F5 (if some  one uses a laptop add the FN-key to the keystroke).
2. Press CTRL-option and the letter u.
3. Press the right arrow until  you see or hear the word: Link.
4. Type: Ru (do this quickly)
5. You'll see that the link can't be found. Instead Voiceover detects the link named Features.
6. Try to open the Safari browser and do the same steps (1-5)
NOTE: You need to hit the escape key to exit the list of links (or as it is called in Voiceover the webroter).
Comment 1 chris fleizach 2011-01-13 10:05:58 PST
what URL are you going to.. i don't see it listed here
Comment 2 chris fleizach 2011-01-13 10:34:40 PST
rdar://8859923
Comment 3 chris fleizach 2011-01-14 09:15:24 PST
at the end of the the top <ul> is an iframe, that for some reason references the main web page, so VoiceOver gets into a loop
Comment 4 chris fleizach 2011-01-14 12:00:12 PST
part of the problem is that the iframe had a title tag on it, causing it to NOT be ignored, which meant the hierarchy was not created correctly for the sub-frame
Comment 5 chris fleizach 2011-01-14 12:57:22 PST
Here are the issues related
1) WebDynamicScrollBars should no longer be returned as an accessible item (since its taken care of by AccessibilityScrollView)
2) accessibilityIsIgnored, cannot query if the helpText is empty, before checking it it's an attachment (in this case the help text (title) was not empty, so the wrong element was being returned)
3) the hit point needs to be altered when hit test within the iframe again to be offset correctly
Comment 6 Thomas Byskov Dalgaard 2011-01-14 13:34:33 PST
Chris,
The issue  which I talked about is not only on that site. I  tried it e.g. on my Facebook mobile-site (which I prefer to use) and the same problem showed up. Voiceover was not able to track some links however I could find them by   scrolling down the page. However it worked until the update which came out a few days ago therefor I guess this might be a webkit related issue?
I have not been able to get the same issue in Safari however I tried on the same page and Voiceover finds the link exactly as it shell. So is the issue  below only for the URL I wrote earlier or a more general fix?

(In reply to comment #5)
> Here are the issues related
> 1) WebDynamicScrollBars should no longer be returned as an accessible item (since its taken care of by AccessibilityScrollView)
> 2) accessibilityIsIgnored, cannot query if the helpText is empty, before checking it it's an attachment (in this case the help text (title) was not empty, so the wrong element was being returned)
> 3) the hit point needs to be altered when hit test within the iframe again to be offset correctly
Comment 7 chris fleizach 2011-01-14 16:00:12 PST
Created attachment 79020 [details]
Patch
Comment 8 Darin Adler 2011-01-18 10:33:09 PST
Comment on attachment 79020 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79020&action=review

> Source/WebCore/accessibility/AccessibilityObject.cpp:1022
> +            IntRect frameRect = widget->frameRect();
> +            IntPoint newPoint = point;
> +            newPoint.setX(point.x() - frameRect.x());
> +            newPoint.setY(point.y() - frameRect.y());
> +            return axObjectCache()->getOrCreate(widget)->accessibilityHitTest(newPoint);

This can be written without doing X and Y separately:

    IntPoint newPoint = point - widget->frameRect().location();

Or even:

    return axObjectCache()->getOrCreate(widget)->accessibilityHitTest(point - widget->frameRect().location());

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1888
> +    // This should be the last check
> +    if (!helpText().isEmpty())
> +        return false;
>      
> -    return !m_renderer->isListMarker() && !isWebArea();
> +    return true;

If it is important that the helpText check be last then you *could* write it like this:

    return helpText().isEmpty();

Unless you think that’s less clear. Also, we prefer comments that are in sentence style.

Also, the comment needs to say why the helpText check should be last. The general rule of thumb is that comments say why and the code itself says what.
Comment 9 chris fleizach 2011-01-18 12:28:42 PST
http://trac.webkit.org/changeset/76044