Fix accessibility coordinates within iframes.

Fix accessibility coordinates within iframes.

WebKit returns local coordinates within an iframe, we need
to convert them to page-global coordinates. In addition,
the test code needs to print page-global coordinates
(not screen-global) for easier debugging and reliable
test output.

BUG=171705
TBR=dtseng


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179468

[dmazzoni, 2013-01-28 22:33:33]

I may need to make a tiny change to the win test helper also, but this is ready
for feedback on the bulk of the change and the test. Thanks.

[aboxhall, 2013-01-29 01:52:40]

No major issues, just some stuff I don't understand and one suggestion.

https://codereview.chromium.org/12084028/diff/1/content/browser/accessibility...
File content/browser/accessibility/browser_accessibility.cc (right):

https://codereview.chromium.org/12084028/diff/1/content/browser/accessibility...
content/browser/accessibility/browser_accessibility.cc:140: bool
need_to_offset_web_area =
I don't completely follow this. Why do we need to offset the bounds if |this| is
a WebArea? Or am I not reading this correctly?

https://codereview.chromium.org/12084028/diff/1/content/browser/accessibility...
File content/browser/accessibility/dump_accessibility_tree_helper_mac.mm
(right):

https://codereview.chromium.org/12084028/diff/1/content/browser/accessibility...
content/browser/accessibility/dump_accessibility_tree_helper_mac.mm:88:
NSString* position_str =
It might be more readable to pull the calculation of size_str and position_str
into two helper methods.

https://codereview.chromium.org/12084028/diff/1/content/test/data/accessibili...
File content/test/data/accessibility/iframe-coordinates-expected-mac.txt
(right):

https://codereview.chromium.org/12084028/diff/1/content/test/data/accessibili...
content/test/data/accessibility/iframe-coordinates-expected-mac.txt:6: AXGroup
position=(0, 200) size=(800, 104)
Where are these 4px coming from?

https://codereview.chromium.org/12084028/diff/1/content/test/data/accessibili...
File content/test/data/accessibility/iframe-coordinates.html (right):

https://codereview.chromium.org/12084028/diff/1/content/test/data/accessibili...
content/test/data/accessibility/iframe-coordinates.html:35: <iframe id="frame2"
style="border: 0; width: 150px; height: 50px;">
Why is this iframe half the size of everything else?

[dmazzoni, 2013-01-29 16:51:50]

https://codereview.chromium.org/12084028/diff/1/content/browser/accessibility...
File content/browser/accessibility/browser_accessibility.cc (right):

https://codereview.chromium.org/12084028/diff/1/content/browser/accessibility...
content/browser/accessibility/browser_accessibility.cc:140: bool
need_to_offset_web_area =
On 2013/01/29 01:52:40, aboxhall wrote:
> I don't completely follow this. Why do we need to offset the bounds if |this|
is
> a WebArea? Or am I not reading this correctly?

We need to offset the bounds of the first parent of a WebArea - the bool is  set
last, checked first.

A WebArea will be in local coordinates - like (0,0) - (300, 100). So we need to
offset by the first parent of the WebArea, but skip over nodes that have no
width and height, that's probably a bug.

https://codereview.chromium.org/12084028/diff/1/content/browser/accessibility...
File content/browser/accessibility/dump_accessibility_tree_helper_mac.mm
(right):

https://codereview.chromium.org/12084028/diff/1/content/browser/accessibility...
content/browser/accessibility/dump_accessibility_tree_helper_mac.mm:88:
NSString* position_str =
On 2013/01/29 01:52:40, aboxhall wrote:
> It might be more readable to pull the calculation of size_str and position_str
> into two helper methods.

Done.

https://codereview.chromium.org/12084028/diff/1/content/test/data/accessibili...
File content/test/data/accessibility/iframe-coordinates-expected-mac.txt
(right):

https://codereview.chromium.org/12084028/diff/1/content/test/data/accessibili...
content/test/data/accessibility/iframe-coordinates-expected-mac.txt:6: AXGroup
position=(0, 200) size=(800, 104)
On 2013/01/29 01:52:40, aboxhall wrote:
> Where are these 4px coming from?

