Summary: | AX WK2 Regression: WebKit outputs incorrect AX position in frames/iframes | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | chris fleizach <cfleizach> | ||||||||
Component: | Accessibility | Assignee: | 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
chris fleizach
2011-05-23 10:14:09 PDT
The Problem is that on WK2, nested frames are not taken into account Created attachment 94437 [details]
patch
It's still an ongoing project to figure out how to add layout tests for WK2 and AX
Comment on attachment 94437 [details]
patch
Why does accessibility have its own coordinate transform methods? Why not use Widget or RenderObject methods for this?
(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 There are methods in widget to produce screen-relative coords. (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? You probably want: Widget::convertToContainingWindow(const FloatQuad&) const; followed by ChromeClient::windowToScreen(). (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 (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.
> 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 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". (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 (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 Created attachment 104397 [details]
patch
This patch does a better job of calculating the positioning using more standardized methods
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? > 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. Created attachment 104548 [details]
patch
address comments
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. |