Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2936173002:
Fix misplacement of signin overlay and critical update message banner (Closed)
|
Created:
3 years, 6 months ago by Wenzhao (Colin) Zang Modified:
3 years, 6 months ago CC:
chromium-reviews, alemate+watch_chromium.org, achuith+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, rkc Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix misplacement of signin overlay and critical update message banner
This CL combines several small changes:
1) Fix crbug.com/732921 by using offsetWidth instead of a fixed width
value when positioning.
2) Fix crbug.com/732683 by combining .user-image and .pod classes. The
oobe container has another class also named as .user-image so they
interfere with each other.
3) CSS style changes, including ones for easy unlock icon
(updating assets is done by CL 2934193002) and removing the sign-in
badge per the new spec.
4) Correct tab order for public session pods, also make sure that the
focus correctly goes to other small pods after leaving the public pod,
by changing display:none to opacity:0.
5) Update border style for reset screen (capturing CL 2879593005)
BUG=732921, 732683
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2936173002
Cr-Commit-Position: refs/heads/master@{#479921}
Committed: https://chromium.googlesource.com/chromium/src/+/27848b1c4ddb114bf2462ba8c6d46d9b49f489b9
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address comments #
Total comments: 7
Patch Set 3 : Address comments and remove an out-dated comment #Patch Set 4 : Add inline comments and rebase with master, no other changes #
Messages
Total messages: 33 (24 generated)
Description was changed from ========== Fix misplacement of signin overlay and critical update message banner BUG=732921, 732683 ========== to ========== Fix misplacement of signin overlay and critical update message banner BUG=732921, 732683 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Fix misplacement of signin overlay and critical update message banner BUG=732921, 732683 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix misplacement of signin overlay and critical update message banner This CL combines several small changes: 1) Fix 732683 BUG=732921, 732683 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Fix misplacement of signin overlay and critical update message banner This CL combines several small changes: 1) Fix 732683 BUG=732921, 732683 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix misplacement of signin overlay and critical update message banner This CL combines several small changes: 1) Fix issue 732921 by using offsetWidth instead of a fixed width value when positioning. BUG=732921, 732683 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Fix misplacement of signin overlay and critical update message banner This CL combines several small changes: 1) Fix issue 732921 by using offsetWidth instead of a fixed width value when positioning. BUG=732921, 732683 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix misplacement of signin overlay and critical update message banner This CL combines several small changes: 1) Fix crbug.com/732921 by using offsetWidth instead of a fixed width value when positioning. 2) Fix crbug.com/732683 by combining .user-image and .pod classes. The oobe container has another class also named as .user-image so it interferes with each other. 3) CSS style changes, including ones for easy unlock icon (updating assets is done by CL 2934193002) and removing the sign-in banner per the new spec. 4) Correct tab order for public session pods, also make sure that the focus correctly goes to other small pods after leaving the public pod, by changing display:none to opacity:0. 5) Update border style for reset screen (capturing CL 2879593005) BUG=732921, 732683 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Fix misplacement of signin overlay and critical update message banner This CL combines several small changes: 1) Fix crbug.com/732921 by using offsetWidth instead of a fixed width value when positioning. 2) Fix crbug.com/732683 by combining .user-image and .pod classes. The oobe container has another class also named as .user-image so it interferes with each other. 3) CSS style changes, including ones for easy unlock icon (updating assets is done by CL 2934193002) and removing the sign-in banner per the new spec. 4) Correct tab order for public session pods, also make sure that the focus correctly goes to other small pods after leaving the public pod, by changing display:none to opacity:0. 5) Update border style for reset screen (capturing CL 2879593005) BUG=732921, 732683 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix misplacement of signin overlay and critical update message banner This CL combines several small changes: 1) Fix crbug.com/732921 by using offsetWidth instead of a fixed width value when positioning. 2) Fix crbug.com/732683 by combining .user-image and .pod classes. The oobe container has another class also named as .user-image so it interferes with each other. 3) CSS style changes, including ones for easy unlock icon (updating assets is done by CL 2934193002) and removing the sign-in banner per the new spec. 4) Correct tab order for public session pods, also make sure that the focus correctly goes to other small pods after leaving the public pod, by changing display:none to opacity:0. 5) Update border style for reset screen (capturing CL 2879593005) BUG=732921, 732683 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Fix misplacement of signin overlay and critical update message banner This CL combines several small changes: 1) Fix crbug.com/732921 by using offsetWidth instead of a fixed width value when positioning. 2) Fix crbug.com/732683 by combining .user-image and .pod classes. The oobe container has another class also named as .user-image so it interferes with each other. 3) CSS style changes, including ones for easy unlock icon (updating assets is done by CL 2934193002) and removing the sign-in banner per the new spec. 4) Correct tab order for public session pods, also make sure that the focus correctly goes to other small pods after leaving the public pod, by changing display:none to opacity:0. 5) Update border style for reset screen (capturing CL 2879593005) BUG=732921, 732683 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix misplacement of signin overlay and critical update message banner This CL combines several small changes: 1) Fix crbug.com/732921 by using offsetWidth instead of a fixed width value when positioning. 2) Fix crbug.com/732683 by combining .user-image and .pod classes. The oobe container has another class also named as .user-image so they interfere with each other. 3) CSS style changes, including ones for easy unlock icon (updating assets is done by CL 2934193002) and removing the sign-in banner per the new spec. 4) Correct tab order for public session pods, also make sure that the focus correctly goes to other small pods after leaving the public pod, by changing display:none to opacity:0. 5) Update border style for reset screen (capturing CL 2879593005) BUG=732921, 732683 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Fix misplacement of signin overlay and critical update message banner This CL combines several small changes: 1) Fix crbug.com/732921 by using offsetWidth instead of a fixed width value when positioning. 2) Fix crbug.com/732683 by combining .user-image and .pod classes. The oobe container has another class also named as .user-image so they interfere with each other. 3) CSS style changes, including ones for easy unlock icon (updating assets is done by CL 2934193002) and removing the sign-in banner per the new spec. 4) Correct tab order for public session pods, also make sure that the focus correctly goes to other small pods after leaving the public pod, by changing display:none to opacity:0. 5) Update border style for reset screen (capturing CL 2879593005) BUG=732921, 732683 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix misplacement of signin overlay and critical update message banner This CL combines several small changes: 1) Fix crbug.com/732921 by using offsetWidth instead of a fixed width value when positioning. 2) Fix crbug.com/732683 by combining .user-image and .pod classes. The oobe container has another class also named as .user-image so they interfere with each other. 3) CSS style changes, including ones for easy unlock icon (updating assets is done by CL 2934193002) and removing the sign-in badge per the new spec. 4) Correct tab order for public session pods, also make sure that the focus correctly goes to other small pods after leaving the public pod, by changing display:none to opacity:0. 5) Update border style for reset screen (capturing CL 2879593005) BUG=732921, 732683 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
wzang@chromium.org changed reviewers: + jdufault@chromium.org, xiyuan@chromium.org
https://codereview.chromium.org/2936173002/diff/1/ui/login/account_picker/md_... File ui/login/account_picker/md_user_pod_row.js (right): https://codereview.chromium.org/2936173002/diff/1/ui/login/account_picker/md_... ui/login/account_picker/md_user_pod_row.js:676: this.getParentPod_().getPodStyle() != UserPod.Style.LARGE) Can parentPod can be null? based on isParentPodFocused_ https://codereview.chromium.org/2936173002/diff/1/ui/login/account_picker/md_... ui/login/account_picker/md_user_pod_row.js:679: if (!this.isParentPodFocused_()) nit: || !this.isParentPodFocused_() https://codereview.chromium.org/2936173002/diff/1/ui/login/md_screen_containe... File ui/login/md_screen_container.css (right): https://codereview.chromium.org/2936173002/diff/1/ui/login/md_screen_containe... ui/login/md_screen_container.css:95: #oobe:not([md-mode]).reset #inner-container, This file is md-only, can this be removed entirely?
https://codereview.chromium.org/2936173002/diff/1/ui/login/account_picker/md_... File ui/login/account_picker/md_user_pod_row.js (right): https://codereview.chromium.org/2936173002/diff/1/ui/login/account_picker/md_... ui/login/account_picker/md_user_pod_row.js:676: this.getParentPod_().getPodStyle() != UserPod.Style.LARGE) On 2017/06/15 17:52:09, jdufault wrote: > Can parentPod can be null? based on isParentPodFocused_ Done. https://codereview.chromium.org/2936173002/diff/1/ui/login/account_picker/md_... ui/login/account_picker/md_user_pod_row.js:679: if (!this.isParentPodFocused_()) On 2017/06/15 17:52:09, jdufault wrote: > nit: || !this.isParentPodFocused_() Done. https://codereview.chromium.org/2936173002/diff/1/ui/login/md_screen_containe... File ui/login/md_screen_container.css (right): https://codereview.chromium.org/2936173002/diff/1/ui/login/md_screen_containe... ui/login/md_screen_container.css:95: #oobe:not([md-mode]).reset #inner-container, On 2017/06/15 17:52:09, jdufault wrote: > This file is md-only, can this be removed entirely? Yes, all 'not([md-mode])' are removed.
https://codereview.chromium.org/2936173002/diff/20001/ui/login/account_picker... File ui/login/account_picker/md_user_pod_row.css (right): https://codereview.chromium.org/2936173002/diff/20001/ui/login/account_picker... ui/login/account_picker/md_user_pod_row.css:777: opacity: 0; why not display: none? https://codereview.chromium.org/2936173002/diff/20001/ui/login/account_picker... ui/login/account_picker/md_user_pod_row.css:782: opacity: 1; Is this needed? https://codereview.chromium.org/2936173002/diff/20001/ui/login/account_picker... File ui/login/account_picker/md_user_pod_row.js (right): https://codereview.chromium.org/2936173002/diff/20001/ui/login/account_picker... ui/login/account_picker/md_user_pod_row.js:677: !this.isParentPodFocused_()) multi-line if means that the body needs braces ({})
The CQ bit was checked by wzang@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: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by wzang@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.
Comments addressed. Thanks. https://codereview.chromium.org/2936173002/diff/20001/ui/login/account_picker... File ui/login/account_picker/md_user_pod_row.css (right): https://codereview.chromium.org/2936173002/diff/20001/ui/login/account_picker... ui/login/account_picker/md_user_pod_row.css:777: opacity: 0; On 2017/06/15 19:04:09, jdufault wrote: > why not display: none? After the focus leaves the expanded public session pod it should go to other small pods. If other small pods have display: none or visibility: hidden their tab indexes are ignored and the focus mistakenly goes to the header bar. We just want the public session pod centered with all other elements hidden, opacity: 0 doesn't have other side effects such as blocking things. https://codereview.chromium.org/2936173002/diff/20001/ui/login/account_picker... ui/login/account_picker/md_user_pod_row.css:782: opacity: 1; On 2017/06/15 19:04:09, jdufault wrote: > Is this needed? It's combined with the previous one. https://codereview.chromium.org/2936173002/diff/20001/ui/login/account_picker... File ui/login/account_picker/md_user_pod_row.js (right): https://codereview.chromium.org/2936173002/diff/20001/ui/login/account_picker... ui/login/account_picker/md_user_pod_row.js:677: !this.isParentPodFocused_()) On 2017/06/15 19:04:09, jdufault wrote: > multi-line if means that the body needs braces ({}) Done. Sorry I missed that in the style guide. FYI the try bots all succeed for dry run.
lgtm https://codereview.chromium.org/2936173002/diff/20001/ui/login/account_picker... File ui/login/account_picker/md_user_pod_row.css (right): https://codereview.chromium.org/2936173002/diff/20001/ui/login/account_picker... ui/login/account_picker/md_user_pod_row.css:777: opacity: 0; On 2017/06/15 21:40:47, Wenzhao (Colin) Zang wrote: > On 2017/06/15 19:04:09, jdufault wrote: > > why not display: none? > > After the focus leaves the expanded public session pod it should go to other > small pods. If other small pods have display: none or visibility: hidden their > tab indexes are ignored and the focus mistakenly goes to the header bar. > > We just want the public session pod centered with all other elements hidden, > opacity: 0 doesn't have other side effects such as blocking things. This should be added as a comment :)
lgtm
The CQ bit was checked by wzang@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.
The CQ bit was checked by wzang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, jdufault@chromium.org Link to the patchset: https://codereview.chromium.org/2936173002/#ps60001 (title: "Add inline comments and rebase with master, no other changes")
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": 60001, "attempt_start_ts": 1497570651244950,
"parent_rev": "a022a07a4b44fef77540b75b371230326eed1664", "commit_rev":
"27848b1c4ddb114bf2462ba8c6d46d9b49f489b9"}
Message was sent while issue was closed.
Description was changed from ========== Fix misplacement of signin overlay and critical update message banner This CL combines several small changes: 1) Fix crbug.com/732921 by using offsetWidth instead of a fixed width value when positioning. 2) Fix crbug.com/732683 by combining .user-image and .pod classes. The oobe container has another class also named as .user-image so they interfere with each other. 3) CSS style changes, including ones for easy unlock icon (updating assets is done by CL 2934193002) and removing the sign-in badge per the new spec. 4) Correct tab order for public session pods, also make sure that the focus correctly goes to other small pods after leaving the public pod, by changing display:none to opacity:0. 5) Update border style for reset screen (capturing CL 2879593005) BUG=732921, 732683 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix misplacement of signin overlay and critical update message banner This CL combines several small changes: 1) Fix crbug.com/732921 by using offsetWidth instead of a fixed width value when positioning. 2) Fix crbug.com/732683 by combining .user-image and .pod classes. The oobe container has another class also named as .user-image so they interfere with each other. 3) CSS style changes, including ones for easy unlock icon (updating assets is done by CL 2934193002) and removing the sign-in badge per the new spec. 4) Correct tab order for public session pods, also make sure that the focus correctly goes to other small pods after leaving the public pod, by changing display:none to opacity:0. 5) Update border style for reset screen (capturing CL 2879593005) BUG=732921, 732683 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2936173002 Cr-Commit-Position: refs/heads/master@{#479921} Committed: https://chromium.googlesource.com/chromium/src/+/27848b1c4ddb114bf2462ba8c6d4... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/27848b1c4ddb114bf2462ba8c6d4... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
