Disable color/date chooser in VR mode via WebPreferences

Disable color/date chooser in VR mode via WebPreferences

On Android, the color and date time choosers overlay the main
content and are monocular. This is disruptive in VR. Until these
are supported in VR, they will be suppressed.

It's important to note that this suppression preference can change
multiple times in the lifetime of a renderer, so a runtime
enabled feature isn't appropriate. It also means that walking
the DOM each time the preference changes to disable form elements
is also an unattractive option.

BUG=715600
Review-Url: https://codereview.chromium.org/2885033007
Cr-Commit-Position: refs/heads/master@{#474534}
Committed: https://chromium.googlesource.com/chromium/src/+/8921b24098caf2612b3e30694ce4250781c2cdca

[Ian Vollick, 2017-05-19 02:33:11]

Description was changed from

==========
Disable color picker in VR mode via WebPreferences

On Android color chooser Opens UI elements on top of the main content.
As a result, if it is opened in VR it can break the experience. This
patch makes it possible to disable color chooser when user is in VR.

BUG=715600
==========

to

==========
Disable color picker in VR mode via WebPreferences

On Android color chooser Opens UI elements on top of the main content.
As a result, if it is opened in VR it can break the experience. This
patch makes it possible to disable color chooser when user is in VR.

BUG=715600
==========

[Ian Vollick, 2017-05-19 02:33:11]

vollick@chromium.org changed reviewers:
+ asimjour@google.com, avi@chromium.org, boliu@chromium.org

[Ian Vollick, 2017-05-19 02:33:46]

vollick@chromium.org changed reviewers:
+ asimjour@chromium.org
- asimjour@google.com

[Ian Vollick, 2017-05-19 02:36:36]

vollick@chromium.org changed reviewers:
+ kbr@chromium.org, rbyers@chromium.org

[Ian Vollick, 2017-05-19 02:37:33]

vollick@chromium.org changed reviewers:
+ tsepez@chromium.org

[Ian Vollick, 2017-05-19 02:37:49]

vollick@chromium.org changed reviewers:
- tsepez@chromium.org

[Ian Vollick, 2017-05-19 02:40:01]

vollick@chromium.org changed reviewers:
+ mthiesse@chromium.org

[Ian Vollick, 2017-05-19 02:45:30]

vollick@chromium.org changed reviewers:
- kbr@chromium.org, rbyers@chromium.org

[Ian Vollick, 2017-05-19 02:56:15]

Description was changed from

==========
Disable color picker in VR mode via WebPreferences

On Android color chooser Opens UI elements on top of the main content.
As a result, if it is opened in VR it can break the experience. This
patch makes it possible to disable color chooser when user is in VR.

BUG=715600
==========

to

==========
WIP - Disable color picker in VR mode via WebPreferences

On Android color chooser Opens UI elements on top of the main content.
As a result, if it is opened in VR it can break the experience. This
patch makes it possible to disable color chooser when user is in VR.

BUG=715600
==========

[Ian Vollick, 2017-05-19 13:55:19]

Description was changed from

==========
WIP - Disable color picker in VR mode via WebPreferences

On Android color chooser Opens UI elements on top of the main content.
As a result, if it is opened in VR it can break the experience. This
patch makes it possible to disable color chooser when user is in VR.

BUG=715600
==========

to

==========
WIP - Disable color picker in VR mode via WebPreferences

NB: while this CL has a specific setting/preference for the color
chooser, we may indeed want to coalesce semantically related
UI elements to reduced the number of preferences we'll need. I'm
actively discussing this with web platform folks.

On Android color chooser Opens UI elements on top of the main content.
As a result, if it is opened in VR it can break the experience. This
patch makes it possible to disable color chooser when user is in VR.

BUG=715600
==========

[Ian Vollick, 2017-05-21 20:54:39]

Description was changed from

==========
WIP - Disable color picker in VR mode via WebPreferences

NB: while this CL has a specific setting/preference for the color
chooser, we may indeed want to coalesce semantically related
UI elements to reduced the number of preferences we'll need. I'm
actively discussing this with web platform folks.

On Android color chooser Opens UI elements on top of the main content.
As a result, if it is opened in VR it can break the experience. This
patch makes it possible to disable color chooser when user is in VR.

BUG=715600
==========

to

==========
Disable color/date chooser in VR mode via WebPreferences

On Android, the color and date time choosers overlay the main
content and are monocular. This is disruptive in VR. Until these
are supported in VR, they will be suppressed.

It's important to note that this suppression preference can change
multiple times in the lifetime of a renderer, so a runtime
enabled feature isn't appropriate. It also means that walking
the DOM each time the preference changes to disable form elements
is also an unattractive option.

BUG=715600
==========

[Ian Vollick, 2017-05-21 20:54:40]

vollick@chromium.org changed reviewers:
+ rbyers@chromium.org

[Ian Vollick, 2017-05-23 13:02:38]

Hey all. I've hopefully spoken with all of you about this change already. PTAL
when you get a chance and let me know if it's in line with what you'd expected.

Thanks!

[boliu, 2017-05-23 15:39:20]

lgtm, although I don't think I own anything anymore :p

[Avi (use Gerrit), 2017-05-23 15:50:56]

content lgtm

[mthiesse, 2017-05-23 15:54:26]

vr_shell/ lgtm

[Ian Vollick, 2017-05-23 16:17:57]

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

[commit-bot: I haz the power, 2017-05-23 16:18:42]

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, 2017-05-23 20:43:31]

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

