Move arrow scrolling into KeyboardEventManager.

Move arrow scrolling into KeyboardEventManager.

This moves the default arrow/pageup/home/end handler (resulting in a smooth scroll
animation) next to all the other keyboard default handlers.  Notes:

- The spacebar scrolling and Ctrl-C/A codepaths in WebViewImpl were dead code
and have likely been for years.  These keys were already handled by WebCore
before reaching WebViewImpl::keyEventDefault.  So I simply deleted them.

- One calendar layout test was timing out on Mac, relating to how
Alt-Down is also a shortcut for opening the calendar month flyout.  I couldn't
repro any problem manually, and it only happens in a racy close-flyout-then-reopen
scenario which seems like an artifact of test structure more than an intended
test case, so I just merged the two parts of the test.

BUG=657158
Committed: https://crrev.com/3f0b7ec4a4573fff9623546c348ee8fa9353d38c
Cr-Commit-Position: refs/heads/master@{#427925}

[aelias_OOO_until_Jul13, 2016-10-19 04:04:52]

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

[aelias_OOO_until_Jul13, 2016-10-19 04:05:12]

Description was changed from

==========
Move arrow and spacebar scrolling into KeyboardEventManager.

BUG=657158
==========

to

==========
Move arrow and spacebar scrolling into KeyboardEventManager.

[WIP, not yet for review]

BUG=657158
==========

[commit-bot: I haz the power, 2016-10-19 04:07:06]

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

[aelias_OOO_until_Jul13, 2016-10-19 04:15:26]

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

[commit-bot: I haz the power, 2016-10-19 04:17:05]

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

[aelias_OOO_until_Jul13, 2016-10-19 04:26:32]

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

[commit-bot: I haz the power, 2016-10-19 04:28:14]

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, 2016-10-19 09:54:05]

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

[commit-bot: I haz the power, 2016-10-19 09:54:06]

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

[aelias_OOO_until_Jul13, 2016-10-21 01:01:05]

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

[commit-bot: I haz the power, 2016-10-21 01:02:30]

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, 2016-10-21 01:27:54]

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

[commit-bot: I haz the power, 2016-10-21 01:27:55]

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

[aelias_OOO_until_Jul13, 2016-10-21 02:08:04]

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

[commit-bot: I haz the power, 2016-10-21 02:08:43]

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, 2016-10-21 02:30:21]

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

[commit-bot: I haz the power, 2016-10-21 02:30:22]

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

[aelias_OOO_until_Jul13, 2016-10-21 23:08:27]

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

[commit-bot: I haz the power, 2016-10-21 23:09:04]

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

[aelias_OOO_until_Jul13, 2016-10-21 23:56:54]

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

[commit-bot: I haz the power, 2016-10-21 23:57:27]

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

[aelias_OOO_until_Jul13, 2016-10-22 00:17:41]

Description was changed from

==========
Move arrow and spacebar scrolling into KeyboardEventManager.

[WIP, not yet for review]

BUG=657158
==========

to

==========
Move arrow scrolling into KeyboardEventManager.

This moves the default arrow/pageup/home/end handler (resulting in a smooth
scroll
animation) next to all the other keyboard default handlers.  Notes:

- The spacebar scrolling and Ctrl-C/A codepaths in WebViewImpl were dead code
and have likely been for years.  These keys were already handled by WebCore
before reaching WebViewImpl::keyEventDefault.  So I simply deleted them.

- isSystemKey simply means that it was an Alt+ key on Windows, so that's
covered by the Alt-modifier suppression and doesn't need to be written
explicitly.

- One <input> calendar layout test was unhappy with the Alt-Down handling on
Mac.
Alt-Down is also a shortcut for opening the calendar month flyout.  But, I
couldn't reproduce any problem manually with Alt-Down'ing the flyout with my
patch applied.  I'm not sure that's worth further investigation, so I just
changed the layout test to use the alternative shortcut F4.

BUG=657158
==========

[commit-bot: I haz the power, 2016-10-22 01:38:03]

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

[commit-bot: I haz the power, 2016-10-22 01:38:04]

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

[aelias_OOO_until_Jul13, 2016-10-22 02:16:29]

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

[commit-bot: I haz the power, 2016-10-22 02:16:56]

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, 2016-10-22 03:35:42]

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

[commit-bot: I haz the power, 2016-10-22 03:35:43]

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

[aelias_OOO_until_Jul13, 2016-10-23 00:16:43]

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

