MacViews: Fix some TableView tests, add ui::EventGenerator::set_assume_window_at_origin(bool)

MacViews: Fix some TableView tests, add ui::EventGenerator::set_assume_window_at_origin(bool)

EventGenerator uses root window coordinates, and many tests using
EventGenerator assume that a window it creates at (0,0) will be placed
at the origin of the root window when Show()n. However, on Mac, this is
rarely the case for "real" Widgets. The WindowServer forcefully shifts
the window into the work area so that it doesn't overlap the OS mainMenu
bar.

On Mac, the EventGenerator currently doesn't remap mouse coordinates to
a "hit" window: effectively treating all windows as if they are
positioned at (0,0). But tests that *do* correctly take into account the
screen position of a test window, and correctly map their test event
coordinates, will not like this.

By calling EventGenerator::set_assume_window_at_origin(false) a test can
disable the "fake" coordinate mapping.

Using EventGenerator and "real" NSEvents exposed some other bugs in
TreeView. Ctrl+LeftClick on Mac is actually a right-click so can't be
used for multi-row selection. Update TableView and its tests to use
Command on Mac.

Also the Mac EventGenerator was passing event->changed_button_flags() as
the event modifiers, but it should pass event->flags() instead for mouse
events. (Only NSFlagsChanged key events should use
changed_button_flags(), but EventGenerator hasn't needed them yet.

BUG=708401
Review-Url: https://codereview.chromium.org/2794213002
Cr-Commit-Position: refs/heads/master@{#462342}
Committed: https://chromium.googlesource.com/chromium/src/+/a1746a908f94b40aea3c37d63734d4667e3835bc

[tapted, 2017-04-04 03:04:22]

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

[commit-bot: I haz the power, 2017-04-04 03:04:53]

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

[tapted, 2017-04-04 03:06:04]

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

[commit-bot: I haz the power, 2017-04-04 03:06:09]

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-04 03:36:30]

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

[commit-bot: I haz the power, 2017-04-04 03:36:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2017-04-04 07:53:59]

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

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

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

[tapted, 2017-04-04 08:00:33]

Description was changed from

==========
nit diff


nit diff


cl format


Passing on Mac

BUG=
==========

to

==========
ui::EventGenerator::set_assume_window_at_origin(bool)

This allows a test using EventGenerator to blahdeblahblah

BUG=
==========

[commit-bot: I haz the power, 2017-04-04 09:30:01]

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

[commit-bot: I haz the power, 2017-04-04 09:30:01]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-04-05 01:39:39]

Description was changed from

==========
ui::EventGenerator::set_assume_window_at_origin(bool)

This allows a test using EventGenerator to blahdeblahblah

BUG=
==========

to

==========
MacViews: Fix some TableView tests, add
ui::EventGenerator::set_assume_window_at_origin(bool)

EventGenerator uses root window coordinates, and many tests using
EventGenerator assume that a window it creates at (0,0) will be placed
at the origin of the root window when Show()n. However, on Mac, this is
rarely the case for "real" Widgets. The WindowServer forcefully shifts
the window into the work area so that it doesn't overlap the OS mainMenu
bar.

On Mac, the EventGenerator currently doesn't remap mouse coordinates to
a "hit" window: effectively treating all windows as if they are
positioned at (0,0). But tests that *do* correctly take into account the
screen position of a test window, and correctly map their test event
coordinates will not like this.

By calling EventGenerator::set_assume_window_at_origin(false) a test can
disable the "fake" coordinate mapping.

Using EventGenerator and "real" NSEvents exposed some other bugs.
Ctrl+LeftClick on Mac is actually a right-click so can't be used for
multi-row selection. Update TableView and its tests to use Cmd on Mac.

