Bug 61289

Summary: AX WK2 Regression: WebKit outputs incorrect AX position in frames/iframes
Product: WebKit Reporter: chris fleizach <cfleizach>
Component: AccessibilityAssignee: chris fleizach <cfleizach>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, simon.fraser
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch
none
patch darin: review+

Description chris fleizach 2011-05-23 10:14:09 PDT
* STEPS TO REPRODUCE
1. Load http://public.yahoo.com/kloots/vo-safari-bugs/dialogs.html
2. Navigate the page with VO and interact with the iframe. 
3. Tab or use VO nav to move through the items.
Comment 1 chris fleizach 2011-05-23 10:14:26 PDT
The Problem is that on WK2, nested frames are not taken into account
Comment 2 chris fleizach 2011-05-23 10:14:49 PDT
rdar://9317698
Comment 3 chris fleizach 2011-05-23 10:24:02 PDT
Created attachment 94437 [details]
patch

It's still an ongoing project to figure out how to add layout tests for WK2 and AX
Comment 4 Simon Fraser (smfr) 2011-08-09 09:56:48 PDT
Comment on attachment 94437 [details]
patch

Why does accessibility have its own coordinate transform methods? Why not use Widget or RenderObject methods for this?
Comment 5 chris fleizach 2011-08-09 09:58:45 PDT
(In reply to comment #4)
> (From update of attachment 94437 [details])
> Why does accessibility have its own coordinate transform methods? Why not use Widget or RenderObject methods for this?

Because it needs to return flipped on-screen coordinates, and it has to come from the actual element itself. 

In WK2, RenderObject doesn't know where "on-screen" the element actually is, it's just told to paint somewhere and it's then placed correctly by the safari process. That's not good enough for accessibility
Comment 6 Simon Fraser (smfr) 2011-08-09 10:09:32 PDT
There are methods in widget to produce screen-relative coords.
Comment 7 chris fleizach 2011-08-09 10:10:50 PDT
(In reply to comment #6)
> There are methods in widget to produce screen-relative coords.

What are they? 

Do they take into account scrolling offset and nested iframes?
Comment 8 Simon Fraser (smfr) 2011-08-09 10:17:04 PDT
You probably want:
  Widget::convertToContainingWindow(const FloatQuad&) const;
followed by
  ChromeClient::windowToScreen().
Comment 9 chris fleizach 2011-08-11 09:37:03 PDT
(In reply to comment #8)
> You probably want:
>   Widget::convertToContainingWindow(const FloatQuad&) const;
> followed by
>   ChromeClient::windowToScreen().

These methods do not appear to take scroll position into account. As soon as the scroll area is scrolled, the rectangles are not correct

Here's the code I'm using. 

AccessibilityObject* root = m_object->axObjectCache()->rootObjectForFrame(m_object->document()->frame());
        if (root && root->isAccessibilityScrollView()) {
            ScrollView *widget = toAccessibilityScrollView(root)->scrollView();
            rect = widget->convertToContainingWindow(rect);
            rect = m_object->document()->page()->chrome()->windowToScreen(rect);
        }


Is this the way this method is supposed to behave? does scroll position need to be accounted for after the fact
Comment 10 Simon Fraser (smfr) 2011-08-11 10:18:15 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > You probably want:
> >   Widget::convertToContainingWindow(const FloatQuad&) const;
> > followed by
> >   ChromeClient::windowToScreen().
> 
> These methods do not appear to take scroll position into account. As soon as the scroll area is scrolled, the rectangles are not correct
> 
> Here's the code I'm using. 
> 
> AccessibilityObject* root = m_object->axObjectCache()->rootObjectForFrame(m_object->document()->frame());
>         if (root && root->isAccessibilityScrollView()) {
>             ScrollView *widget = toAccessibilityScrollView(root)->scrollView();
>             rect = widget->convertToContainingWindow(rect);
>             rect = m_object->document()->page()->chrome()->windowToScreen(rect);
>         }
> 
> 
> Is this the way this method is supposed to behave? does scroll position need to be accounted for after the fact

What coordinate space does 'rect' start in? Is it in 'page' (aka "absolute") coordinates? If so, you want

ScrollView::contentsToWindow() on the innermost scroll view (i.e. the one for the document's frame), followed by chrome()->windowToScreen(). Sadly ScrollView::contentsToScreen() won't do the right thing with CSS transforms.
Comment 11 chris fleizach 2011-08-11 11:47:36 PDT
> What coordinate space does 'rect' start in? Is it in 'page' (aka "absolute") coordinates? If so, you want
> 
> ScrollView::contentsToWindow() on the innermost scroll view (i.e. the one for the document's frame), followed by chrome()->windowToScreen(). Sadly ScrollView::contentsToScreen() won't do the right thing with CSS transforms.

Thanks for the suggestion. it looks like this is working
Comment 12 Darin Adler 2011-08-17 10:32:33 PDT
Comment on attachment 94437 [details]
patch

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

Looks fine.

> Source/WebCore/accessibility/mac/AccessibilityObjectWrapper.mm:1183
> +        Document* doc = m_object->document();

Normally we prefer to use the name "document" rather than the abbreviation "doc".
Comment 13 chris fleizach 2011-08-17 10:35:16 PDT
(In reply to comment #12)
> (From update of attachment 94437 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94437&action=review
> 
> Looks fine.
> 
> > Source/WebCore/accessibility/mac/AccessibilityObjectWrapper.mm:1183
> > +        Document* doc = m_object->document();
> 
> Normally we prefer to use the name "document" rather than the abbreviation "doc".

Thanks for the review. I'm going to try to see if I can reconcile the WK1 and the WK2 methods for determining frames and potentially remove a lot of code before falling back to this patch
Comment 14 chris fleizach 2011-08-18 12:52:56 PDT
(In reply to comment #11)
> > What coordinate space does 'rect' start in? Is it in 'page' (aka "absolute") coordinates? If so, you want
> > 
> > ScrollView::contentsToWindow() on the innermost scroll view (i.e. the one for the document's frame), followed by chrome()->windowToScreen(). Sadly ScrollView::contentsToScreen() won't do the right thing with CSS transforms.
> 
> Thanks for the suggestion. it looks like this is working

Unfortunately 

m_object->document()->page()->chrome()->windowToScreen(rect);

will not work on WK1 because of

IntRect WebChromeClient::windowToScreen(const IntRect& r) const
{
    if ([m_webView _usesDocumentViews])
        return r;

So I can't unify the two approaches
Comment 15 chris fleizach 2011-08-18 14:15:04 PDT
Created attachment 104397 [details]
patch

This patch does a better job of calculating the positioning using more standardized methods
Comment 16 Darin Adler 2011-08-18 16:23:37 PDT
Comment on attachment 104397 [details]
patch

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

> Source/WebCore/accessibility/mac/AccessibilityObjectWrapper.mm:1390
> +        ScrollView* widget = 0;

This should be named "view" or "scrollView" rather than "widget".

> Source/WebCore/accessibility/mac/AccessibilityObjectWrapper.mm:1402
> +        rect = m_object->document()->page()->chrome()->windowToScreen(rect);
> +        point = NSMakePoint(rect.x(), rect.y());

You could write this like this:

    point = m_object->document()->page()->chrome()->windowToScreen(rect).location();

There is no need to use NSMakePoint.

What guarantees that page is non-zero?

Is there a way to test this with regression tests?
Comment 17 chris fleizach 2011-08-18 16:50:11 PDT
> Is there a way to test this with regression tests?

I have an open bug (rdar://9980641) to make accessibility work with WebKitTestRunner (I believe that's the name), the WK2 test mechanism. 

Right now there is nothing in place to handle that. I'll add a note to also test positioning when doing so.
Comment 18 chris fleizach 2011-08-19 13:04:38 PDT
Created attachment 104548 [details]
patch

address comments
Comment 19 Darin Adler 2011-08-19 16:06:59 PDT
Comment on attachment 104548 [details]
patch

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

Not sure why the EWS couldn’t apply the patch. Would be better to put up a patch that it can apply.

> Source/WebCore/accessibility/mac/AccessibilityObjectWrapper.mm:1402
> +        if (m_object->page())
> +            point = m_object->page()->chrome()->windowToScreen(rect).location();

If page is 0, then point is left uninitialized. Then we’ll return a random point. We could just use rect.location() in that case, or initialize the point to 0,0.

> Source/WebCore/accessibility/AccessibilityObject.cpp:990
> +    Document *document = this->document();

It should be Document*, with the * next to the Document.
Comment 20 chris fleizach 2011-08-19 18:14:55 PDT
http://trac.webkit.org/changeset/93461