Fix case where accessible scrollToMakeVisible didn't work.

Fix case where accessible scrollToMakeVisible didn't work.

The particular broken case is when the object to be made visible is inside of
a scrollable container that's larger than the viewport (window).

Previously, the code scrolled the object to be visible within the container,
then recursively scrolled the container to be visible within the viewport.
That doesn't necessarily work if the container is larger than the viewport.
Now it passes the coordinates of the original object up recursively as the
"subfocus".

In addition, this change centers the object within the viewport, rather than
scrolling minimally to get it in view. This more closely matches how the
browser normally behaves when you focus something offscreen, and makes for
a better experience on Android, where we use this function extensively
every time you move accessibility focus.

Well-covered by existing tests, plus a new test for the case that didn't
previously work.

BUG=374316, 424691
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183926

[dmazzoni, 2014-10-17 18:57:33]

dmazzoni@chromium.org changed reviewers:
+ aboxhall@chromium.org

[aboxhall, 2014-10-17 19:30:09]

Trying to understand why the changes to computeBestScrollOffset are necessary...

https://codereview.chromium.org/661183002/diff/1/Source/core/accessibility/AX...
File Source/core/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/661183002/diff/1/Source/core/accessibility/AX...
Source/core/accessibility/AXObject.cpp:664: //   Before:
Is this description/ascii art still accurate?

https://codereview.chromium.org/661183002/diff/1/Source/core/accessibility/AX...
Source/core/accessibility/AXObject.cpp:684: // If the object size is larger than
the viewport size, shrink it in the
I think 'shrink it' is a little confusing - my reading is that it's not actually
changing the size of the object, but reducing the size of the portion of it to
be scrolled into view. Perhaps something like 'If the ..., consider only the
portion centred on the subfocus which will fit inside the viewport'?

https://codereview.chromium.org/661183002/diff/1/Source/core/accessibility/AX...
Source/core/accessibility/AXObject.cpp:695: // Compute the size of an object
centered on the subfocus, the size of the viewport.
Why do we want to centre it rather than keeping it at the nearest edge as we did
previously?

[dmazzoni, 2014-10-17 19:41:35]

https://codereview.chromium.org/661183002/diff/1/Source/core/accessibility/AX...
File Source/core/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/661183002/diff/1/Source/core/accessibility/AX...
Source/core/accessibility/AXObject.cpp:664: //   Before:
On 2014/10/17 19:30:09, aboxhall wrote:
> Is this description/ascii art still accurate?

Oh - it didn't mention centering. Updated.

https://codereview.chromium.org/661183002/diff/1/Source/core/accessibility/AX...
Source/core/accessibility/AXObject.cpp:684: // If the object size is larger than
the viewport size, shrink it in the
On 2014/10/17 19:30:09, aboxhall wrote:
> I think 'shrink it' is a little confusing - my reading is that it's not
actually
> changing the size of the object, but reducing the size of the portion of it to
> be scrolled into view. Perhaps something like 'If the ..., consider only the
> portion centred on the subfocus which will fit inside the viewport'?

Done.

https://codereview.chromium.org/661183002/diff/1/Source/core/accessibility/AX...
Source/core/accessibility/AXObject.cpp:695: // Compute the size of an object
centered on the subfocus, the size of the viewport.
On 2014/10/17 19:30:09, aboxhall wrote:
> Why do we want to centre it rather than keeping it at the nearest edge as we
did
> previously?

I mentioned this in the change description - does that explanation make sense?

[aboxhall, 2014-10-17 19:59:18]

lgtm

[dmazzoni, 2014-10-17 20:25:57]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-10-17 20:26:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/661183002/40001

[commit-bot: I haz the power, 2014-10-17 21:35:21]

Committed patchset #3 (id:40001) as 183926