Allow Spatial Navigation ignore elements with event handlers

Allow Spatial Navigation ignore elements with event handlers.

When deviceSupportsTouch is enabled, it is preferable
Spatial Navigation not put focus on elements with
event handlers, as it may make undesirable stops, and the
event handle can still be triggered through touch UI.

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

[c.shu, 2015-01-19 19:21:51]

c.shu@samsung.com changed reviewers:
+ fs@opera.com

[fs, 2015-01-20 09:12:50]

The issue subject:

"Add a flag in WebSettings to allow client of Spatial Navigation"

is highly confusing. Please fix it to better describe the fix.

Also remember to run a try-bot which runs layout-tests - then you might have
caught the below ;-)

https://codereview.chromium.org/832083008/diff/20001/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/832083008/diff/20001/Source/core/dom/Element....
Source/core/dom/Element.cpp:2239: if
(isSpatialNavigationIgnoreEventHandlers(document().frame()))
Expanding this condition I get:

is-enabled && ignore-event-handlers

which means that !is-enabled means we'll fall through - which I don't think is
intended?

[c.shu, 2015-01-20 15:51:21]

https://codereview.chromium.org/832083008/diff/20001/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/832083008/diff/20001/Source/core/dom/Element....
Source/core/dom/Element.cpp:2239: if
(isSpatialNavigationIgnoreEventHandlers(document().frame()))
On 2015/01/20 09:12:50, fs wrote:
> Expanding this condition I get:
> 
> is-enabled && ignore-event-handlers
> 
> which means that !is-enabled means we'll fall through - which I don't think is
> intended?
Thanks for catching this issue!

[fs, 2015-01-22 12:20:04]

fs@opera.com changed reviewers:
+ philipj@opera.com

[fs, 2015-01-22 12:20:05]

LGTM - but let's try to get rid of this new setting (and absolutely not add any
more...)

You'll need to find another reviewer for the */web/ bits (I'm adding philipj).

https://codereview.chromium.org/832083008/diff/40001/Source/core/page/Spatial...
File Source/core/page/SpatialNavigation.h (right):

https://codereview.chromium.org/832083008/diff/40001/Source/core/page/Spatial...
Source/core/page/SpatialNavigation.h:47: bool
isSpatialNavigationIgnoreEventHandlers(const LocalFrame*);
The 'is' prefix sounds weird - suggest this is changed to
"spatialNavigationIgnoresEventHandlers" instead.

[philipj_slow, 2015-01-22 14:11:29]

I looks like you've edited the description after uploading. You need to keep the
subject line in the description as well, or you'll end up with a summary-less
commit like https://src.chromium.org/viewvc/blink?revision=154102&view=revision

[philipj_slow, 2015-01-22 14:16:19]

I can't tell from the code when enabling this setting is a good idea. Can you
link to a bug describing the problem for which this is the solution?

[c.shu, 2015-01-22 16:17:26]

On 2015/01/22 12:20:05, fs wrote:
> LGTM - but let's try to get rid of this new setting (and absolutely not add
any
> more...)
agree.

[c.shu, 2015-01-22 16:19:17]

https://codereview.chromium.org/832083008/diff/40001/Source/core/page/Spatial...
File Source/core/page/SpatialNavigation.h (right):

https://codereview.chromium.org/832083008/diff/40001/Source/core/page/Spatial...
Source/core/page/SpatialNavigation.h:47: bool
isSpatialNavigationIgnoreEventHandlers(const LocalFrame*);
On 2015/01/22 12:20:04, fs wrote:
> The 'is' prefix sounds weird - suggest this is changed to
> "spatialNavigationIgnoresEventHandlers" instead.

Done.

[c.shu, 2015-01-22 16:41:26]

On 2015/01/22 14:16:19, philipj wrote:
> I can't tell from the code when enabling this setting is a good idea. Can you
> link to a bug describing the problem for which this is the solution?

Thanks, philipj. There was some discussion in another CL
https://codereview.chromium.org/815573002.
Basically, we observed that spatial navigation may stop on elements
containing event listeners that don't do anything, which caused
unpleasant user experience. In addition, our use case is mobile phone
with Bluetooth keyboard in which we can still touch the phone in
case we definitely need to trigger a link. Thus, we need a setting
to control the spatial navigation behavior.

[c.shu, 2015-01-22 20:37:31]

I uploaded a new CL that does not introduce a new setting.
Instead, it uses an existing one: DeviceSupportsTouch.
Thus we can make less stops in spatial navigation when
the touch UI is still available.
For example, it helps the mobile phone + bluetooth keyboard
configuration and it should not affect TV use cases.

[philipj_slow, 2015-01-22 23:10:08]

I would suggest asking Rick Byers <rbyers@chromium.org> to look at this, I'm not
comfortable OK'ing a change to spatnav as I've never worked with it.

[fs, 2015-01-23 09:45:18]

On 2015/01/22 20:37:31, c.shu wrote:
> I uploaded a new CL that does not introduce a new setting.
> Instead, it uses an existing one: DeviceSupportsTouch.
> Thus we can make less stops in spatial navigation when
> the touch UI is still available.
> For example, it helps the mobile phone + bluetooth keyboard
> configuration and it should not affect TV use cases.

Thanks this seems ok to me (when I manage to find time to get
https://codereview.chromium.org/810493003 into shape it'll be even more local
knowledge too.)

LGTM from my PoV.

[c.shu, 2015-01-23 15:17:10]

The CQ bit was checked by c.shu@samsung.com

[commit-bot: I haz the power, 2015-01-23 15:17:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/832083008/80001

[c.shu, 2015-01-23 15:18:12]

On 2015/01/22 23:10:08, philipj wrote:
> I would suggest asking Rick Byers <mailto:rbyers@chromium.org> to look at
this, I'm not
> comfortable OK'ing a change to spatnav as I've never worked with it.

Thanks, philipj, as the new CL only impacts core/dom and core/page, fs should be
good to cover it.

[c.shu, 2015-01-23 15:18:31]

On 2015/01/23 09:45:18, fs wrote:
> On 2015/01/22 20:37:31, c.shu wrote:
> > I uploaded a new CL that does not introduce a new setting.
> > Instead, it uses an existing one: DeviceSupportsTouch.
> > Thus we can make less stops in spatial navigation when
> > the touch UI is still available.
> > For example, it helps the mobile phone + bluetooth keyboard
> > configuration and it should not affect TV use cases.
> 
> Thanks this seems ok to me (when I manage to find time to get
> https://codereview.chromium.org/810493003 into shape it'll be even more local
> knowledge too.)
> 
> LGTM from my PoV.

Thanks for the support, fs.

[commit-bot: I haz the power, 2015-01-23 16:27:58]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188894