Add an experimental keyboard lock, where Esc must be held to exit.

Add an experimental keyboard lock, where Esc must be held to exit.

This CL adds an experimental fullscreen with keyboard lock feature,
which can be enabled in chrome://flags. Once toggled, the browser
requires Esc to be held down for at least 1.5 seconds to exit
fullscreen. This is to evaluate how the interaction feels in the browser.

BUG=595958
Committed: https://crrev.com/177196c218135882ba6438dfd885fd540be00852
Cr-Commit-Position: refs/heads/master@{#383031}

[dominickn, 2016-03-18 04:04:18]

dominickn@chromium.org changed reviewers:
+ mgiuca@chromium.org

[dominickn, 2016-03-18 04:04:18]

PTAL and let me know what you think of the implementation. Thanks!

[Matt Giuca, 2016-03-21 00:36:07]

https://codereview.chromium.org/1814983003/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1814983003/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:6509: +        Experimental full screen UI.
I think this flag name is too generic (and confusable with
simplified-fullscreen-ui). "experimental-x" means we're trying this out and
planning to flip it on at some point. But you're not planning to do that, right,
you're planning to make this a separate mode? So maybe call it
experimental-keyboard-lock-ui or experimental-keylock-ui.

https://codereview.chromium.org/1814983003/diff/1/chrome/browser/ui/exclusive...
File chrome/browser/ui/exclusive_access/exclusive_access_manager.cc (right):

https://codereview.chromium.org/1814983003/diff/1/chrome/browser/ui/exclusive...
chrome/browser/ui/exclusive_access/exclusive_access_manager.cc:25: const double
kDurationToHoldEscapeToExitFullscreen = 1.5;
I think this should be in c/b/u/exclusive_access/exclusive_access_bubble, along
with all of the other timing constants. And it should be int, in ms.

https://codereview.chromium.org/1814983003/diff/1/chrome/browser/ui/exclusive...
chrome/browser/ui/exclusive_access/exclusive_access_manager.cc:132: if
(event.windowsKeyCode != ui::VKEY_ESCAPE) {
This block got duplicated. It's the same for both the flag on and off, so move
it out (and keep the "return false").

https://codereview.chromium.org/1814983003/diff/1/chrome/browser/ui/exclusive...
chrome/browser/ui/exclusive_access/exclusive_access_manager.cc:147:
base::TimeDelta::FromSeconds(kDurationToHoldEscapeToExitFullscreen),
FromSeconds takes an int, so this will round down to 1s. You want FromSecondsD.
(But also, the constant should be in ms, so then you want FromSeconds.)

https://codereview.chromium.org/1814983003/diff/1/chrome/common/chrome_featur...
File chrome/common/chrome_features.cc (right):

https://codereview.chromium.org/1814983003/diff/1/chrome/common/chrome_featur...
chrome/common/chrome_features.cc:18: // and system-reserved keybaord shortcuts.
keyboard

[dominickn, 2016-03-21 21:54:40]

Description was changed from

==========
Add an experimental fullscreen, where Esc must be held to exit.

This CL adds an experimental fullscreen feature, which can be enabled in
chrome://flags. Once toggled, the browser will require Esc to be held
down for at least 1.5 seconds to exit fullscreen. This is to evaluate
how the interaction feels in the browser.

BUG=595958
==========

to

==========
Add an experimental keyboard lock, where Esc must be held to exit.

This CL adds an experimental fullscreen with keyboard lock feature,
which can be enabled in chrome://flags. Once toggled, the browser
requires Esc to be held down for at least 1.5 seconds to exit
fullscreen. This is to evaluate how the interaction feels in the browser.

BUG=595958
==========

[dominickn, 2016-03-21 21:58:03]

dominickn@chromium.org changed reviewers:
+ msw@chromium.org

[dominickn, 2016-03-21 21:58:04]

Thanks!

+msw: PTAL. This flag and experimental mode is a step towards implementing a
"keyboard lock" feature, where sites in fullscreen would be allowed to map Esc
and other browser (and possibly system) keyboard shortcuts.

https://codereview.chromium.org/1814983003/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1814983003/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:6509: +        Experimental full screen UI.
On 2016/03/21 00:36:07, Matt Giuca wrote:
> I think this flag name is too generic (and confusable with
> simplified-fullscreen-ui). "experimental-x" means we're trying this out and
> planning to flip it on at some point. But you're not planning to do that,
right,
> you're planning to make this a separate mode? So maybe call it
> experimental-keyboard-lock-ui or experimental-keylock-ui.

Done.

https://codereview.chromium.org/1814983003/diff/1/chrome/browser/ui/exclusive...
File chrome/browser/ui/exclusive_access/exclusive_access_manager.cc (right):

https://codereview.chromium.org/1814983003/diff/1/chrome/browser/ui/exclusive...
chrome/browser/ui/exclusive_access/exclusive_access_manager.cc:25: const double
kDurationToHoldEscapeToExitFullscreen = 1.5;
On 2016/03/21 00:36:07, Matt Giuca wrote:
> I think this should be in c/b/u/exclusive_access/exclusive_access_bubble,
along
> with all of the other timing constants. And it should be int, in ms.

That means a) pulling in exclusive_access_bubble.h here, so there's a circular
include, and b) making the constant a public or friend member since we need it
here in the manager (the other constants are all protected). I'm going to leave
it here for now, pending OWNERs comments or a larger refactor later down the
line.

