Add GetScreenBoundsRect to ui::AXPlatformNodeDelegate

Add GetScreenBoundsRect() to ui::AXPlatformNodeDelegate

In converting accLocation() in BrowserAccessibility to use AXPlatformNode, we
found that the simple model of using local coordinates plus a global offset just
doesn't work.

In Views we've been able to get away with this simple model because Views do not
have the same nested transformations that the web can like rotation and scaling.

This change also allows us to remove GetGlobalCoordinateOffset() as 
the  x.() and y.() of the result of GetScreenBoundsRect() can be used instead.


BUG=703369
Review-Url: https://codereview.chromium.org/2795543002
Cr-Commit-Position: refs/heads/master@{#462971}
Committed: https://chromium.googlesource.com/chromium/src/+/365c1380f2e6f346ec267b48f9e238fce309ac38

[dougt, 2017-04-01 04:18:38]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-01 04:18:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dougt, 2017-04-01 04:32:46]

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

[dougt, 2017-04-01 04:32:47]

dmazzoni, ptal.

[dougt, 2017-04-01 04:37:25]

Oh, I should also say that I left most of the GetScreenBoundsRect()
implementations empty.  I can follow up if you're happy with the name and
general direction or we can leave them as-is until needed.

[commit-bot: I haz the power, 2017-04-01 05:15:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-01 05:15:07]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2017-04-03 05:19:56]

The name sounds great, but I'd prefer this to be a complete
change that gets rid of GetGlobalCoordinateOffset and
changes get_accLocation to use GetScreenBoundsRect
instead. I don't think the change will be that large and
as we already established, it should be covered by existing
tests once you add the windows expectation.

https://codereview.chromium.org/2795543002/diff/1/ui/accessibility/platform/a...
File ui/accessibility/platform/ax_platform_node_delegate.h (right):

https://codereview.chromium.org/2795543002/diff/1/ui/accessibility/platform/a...
ui/accessibility/platform/ax_platform_node_delegate.h:53: virtual gfx::Vector2d
GetGlobalCoordinateOffset() = 0;
Please get rid of GetGlobalCoordinateOffset now, we shouldn't need it anymore.

[dougt, 2017-04-03 22:46:33]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 22:47:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dougt, 2017-04-04 00:13:01]

Description was changed from

==========
Add GetScreenBoundsRect to ui::AXPlatformNodeDelegate

In converting accLocation in BrowserAccessibility to use AXPlatformNode, we
found that the simple model of using local coordinates plus a global offset just
doesn't work.

In Views we've been able to get away with this simple model because Views do not
have the same nested transformations that the web can like rotation and scaling.

BUG=703369
==========

to

==========
Add GetScreenBoundsRect() to ui::AXPlatformNodeDelegate

In converting accLocation() in BrowserAccessibility to use AXPlatformNode, we
found that the simple model of using local coordinates plus a global offset just
doesn't work.

In Views we've been able to get away with this simple model because Views do not
have the same nested transformations that the web can like rotation and scaling.

This change also allows us to remove GetGlobalCoordinateOffset() as 
the  x.() and y.() of the result of GetScreenBoundsRect() can be used instead.


BUG=703369
==========

[dougt, 2017-04-04 00:13:21]

On 2017/04/03 05:19:56, dmazzoni wrote:
> The name sounds great, but I'd prefer this to be a complete
> change that gets rid of GetGlobalCoordinateOffset and
> changes get_accLocation to use GetScreenBoundsRect
> instead. I don't think the change will be that large and
> as we already established, it should be covered by existing
> tests once you add the windows expectation.
> 
>
https://codereview.chromium.org/2795543002/diff/1/ui/accessibility/platform/a...
> File ui/accessibility/platform/ax_platform_node_delegate.h (right):
> 
>
https://codereview.chromium.org/2795543002/diff/1/ui/accessibility/platform/a...
> ui/accessibility/platform/ax_platform_node_delegate.h:53: virtual
gfx::Vector2d
> GetGlobalCoordinateOffset() = 0;
> Please get rid of GetGlobalCoordinateOffset now, we shouldn't need it anymore.

Yup, exactly!  PTAL.

[commit-bot: I haz the power, 2017-04-04 00:24:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-04 00:24:25]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2017-04-04 15:05:19]

I think we might have a misunderstanding still.
I'm all in favor of getting there incrementally but each
change should make sense in isolation. This one doesn't
seem to, to me - GetScreenBoundsRect() is documented
as returning the bounds of the node in screen coordinates
but it's just returning an empty rect and being used as
an offset.