Borders, margins, and padding, oh my!
Borders, margins, and padding, oh my!

If I fix this, the html won't fit in 80 columns without splitting things into
style declarations in the header. I'm just not sure which is more or less
readable, round numbers in the expectations or more compact html.

https://codereview.chromium.org/12084028/diff/1/content/test/data/accessibili...
File content/test/data/accessibility/iframe-coordinates.html (right):

https://codereview.chromium.org/12084028/diff/1/content/test/data/accessibili...
content/test/data/accessibility/iframe-coordinates.html:35: <iframe id="frame2"
style="border: 0; width: 150px; height: 50px;">
On 2013/01/29 01:52:40, aboxhall wrote:
> Why is this iframe half the size of everything else?

It's deliberately scrolled also (see below) - so we can test the coordinates of
an element when they're inside a scrolled iframe.

[dmazzoni, 2013-01-29 16:52:25]

BTW, the windows changes are there now too, so this is now complete.

[aboxhall, 2013-01-29 17:43:26]

Windows changes looking good; sorry to be a pain with the test details but I
really think it could be easier to eyeball than it is now.

https://codereview.chromium.org/12084028/diff/1/content/browser/accessibility...
File content/browser/accessibility/browser_accessibility.cc (right):

https://codereview.chromium.org/12084028/diff/1/content/browser/accessibility...
content/browser/accessibility/browser_accessibility.cc:140: bool
need_to_offset_web_area =
On 2013/01/29 16:51:50, Dominic Mazzoni wrote:
> On 2013/01/29 01:52:40, aboxhall wrote:
> > I don't completely follow this. Why do we need to offset the bounds if
|this|
> is
> > a WebArea? Or am I not reading this correctly?
> 
> We need to offset the bounds of the first parent of a WebArea - the bool is 
set
> last, checked first.
> 
> A WebArea will be in local coordinates - like (0,0) - (300, 100). So we need
to
> offset by the first parent of the WebArea, but skip over nodes that have no
> width and height, that's probably a bug.

So a WebArea will have its _own_ coordinates in terms of its local frame, rather
than the containing one?

Obviously this code works, I just don't understand why.

https://codereview.chromium.org/12084028/diff/1/content/test/data/accessibili...
File content/test/data/accessibility/iframe-coordinates-expected-mac.txt
(right):

https://codereview.chromium.org/12084028/diff/1/content/test/data/accessibili...
content/test/data/accessibility/iframe-coordinates-expected-mac.txt:6: AXGroup
position=(0, 200) size=(800, 104)
On 2013/01/29 16:51:50, Dominic Mazzoni wrote:
> On 2013/01/29 01:52:40, aboxhall wrote:
> > Where are these 4px coming from?
> 
> Borders, margins, and padding, oh my!
> Borders, margins, and padding, oh my!
> 
> If I fix this, the html won't fit in 80 columns without splitting things into
> style declarations in the header. I'm just not sure which is more or less
> readable, round numbers in the expectations or more compact html.

Ok - I was just confused because it looked like you'd set borders, margin and
padding all to zero at some point. I think it probably would be easier to put a
style block in the header to keep all the numbers round - I really did have to
spend quite a long time (although it didn't help that I was on the bus and kept
getting distracted) trying to make sure everything added up.

https://codereview.chromium.org/12084028/diff/1/content/test/data/accessibili...
File content/test/data/accessibility/iframe-coordinates.html (right):

https://codereview.chromium.org/12084028/diff/1/content/test/data/accessibili...
content/test/data/accessibility/iframe-coordinates.html:35: <iframe id="frame2"
style="border: 0; width: 150px; height: 50px;">
On 2013/01/29 16:51:50, Dominic Mazzoni wrote:
> On 2013/01/29 01:52:40, aboxhall wrote:
> > Why is this iframe half the size of everything else?
> 
> It's deliberately scrolled also (see below) - so we can test the coordinates
of
> an element when they're inside a scrolled iframe.

Right, I somehow forgot that it has to be smaller than the contents in order to
scroll, duh.

