Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2361193002:
Fix clicks on Public Session login page after OOBE (Closed)
|
Created:
4 years, 3 months ago by Greg Levin Modified:
4 years, 3 months ago CC:
chromium-reviews, alemate+watch_chromium.org, achuith+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix clicks on Public Session login page after OOBE
BUG=647082
TEST=Enroll device with Public Session policy enabled. When Public
Session user pod appears, verify that mouse / touchpad clicks work on
the user pod and shelf buttons.
Committed: https://crrev.com/1031e3dc40e1af209cf973ccaf22ea8b3b409122
Cr-Commit-Position: refs/heads/master@{#420829}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Only load pin keyboard on Lock screen #
Total comments: 7
Patch Set 3 : Remove change to 'get submitButton()' #Messages
Total messages: 15 (4 generated)
glevin@chromium.org changed reviewers: + sammiequon@chromium.org, xiyuan@chromium.org
This CL seems to fix the problem, and doesn't seem to break normal ChromeOS user pods, but I'm not sure it's quite the right fix. Let me know if you have a better idea! I have a few specific questions / comments in the code. sammiequon@ - This is messing with your code from https://codereview.chromium.org/2254623003/ , so I wanted to make sure these changes make sense to you. +jdufault@, in case you're interested, since you reviewed the original CL. Please have a look! https://codereview.chromium.org/2361193002/diff/1/ui/login/account_picker/use... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2361193002/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.js:775: !cr.ui.login.ResourceLoader.alreadyLoadedAssets( I assume this condition makes sense, and that this code block is unneeded if we're not displaying the Submit button...? https://codereview.chromium.org/2361193002/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.js:781: cr.ui.login.ResourceLoader.loadAssetsOnIdle('custom-elements-user-pod'); After OOBE completed and the Public Session user pod came up, this block of code was causing the bug, and particularly the weirdness described in Comment #13: https://bugs.chromium.org/p/chromium/issues/detail?id=647082#c13 What exactly is this code for, anyway? I ran the normal login page with it commented out, and the Submit button arrows appeared and worked normally. https://codereview.chromium.org/2361193002/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.js:900: this.querySelector('.submit-button'); Public Session pods don't have Submit buttons. Is it okay to skip all the "if (this.submitButton)" code in that case?
jdufault@chromium.org changed reviewers: + jdufault@chromium.org
https://codereview.chromium.org/2361193002/diff/1/ui/login/account_picker/use... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2361193002/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.js:775: !cr.ui.login.ResourceLoader.alreadyLoadedAssets( On 2016/09/22 20:31:39, Greg Levin wrote: > I assume this condition makes sense, and that this code block is unneeded if > we're not displaying the Submit button...? Discussed a bit offline. We should probably just add a check to only load the resource if the display type[1] is set to LOCK. 1: https://cs.chromium.org/chromium/src/ui/login/display_manager.js?l=84
https://codereview.chromium.org/2361193002/diff/20001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (left): https://codereview.chromium.org/2361193002/diff/20001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:3249: var hadFocus = !!this.focusedPod_; This variable doesn't appear to be used anywhere. https://codereview.chromium.org/2361193002/diff/20001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2361193002/diff/20001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:776: !cr.ui.login.ResourceLoader.alreadyLoadedAssets( As per offline discussion, switched this to only activating on Lock screen. https://codereview.chromium.org/2361193002/diff/20001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:901: this.querySelector('.submit-button'); Since I'm not using this in the if-condition above, I'm not sure if this check is of any value. It disables a bit of code in a case where it's not apparently used, but I don't know if it's a net improvement.
https://codereview.chromium.org/2361193002/diff/20001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (left): https://codereview.chromium.org/2361193002/diff/20001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:3249: var hadFocus = !!this.focusedPod_; On 2016/09/23 16:12:40, Greg Levin wrote: > This variable doesn't appear to be used anywhere. Let's remove it then. https://codereview.chromium.org/2361193002/diff/20001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2361193002/diff/20001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:901: this.querySelector('.submit-button'); On 2016/09/23 16:12:40, Greg Levin wrote: > Since I'm not using this in the if-condition above, I'm not sure if this check > is of any value. It disables a bit of code in a case where it's not apparently > used, but I don't know if it's a net improvement. I would vote for revert since it is not needed for this CL.
https://codereview.chromium.org/2361193002/diff/20001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (left): https://codereview.chromium.org/2361193002/diff/20001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:3249: var hadFocus = !!this.focusedPod_; On 2016/09/23 16:26:01, xiyuan wrote: > On 2016/09/23 16:12:40, Greg Levin wrote: > > This variable doesn't appear to be used anywhere. > > Let's remove it then. Done. https://codereview.chromium.org/2361193002/diff/20001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2361193002/diff/20001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:901: this.querySelector('.submit-button'); On 2016/09/23 16:26:00, xiyuan wrote: > On 2016/09/23 16:12:40, Greg Levin wrote: > > Since I'm not using this in the if-condition above, I'm not sure if this check > > is of any value. It disables a bit of code in a case where it's not > apparently > > used, but I don't know if it's a net improvement. > > I would vote for revert since it is not needed for this CL. Done.
lgtm
lgtm. Don't think this should affect my changes.
lgtm
The CQ bit was checked by glevin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix clicks on Public Session login page after OOBE BUG=647082 TEST=Enroll device with Public Session policy enabled. When Public Session user pod appears, verify that mouse / touchpad clicks work on the user pod and shelf buttons. ========== to ========== Fix clicks on Public Session login page after OOBE BUG=647082 TEST=Enroll device with Public Session policy enabled. When Public Session user pod appears, verify that mouse / touchpad clicks work on the user pod and shelf buttons. Committed: https://crrev.com/1031e3dc40e1af209cf973ccaf22ea8b3b409122 Cr-Commit-Position: refs/heads/master@{#420829} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/1031e3dc40e1af209cf973ccaf22ea8b3b409122 Cr-Commit-Position: refs/heads/master@{#420829} |