https://codereview.chromium.org/1814983003/diff/1/chrome/browser/ui/exclusive...
chrome/browser/ui/exclusive_access/exclusive_access_manager.cc:132: if
(event.windowsKeyCode != ui::VKEY_ESCAPE) {
On 2016/03/21 00:36:07, Matt Giuca wrote:
> This block got duplicated. It's the same for both the flag on and off, so move
> it out (and keep the "return false").

Good point, thanks

https://codereview.chromium.org/1814983003/diff/1/chrome/browser/ui/exclusive...
chrome/browser/ui/exclusive_access/exclusive_access_manager.cc:147:
base::TimeDelta::FromSeconds(kDurationToHoldEscapeToExitFullscreen),
On 2016/03/21 00:36:07, Matt Giuca wrote:
> FromSeconds takes an int, so this will round down to 1s. You want
FromSecondsD.
> (But also, the constant should be in ms, so then you want FromSeconds.)

FromMillseeconds, done

https://codereview.chromium.org/1814983003/diff/1/chrome/common/chrome_featur...
File chrome/common/chrome_features.cc (right):

https://codereview.chromium.org/1814983003/diff/1/chrome/common/chrome_featur...
chrome/common/chrome_features.cc:18: // and system-reserved keybaord shortcuts.
On 2016/03/21 00:36:07, Matt Giuca wrote:
> keyboard

Done.

[msw, 2016-03-23 07:20:55]

https://codereview.chromium.org/1814983003/diff/1/chrome/browser/ui/exclusive...
File chrome/browser/ui/exclusive_access/exclusive_access_manager.cc (right):

https://codereview.chromium.org/1814983003/diff/1/chrome/browser/ui/exclusive...
chrome/browser/ui/exclusive_access/exclusive_access_manager.cc:25: const double
kDurationToHoldEscapeToExitFullscreen = 1.5;
On 2016/03/21 21:58:04, dominickn wrote:
> On 2016/03/21 00:36:07, Matt Giuca wrote:
> > I think this should be in c/b/u/exclusive_access/exclusive_access_bubble,
> along
> > with all of the other timing constants. And it should be int, in ms.
> 
> That means a) pulling in exclusive_access_bubble.h here, so there's a circular
> include, and b) making the constant a public or friend member since we need it
> here in the manager (the other constants are all protected). I'm going to
leave
> it here for now, pending OWNERs comments or a larger refactor later down the
> line.

nit: add a todo if you think it should be moved

https://codereview.chromium.org/1814983003/diff/20001/chrome/browser/ui/exclu...
File chrome/browser/ui/exclusive_access/exclusive_access_manager.cc (right):

https://codereview.chromium.org/1814983003/diff/20001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_manager.cc:69: if
(IsExperimentalKeyboardLockUIEnabled())
Shouldn't this key off of something other than the experiment being enabled
(like the page actually engaging this keyboard lock mode?), or is this just a
hacky way of testing the mode anytime a site enters fullscreen with the
experiment enabled?