Also the Mac EventGenerator was passing event->changed_button_flags() as
the event modifiers, but it should pass event->flags() instead for mouse
events. (Only NSFlagsChanged key events should use
changed_button_flags(), but EventGenerator hasn't needed them yet.

BUG=None.
==========

[tapted, 2017-04-05 01:39:39]

tapted@chromium.org changed reviewers:
+ sky@chromium.org

[tapted, 2017-04-05 01:41:33]

Description was changed from

==========
MacViews: Fix some TableView tests, add
ui::EventGenerator::set_assume_window_at_origin(bool)

EventGenerator uses root window coordinates, and many tests using
EventGenerator assume that a window it creates at (0,0) will be placed
at the origin of the root window when Show()n. However, on Mac, this is
rarely the case for "real" Widgets. The WindowServer forcefully shifts
the window into the work area so that it doesn't overlap the OS mainMenu
bar.

On Mac, the EventGenerator currently doesn't remap mouse coordinates to
a "hit" window: effectively treating all windows as if they are
positioned at (0,0). But tests that *do* correctly take into account the
screen position of a test window, and correctly map their test event
coordinates will not like this.

By calling EventGenerator::set_assume_window_at_origin(false) a test can
disable the "fake" coordinate mapping.

Using EventGenerator and "real" NSEvents exposed some other bugs.
Ctrl+LeftClick on Mac is actually a right-click so can't be used for
multi-row selection. Update TableView and its tests to use Cmd on Mac.

Also the Mac EventGenerator was passing event->changed_button_flags() as
the event modifiers, but it should pass event->flags() instead for mouse
events. (Only NSFlagsChanged key events should use
changed_button_flags(), but EventGenerator hasn't needed them yet.

BUG=None.
==========

to

==========
MacViews: Fix some TableView tests, add
ui::EventGenerator::set_assume_window_at_origin(bool)

EventGenerator uses root window coordinates, and many tests using
EventGenerator assume that a window it creates at (0,0) will be placed
at the origin of the root window when Show()n. However, on Mac, this is
rarely the case for "real" Widgets. The WindowServer forcefully shifts
the window into the work area so that it doesn't overlap the OS mainMenu
bar.

On Mac, the EventGenerator currently doesn't remap mouse coordinates to
a "hit" window: effectively treating all windows as if they are
positioned at (0,0). But tests that *do* correctly take into account the
screen position of a test window, and correctly map their test event
coordinates, will not like this.

By calling EventGenerator::set_assume_window_at_origin(false) a test can
disable the "fake" coordinate mapping.

Using EventGenerator and "real" NSEvents exposed some other bugs in
TreeView. Ctrl+LeftClick on Mac is actually a right-click so can't be
used for multi-row selection. Update TableView and its tests to use
Command on Mac.

Also the Mac EventGenerator was passing event->changed_button_flags() as
the event modifiers, but it should pass event->flags() instead for mouse
events. (Only NSFlagsChanged key events should use
changed_button_flags(), but EventGenerator hasn't needed them yet.

BUG=None.
==========

[tapted, 2017-04-05 01:45:55]

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

[commit-bot: I haz the power, 2017-04-05 01:46:23]

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

[tapted, 2017-04-05 01:54:21]

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

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

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-05 02:56:06]

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

[commit-bot: I haz the power, 2017-04-05 02:56:07]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tapted, 2017-04-05 03:50:13]

Description was changed from

==========
MacViews: Fix some TableView tests, add
ui::EventGenerator::set_assume_window_at_origin(bool)

EventGenerator uses root window coordinates, and many tests using
EventGenerator assume that a window it creates at (0,0) will be placed
at the origin of the root window when Show()n. However, on Mac, this is
rarely the case for "real" Widgets. The WindowServer forcefully shifts
the window into the work area so that it doesn't overlap the OS mainMenu
bar.

On Mac, the EventGenerator currently doesn't remap mouse coordinates to
a "hit" window: effectively treating all windows as if they are
positioned at (0,0). But tests that *do* correctly take into account the
screen position of a test window, and correctly map their test event
coordinates, will not like this.

By calling EventGenerator::set_assume_window_at_origin(false) a test can
disable the "fake" coordinate mapping.

Using EventGenerator and "real" NSEvents exposed some other bugs in
TreeView. Ctrl+LeftClick on Mac is actually a right-click so can't be
used for multi-row selection. Update TableView and its tests to use
Command on Mac.

Also the Mac EventGenerator was passing event->changed_button_flags() as
the event modifiers, but it should pass event->flags() instead for mouse
events. (Only NSFlagsChanged key events should use
changed_button_flags(), but EventGenerator hasn't needed them yet.

BUG=None.
==========

to

==========
MacViews: Fix some TableView tests, add
ui::EventGenerator::set_assume_window_at_origin(bool)

EventGenerator uses root window coordinates, and many tests using
EventGenerator assume that a window it creates at (0,0) will be placed
at the origin of the root window when Show()n. However, on Mac, this is
rarely the case for "real" Widgets. The WindowServer forcefully shifts
the window into the work area so that it doesn't overlap the OS mainMenu
bar.

On Mac, the EventGenerator currently doesn't remap mouse coordinates to
a "hit" window: effectively treating all windows as if they are
positioned at (0,0). But tests that *do* correctly take into account the
screen position of a test window, and correctly map their test event
coordinates, will not like this.

By calling EventGenerator::set_assume_window_at_origin(false) a test can
disable the "fake" coordinate mapping.

Using EventGenerator and "real" NSEvents exposed some other bugs in
TreeView. Ctrl+LeftClick on Mac is actually a right-click so can't be
used for multi-row selection. Update TableView and its tests to use
Command on Mac.

Also the Mac EventGenerator was passing event->changed_button_flags() as
the event modifiers, but it should pass event->flags() instead for mouse
events. (Only NSFlagsChanged key events should use
changed_button_flags(), but EventGenerator hasn't needed them yet.

BUG=708401
==========

[tapted, 2017-04-05 03:54:48]

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

[commit-bot: I haz the power, 2017-04-05 03:55:00]

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-05 04:50:04]

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

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

Dry run: This issue passed the CQ dry run.

[tapted, 2017-04-05 04:54:17]

Hi sky, please take a look

https://codereview.chromium.org/2794213002/diff/100001/ui/events/test/event_g...
File ui/events/test/event_generator.h (right):

https://codereview.chromium.org/2794213002/diff/100001/ui/events/test/event_g...
ui/events/test/event_generator.h:476: bool assume_window_at_origin_ = true;
alternatively.. we could default this to `false` and update the ~8 test
harnesses that are relying on it -- see http://crbug.com/708401

[sky, 2017-04-05 15:39:53]

Suggestion on naming.

https://codereview.chromium.org/2794213002/diff/100001/ui/events/test/event_g...
File ui/events/test/event_generator.h (right):

https://codereview.chromium.org/2794213002/diff/100001/ui/events/test/event_g...
ui/events/test/event_generator.h:171: void set_assume_window_at_origin(bool
assume_window_at_origin) {
Is a better name and description for this coordinates_relative_to_root_window?
Where the root_window is generally the widget?

[tapted, 2017-04-05 23:57:18]

https://codereview.chromium.org/2794213002/diff/100001/ui/events/test/event_g...
File ui/events/test/event_generator.h (right):

https://codereview.chromium.org/2794213002/diff/100001/ui/events/test/event_g...
ui/events/test/event_generator.h:171: void set_assume_window_at_origin(bool
assume_window_at_origin) {
On 2017/04/05 15:39:53, sky wrote:
> Is a better name and description for this coordinates_relative_to_root_window?
> Where the root_window is generally the widget?

I see how that would fit with the way the coordinates are remapped, but it might
not capture the use case properly. E.g. consider one of the problematic tests,
CustomButtonTest.HideInkDropHighlightOnDisable [1]

It's doing

  ui::test::EventGenerator generator(widget()->GetNativeWindow());
  generator.MoveMouseToInHost(10, 10);

And it sets up and Show()s the Widget with

    Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_POPUP);
    params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
    params.bounds = gfx::Rect(0, 0, 650, 650);
    widget_->Init(params);
    widget_->Show();
    button_ = new TestCustomButton(false);
    widget_->SetContentsView(button_);

POPUP + origin=(0,0) results in a Window with its ContentsView at (0,0) in Ash,
but other window managers (Mac and Linux with desktop widgets) are free move the
window.

(Also, using a different Widget type (e.g. TYPE_WINDOW) will add a titlebar
under Ash as well.)

So, for example, if the widget set up set a non-zero origin:

    params.bounds = gfx::Rect(100, 100, 650, 650);

The test as it stands will fail under Linux (non-desktop widgets).

That is, "MoveMouseToInHost(10, 10)" only works because the test case is
assuming that the window is at the origin of the root window, but really the
coordinates it _wants_ to use should be relative to the window's ContentView
(i.e. the CustomButton View).

So when TableViewTest now comes along and actually _does_ have a titlebar on its
Widget, it must rightly offset its coordinates on all platforms:
  gfx::Point GetPointForRow(int row) {
    const int y = (row + 0.5) * table_->row_height();
    return table_->GetBoundsInScreen().origin() + gfx::Vector2d(5, y);
  }


The problem with set_coordinates_relative_to_root_window(bool) is that
HideInkDropHighlightOnDisable would need a value of `false`, and that feels
misleading because the problem is that HideInkDropHighlightOnDisable *is*
sending coordinates relative to the root window - not that it isn't.



[1]
https://cs.chromium.org/chromium/src/ui/views/controls/button/custom_button_u...

[sky, 2017-04-06 02:05:58]

Ok, fair enough. LGTM

[tapted, 2017-04-06 03:13:16]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2017-04-06 03:13:33]

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-06 03:19:35]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491448396230600,
"parent_rev": "23efec570430bd32de011cde12474df857fa51b4", "commit_rev":
"a1746a908f94b40aea3c37d63734d4667e3835bc"}