It really does get quite tricky verifying the numbers when it starts to be
things like "AXButton position=(-130, 274)" - could we perhaps keep things in
multiples of 100, e.g. have the standard div size be 300x200, have this be
200x100, scroll by 100/100? Then when there's an offset of 20px it'll be obvious
where that's come from.

https://codereview.chromium.org/12084028/diff/2003/content/browser/accessibil...
File content/browser/accessibility/dump_accessibility_tree_helper_win.cc
(right):

https://codereview.chromium.org/12084028/diff/2003/content/browser/accessibil...
content/browser/accessibility/dump_accessibility_tree_helper_win.cc:107: if
(S_FALSE != acc_obj->accLocation(
It always makes me sad when APIs force you to use Yoda conditions :(

https://codereview.chromium.org/12084028/diff/2003/content/test/data/accessib...
File content/test/data/accessibility/iframe-coordinates-expected-win.txt
(right):

https://codereview.chromium.org/12084028/diff/2003/content/test/data/accessib...
content/test/data/accessibility/iframe-coordinates-expected-win.txt:6:
IA2_ROLE_SECTION location=(0, 200) size=(800, 105)
Different UA stylesheet? :\

[dmazzoni, 2013-01-29 18:51:24]

https://codereview.chromium.org/12084028/diff/1/content/browser/accessibility...
File content/browser/accessibility/browser_accessibility.cc (right):

https://codereview.chromium.org/12084028/diff/1/content/browser/accessibility...
content/browser/accessibility/browser_accessibility.cc:140: bool
need_to_offset_web_area =
On 2013/01/29 17:43:26, aboxhall wrote:
> On 2013/01/29 16:51:50, Dominic Mazzoni wrote:
> > On 2013/01/29 01:52:40, aboxhall wrote:
> > > I don't completely follow this. Why do we need to offset the bounds if
> |this|
> > is
> > > a WebArea? Or am I not reading this correctly?
> > 
> > We need to offset the bounds of the first parent of a WebArea - the bool is 
> set
> > last, checked first.
> > 
> > A WebArea will be in local coordinates - like (0,0) - (300, 100). So we need
> to
> > offset by the first parent of the WebArea, but skip over nodes that have no
> > width and height, that's probably a bug.
> 
> So a WebArea will have its _own_ coordinates in terms of its local frame,
rather
> than the containing one?
> 
> Obviously this code works, I just don't understand why.

Yeah, internally the tree looks something like:

DIV x=0 y=0     width=300 height=100
    BUTTON ...
DIV x=0 y=100 width=300 height=100
    BUTTON ...
DIV x=0 y=200 width=300 height=100
    IFRAME x=0 y=200 width=300 height=100
        DOCUMENT x=0 y=0 width=300 height=100
            DIV x=0 y=0 width=300 height=100
                BUTTON ...

So the iframe element is one of the containers of the web area, and it's in the
coordinates of the outer frame. The web area is the root document of the inner
frame - so it's in local coordinates.

So when computing the coordinates of an element, as you walk up the parent chain
the algorithm is: when you reach a webarea role (a document element), add the
top/left of the NEXT parent you find while walking up, to transform to local
coordinates of the outer frame.

https://codereview.chromium.org/12084028/diff/1/content/test/data/accessibili...
File content/test/data/accessibility/iframe-coordinates-expected-mac.txt
(right):

https://codereview.chromium.org/12084028/diff/1/content/test/data/accessibili...
content/test/data/accessibility/iframe-coordinates-expected-mac.txt:6: AXGroup
position=(0, 200) size=(800, 104)
On 2013/01/29 17:43:26, aboxhall wrote:
> On 2013/01/29 16:51:50, Dominic Mazzoni wrote:
> > On 2013/01/29 01:52:40, aboxhall wrote:
> > > Where are these 4px coming from?
> > 
> > Borders, margins, and padding, oh my!
> > Borders, margins, and padding, oh my!
> > 
> > If I fix this, the html won't fit in 80 columns without splitting things
into
> > style declarations in the header. I'm just not sure which is more or less
> > readable, round numbers in the expectations or more compact html.
> 
> Ok - I was just confused because it looked like you'd set borders, margin and
> padding all to zero at some point. I think it probably would be easier to put
a
> style block in the header to keep all the numbers round - I really did have to
> spend quite a long time (although it didn't help that I was on the bus and
kept
> getting distracted) trying to make sure everything added up.

Seems reasonable. I fixed it to use nice, round numbers.

[aboxhall, 2013-01-29 19:53:46]

https://codereview.chromium.org/12084028/diff/1/content/browser/accessibility...
File content/browser/accessibility/browser_accessibility.cc (right):

https://codereview.chromium.org/12084028/diff/1/content/browser/accessibility...
content/browser/accessibility/browser_accessibility.cc:140: bool
need_to_offset_web_area =
On 2013/01/29 18:51:25, Dominic Mazzoni wrote:
> On 2013/01/29 17:43:26, aboxhall wrote:
> > On 2013/01/29 16:51:50, Dominic Mazzoni wrote:
> > > On 2013/01/29 01:52:40, aboxhall wrote:
> > > > I don't completely follow this. Why do we need to offset the bounds if
> > |this|
> > > is
> > > > a WebArea? Or am I not reading this correctly?
> > > 
> > > We need to offset the bounds of the first parent of a WebArea - the bool
is 
> > set
> > > last, checked first.
> > > 
> > > A WebArea will be in local coordinates - like (0,0) - (300, 100). So we
need
> > to
> > > offset by the first parent of the WebArea, but skip over nodes that have
no
> > > width and height, that's probably a bug.
> > 
> > So a WebArea will have its _own_ coordinates in terms of its local frame,
> rather
> > than the containing one?
> > 
> > Obviously this code works, I just don't understand why.
> 
> Yeah, internally the tree looks something like:
> 
> DIV x=0 y=0     width=300 height=100
>     BUTTON ...
> DIV x=0 y=100 width=300 height=100
>     BUTTON ...
> DIV x=0 y=200 width=300 height=100
>     IFRAME x=0 y=200 width=300 height=100
>         DOCUMENT x=0 y=0 width=300 height=100
>             DIV x=0 y=0 width=300 height=100
>                 BUTTON ...
> 
> So the iframe element is one of the containers of the web area, and it's in
the
> coordinates of the outer frame. The web area is the root document of the inner
> frame - so it's in local coordinates.
> 
> So when computing the coordinates of an element, as you walk up the parent
chain
> the algorithm is: when you reach a webarea role (a document element), add the
> top/left of the NEXT parent you find while walking up, to transform to local
> coordinates of the outer frame.

Got it, thanks for the explanation.

https://codereview.chromium.org/12084028/diff/1/content/test/data/accessibili...
File content/test/data/accessibility/iframe-coordinates-expected-mac.txt
(right):

https://codereview.chromium.org/12084028/diff/1/content/test/data/accessibili...
content/test/data/accessibility/iframe-coordinates-expected-mac.txt:15:
AXWebArea position=(0, 304) size=(300, 100) scroll=(150, 50)
Where did the scroll coordinates go?

[dmazzoni, 2013-01-29 21:02:06]

lg?

https://codereview.chromium.org/12084028/diff/1/content/test/data/accessibili...
File content/test/data/accessibility/iframe-coordinates-expected-mac.txt
(right):

https://codereview.chromium.org/12084028/diff/1/content/test/data/accessibili...
content/test/data/accessibility/iframe-coordinates-expected-mac.txt:15:
AXWebArea position=(0, 304) size=(300, 100) scroll=(150, 50)
On 2013/01/29 19:53:47, aboxhall wrote:
> Where did the scroll coordinates go?

Oh, that was left over from my own internal debugging. Those aren't actually
exposed in the native API in a straightforward way so it doesn't make sense to
include them here. Maybe we can expose those on the AXScrollBars at some point,
but either way that's outside of the scope of this change.

[aboxhall, 2013-01-29 21:06:24]

lgtm

[commit-bot: I haz the power, 2013-01-29 21:26:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/12084028/12004

[commit-bot: I haz the power, 2013-01-29 23:47:57]

Change committed as 179468