Dynamically enable spatial navigation based on events

Dynamically enable spatial navigation based on events

As currently implemented spatial navigation and touch event handling
have some unexpected interactions (see bug link) so instead:
1/ make a better-guess initial value based on presence of touchscreen
2/ dynamically enable spatnav when a DPAD event is received, and disable
   it when touch down event received.

BUG=309425
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=230304

[dmazzoni, 2013-10-22 06:47:55]

lgtm

https://codereview.chromium.org/33823005/diff/1/android_webview/java/src/org/...
File android_webview/java/src/org/chromium/android_webview/AwSettings.java
(right):

https://codereview.chromium.org/33823005/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwSettings.java:197: //
Best-guess  a sensible initial value based on the features supported on the
device.
nit: delete extra space after "guess"

https://codereview.chromium.org/33823005/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwSettings.java:353:
mEventHandler.updateWebkitPreferencesLocked();
Does this have the right effect, visually? I didn't see code in Blink that
triggers when the pref changes, so visual artifacts might remain when
disabling it.

https://codereview.chromium.org/33823005/diff/1/android_webview/java/src/org/...
File
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java
(right):

https://codereview.chromium.org/33823005/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:43:
static int getViewDirectionFromKeyEvent(KeyEvent event) {
Would it be too verbose to say getViewFocusDirectionFromDpadKeyEvent?

[sgurun-gerrit only, 2013-10-22 17:18:00]

lgtm

the code seems fine to me as far as I can read it. I have to admit that I found
it a little bit unusual that we dynamically change the mode potentially on each
touch/key event, just to show the correct visual clue to the user. Is this a
temporary solution?

https://codereview.chromium.org/33823005/diff/1/android_webview/java/src/org/...
File android_webview/java/src/org/chromium/android_webview/AwSettings.java
(right):

https://codereview.chromium.org/33823005/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwSettings.java:84:
private boolean mSpatialNavigationEnabled;  // Default depends on device
feautres.
s/feautres/features/

https://codereview.chromium.org/33823005/diff/1/android_webview/java/src/org/...
File
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java
(right):

https://codereview.chromium.org/33823005/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:56:
return 0;
0 is a special number, i.e. View.FOCUSABLES_ALL, but I guess it is not very
important here.

[joth, 2013-10-22 22:02:41]

https://codereview.chromium.org/33823005/diff/1/android_webview/java/src/org/...
File android_webview/java/src/org/chromium/android_webview/AwSettings.java
(right):

https://codereview.chromium.org/33823005/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwSettings.java:84:
private boolean mSpatialNavigationEnabled;  // Default depends on device
feautres.
On 2013/10/22 17:18:00, sgurun wrote:
> s/feautres/features/

Done.

https://codereview.chromium.org/33823005/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwSettings.java:197: //
Best-guess  a sensible initial value based on the features supported on the
device.
On 2013/10/22 06:47:55, Dominic Mazzoni wrote:
> nit: delete extra space after "guess"

Done.

https://codereview.chromium.org/33823005/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwSettings.java:353:
mEventHandler.updateWebkitPreferencesLocked();
On 2013/10/22 06:47:55, Dominic Mazzoni wrote:
> Does this have the right effect, visually? I didn't see code in Blink that
> triggers when the pref changes, so visual artifacts might remain when
> disabling it.

I think you're right that when transitioning enabled -> disabled there's no
specific code in place to ensure the  focus ring is not left on an item that is
now no-longer clickable.

In practice it seems to work OK. The main issues of spatial nav == on are in
erroneous focus changes from touch events, and having touch-down event turn off
spat-nav avoids those.

I also toyed with:
- making spatial nav be a one-way latch (off->on)
- calling  RenderViewHost::ClearFocusedNode() when transitioning to off

neither those looked like an improvement, and having additional
ClearFocusedNode() may have other unintended consequences. So I think I'll go
for this strategy for this CL and see about following up with something more
elaborate depending on the outcome of ongoing discussion on the bug.

https://codereview.chromium.org/33823005/diff/1/android_webview/java/src/org/...
File
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java
(right):

https://codereview.chromium.org/33823005/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:43:
static int getViewDirectionFromKeyEvent(KeyEvent event) {
On 2013/10/22 06:47:55, Dominic Mazzoni wrote:
> Would it be too verbose to say getViewFocusDirectionFromDpadKeyEvent?

Good suggestion, however in the end I reverted this file due to this switch not
including DPAD_CENTER key

https://codereview.chromium.org/33823005/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:56:
return 0;
On 2013/10/22 17:18:00, sgurun wrote:
> 0 is a special number, i.e. View.FOCUSABLES_ALL, but I guess it is not very
> important here.

FOCUSABLES_ALL is used with  addFocusables(), these constants are the ones for
passing to focusSearch(int) for which 0 is invalid value
(but again, this file now reverted)

[commit-bot: I haz the power, 2013-10-22 23:14:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joth@chromium.org/33823005/120001

[commit-bot: I haz the power, 2013-10-23 03:27:46]

Change committed as 230304