[commit-bot: I haz the power, 2016-10-23 00:16:58]

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, 2016-10-23 01:40:39]

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

[commit-bot: I haz the power, 2016-10-23 01:40:39]

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

[aelias_OOO_until_Jul13, 2016-10-26 02:27:09]

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

[commit-bot: I haz the power, 2016-10-26 02:27:25]

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

[aelias_OOO_until_Jul13, 2016-10-26 02:40:58]

Description was changed from

==========
Move arrow scrolling into KeyboardEventManager.

This moves the default arrow/pageup/home/end handler (resulting in a smooth
scroll
animation) next to all the other keyboard default handlers.  Notes:

- The spacebar scrolling and Ctrl-C/A codepaths in WebViewImpl were dead code
and have likely been for years.  These keys were already handled by WebCore
before reaching WebViewImpl::keyEventDefault.  So I simply deleted them.

- isSystemKey simply means that it was an Alt+ key on Windows, so that's
covered by the Alt-modifier suppression and doesn't need to be written
explicitly.

- One <input> calendar layout test was unhappy with the Alt-Down handling on
Mac.
Alt-Down is also a shortcut for opening the calendar month flyout.  But, I
couldn't reproduce any problem manually with Alt-Down'ing the flyout with my
patch applied.  I'm not sure that's worth further investigation, so I just
changed the layout test to use the alternative shortcut F4.

BUG=657158
==========

to

==========
Move arrow scrolling into KeyboardEventManager.

This moves the default arrow/pageup/home/end handler (resulting in a smooth
scroll
animation) next to all the other keyboard default handlers.  Notes:

- The spacebar scrolling and Ctrl-C/A codepaths in WebViewImpl were dead code
and have likely been for years.  These keys were already handled by WebCore
before reaching WebViewImpl::keyEventDefault.  So I simply deleted them.

- One calendar layout test was timing out on Mac, relating to how
Alt-Down is also a shortcut for opening the calendar month flyout.  I couldn't
repro any problem manually, and it only happens in a racy
close-flyout-then-reopen
scenario which seems like an artifact of test structure more than an intended
test case, so I just merged the two parts of the test.

BUG=657158
==========

[aelias_OOO_until_Jul13, 2016-10-26 02:45:17]

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

[commit-bot: I haz the power, 2016-10-26 02:45:36]

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

[aelias_OOO_until_Jul13, 2016-10-26 04:34:07]

aelias@chromium.org changed reviewers:
+ skobes@chromium.org

[aelias_OOO_until_Jul13, 2016-10-26 04:34:36]

Hi skobes@, PTAL.

[commit-bot: I haz the power, 2016-10-26 04:45:17]

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

[commit-bot: I haz the power, 2016-10-26 04:45:18]

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

[dtapuska, 2016-10-26 14:55:20]

On 2016/10/26 04:34:36, aelias wrote:
> Hi skobes@, PTAL.

Does Ctrl-C/Ctrl-A still work for plugins?

I recall investigating this code some time ago and had found this:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebLocalFr...


Also ideally we'd get rid of the VKEY's and just use key == "ArrowUp" instead.

[skobes, 2016-10-26 17:57:44]

lgtm w/ nits

https://codereview.chromium.org/2430183002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/KeyboardEventManager.cpp (right):

https://codereview.chromium.org/2430183002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/KeyboardEventManager.cpp:64: // on other
platforms are suppressed due to isSystemKey being set.)
Why do Alt-keys not set isSystemKey on Mac?

I wonder if it's safer to leave the #if guard on this, since it is Mac specific.

https://codereview.chromium.org/2430183002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/KeyboardEventManager.cpp:340: if
(event->ctrlKey() || event->metaKey() || event->shiftKey())
Maybe this check should be in focusDirectionForKey (and pass it the event).

[aelias_OOO_until_Jul13, 2016-10-26 19:27:33]

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

[aelias_OOO_until_Jul13, 2016-10-26 19:27:39]

On 2016/10/26 at 14:55:20, dtapuska wrote:
> On 2016/10/26 04:34:36, aelias wrote:
> > Hi skobes@, PTAL.
> 
> Does Ctrl-C/Ctrl-A still work for plugins?

Yes, I just tested this works with the Pdfium plugin.

> Also ideally we'd get rid of the VKEY's and just use key == "ArrowUp" instead.

I don't agree that there's any reason to prefer the strings.  Both of them are
equally as stable/well-defined (VKEYs are effectively part of the web standard
too).  I prefer the VKEYs if only because the compiler will produce an error in
case of typo.

