Improve TableView unit tests and fix bugs that were uncovered.

Improve TableView unit tests and fix bugs that were uncovered.

Switch to using a Widget and an EventGenerator, which simulates the
sequence of events more accurately than directly calling event handlers.

Change the implementation so we never change row selection due to focus changes.

BUG=none

Review-Url: https://codereview.chromium.org/2780833004
Cr-Commit-Position: refs/heads/master@{#461775}
Committed: https://chromium.googlesource.com/chromium/src/+/3e70c6e83884c8c84864e20b89c367df6562c711

[Evan Stade, 2017-03-30 00:56:45]

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

[Evan Stade, 2017-03-30 00:58:59]

Description was changed from

==========
Improve TableView unit tests and fix bugs that were uncovered.

Switch to using a Widget and an EventGenerator, which simulates the
sequence of events more accurately than directly calling event handlers.

Change it so focus on initial selection doesn't occur synchronously. This
way when you press a row, and the press handler requests focus, the table
doesn't select the first row then immediately afterwards select the row
that was pressed.

BUG=none
==========

to

==========
Improve TableView unit tests and fix bugs that were uncovered.

Switch to using a Widget and an EventGenerator, which simulates the
sequence of events more accurately than directly calling event handlers.

Change the implementation so focus on initial selection doesn't occur
synchronously. This
way when you press a row, and the press handler requests focus, the table
doesn't select the first row then immediately afterwards select the row
that was pressed.

BUG=none
==========

[sky, 2017-03-30 03:25:49]

https://codereview.chromium.org/2780833004/diff/20001/ui/views/controls/table...
File ui/views/controls/table/table_view.cc (right):

https://codereview.chromium.org/2780833004/diff/20001/ui/views/controls/table...
ui/views/controls/table/table_view.cc:630:
base::ThreadTaskRunnerHandle::Get()->PostTask(
We shouldn't need to delay this. The press handler can change the selection,
then request focus if it needs to.

[sky, 2017-03-30 03:42:11]

On 2017/03/30 03:25:49, sky wrote:
>
https://codereview.chromium.org/2780833004/diff/20001/ui/views/controls/table...
> File ui/views/controls/table/table_view.cc (right):
> 
>
https://codereview.chromium.org/2780833004/diff/20001/ui/views/controls/table...
> ui/views/controls/table/table_view.cc:630:
> base::ThreadTaskRunnerHandle::Get()->PostTask(
> We shouldn't need to delay this. The press handler can change the selection,
> then request focus if it needs to.

For clarification I don't think we should delay processing as it leads to subtle
changes that may not be intentional. Who knows what's changed between the time
you want to change the selection and when the task it's processed. It's
preferred to process immediately when possible.

In thinking about this perhaps that real bug is that when focus is gained the
selection shouldn't change. I could see updating the anchor if it's not set, but
that wouldn't matter for tableview as it doesn't render the anchor. Perhaps it
should. Either way, I think we shouldn't change the selection on focus.

[Evan Stade, 2017-03-30 15:58:22]

Description was changed from

==========
Improve TableView unit tests and fix bugs that were uncovered.

Switch to using a Widget and an EventGenerator, which simulates the
sequence of events more accurately than directly calling event handlers.

Change the implementation so focus on initial selection doesn't occur
synchronously. This
way when you press a row, and the press handler requests focus, the table
doesn't select the first row then immediately afterwards select the row
that was pressed.

BUG=none
==========

to

==========
Improve TableView unit tests and fix bugs that were uncovered.

Switch to using a Widget and an EventGenerator, which simulates the
sequence of events more accurately than directly calling event handlers.

Change the implementation so we never change row selection due to focus changes.

BUG=none
==========

[Evan Stade, 2017-03-30 15:58:35]

On 2017/03/30 03:42:11, sky wrote:
> Either way, I think we shouldn't change the selection on focus.

ok, done.

[sky, 2017-03-30 20:12:47]

LGTM

[Evan Stade, 2017-03-31 00:04:28]

The CQ bit was checked by estade@chromium.org

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

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-03-31 00:46:24]

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

[commit-bot: I haz the power, 2017-03-31 00:46:25]

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_...)

[Evan Stade, 2017-04-03 16:08:47]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2017-04-03 16:08:48]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2780833004/#ps60001
(title: "fix TapOnRow for desktop linux")

[commit-bot: I haz the power, 2017-04-03 16:09: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-03 17:11:23]

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

[commit-bot: I haz the power, 2017-04-03 17:11:24]

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_...)

[Evan Stade, 2017-04-03 18:22:23]

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

[Evan Stade, 2017-04-03 18:22:23]

+tapted, any advice on mac failures? For the SelectOnTap one, I think we can
just disable on mac - http://crbug.com/445520

For the ones that use ClickOnRow, I think this assumption is not holding true:
https://cs.chromium.org/chromium/src/ui/views/test/event_generator_delegate_m...

[tapted, 2017-04-04 00:07:36]

https://codereview.chromium.org/2780833004/diff/60001/ui/views/controls/table...
File ui/views/controls/table/table_view_unittest.cc (right):

https://codereview.chromium.org/2780833004/diff/60001/ui/views/controls/table...
ui/views/controls/table/table_view_unittest.cc:209: params.bounds = gfx::Rect(0,
0, 650, 650);
Offsetting the (x,y) coordinate here might resolve the issues. E.g. use (100,
100, 650, 650); (but that might throw off coordinates elsewhere in the tests).

Alternatively, I think Widget::InitParams::TYPE_WINDOW_FRAMELESS above will work
too (perhaps better, but changing the Widget type can have other effects around
activation and things).

