Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2958533003:
Disable Credential management API dialog in VR mode (Closed)
DescriptionDisable Credential management API dialog in VR mode
2D popups can cause discomfort while in VR. This change disables the 2D
Credential management UI while in VR mode.
BUG=735769
Review-Url: https://codereview.chromium.org/2958533003
Cr-Commit-Position: refs/heads/master@{#484053}
Committed: https://chromium.googlesource.com/chromium/src/+/871c5d1221b7ae6a3a6be3a7c7880c4190bafeea
Patch Set 1 #
Total comments: 2
Patch Set 2 : cr feedback #
Total comments: 4
Patch Set 3 : cr feedback #
Total comments: 12
Patch Set 4 : cr feedback #Patch Set 5 : removing an unnecessary whitespace change #Messages
Total messages: 29 (9 generated)
billorr@chromium.org changed reviewers: + kolos@chromium.org
Kolos@chromium.org, PTAL
vabr@chromium.org changed reviewers: + vabr@chromium.org, vasilii@chromium.org
Hi Bill, Thanks for looking into the issue with VR. However, before this change is approved, I think we might need to have more context. First of all, what is exactly "VR", how to try it out, etc. ? Second, who is behind the decision to not show the CM API dialogue in VR? Why is it the best user experience? Would the user be surprised not to see this? What about other password manager-related UI, such as the save password infobar? Third, is this also a potential issue of other (non-password) Chrome UI? Should it be treated inside the general UI code rather than #ifdefing parts inside UI-calling code? As a code OWNER of the password manager code, I'm not thrilled to add yet another chunk of #ifdefed code without a strong justification. I also added vasilii@ as a reviewer. Vasilii is our CM API and UI code expert, and might have further comments to make on this. Cheers, Vaclav
You suppress the autosign-in first run and the account chooser. On the other hand you don't suppress the save password bubble and the autosign-in toast. I'm curious how would you like the API and generally password manager to work in VR (virtual reality). Currently the user won't be able to sign-in which is controversial. https://codereview.chromium.org/2958533003/diff/1/chrome/browser/password_man... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/2958533003/diff/1/chrome/browser/password_man... chrome/browser/password_manager/chrome_password_manager_client.cc:89: #include "device/vr/features/features.h" unused
> > First of all, what is exactly "VR", how to try it out, etc. ? VR consists of 2 scenarios for users. First, there is a WebVR API exposed to javascript, where sites can use WebGL to render virtual-reality experiences in a VR headset. The API is currently behind an origin trial, but it is usable on Android with cardboard or daydream headset. Second, there is a "VR Browser" experience, which creates a virtual monitor floating in front of the user, which shows web contents. The implementation for this is in progress, and it is enabled with a feature flag, and requires a daydream headset on Android. > Second, who is behind the decision to not show the CM API dialogue in VR? > Why is > it the best user experience? Would the user be surprised not to see this? > What > about other password manager-related UI, such as the save password infobar? There are many UI popups that Chrome for Android shows in distributed locations in the code. These currently show up as 2D UI on top of the VR rendering, which means that one eye sees part of the UI, and the other eye sees a different part of the popup, causing discomfort and eye strain. The immediate goal is to disable these popups while in VR. We also have a longer-term goal of re-enabling these popups but handling them correctly for VR (ie - they show up in the 3d VR scene). The short-term workaround is that users can exit VR and interact with the browser in 2D to handle broken behavior. +*ASimjour@* and *Vollick@* for general discussion on disabling popup UI in VR. There are some docs we could share. I believe this approach was discussed with clank team owners, but may not have been publicized to all owners of code where UI could be shown. > Third, is this also a potential issue of other (non-password) Chrome UI? > Should > it be treated inside the general UI code rather than #ifdefing parts inside > UI-calling code? As a code OWNER of the password manager code, I'm not > thrilled > to add yet another chunk of #ifdefed code without a strong justification. Yes, this is a problem with other parts of Chrome. Unfortunately dialogs and other popup UI don't go through a central location to hook - there are many distributed places that each require different behavior. I was attempting to find a location close to where the UI was actually shown, approximately where we might need to branch to VR-specific UI if/when we implement that, and in a location where we won't break assumptions (ie - Theoretically consider some part of the code that may show a dialog then expects some callback when the dialog dismisses. If we don't show the dialog we'd wait forever for the callback.). On Mon, Jun 26, 2017 at 3:38 AM, <vasilii@chromium.org> wrote: > You suppress the autosign-in first run and the account chooser. On the > other > hand you don't suppress the save password bubble and the autosign-in > toast. I'm > curious how would you like the API and generally password manager to work > in VR > (virtual reality). Currently the user won't be able to sign-in which is > controversial. > From my testing, I didn't see any other popups that become visible while interacting with passwords while in VR. This could mean I missed things while testing, or that they are already handled in lower-layers. In particular, there were other popups I saw in 2d but didn't see while in VR. *asimjour@*, do we disable toasts in general in VR already, or other bubbles like the save password bubble? Longer term, we do want to show the pop-ups, but the immediate goal is identifying 2D UI that will show on top of VR, and disabling it. > https://codereview.chromium.org/2958533003/diff/1/chrome/ > browser/password_manager/chrome_password_manager_client.cc > File chrome/browser/password_manager/chrome_password_manager_client.cc > (right): > > https://codereview.chromium.org/2958533003/diff/1/chrome/ > browser/password_manager/chrome_password_manager_client.cc#newcode89 > chrome/browser/password_manager/chrome_password_manager_client.cc:89: > #include "device/vr/features/features.h" > unused > > https://codereview.chromium.org/2958533003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thanks, Bill, for the helpful answers. Now that I understand your motivation and plan, this looks much better to me. If this has been approved by Chrome-on-Android owners and happens across all features, I am fine with this as a temporary solution, but I would appreciate ideally a tracking bug for the effort to re-enable UI in VR mode. In the end, if password manager won't work, my team will be the first to receive the bugreports, and I would like a clear understanding about who is responsible for fixing it and when. On a more technical level: (1) To get rid of the annoying #ifdefs (which I suppose are also present in other code locations than just in this CL), would it be possible to have a helper function somewhere, which is always defined, and internally uses the #ifdefs to switch between a no-op and checking vr_shell::VrTabHelper::IsInVr? This could limit the complexity at the callsites. (2) Do you want to add a test for this? Or is this tested manually? (3) Just to answer your observation, that you did not see any other password-related prompts: the most common one is the "Save password?" infobar (you can trigger it by submitting a password form at https://rsolomakhin.github.io/autofill/). It is possible that disabling infobars in general is already done for VR? Finally, I would like for Vasilii to have a chance to respond to your answer, so let's wait until he has a chance to do so with approving this CL. Cheers, Vaclav
> > (1) To get rid of the annoying #ifdefs (which I suppose are also present in > other code locations than just in this CL), would it be possible to have a > helper function somewhere, which is always defined, and internally uses the > #ifdefs to switch between a no-op and checking > vr_shell::VrTabHelper::IsInVr? > This could limit the complexity at the callsites. We could move vr_shell::VrTabHelper::IsInVR to be always defined, or make a wrapper that is always defined. Currently it is only built on Android. I'll discuss with asimjour@ and vollick@ to see where this should live. (2) Do you want to add a test for this? Or is this tested manually? It is manually tested for now, but you bring up a good point. I'll think about whether there is a reasonable way to automate this (ie, navigate to a page that would show a popup, enter vr, trigger UI interactions that would show the popup, and validate that it isn't shown). Everything outside of validating that the popup isn't shown can be easily done by our VR testing framework, but I'm not sure of how to validate in automation that a popup isn't shown, short of looking at pixels, which could be flaky. Given the simplicity and that it is temporary, I think I'm OK with manual testing, but I'll try to find a good way to detect whether a popup is visible. (3) Just to answer your observation, that you did not see any other > password-related prompts: the most common one is the "Save password?" > infobar > (you can trigger it by submitting a password form at > https://rsolomakhin.github.io/autofill/). It is possible that disabling > infobars > in general is already done for VR? This appears to work automatically (no custom code). It looks like infobars are added to the android view hierarchy, rather than popped up on top of the current view. For VR, the daydream APIs hide the current view hierarchy to do custom rendering, but popup UI still shows on top of the view. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I want to restate my point above. It's not clear to me how the feature (Credential Manager API and password manager) should work in VR. Currently you suppress the UI which essentially means that the product is broken. E.g. you suppress the autosign-in toast but the auto sign-in is still happening internally. It's a privacy issue. How about imitating the Incognito mode or similar so that the feature is properly suppressed? We have relevant hooks in the same file like OnCredentialManagerUsed() or IsFillingEnabledForCurrentPage()
On 2017/06/27 09:06:47, vasilii wrote: > I want to restate my point above. It's not clear to me how the feature > (Credential Manager API and password manager) should work in VR. Currently you > suppress the UI which essentially means that the product is broken. E.g. you > suppress the autosign-in toast but the auto sign-in is still happening > internally. It's a privacy issue. > How about imitating the Incognito mode or similar so that the feature is > properly suppressed? > We have relevant hooks in the same file like OnCredentialManagerUsed() or > IsFillingEnabledForCurrentPage() That sounds good - I can send out an updated CR to use these hooks. Ideally the user could exit VR (staying on the same page) and then use the credential manager. If this required a reload of the page, that would be acceptable. I'll test that this scenario still works.
On 2017/06/27 17:32:23, billorr wrote: > On 2017/06/27 09:06:47, vasilii wrote: > > I want to restate my point above. It's not clear to me how the feature > > (Credential Manager API and password manager) should work in VR. Currently you > > suppress the UI which essentially means that the product is broken. E.g. you > > suppress the autosign-in toast but the auto sign-in is still happening > > internally. It's a privacy issue. > > How about imitating the Incognito mode or similar so that the feature is > > properly suppressed? > > We have relevant hooks in the same file like OnCredentialManagerUsed() or > > IsFillingEnabledForCurrentPage() > > That sounds good - I can send out an updated CR to use these hooks. Ideally the > user could exit VR (staying on the same page) and then use the credential > manager. If this required a reload of the page, that would be acceptable. I'll > test that this scenario still works. Thanks, Bill. To avoid a misunderstanding, I understand that you will first check the above, and then ping us for a new review. If you instead wait for our review, please explicitly say so. Thanks! Vaclav
I've made the suggested changes to disable the credential manager while in VR. I added a few CR feedback comments to myself that I had missed. I'll upload a new iteration shortly to address these, but they are nit issues. https://codereview.chromium.org/2958533003/diff/1/chrome/browser/password_man... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/2958533003/diff/1/chrome/browser/password_man... chrome/browser/password_manager/chrome_password_manager_client.cc:89: #include "device/vr/features/features.h" It is used for the definition of ENABLE_VR. https://codereview.chromium.org/2958533003/diff/20001/chrome/browser/password... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/2958533003/diff/20001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.cc:226: } moving above the logging... https://codereview.chromium.org/2958533003/diff/20001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.cc:305: // Do not show popup dialogs while in VR nit: period.
This LGTM once you address the comments below, but please do wait for Vasilii's approval before landing this. Thanks! Vaclav https://codereview.chromium.org/2958533003/diff/40001/chrome/browser/password... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/2958533003/diff/40001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.cc:217: if (IsInVr()) { Given that this is the only necessary use of IsInVr (see below), you can as well scrap the method and inline it here. https://codereview.chromium.org/2958533003/diff/40001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.cc:265: return !IsInVr(); No need to check this again, the IsPasswordManagementEnabledForCurrentPage check returning false overrides the result of OnCredentialManagerUsed in all cases. https://codereview.chromium.org/2958533003/diff/40001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.cc:306: if (IsInVr()) This is also covered by IsPasswordManagementEnabledForCurrentPage (which causes CredentialManagerImpl::Get to exit early and never create the request which would call this). https://codereview.chromium.org/2958533003/diff/40001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.cc:650: IsIncognito() || IsInVr()) This is also unnecessary. The two codepaths leading to here are blocked already, one in PromptUserToChooseCredentials, and the other through PasswordManager::ProvisionallySavePassword never remembering a password which can then later not be offered to the user for saving and trigger the autosignin prompt as a side-effect. https://codereview.chromium.org/2958533003/diff/40001/chrome/browser/password... File chrome/browser/password_manager/chrome_password_manager_client.h (right): https://codereview.chromium.org/2958533003/diff/40001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.h:183: bool IsInVr() const; nit: Please comment on the purpose of this, bearing in mind that not everybody knows what "Vr" means in this context, and why it is important (-> to limit the use of conventional UI elements, which might harm user experience in VR).
I agree with what Vaclav posted. https://codereview.chromium.org/2958533003/diff/40001/chrome/browser/password... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/2958533003/diff/40001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.cc:91: #include "device/vr/features/features.h" unused?
https://codereview.chromium.org/2958533003/diff/40001/chrome/browser/password... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/2958533003/diff/40001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.cc:91: #include "device/vr/features/features.h" On 2017/06/30 11:52:15, vasilii wrote: > unused? It's used for ENABLE_VR. Theoretically, this should be part of the unconditional #includes block (i.e., on line 66). To me it makes more sense here, though. Unless this makes a presubmit yell at you, Bill, I prefer your current solution.
https://codereview.chromium.org/2958533003/diff/20001/chrome/browser/password... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/2958533003/diff/20001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.cc:226: } On 2017/06/29 22:26:02, billorr wrote: > moving above the logging... Done. https://codereview.chromium.org/2958533003/diff/20001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.cc:305: // Do not show popup dialogs while in VR On 2017/06/29 22:26:02, billorr wrote: > nit: period. Done. https://codereview.chromium.org/2958533003/diff/40001/chrome/browser/password... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/2958533003/diff/40001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.cc:217: if (IsInVr()) { On 2017/06/30 10:06:07, vabr (Chromium) wrote: > Given that this is the only necessary use of IsInVr (see below), you can as well > scrap the method and inline it here. Done. https://codereview.chromium.org/2958533003/diff/40001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.cc:265: return !IsInVr(); On 2017/06/30 10:06:07, vabr (Chromium) wrote: > No need to check this again, the IsPasswordManagementEnabledForCurrentPage check > returning false overrides the result of OnCredentialManagerUsed in all cases. Done. https://codereview.chromium.org/2958533003/diff/40001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.cc:306: if (IsInVr()) On 2017/06/30 10:06:07, vabr (Chromium) wrote: > This is also covered by IsPasswordManagementEnabledForCurrentPage (which causes > CredentialManagerImpl::Get to exit early and never create the request which > would call this). Done. https://codereview.chromium.org/2958533003/diff/40001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.cc:650: IsIncognito() || IsInVr()) On 2017/06/30 10:06:07, vabr (Chromium) wrote: > This is also unnecessary. The two codepaths leading to here are blocked already, > one in PromptUserToChooseCredentials, and the other through > PasswordManager::ProvisionallySavePassword never remembering a password which > can then later not be offered to the user for saving and trigger the autosignin > prompt as a side-effect. Done. https://codereview.chromium.org/2958533003/diff/40001/chrome/browser/password... File chrome/browser/password_manager/chrome_password_manager_client.h (right): https://codereview.chromium.org/2958533003/diff/40001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.h:183: bool IsInVr() const; On 2017/06/30 10:06:07, vabr (Chromium) wrote: > nit: Please comment on the purpose of this, bearing in mind that not everybody > knows what "Vr" means in this context, and why it is important (-> to limit the > use of conventional UI elements, which might harm user experience in VR). Done.
The CQ bit was checked by billorr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
vasilii@, did you have any further comments, or does this look good to you? Thanks both vasilii@ and vabr@ for the detailed comments so far.
Hi Bill, Thanks for addressing the comments. I'm just confirming that my earlier LGTM still holds and that I have no further comments. Vasilii, over to you. Cheers, Vaclav
lgtm
The CQ bit was checked by billorr@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1499141838717860,
"parent_rev": "e4a97e5fce489ac13e86552e292442e0099c2740", "commit_rev":
"871c5d1221b7ae6a3a6be3a7c7880c4190bafeea"}
Message was sent while issue was closed.
Description was changed from ========== Disable Credential management API dialog in VR mode 2D popups can cause discomfort while in VR. This change disables the 2D Credential management UI while in VR mode. BUG=735769 ========== to ========== Disable Credential management API dialog in VR mode 2D popups can cause discomfort while in VR. This change disables the 2D Credential management UI while in VR mode. BUG=735769 Review-Url: https://codereview.chromium.org/2958533003 Cr-Commit-Position: refs/heads/master@{#484053} Committed: https://chromium.googlesource.com/chromium/src/+/871c5d1221b7ae6a3a6be3a7c788... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/871c5d1221b7ae6a3a6be3a7c788... |
