Bug 61289 – AX WK2 Regression: WebKit outputs incorrect AX position in frames/iframes

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[chris fleizach]

* 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.

[chris fleizach]

The Problem is that on WK2, nested frames are not taken into account

[chris fleizach]

rdar://9317698

[chris fleizach]

Created attachment 94437 [details]
patch

It's still an ongoing project to figure out how to add layout tests for WK2 and AX

[Simon Fraser (smfr)]

Comment on attachment 94437 [details]
patch

Why does accessibility have its own coordinate transform methods? Why not use Widget or RenderObject methods for this?

[chris fleizach]

(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

[Simon Fraser (smfr)]

There are methods in widget to produce screen-relative coords.

[chris fleizach]

(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?

[Simon Fraser (smfr)]

You probably want:
  Widget::convertToContainingWindow(const FloatQuad&) const;
followed by
  ChromeClient::windowToScreen().

[chris fleizach]

(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

[Simon Fraser (smfr)]

(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.

[chris fleizach]

> 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

[Darin Adler]

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".

[chris fleizach]

(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

[chris fleizach]

(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

[chris fleizach]

Created attachment 104397 [details]
patch

This patch does a better job of calculating the positioning using more standardized methods

[Darin Adler]

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?

[chris fleizach]

> 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.

[chris fleizach]

Created attachment 104548 [details]
patch

address comments

[Darin Adler]

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.

[chris fleizach]

http://trac.webkit.org/changeset/93461