On Mac, when you Show() a real Widget (with a titlebar, etc.) at (0,0), the
window manager moves it into the work area (below menu bar, or possibly a Dock
hugging the left of the screen). Anticipating the move, or using a window type
that the window manager doesn't move should work around that.

If you're still stuck, I might have a chance later to look into it more.. I
should be able to detect this situation in the event generator and warn about it
too.. (sadly auto-correcting for it turned out to be a dead-end since tests
sometimes do and sometimes don't convert from root window coordinates to Widget
coordinates -- so some tests will break if we auto-correct).

[Evan Stade, 2017-04-04 00:41:52]

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

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

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 01:17:13]

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

[commit-bot: I haz the power, 2017-04-04 01:17:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_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:58:48]

On 2017/04/04 00:07:36, tapted wrote:
>
https://codereview.chromium.org/2780833004/diff/60001/ui/views/controls/table...
> File ui/views/controls/table/table_view_unittest.cc (right):
> 
>
https://codereview.chromium.org/2780833004/diff/60001/ui/views/controls/table...
> ui/views/controls/table/table_view_unittest.cc:209: params.bounds =
gfx::Rect(0,
> 0, 650, 650);
> Offsetting the (x,y) coordinate here might resolve the issues. E.g. use (100,
> 100, 650, 650); (but that might throw off coordinates elsewhere in the tests).
> 
> Alternatively, I think Widget::InitParams::TYPE_WINDOW_FRAMELESS above will
work
> too (perhaps better, but changing the Widget type can have other effects
around
> activation and things).
> 
> On Mac, when you Show() a real Widget (with a titlebar, etc.) at (0,0), the
> window manager moves it into the work area (below menu bar, or possibly a Dock
> hugging the left of the screen). Anticipating the move, or using a window type
> that the window manager doesn't move should work around that.

OK - I think I've nutted out a path towards removing the Mac weirdness around
this in https://codereview.chromium.org/2794213002 . Specifically, see the long
comment at
https://codereview.chromium.org/2794213002/diff/40001/ui/events/test/event_ge...
for ui::EventGenerator::set_assume_window_at_origin(bool)

That CL should get these tests passing on Mac, but there's enough new stuff
there that it can be a follow-up. I'm fine with disabling the failing tests on
Mac in the meantime.

[Evan Stade, 2017-04-04 15:13:18]

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

[commit-bot: I haz the power, 2017-04-04 15:16:45]

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

[Evan Stade, 2017-04-04 17:08:54]

On 2017/04/04 07:58:48, tapted wrote:
> On 2017/04/04 00:07:36, tapted wrote:
> >
>
https://codereview.chromium.org/2780833004/diff/60001/ui/views/controls/table...
> > File ui/views/controls/table/table_view_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2780833004/diff/60001/ui/views/controls/table...
> > ui/views/controls/table/table_view_unittest.cc:209: params.bounds =
> gfx::Rect(0,
> > 0, 650, 650);
> > Offsetting the (x,y) coordinate here might resolve the issues. E.g. use
(100,
> > 100, 650, 650); (but that might throw off coordinates elsewhere in the
tests).
> > 
> > Alternatively, I think Widget::InitParams::TYPE_WINDOW_FRAMELESS above will
> work
> > too (perhaps better, but changing the Widget type can have other effects
> around
> > activation and things).
> > 
> > On Mac, when you Show() a real Widget (with a titlebar, etc.) at (0,0), the
> > window manager moves it into the work area (below menu bar, or possibly a
Dock
> > hugging the left of the screen). Anticipating the move, or using a window
type
> > that the window manager doesn't move should work around that.
> 
> OK - I think I've nutted out a path towards removing the Mac weirdness around
> this in https://codereview.chromium.org/2794213002 . Specifically, see the
long
> comment at
>
https://codereview.chromium.org/2794213002/diff/40001/ui/events/test/event_ge...
> for ui::EventGenerator::set_assume_window_at_origin(bool)
> 
> That CL should get these tests passing on Mac, but there's enough new stuff
> there that it can be a follow-up. I'm fine with disabling the failing tests on
> Mac in the meantime.

ok, I've disabled the tests on mac with a TODO (parts of which don't make sense
anyway, as Trent points out, because ctrl+click shouldn't multi-select on mac).

[Evan Stade, 2017-04-04 17:08:58]

The CQ bit was unchecked by estade@chromium.org

[Evan Stade, 2017-04-04 17:09:03]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2017-04-04 17:09:03]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2780833004/#ps100001
(title: "disable for now on mac")

[commit-bot: I haz the power, 2017-04-04 17:09:54]

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 18:24:12]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491325743643300,
"parent_rev": "e98ffa8cb2bba874b43a55fe6c6bfbd6781d0b52", "commit_rev":
"3e70c6e83884c8c84864e20b89c367df6562c711"}

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

Description was changed from

==========
Improve TableView unit tests and fix bugs that were uncovered.

Switch to using a Widget and an EventGenerator, which simulates the
sequence of events more accurately than directly calling event handlers.

Change the implementation so we never change row selection due to focus changes.

BUG=none
==========

to

==========
Improve TableView unit tests and fix bugs that were uncovered.

Switch to using a Widget and an EventGenerator, which simulates the
sequence of events more accurately than directly calling event handlers.

Change the implementation so we never change row selection due to focus changes.

BUG=none

Review-Url: https://codereview.chromium.org/2780833004
Cr-Commit-Position: refs/heads/master@{#461775}
Committed:
https://chromium.googlesource.com/chromium/src/+/3e70c6e83884c8c84864e20b89c3...
==========

[commit-bot: I haz the power, 2017-04-04 18:25:06]

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