[commit-bot: I haz the power, 2017-04-06 03:21:00]

Description was changed from

==========
MacViews: Fix some TableView tests, add
ui::EventGenerator::set_assume_window_at_origin(bool)

EventGenerator uses root window coordinates, and many tests using
EventGenerator assume that a window it creates at (0,0) will be placed
at the origin of the root window when Show()n. However, on Mac, this is
rarely the case for "real" Widgets. The WindowServer forcefully shifts
the window into the work area so that it doesn't overlap the OS mainMenu
bar.

On Mac, the EventGenerator currently doesn't remap mouse coordinates to
a "hit" window: effectively treating all windows as if they are
positioned at (0,0). But tests that *do* correctly take into account the
screen position of a test window, and correctly map their test event
coordinates, will not like this.

By calling EventGenerator::set_assume_window_at_origin(false) a test can
disable the "fake" coordinate mapping.

Using EventGenerator and "real" NSEvents exposed some other bugs in
TreeView. Ctrl+LeftClick on Mac is actually a right-click so can't be
used for multi-row selection. Update TableView and its tests to use
Command on Mac.

Also the Mac EventGenerator was passing event->changed_button_flags() as
the event modifiers, but it should pass event->flags() instead for mouse
events. (Only NSFlagsChanged key events should use
changed_button_flags(), but EventGenerator hasn't needed them yet.

BUG=708401
==========

to

==========
MacViews: Fix some TableView tests, add
ui::EventGenerator::set_assume_window_at_origin(bool)

EventGenerator uses root window coordinates, and many tests using
EventGenerator assume that a window it creates at (0,0) will be placed
at the origin of the root window when Show()n. However, on Mac, this is
rarely the case for "real" Widgets. The WindowServer forcefully shifts
the window into the work area so that it doesn't overlap the OS mainMenu
bar.

On Mac, the EventGenerator currently doesn't remap mouse coordinates to
a "hit" window: effectively treating all windows as if they are
positioned at (0,0). But tests that *do* correctly take into account the
screen position of a test window, and correctly map their test event
coordinates, will not like this.

By calling EventGenerator::set_assume_window_at_origin(false) a test can
disable the "fake" coordinate mapping.

Using EventGenerator and "real" NSEvents exposed some other bugs in
TreeView. Ctrl+LeftClick on Mac is actually a right-click so can't be
used for multi-row selection. Update TableView and its tests to use
Command on Mac.

Also the Mac EventGenerator was passing event->changed_button_flags() as
the event modifiers, but it should pass event->flags() instead for mouse
events. (Only NSFlagsChanged key events should use
changed_button_flags(), but EventGenerator hasn't needed them yet.

BUG=708401

Review-Url: https://codereview.chromium.org/2794213002
Cr-Commit-Position: refs/heads/master@{#462342}
Committed:
https://chromium.googlesource.com/chromium/src/+/a1746a908f94b40aea3c37d63734...
==========

[commit-bot: I haz the power, 2017-04-06 03:21:02]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/a1746a908f94b40aea3c37d63734...