https://codereview.chromium.org/2430183002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/KeyboardEventManager.cpp (right):

https://codereview.chromium.org/2430183002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/KeyboardEventManager.cpp:64: // on other
platforms are suppressed due to isSystemKey being set.)
On 2016/10/26 at 17:57:44, skobes wrote:
> Why do Alt-keys not set isSystemKey on Mac?
> 
> I wonder if it's safer to leave the #if guard on this, since it is Mac
specific.

On Mac, the system key is the meta key instead:
https://cs.chromium.org/chromium/src/content/browser/renderer_host/input/web_...

I have a strong bias against #if guards because they make unit testing much more
painful (issues only appear on platform trybots, not locally).  AFAIK this is
safe and well-aligned with the intent of the platform-specific suppression.  If
there's a regression reported out of this, I'll be happy to fix.

https://codereview.chromium.org/2430183002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/KeyboardEventManager.cpp:340: if
(event->ctrlKey() || event->metaKey() || event->shiftKey())
On 2016/10/26 at 17:57:44, skobes wrote:
> Maybe this check should be in focusDirectionForKey (and pass it the event).

Done.

[commit-bot: I haz the power, 2016-10-26 19:28:28]

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, 2016-10-26 19:36:57]

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

[commit-bot: I haz the power, 2016-10-26 19:37:00]

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

[aelias_OOO_until_Jul13, 2016-10-27 00:14:23]

The CQ bit was checked by aelias@chromium.org

[aelias_OOO_until_Jul13, 2016-10-27 00:14:24]

The patchset sent to the CQ was uploaded after l-g-t-m from skobes@chromium.org
Link to the patchset: https://codereview.chromium.org/2430183002/#ps220001
(title: "FocusDirection change")

[commit-bot: I haz the power, 2016-10-27 00:14:53]

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, 2016-10-27 01:38:02]

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

[commit-bot: I haz the power, 2016-10-27 01:38:03]

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

[aelias_OOO_until_Jul13, 2016-10-27 01:59:06]

The CQ bit was checked by aelias@chromium.org

[aelias_OOO_until_Jul13, 2016-10-27 01:59:06]

The patchset sent to the CQ was uploaded after l-g-t-m from skobes@chromium.org
Link to the patchset: https://codereview.chromium.org/2430183002/#ps240001
(title: "Add keyEvent() null check to fix test")

[commit-bot: I haz the power, 2016-10-27 01:59:22]

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, 2016-10-27 03:22:56]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-10-27 03:28:22]

Description was changed from

==========
Move arrow scrolling into KeyboardEventManager.

This moves the default arrow/pageup/home/end handler (resulting in a smooth
scroll
animation) next to all the other keyboard default handlers.  Notes:

- The spacebar scrolling and Ctrl-C/A codepaths in WebViewImpl were dead code
and have likely been for years.  These keys were already handled by WebCore
before reaching WebViewImpl::keyEventDefault.  So I simply deleted them.

- One calendar layout test was timing out on Mac, relating to how
Alt-Down is also a shortcut for opening the calendar month flyout.  I couldn't
repro any problem manually, and it only happens in a racy
close-flyout-then-reopen
scenario which seems like an artifact of test structure more than an intended
test case, so I just merged the two parts of the test.

BUG=657158
==========

to

==========
Move arrow scrolling into KeyboardEventManager.

This moves the default arrow/pageup/home/end handler (resulting in a smooth
scroll
animation) next to all the other keyboard default handlers.  Notes:

- The spacebar scrolling and Ctrl-C/A codepaths in WebViewImpl were dead code
and have likely been for years.  These keys were already handled by WebCore
before reaching WebViewImpl::keyEventDefault.  So I simply deleted them.

- One calendar layout test was timing out on Mac, relating to how
Alt-Down is also a shortcut for opening the calendar month flyout.  I couldn't
repro any problem manually, and it only happens in a racy
close-flyout-then-reopen
scenario which seems like an artifact of test structure more than an intended
test case, so I just merged the two parts of the test.

BUG=657158

Committed: https://crrev.com/3f0b7ec4a4573fff9623546c348ee8fa9353d38c
Cr-Commit-Position: refs/heads/master@{#427925}
==========

[commit-bot: I haz the power, 2016-10-27 03:28:24]

Patchset 13 (id:??) landed as
https://crrev.com/3f0b7ec4a4573fff9623546c348ee8fa9353d38c
Cr-Commit-Position: refs/heads/master@{#427925}