https://codereview.chromium.org/2795543002/diff/20001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_base.cc (right):

https://codereview.chromium.org/2795543002/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.cc:28:
bounds.Offset(screen_bounds.x(), screen_bounds.y());
I don't think this should be here.

It should just call delegate_->GetScreenBoundsRect(), nothing more should be
needed.

That iframe transform example would fail here - the width and height would be
incorrect at minimum.

https://codereview.chromium.org/2795543002/diff/20001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_win.cc (right):

https://codereview.chromium.org/2795543002/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_win.cc:264: gfx::Rect screen_bounds =
delegate_->GetScreenBoundsRect();
Same here - it should just call GetScreenBoundsRect and no longer check
location.

https://codereview.chromium.org/2795543002/diff/20001/ui/views/accessibility/...
File ui/views/accessibility/native_view_accessibility_base.cc (right):

https://codereview.chromium.org/2795543002/diff/20001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility_base.cc:126: // location is
already in screen coordinates.
This is supposed to return those coordinates, though.

[dougt, 2017-04-07 02:39:31]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 02:39:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 03:34:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-07 03:34:35]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2017-04-07 18:24:40]

Thanks, latest patchset lgtm!

https://codereview.chromium.org/2795543002/diff/40001/ui/views/accessibility/...
File ui/views/accessibility/native_view_accessibility_auralinux.cc (right):

https://codereview.chromium.org/2795543002/diff/40001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility_auralinux.cc:94: gfx::Rect
GetScreenBoundsRect() const override { return gfx::Rect(); }
nit: delete this - no need to override, the inherited method works fine

[dougt, 2017-04-07 19:22:25]

On 2017/04/07 18:24:40, dmazzoni wrote:
> Thanks, latest patchset lgtm!
> 
>
https://codereview.chromium.org/2795543002/diff/40001/ui/views/accessibility/...
> File ui/views/accessibility/native_view_accessibility_auralinux.cc (right):
> 
>
https://codereview.chromium.org/2795543002/diff/40001/ui/views/accessibility/...
> ui/views/accessibility/native_view_accessibility_auralinux.cc:94: gfx::Rect
> GetScreenBoundsRect() const override { return gfx::Rect(); }
> nit: delete this - no need to override, the inherited method works fine

Ack. The class you're referencing is the AuraLinuxApplication which doesn't
derive from any base that provides a default impl.  We should discuss at some
point why it's even a delegate given it only uses the child-indexing methods.

[dougt, 2017-04-07 19:22:35]

The CQ bit was checked by dougt@chromium.org

[commit-bot: I haz the power, 2017-04-07 19:23:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 19:50:04]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491592955518580,
"parent_rev": "b7faac578f403d2a6bb4a767578661079cbf5963", "commit_rev":
"365c1380f2e6f346ec267b48f9e238fce309ac38"}

[commit-bot: I haz the power, 2017-04-07 19:50:59]

Description was changed from

==========
Add GetScreenBoundsRect() to ui::AXPlatformNodeDelegate

In converting accLocation() in BrowserAccessibility to use AXPlatformNode, we
found that the simple model of using local coordinates plus a global offset just
doesn't work.

In Views we've been able to get away with this simple model because Views do not
have the same nested transformations that the web can like rotation and scaling.

This change also allows us to remove GetGlobalCoordinateOffset() as 
the  x.() and y.() of the result of GetScreenBoundsRect() can be used instead.


BUG=703369
==========

to

==========
Add GetScreenBoundsRect() to ui::AXPlatformNodeDelegate

In converting accLocation() in BrowserAccessibility to use AXPlatformNode, we
found that the simple model of using local coordinates plus a global offset just
doesn't work.

In Views we've been able to get away with this simple model because Views do not
have the same nested transformations that the web can like rotation and scaling.

This change also allows us to remove GetGlobalCoordinateOffset() as 
the  x.() and y.() of the result of GetScreenBoundsRect() can be used instead.


BUG=703369

Review-Url: https://codereview.chromium.org/2795543002
Cr-Commit-Position: refs/heads/master@{#462971}
Committed:
https://chromium.googlesource.com/chromium/src/+/365c1380f2e6f346ec267b48f9e2...
==========

[commit-bot: I haz the power, 2017-04-07 19:51:10]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/365c1380f2e6f346ec267b48f9e2...