[commit-bot: I haz the power, 2017-05-23 20:43:32]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Rick Byers, 2017-05-23 21:36:36]

Big picture I think having a blink setting to disable things somewhat ad-hoc
like this is fine.

WebKit LGTM with nits on naming/documentation.

/cc tkent@ as the form control owner - he may have additional input.

https://codereview.chromium.org/2885033007/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/Settings.json5 (right):

https://codereview.chromium.org/2885033007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Settings.json5:233: {
nit: please add a comment that describes (as precisely as possible) what exactly
is being enabled/disabled.  I think "Page Popup" is a bit of an overloaded term.
 If you can qualify specifically what properties makes it so that VR needs to
disable this, that would make things more clear.  It's probably also useful to
specifically describe the current use case in the comments ("used by VR mode" or
whatever).

https://codereview.chromium.org/2885033007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Settings.json5:234: name:
"pagePopupsSuppressed",
Nit: Prefer a positive name?
From a quick search of other settings it seems we prefer "fooEnabled" settings
that default to true over "fooSuppressed" that default to false.  Eg.
"supportsMultipleWindows".  Is there a reason this case should be special in
using the inverted "suppressed" semantics?

[tkent, 2017-05-23 23:22:53]

What about file chooser for <input type=file>, <select> popups, <datalist>
popups, form validation bubble, title= tooltips, window.print(),
window.alert()/confirm()/prompt() ?

[Ian Vollick, 2017-05-23 23:40:44]

On 2017/05/23 23:22:53, tkent wrote:
> What about file chooser for <input type=file>, <select> popups, <datalist>
> popups, form validation bubble, title= tooltips, window.print(),
> window.alert()/confirm()/prompt() ?

Ah, I should have mentioned that I planned for follow up CLs to address other
popups. This CL was mainly to check the plumbing and establish the
preference/setting that would be used elsewhere.

Also some of these were easily handled in the chrome/ layer. I'll follow up with
you offline.

[Ian Vollick, 2017-05-24 14:52:17]

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

[commit-bot: I haz the power, 2017-05-24 14:52:33]

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, 2017-05-24 16:13:26]

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

[commit-bot: I haz the power, 2017-05-24 16:13:27]

Dry run: This issue passed the CQ dry run.

[Ian Vollick, 2017-05-24 16:38:46]

On 2017/05/23 21:36:36, Rick Byers wrote:
> Big picture I think having a blink setting to disable things somewhat ad-hoc
> like this is fine.
> 
> WebKit LGTM with nits on naming/documentation.
> 
> /cc tkent@ as the form control owner - he may have additional input.
> 
>
https://codereview.chromium.org/2885033007/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/Settings.json5 (right):
> 
>
https://codereview.chromium.org/2885033007/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/Settings.json5:233: {
> nit: please add a comment that describes (as precisely as possible) what
exactly
> is being enabled/disabled.  I think "Page Popup" is a bit of an overloaded
term.
>  If you can qualify specifically what properties makes it so that VR needs to
> disable this, that would make things more clear.  It's probably also useful to
> specifically describe the current use case in the comments ("used by VR mode"
or
> whatever).
> 
>
https://codereview.chromium.org/2885033007/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/Settings.json5:234: name:
> "pagePopupsSuppressed",
> Nit: Prefer a positive name?
> From a quick search of other settings it seems we prefer "fooEnabled" settings
> that default to true over "fooSuppressed" that default to false.  Eg.
> "supportsMultipleWindows".  Is there a reason this case should be special in
> using the inverted "suppressed" semantics?

I agree with you about the preference naming, but I was having a hard time
coming up with something that was different enough from pagePopupEnabled
that it wouldn't be confusing. The best I could think of was
pagePopupRenderingSupported. If you like that (or if you have another
suggestion), please let me know -- I'd be happy to change this.

I've added a hopefully descriptive comment to the setting in Settings.json5
that should capture my thinking.

[Ian Vollick, 2017-05-24 17:30:10]

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

[commit-bot: I haz the power, 2017-05-24 17:30:57]

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, 2017-05-24 19:32:54]

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

[commit-bot: I haz the power, 2017-05-24 19:32:56]

Dry run: This issue passed the CQ dry run.

[Rick Byers, 2017-05-24 22:57:13]