https://codereview.chromium.org/1814983003/diff/20001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_manager.cc:136: if
(event.type == content::NativeWebKeyboardEvent::KeyUp &&
Shouldn't this check if the key pressed/released is esc?

https://codereview.chromium.org/1814983003/diff/20001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_manager.cc:154: } else {
nit: no else after return (just put the old code below the conditional block)

https://codereview.chromium.org/1814983003/diff/20001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_manager.cc:157: handled |=
mouse_lock_controller_.HandleUserPressedEscape();
Should this feature add a corresponding key_lock_controller?

https://codereview.chromium.org/1814983003/diff/20001/chrome/browser/ui/exclu...
File chrome/browser/ui/exclusive_access/exclusive_access_manager.h (right):

https://codereview.chromium.org/1814983003/diff/20001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_manager.h:83: base::Timer
hold_timer_;
nit: use OneShotTimer

https://codereview.chromium.org/1814983003/diff/20001/chrome/common/chrome_fe...
File chrome/common/chrome_features.cc (right):

https://codereview.chromium.org/1814983003/diff/20001/chrome/common/chrome_fe...
chrome/common/chrome_features.cc:17: // An experimental fullscreen prototyping
an API allowing pages to map browser-
nit: "prototyping an API allowing" is awkward; consider: "prototype that allows"

[dominickn, 2016-03-23 10:07:03]

Thanks!

https://codereview.chromium.org/1814983003/diff/20001/chrome/browser/ui/exclu...
File chrome/browser/ui/exclusive_access/exclusive_access_manager.cc (right):

https://codereview.chromium.org/1814983003/diff/20001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_manager.cc:69: if
(IsExperimentalKeyboardLockUIEnabled())
On 2016/03/23 07:20:54, msw wrote:
> Shouldn't this key off of something other than the experiment being enabled
> (like the page actually engaging this keyboard lock mode?), or is this just a
> hacky way of testing the mode anytime a site enters fullscreen with the
> experiment enabled?

This is a hacky way of testing the mode for now - we want to get some feedback
from UI. Plus, much of this code will soon disappear when
simplified-fullscreen-ui is the default on all platforms, so it'll be good to
wait until after then before introducing more complexity.

https://codereview.chromium.org/1814983003/diff/20001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_manager.cc:136: if
(event.type == content::NativeWebKeyboardEvent::KeyUp &&
On 2016/03/23 07:20:54, msw wrote:
> Shouldn't this check if the key pressed/released is esc?

Only Esc will get this far - all other keys are captured in the if statement
above.

https://codereview.chromium.org/1814983003/diff/20001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_manager.cc:154: } else {
On 2016/03/23 07:20:55, msw wrote:
> nit: no else after return (just put the old code below the conditional block)

Done.

https://codereview.chromium.org/1814983003/diff/20001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_manager.cc:157: handled |=
mouse_lock_controller_.HandleUserPressedEscape();
On 2016/03/23 07:20:55, msw wrote:
> Should this feature add a corresponding key_lock_controller?

At this stage, we want to test how the hold to escape interaction feels. I feel
like adding a full new controller should come as we scope out the feature more
(and refactor. But if you prefer, I can add a new controller now. Thoughts?

https://codereview.chromium.org/1814983003/diff/20001/chrome/browser/ui/exclu...
File chrome/browser/ui/exclusive_access/exclusive_access_manager.h (right):

https://codereview.chromium.org/1814983003/diff/20001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_manager.h:83: base::Timer
hold_timer_;
On 2016/03/23 07:20:55, msw wrote:
> nit: use OneShotTimer

Done.

https://codereview.chromium.org/1814983003/diff/20001/chrome/common/chrome_fe...
File chrome/common/chrome_features.cc (right):

https://codereview.chromium.org/1814983003/diff/20001/chrome/common/chrome_fe...
chrome/common/chrome_features.cc:17: // An experimental fullscreen prototyping
an API allowing pages to map browser-
On 2016/03/23 07:20:55, msw wrote:
> nit: "prototyping an API allowing" is awkward; consider: "prototype that
allows"

Done.

[msw, 2016-03-23 17:07:32]

lgtm with a nit

https://codereview.chromium.org/1814983003/diff/20001/chrome/browser/ui/exclu...
File chrome/browser/ui/exclusive_access/exclusive_access_manager.cc (right):

https://codereview.chromium.org/1814983003/diff/20001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_manager.cc:136: if
(event.type == content::NativeWebKeyboardEvent::KeyUp &&
On 2016/03/23 10:07:03, dominickn wrote:
> On 2016/03/23 07:20:54, msw wrote:
> > Shouldn't this check if the key pressed/released is esc?
> 
> Only Esc will get this far - all other keys are captured in the if statement
> above.

Acknowledged.

https://codereview.chromium.org/1814983003/diff/20001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_manager.cc:157: handled |=
mouse_lock_controller_.HandleUserPressedEscape();
On 2016/03/23 10:07:03, dominickn wrote:
> On 2016/03/23 07:20:55, msw wrote:
> > Should this feature add a corresponding key_lock_controller?
> 
> At this stage, we want to test how the hold to escape interaction feels. I
feel
> like adding a full new controller should come as we scope out the feature more
> (and refactor. But if you prefer, I can add a new controller now. Thoughts?

Nope; now I understand the hackyness level; this is fine.

https://codereview.chromium.org/1814983003/diff/40001/chrome/browser/ui/exclu...
File chrome/browser/ui/exclusive_access/exclusive_access_manager.cc (right):

https://codereview.chromium.org/1814983003/diff/40001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_manager.cc:36: hold_timer_()
{
nit: this explicit call to the default ctor shouldn't be necessary.

[dominickn, 2016-03-23 23:35:42]

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

[dominickn, 2016-03-23 23:36:22]

Thanks!

+sky: PTAL at chrome/common.

https://codereview.chromium.org/1814983003/diff/40001/chrome/browser/ui/exclu...
File chrome/browser/ui/exclusive_access/exclusive_access_manager.cc (right):

https://codereview.chromium.org/1814983003/diff/40001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_manager.cc:36: hold_timer_()
{
On 2016/03/23 17:07:32, msw wrote:
> nit: this explicit call to the default ctor shouldn't be necessary.

Done.

[sky, 2016-03-23 23:46:10]

FTR this features sounds AWFUL! If we add this feature can any site use it?

https://codereview.chromium.org/1814983003/diff/60001/chrome/common/chrome_fe...
File chrome/common/chrome_features.cc (right):

https://codereview.chromium.org/1814983003/diff/60001/chrome/common/chrome_fe...
chrome/common/chrome_features.cc:17: // An experimental fullscreen prototype
that allows pages to map browser- and
'browser-' -> 'browser'

[dominickn, 2016-03-23 23:53:56]

On 2016/03/23 23:46:10, sky wrote:
> FTR this features sounds AWFUL! If we add this feature can any site use it?

The overall idea is that there would be an API to request fullscreen with
keyboard lock, and
then sites can capture Ctrl-T/Ctrl-W/Esc etc., and possibly system level
shortcuts as well. The
big use cases are Chromoting/gaming/VR/etc., which would use the API to create
an immersive
experience that allows users to use all the usual shortcuts naturally.

The question is, if this exists, how will it be signalled, and how will users
get out? The
proposal is that the presentation is similar to the existing fullscreen, except
the user holds
Escape to get out, rather than just pressing it. This CL allows us and UI to
experiment with
the hold to get out to see how it actually feels. There's no keyboard lock
implemented yet.

More generally, having a way to allow sites to map the reserved
Ctrl-T/Ctrl-W/etc. shortcuts
has been a long standing question. We forbid it, but Firefox allows it. This
feature is an
attempt to find a compromise where we allow remapping when the tabbed UI is
obviously
not there (i.e. fullscreen). By making it an API, there's the possibility to
make it available
more broadly, but that's a (much) longer term discussion.

[sky, 2016-03-24 02:45:01]

LGTM

[dominickn, 2016-03-24 03:03:53]

Thanks!

https://codereview.chromium.org/1814983003/diff/60001/chrome/common/chrome_fe...
File chrome/common/chrome_features.cc (right):

https://codereview.chromium.org/1814983003/diff/60001/chrome/common/chrome_fe...
chrome/common/chrome_features.cc:17: // An experimental fullscreen prototype
that allows pages to map browser- and
On 2016/03/23 23:46:10, sky wrote:
> 'browser-' -> 'browser'

Done.

[dominickn, 2016-03-24 04:23:16]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2016-03-24 04:23:16]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1814983003/#ps80001
(title: "Addressing comment nit")

[commit-bot: I haz the power, 2016-03-24 04:23:46]

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

[commit-bot: I haz the power, 2016-03-24 04:32:06]

Description was changed from

==========
Add an experimental keyboard lock, where Esc must be held to exit.

This CL adds an experimental fullscreen with keyboard lock feature,
which can be enabled in chrome://flags. Once toggled, the browser
requires Esc to be held down for at least 1.5 seconds to exit
fullscreen. This is to evaluate how the interaction feels in the browser.

BUG=595958
==========

to

==========
Add an experimental keyboard lock, where Esc must be held to exit.

This CL adds an experimental fullscreen with keyboard lock feature,
which can be enabled in chrome://flags. Once toggled, the browser
requires Esc to be held down for at least 1.5 seconds to exit
fullscreen. This is to evaluate how the interaction feels in the browser.

BUG=595958
==========

[commit-bot: I haz the power, 2016-03-24 04:32:07]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-24 04:33:11]

Description was changed from

==========
Add an experimental keyboard lock, where Esc must be held to exit.

This CL adds an experimental fullscreen with keyboard lock feature,
which can be enabled in chrome://flags. Once toggled, the browser
requires Esc to be held down for at least 1.5 seconds to exit
fullscreen. This is to evaluate how the interaction feels in the browser.

BUG=595958
==========

to

==========
Add an experimental keyboard lock, where Esc must be held to exit.

This CL adds an experimental fullscreen with keyboard lock feature,
which can be enabled in chrome://flags. Once toggled, the browser
requires Esc to be held down for at least 1.5 seconds to exit
fullscreen. This is to evaluate how the interaction feels in the browser.

BUG=595958

Committed: https://crrev.com/177196c218135882ba6438dfd885fd540be00852
Cr-Commit-Position: refs/heads/master@{#383031}
==========

[commit-bot: I haz the power, 2016-03-24 04:33:12]

Patchset 5 (id:??) landed as
https://crrev.com/177196c218135882ba6438dfd885fd540be00852
Cr-Commit-Position: refs/heads/master@{#383031}