Spatial navigation enhancements

This patch improves the user experience when using spatial navigation (enabled in https://codereview.chromium.org/13811041 and https://codereview.chromium.org/14110006 ):

- Dispatch onClick when pressing ENTER or SPACE on any element (not only input buttons as it was before the patch)
- Make HTMLElement focusable by spatnav if it has click or key events (before it could only navigate to links and form items). 

The changes are effective only if spatialNavigationEnabled pref is enabled.



BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=152930

[Krzysztof Olczyk, 2013-06-14 07:48:27]

Dear tkent and morrita1,

I'm adding you as the reviewers as I have found you in other review touching
similar piece of code.

Could you please take a look at this small change?

Thanks in advance!

[esprehn, 2013-06-14 08:01:24]

https://codereview.chromium.org/16959002/diff/1/Source/core/html/HTMLElement.cpp
File Source/core/html/HTMLElement.cpp (right):

https://codereview.chromium.org/16959002/diff/1/Source/core/html/HTMLElement....
Source/core/html/HTMLElement.cpp:605: EventTarget* target =
const_cast<HTMLElement*>(element);
I don't understand why you need this cast. Just call hasEventListeners directly?
It'd be better if this was just a private method on HTMLElement and you put the
spatialNavigationEnabled() check in there too.

supportsFocus() would do return ... || supportsSpatialNavigationFocus();

https://codereview.chromium.org/16959002/diff/1/Source/core/html/HTMLElement....
Source/core/html/HTMLElement.cpp:1039: void
HTMLElement::defaultEventHandler(Event* evt)
event. Please don't abbreviate.

https://codereview.chromium.org/16959002/diff/1/Source/core/html/HTMLElement....
Source/core/html/HTMLElement.cpp:1041: if (evt->isKeyboardEvent() && evt->type()
== eventNames().keypressEvent) {
I would put the type() check first so you can avoid the virtual call to
isKeyboardEvent() in the common case.

https://codereview.chromium.org/16959002/diff/1/Source/core/html/HTMLElement....
Source/core/html/HTMLElement.cpp:1042:
handleKeypressEvent(static_cast<KeyboardEvent*>(evt));
Please add a toKeyboardEvent() function, they're in lots of headers. It should
ASSERT_WITH_SECURITY_IMPLICATION that event->isKeyboardEvent().

https://codereview.chromium.org/16959002/diff/1/Source/core/html/HTMLElement....
Source/core/html/HTMLElement.cpp:1054: if
(document()->settings()->spatialNavigationEnabled() && supportsFocus()) {
The spatial navigation check should be outside this if() and you should early
return from the function if either that is false or supportsFocus() is false.

https://codereview.chromium.org/16959002/diff/1/Source/core/html/HTMLElement....
Source/core/html/HTMLElement.cpp:1057: return;
Return is not needed.

https://codereview.chromium.org/16959002/diff/1/Source/core/html/HTMLElement.h
File Source/core/html/HTMLElement.h (right):

https://codereview.chromium.org/16959002/diff/1/Source/core/html/HTMLElement....
Source/core/html/HTMLElement.h:133: virtual void
handleKeypressEvent(KeyboardEvent*);
This shouldn't be virtual.

[Hajime Morrita, 2013-06-14 08:09:48]

Any idea about testing?
Seems like we can add an Internals API  to toggle the setting, then this feature
will be easily testable.

[Krzysztof Olczyk, 2013-06-14 09:46:52]

On 2013/06/14 08:01:24, esprehn wrote:
>
https://codereview.chromium.org/16959002/diff/1/Source/core/html/HTMLElement.cpp
> File Source/core/html/HTMLElement.cpp (right):
> 
>
https://codereview.chromium.org/16959002/diff/1/Source/core/html/HTMLElement....
> Source/core/html/HTMLElement.cpp:605: EventTarget* target =
> const_cast<HTMLElement*>(element);
> I don't understand why you need this cast. Just call hasEventListeners
directly?
> It'd be better if this was just a private method on HTMLElement and you put
the
> spatialNavigationEnabled() check in there too.

This is needed because supportsFocus is a const method and hasEventListener is
not.

Other changes applied as suggested.

[tkent, 2013-06-16 22:50:24]

https://codereview.chromium.org/16959002/diff/8001/Source/core/dom/KeyboardEv...
File Source/core/dom/KeyboardEvent.h (right):

https://codereview.chromium.org/16959002/diff/8001/Source/core/dom/KeyboardEv...
Source/core/dom/KeyboardEvent.h:128: ASSERT_WITH_SECURITY_IMPLICATION(event &&
event->isKeyboardEvent());
We usually allow NULL argument for toFoo functions.

https://codereview.chromium.org/16959002/diff/8001/Source/core/html/HTMLEleme...
File Source/core/html/HTMLElement.cpp (right):

https://codereview.chromium.org/16959002/diff/8001/Source/core/html/HTMLEleme...
Source/core/html/HTMLElement.cpp:606: return
target->hasEventListeners(eventNames().clickEvent)
This looks an incompatible change. An element with click event handlers receives
focus/blur events unexpectedly.  Is it safe?

[Krzysztof Olczyk, 2013-06-17 12:04:05]

>
https://codereview.chromium.org/16959002/diff/8001/Source/core/dom/KeyboardEv...
> File Source/core/dom/KeyboardEvent.h (right):
> 
>
https://codereview.chromium.org/16959002/diff/8001/Source/core/dom/KeyboardEv...
> Source/core/dom/KeyboardEvent.h:128: ASSERT_WITH_SECURITY_IMPLICATION(event &&
> event->isKeyboardEvent());
> We usually allow NULL argument for toFoo functions.

I took toMouseEvent as a model implementation and it asserts event is not null.

>
https://codereview.chromium.org/16959002/diff/8001/Source/core/html/HTMLEleme...
> File Source/core/html/HTMLElement.cpp (right):
> 
>
https://codereview.chromium.org/16959002/diff/8001/Source/core/html/HTMLEleme...
> Source/core/html/HTMLElement.cpp:606: return
> target->hasEventListeners(eventNames().clickEvent)
> This looks an incompatible change. An element with click event handlers
receives
> focus/blur events unexpectedly.  Is it safe?

That is true, this patch extends the criteria for the element to be focusable.
This improves
the experience of keyboard-controlling the web pages which originally were not
designed for keyboard navigation.
This is the way to make it possible to navigate to (focus) elements which web
designer meant for being active
(made them respond to click events) with keyboard focus and activate the element
with keyboard (simulate clicks).
This mimics the spatial navigation experience present in previous Opera
browsers.

Note that this patch introduces this facility only if spatial navigation feature
pref is ON.

[esprehn, 2013-06-17 19:54:56]

On 2013/06/17 12:04:05, Krzysztof Olczyk wrote:
> ...
> >
>
https://codereview.chromium.org/16959002/diff/8001/Source/core/dom/KeyboardEv...
> > Source/core/dom/KeyboardEvent.h:128: ASSERT_WITH_SECURITY_IMPLICATION(event
&&
> > event->isKeyboardEvent());
> > We usually allow NULL argument for toFoo functions.
> 
> I took toMouseEvent as a model implementation and it asserts event is not
null.
> 

Sounds like toMouseEvent is wrong. :) I definitely wouldn't expect any toFoo(0)
call in core/ to ASSERT.

[tkent, 2013-06-17 22:58:24]

Please use "reply" feature on the diff view if possible.

On 2013/06/17 12:04:05, Krzysztof Olczyk wrote:
> > This looks an incompatible change. An element with click event handlers
> receives
> > focus/blur events unexpectedly.  Is it safe?
> 
> That is true, this patch extends the criteria for the element to be focusable.
> This improves
> the experience of keyboard-controlling the web pages which originally were not
> designed for keyboard navigation.
> This is the way to make it possible to navigate to (focus) elements which web
> designer meant for being active
> (made them respond to click events) with keyboard focus and activate the
element
> with keyboard (simulate clicks).
> This mimics the spatial navigation experience present in previous Opera
> browsers.
> 
> Note that this patch introduces this facility only if spatial navigation
feature
> pref is ON.

Yeah, I understand the motivation and the behavior change is only in spatial
navigation mode.
Though I think this is a minor issue, we had better document this behavior.
 - Add a comment to HTMLElement::supportsSpatialNavigationFocus
 - Add a comment to public/web/WebSettings.h
 - Document this on webplatform.org later.

[Krzysztof Olczyk, 2013-06-18 11:55:01]

Thank you for your comments/suggestions.
Please take a look at new diff with the suggestions applied.

[tkent, 2013-06-19 06:30:40]

lgtm

[tkent, 2013-06-19 06:32:31]

Please rebase and re-upload new patch.  Patch Set 3 will conflict with ToT.

[Krzysztof Olczyk, 2013-06-19 08:06:54]

Patch rebased and submitted as Patch set 4.

[commit-bot: I haz the power, 2013-06-19 08:10:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kolczyk@opera.com/16959002/22001

[commit-bot: I haz the power, 2013-06-19 11:07:39]

Retried try job too often on mac_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[commit-bot: I haz the power, 2013-06-19 12:28:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kolczyk@opera.com/16959002/22001

[commit-bot: I haz the power, 2013-06-19 13:33:30]

Retried try job too often on mac_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[commit-bot: I haz the power, 2013-06-20 01:44:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kolczyk@opera.com/16959002/22001

[commit-bot: I haz the power, 2013-06-20 03:52:00]

Retried try job too often on mac_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[commit-bot: I haz the power, 2013-06-20 03:53:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kolczyk@opera.com/16959002/22001

[commit-bot: I haz the power, 2013-06-20 05:04:31]

Retried try job too often on mac_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[commit-bot: I haz the power, 2013-06-20 06:36:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kolczyk@opera.com/16959002/22001

[commit-bot: I haz the power, 2013-06-20 11:45:18]

Retried try job too often on mac_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[tkent, 2013-06-21 02:36:52]

https://chromiumcodereview.appspot.com/16959002/diff/22001/Source/core/html/H...
File Source/core/html/HTMLElement.cpp (right):

https://chromiumcodereview.appspot.com/16959002/diff/22001/Source/core/html/H...
Source/core/html/HTMLElement.cpp:611: if
(!document()->settings()->spatialNavigationEnabled())
document()->settings() can be NULL.

[Krzysztof Olczyk, 2013-06-21 07:22:50]

Thanks for pointing the potential null out. Fixed now.

https://chromiumcodereview.appspot.com/16959002/diff/22001/Source/core/html/H...
File Source/core/html/HTMLElement.cpp (right):

https://chromiumcodereview.appspot.com/16959002/diff/22001/Source/core/html/H...
Source/core/html/HTMLElement.cpp:611: if
(!document()->settings()->spatialNavigationEnabled())
On 2013/06/21 02:36:53, tkent wrote:
> document()->settings() can be NULL.

Done.

[commit-bot: I haz the power, 2013-06-23 04:00:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kolczyk@opera.com/16959002/86001

[commit-bot: I haz the power, 2013-06-23 05:27:36]

Change committed as 152930