This seems fine to me for now, still LGTM.  Thanks for the description.  

But it seems like this is a temporary hack to me.  Can you please add a TODO
referencing a bug that tracks removing or clarifying this setting further?

I dug through the code to try to understand what EXACTLY would qualify as a
"page popup" and I don't think that's possible at the blink layer.  In these two
cases it's a specific code path with an embedder-defined implementation.  So the
embedder really defines the properties of the pop-up and is the only one that
knows whether it'll be OK for VR mode or not, right?  And ultimately that's
where you will end up providing a VR-specific implementation.  So perhaps each
case should just have some way to fail (DateTimeChooser appears to already) and
blink should be able to handle the failure gracefully.  

But you're going to have to figure this all out when you work on the solution
for this UI in VR mode, so I think it's OK to have a temporary setting that
disables these for now if that's simpler for you in the short run.

[Ian Vollick, 2017-05-24 23:17:15]

On 2017/05/24 22:57:13, Rick Byers wrote:
> This seems fine to me for now, still LGTM.  Thanks for the description.  
> 
> But it seems like this is a temporary hack to me.  Can you please add a TODO
> referencing a bug that tracks removing or clarifying this setting further?
> 
> I dug through the code to try to understand what EXACTLY would qualify as a
> "page popup" and I don't think that's possible at the blink layer.  In these
two
> cases it's a specific code path with an embedder-defined implementation.  So
the
> embedder really defines the properties of the pop-up and is the only one that
> knows whether it'll be OK for VR mode or not, right? And ultimately that's
> where you will end up providing a VR-specific implementation.  So perhaps each
> case should just have some way to fail (DateTimeChooser appears to already)
and
> blink should be able to handle the failure gracefully.

+1 to all of this. We'll be handling the alternate implementation in the
embedder eventually, but it would indeed be nice if we could gracefully fallback
within blink.  
> 
> But you're going to have to figure this all out when you work on the solution
> for this UI in VR mode, so I think it's OK to have a temporary setting that
> disables these for now if that's simpler for you in the short run.

Sg, thanks.

Also, happy to change this if you'd like changes, tkent@ -- please let me know
if that's the case.

[Ian Vollick, 2017-05-24 23:17:20]

The CQ bit was checked by vollick@chromium.org

[Ian Vollick, 2017-05-24 23:17:21]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
mthiesse@chromium.org, boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2885033007/#ps100001
(title: "added explanatory comment.")

[commit-bot: I haz the power, 2017-05-24 23:18: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, 2017-05-24 23:27:46]

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

[commit-bot: I haz the power, 2017-05-24 23:27:47]

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

[Ian Vollick, 2017-05-25 01:51:36]

vollick@chromium.org changed reviewers:
+ estark@chromium.org

[Ian Vollick, 2017-05-25 01:51:51]

vollick@chromium.org changed reviewers:
+ kenrb@chromium.org
- estark@chromium.org

[Ian Vollick, 2017-05-25 01:52:16]

On 2017/05/24 23:27:47, commit-bot: I haz the power wrote:
> 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...)

+kenrb for content/public/common/common_param_traits_macros.h

[kenrb, 2017-05-25 02:21:58]

lgtm

[Ian Vollick, 2017-05-25 02:40:30]

The CQ bit was checked by vollick@chromium.org

[commit-bot: I haz the power, 2017-05-25 02:41:37]

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, 2017-05-25 02:48:20]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495680030018410,
"parent_rev": "b47d9268ef018f9ed66378c33b317d9a4cb3553e", "commit_rev":
"8921b24098caf2612b3e30694ce4250781c2cdca"}

[commit-bot: I haz the power, 2017-05-25 02:48:35]

Description was changed from

==========
Disable color/date chooser in VR mode via WebPreferences

On Android, the color and date time choosers overlay the main
content and are monocular. This is disruptive in VR. Until these
are supported in VR, they will be suppressed.

It's important to note that this suppression preference can change
multiple times in the lifetime of a renderer, so a runtime
enabled feature isn't appropriate. It also means that walking
the DOM each time the preference changes to disable form elements
is also an unattractive option.

BUG=715600
==========

to

==========
Disable color/date chooser in VR mode via WebPreferences

On Android, the color and date time choosers overlay the main
content and are monocular. This is disruptive in VR. Until these
are supported in VR, they will be suppressed.

It's important to note that this suppression preference can change
multiple times in the lifetime of a renderer, so a runtime
enabled feature isn't appropriate. It also means that walking
the DOM each time the preference changes to disable form elements
is also an unattractive option.

BUG=715600

Review-Url: https://codereview.chromium.org/2885033007
Cr-Commit-Position: refs/heads/master@{#474534}
Committed:
https://chromium.googlesource.com/chromium/src/+/8921b24098caf2612b3e30694ce4...
==========

[commit-bot: I haz the power, 2017-05-25 02:48:39]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/8921b24098caf2612b3e30694ce4...