Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1642323004:
User Manager MD User Pods (Closed)
|
Created:
4 years, 10 months ago by Moe Modified:
4 years, 8 months ago CC:
chromium-reviews, achuith+watch_chromium.org, dzhioev+watch_chromium.org, pam+watch_chromium.org, arv+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUser Manager MD User Pods
BUG=563722
based on=https://codereview.chromium.org/1630903002/
Committed: https://crrev.com/ec4fcd8d8cc2b5935cbc1e79a691251feaa53dae
Cr-Commit-Position: refs/heads/master@{#388503}
Patch Set 1 : User Manager MD User Pods #
Total comments: 4
Patch Set 2 : addressed Roger's comments, css cleanup, rtl css tweak #
Total comments: 9
Patch Set 3 : addressed pasha's comments #
Total comments: 43
Patch Set 4 : addressed tommy's comments #Patch Set 5 : rebase, css overview, bug fix #
Total comments: 6
Patch Set 6 : Addressed comments, fixed google colors #
Total comments: 14
Patch Set 7 : Addressed Dan's comments #
Total comments: 14
Patch Set 8 : Addressed Dan's comments #2 #Patch Set 9 : Addressed Dan's comments and fix for remove button #
Total comments: 9
Patch Set 10 : Addressed comments. fixed a couple of issues with the existing user manager (overflowing user pod r… #
Total comments: 2
Patch Set 11 : Addressed comments #
Total comments: 6
Patch Set 12 : Addressed comment, rebase, and fix for browser test #Messages
Total messages: 59 (16 generated)
Patchset #1 (id:1) has been deleted
mahmadi@chromium.org changed reviewers: + anthonyvd@chromium.org, dbeam@chromium.org, dzhioev@chromium.org, rogerta@chromium.org
This CL updates desktop user manager user pods to MD design. Please review my changes. @dzhioev, I've touched some of the files used by cros. The only visible cros change is a changing a regular button to a paper-button on the remove user dropdown.
On 2016/01/29 23:43:58, Moe wrote: > This CL updates desktop user manager user pods to MD design. Please review my > changes. > > @dzhioev, I've touched some of the files used by cros. The only visible cros > change is a changing a regular button to a paper-button on the remove user > dropdown. There is no chrome/browser/resources/md_user_manager directory in chromium master. Is this CL based on some other CL?
On 2016/01/30 02:52:01, dzhioev wrote: > On 2016/01/29 23:43:58, Moe wrote: > > This CL updates desktop user manager user pods to MD design. Please review my > > changes. > > > > @dzhioev, I've touched some of the files used by cros. The only visible cros > > change is a changing a regular button to a paper-button on the remove user > > dropdown. > > There is no chrome/browser/resources/md_user_manager directory in chromium > master. > Is this CL based on some other CL? My bad Pavel, yes it is based on https://codereview.chromium.org/1630903002/
Description was changed from ========== User Manager MD User Pods ========== to ========== User Manager MD User Pods BUG=563722 based on=https://codereview.chromium.org/1630903002/ ==========
mahmadi@chromium.org changed reviewers: + tommycli@chromium.org - anthonyvd@chromium.org
updated the issue...
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
mahmadi@chromium.org changed reviewers: - dbeam@chromium.org
rebased according to https://codereview.chromium.org/1630903002/
Patchset #1 (id:60001) has been deleted
lgtm, thanks for including screenshots in bug report. https://codereview.chromium.org/1642323004/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_user_manager/user_manager.js (right): https://codereview.chromium.org/1642323004/diff/80001/chrome/browser/resource... chrome/browser/resources/md_user_manager/user_manager.js:42: $('animatedPages').offsetHeight - 57; Can you add a comment explain why 57? https://codereview.chromium.org/1642323004/diff/80001/ui/login/account_picker... File ui/login/account_picker/screen_account_picker.js (right): https://codereview.chromium.org/1642323004/diff/80001/ui/login/account_picker... ui/login/account_picker/screen_account_picker.js:206: // On Desktop, |login-header-bar| has a shadow if there are 8+ profiles. Is this code used by cros too? If not, maybe don't need the "On Desktop" part of the comment. Otherwise, is there a check missing for cros?
https://codereview.chromium.org/1642323004/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_user_manager/user_manager.js (right): https://codereview.chromium.org/1642323004/diff/80001/chrome/browser/resource... chrome/browser/resources/md_user_manager/user_manager.js:42: $('animatedPages').offsetHeight - 57; On 2016/02/10 15:58:29, Roger Tawa wrote: > Can you add a comment explain why 57? Done. https://codereview.chromium.org/1642323004/diff/80001/ui/login/account_picker... File ui/login/account_picker/screen_account_picker.js (right): https://codereview.chromium.org/1642323004/diff/80001/ui/login/account_picker... ui/login/account_picker/screen_account_picker.js:206: // On Desktop, |login-header-bar| has a shadow if there are 8+ profiles. On 2016/02/10 15:58:30, Roger Tawa wrote: > Is this code used by cros too? If not, maybe don't need the "On Desktop" part > of the comment. Otherwise, is there a check missing for cros? This is also called from chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc the ".shadow" is really only used in desktop user manager. but I added a check to be safe.
https://codereview.chromium.org/1642323004/diff/100001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1642323004/diff/100001/chrome/app/generated_r... chrome/app/generated_resources.grd:12922: + Password Not too crucial, but was this change approved by UX guys? https://codereview.chromium.org/1642323004/diff/100001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/1642323004/diff/100001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1545: * @param {number|string=} count The number or string to replace $1 in Add a comment about |is_synced_user|. https://codereview.chromium.org/1642323004/diff/100001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1556: element.textContent = ''; Wow, I haven't seen this function before. I think it's weird. Can we make it a little bit simpler? Something like: message = message.replace('$1', '<span class="total-count">' + count + '</span>'); message = message.replace('$2', '<span class="email">' + this.user.emailAddress + '</span>'); element.innerHTML = message; Also I beleive |is_synced_user| param is excessive, because a) it equals to |!!this.user.emailAddress| b) the |message| doesn't contain '$2' when |is_synced_user| is false. https://codereview.chromium.org/1642323004/diff/100001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1579: substituteElement_: function(placeholder, elementToAdd) { This seems unused.
https://codereview.chromium.org/1642323004/diff/100001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1642323004/diff/100001/chrome/app/generated_r... chrome/app/generated_resources.grd:12922: + Password On 2016/02/19 06:09:41, dzhioev wrote: > Not too crucial, but was this change approved by UX guys? yes, the mocks are here (slides #5 to #8): https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... https://codereview.chromium.org/1642323004/diff/100001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/1642323004/diff/100001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1545: * @param {number|string=} count The number or string to replace $1 in On 2016/02/19 06:09:41, dzhioev wrote: > Add a comment about |is_synced_user|. Removed the parameter altogether. https://codereview.chromium.org/1642323004/diff/100001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1556: element.textContent = ''; On 2016/02/19 06:09:41, dzhioev wrote: > Wow, I haven't seen this function before. I think it's weird. > Can we make it a little bit simpler? Something like: > > message = message.replace('$1', '<span class="total-count">' + count + > '</span>'); > message = message.replace('$2', '<span class="email">' + > this.user.emailAddress + '</span>'); > element.innerHTML = message; > > Also I beleive |is_synced_user| param is excessive, because a) it equals to > |!!this.user.emailAddress| b) the |message| doesn't contain '$2' when > |is_synced_user| is false. Agreed! Done. https://codereview.chromium.org/1642323004/diff/100001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1579: substituteElement_: function(placeholder, elementToAdd) { On 2016/02/19 06:09:41, dzhioev wrote: > This seems unused. Done.
On 2016/02/19 15:07:41, Moe wrote: > https://codereview.chromium.org/1642323004/diff/100001/chrome/app/generated_r... > File chrome/app/generated_resources.grd (right): > > https://codereview.chromium.org/1642323004/diff/100001/chrome/app/generated_r... > chrome/app/generated_resources.grd:12922: + Password > On 2016/02/19 06:09:41, dzhioev wrote: > > Not too crucial, but was this change approved by UX guys? > > yes, the mocks are here (slides #5 to #8): > https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... > > https://codereview.chromium.org/1642323004/diff/100001/ui/login/account_picke... > File ui/login/account_picker/user_pod_row.js (right): > > https://codereview.chromium.org/1642323004/diff/100001/ui/login/account_picke... > ui/login/account_picker/user_pod_row.js:1545: * @param {number|string=} count > The number or string to replace $1 in > On 2016/02/19 06:09:41, dzhioev wrote: > > Add a comment about |is_synced_user|. > > Removed the parameter altogether. > > https://codereview.chromium.org/1642323004/diff/100001/ui/login/account_picke... > ui/login/account_picker/user_pod_row.js:1556: element.textContent = ''; > On 2016/02/19 06:09:41, dzhioev wrote: > > Wow, I haven't seen this function before. I think it's weird. > > Can we make it a little bit simpler? Something like: > > > > message = message.replace('$1', '<span class="total-count">' + count + > > '</span>'); > > message = message.replace('$2', '<span class="email">' + > > this.user.emailAddress + '</span>'); > > element.innerHTML = message; > > > > Also I beleive |is_synced_user| param is excessive, because a) it equals to > > |!!this.user.emailAddress| b) the |message| doesn't contain '$2' when > > |is_synced_user| is false. > > Agreed! Done. > > https://codereview.chromium.org/1642323004/diff/100001/ui/login/account_picke... > ui/login/account_picker/user_pod_row.js:1579: substituteElement_: > function(placeholder, elementToAdd) { > On 2016/02/19 06:09:41, dzhioev wrote: > > This seems unused. > > Done. LGTM
moe: I mostly have some suggestions on the CSS. I'm not very familiar with the user manager code, so I don't have the full context here. Hope this helps. I will be here the rest of the day, then I'm OOO all next week. https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_user_manager/user_manager.css (right): https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:34: control-bar { Absolute positioning is generally speaking a last resort - only if you must overlay an element over another. Given the mocks, I think it's possible to put the control-bar at the fixed-bottom of the screen without using it. Perhaps with CSS flexbox? https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:50: cursor: default; Is this necessary? https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:52: transform: none; And this? https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:75: position: static; The default value is static right? Is this also necessary? https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:86: position: static; same comment as above https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:91: position: static; and here https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:100: text-align: left; So I thought the default text-align is left if it's ltr, and right if it's rtl. So is this necessary? If it is because something else is causing it to be center, can you try using text-align: initial and then removing the rtl clause below? https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:130: background-color: rgb(150, 150, 150); Can this be one of the standard --paper or --google colors? I know --paper has --paper-gray etc. https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:163: position: absolute; Can this be done without absolute positioning? https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:182: color: #fff; just 'white' might be better here https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:221: color: #333; Does a --paper-grey work here? https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:248: color: #333; paper-grey? https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:259: color: rgb(103, 103, 103); paper-grey? https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:264: color: #333; and here https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:272: color: #333; and here https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:283: text-align: left; same comment as above: does text-align: initial work? https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_user_manager/user_manager.html (right): https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.html:68: <link rel="import" href="chrome://resources/html/load_time_data.html"> What was the motivation to move these imports? https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_user_manager/user_manager.js (right): https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.js:37: width = $('outer-container').offsetWidth || Do you mean to declare globals here? https://codereview.chromium.org/1642323004/diff/120001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/1642323004/diff/120001/ui/login/account_picke... ui/login/account_picker/user_pod_row.css:578: background: rgb(210, 63, 49); Where is this color from? Is it a paper-color? https://codereview.chromium.org/1642323004/diff/120001/ui/login/account_picke... ui/login/account_picker/user_pod_row.css:580: color: #fff; white
tommycli@ Thanks for the detailed look at the css. As strange as it may seem, user_manager.css is supposed to completely override the look and feel of cros user manager for desktop. (that's what it did before as well). The original cros css uses lots of absolute positioning. In order to override those I had no choice but to go along with all those absolute positioning. I've gone over the css a couple of times trying to simplify it and use fewer overrides. Hopefully once we fork cros's user manager we can do this right. https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_user_manager/user_manager.css (right): https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:34: control-bar { On 2016/02/20 00:36:39, tommycli wrote: > Absolute positioning is generally speaking a last resort - only if you must > overlay an element over another. > > Given the mocks, I think it's possible to put the control-bar at the > fixed-bottom of the screen without using it. Perhaps with CSS flexbox? Acknowledged. https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:50: cursor: default; On 2016/02/20 00:36:38, tommycli wrote: > Is this necessary? Acknowledged. https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:52: transform: none; On 2016/02/20 00:36:38, tommycli wrote: > And this? Acknowledged. https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:75: position: static; On 2016/02/20 00:36:38, tommycli wrote: > The default value is static right? Is this also necessary? Acknowledged. https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:86: position: static; On 2016/02/20 00:36:38, tommycli wrote: > same comment as above Acknowledged. https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:91: position: static; On 2016/02/20 00:36:39, tommycli wrote: > and here Acknowledged. https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:100: text-align: left; On 2016/02/20 00:36:38, tommycli wrote: > So I thought the default text-align is left if it's ltr, and right if it's rtl. > > So is this necessary? > > If it is because something else is causing it to be center, can you try using > text-align: initial and then removing the rtl clause below? +1 Done! https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:130: background-color: rgb(150, 150, 150); On 2016/02/20 00:36:38, tommycli wrote: > Can this be one of the standard --paper or --google colors? I know --paper has > --paper-gray etc. unfortunately this one isn't one of those greys either. https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:163: position: absolute; On 2016/02/20 00:36:38, tommycli wrote: > Can this be done without absolute positioning? Acknowledged. https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:182: color: #fff; On 2016/02/20 00:36:38, tommycli wrote: > just 'white' might be better here Done. https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:221: color: #333; On 2016/02/20 00:36:38, tommycli wrote: > Does a --paper-grey work here? I can't find this in paper-styles/color.html either. https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:248: color: #333; On 2016/02/20 00:36:38, tommycli wrote: > paper-grey? Acknowledged. https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:259: color: rgb(103, 103, 103); On 2016/02/20 00:36:38, tommycli wrote: > paper-grey? we have paper-grey for rgb(97,97,97) but not for rgb(103, 103, 103). perhaps something designers should pay more attention to. I'll bring up the topic with them. https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:264: color: #333; On 2016/02/20 00:36:38, tommycli wrote: > and here Acknowledged. https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:272: color: #333; On 2016/02/20 00:36:38, tommycli wrote: > and here Acknowledged. https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:283: text-align: left; On 2016/02/20 00:36:38, tommycli wrote: > same comment as above: does text-align: initial work? Done. https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_user_manager/user_manager.html (right): https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.html:68: <link rel="import" href="chrome://resources/html/load_time_data.html"> On 2016/02/20 00:36:39, tommycli wrote: > What was the motivation to move these imports? I remember having problem with some i18n-* attributes not being substituted. Importing i18n_template.html last solved the problem. there are a few examples in the codebase including: md_extensions/extensions.html md_downloads/downloads.html https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_user_manager/user_manager.js (right): https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.js:37: width = $('outer-container').offsetWidth || On 2016/02/20 00:36:39, tommycli wrote: > Do you mean to declare globals here? Done. Added var. https://codereview.chromium.org/1642323004/diff/120001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/1642323004/diff/120001/ui/login/account_picke... ui/login/account_picker/user_pod_row.css:578: background: rgb(210, 63, 49); On 2016/02/20 00:36:39, tommycli wrote: > Where is this color from? Is it a paper-color? it's specified in the mocks and it's not a google or paper color. https://codereview.chromium.org/1642323004/diff/120001/ui/login/account_picke... ui/login/account_picker/user_pod_row.css:580: color: #fff; On 2016/02/20 00:36:39, tommycli wrote: > white Done.
Hi Moe: I think Patchset 4 has extra stuff in it compared to Patchset 3.
Patchset #4 (id:140001) has been deleted
Hey Tommy, my bad. I forgot to pass the base branch to cl upload.
Hey Moe, Per our conversation, I think we're close. I'm okay with the css continuing to override the CrOS css. It's kind of confusing, but I understand why you did this now. One sticking point: Can we remove all the custom colors and shadows and styling as much as possible, conforming to the paper-style defined colors and shadows. I feel like your designers meant to use those colors and shadows anyways... https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... File chrome/browser/resources/md_user_manager/user_manager.css (right): https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:48: 0 2px 2px 0 rgba(0, 0, 0, .24), Can all these shadows (in the whole file) be normalized to use the shadows within paper-styles? I think if you're making it Material design it needs to follow these standard shadows anyways. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/polyme... https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:130: background-color: rgb(150, 150, 150); On 2016/02/22 14:46:45, Moe wrote: > On 2016/02/20 00:36:38, tommycli wrote: > > Can this be one of the standard --paper or --google colors? I know --paper has > > --paper-gray etc. > > unfortunately this one isn't one of those greys either. --paper-grey-500 is 9e9e9e aka rgb(158, 158, 158). That seems to be a pretty close match. I'd imagine if you guys are following Material Design, you'd want to use the paper grays right? Did the designer have a specific reason to use 150 instead of 158? https://codereview.chromium.org/1642323004/diff/120001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:221: color: #333; On 2016/02/22 14:46:45, Moe wrote: > On 2016/02/20 00:36:38, tommycli wrote: > > Does a --paper-grey work here? > > I can't find this in paper-styles/color.html either. See above comment. I'm think material design requires conformance to one of the set paper-greys
tommycli@ I went over the css and further simplified the rules. Also used google and polymer colors. the patch includes a bug fix where the gradient shadow that appears on the user image on hover was handling click events and causing the menu to open. I added a hidden div to the original html and override the css to style and display it on the desktop user manager.
lgtm sans below nits https://codereview.chromium.org/1642323004/diff/180001/chrome/browser/resourc... File chrome/browser/resources/md_user_manager/user_manager.css (right): https://codereview.chromium.org/1642323004/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:6: background-color: #eee; can this also be a paper-grey? https://codereview.chromium.org/1642323004/diff/180001/chrome/browser/resourc... File chrome/browser/resources/md_user_manager/user_manager.js (right): https://codereview.chromium.org/1642323004/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.js:33: * @type {{width:string, height:string}} I think it should be {width: string, height: string} (spaces) https://codereview.chromium.org/1642323004/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.js:43: animatedPages.offsetHeight - 57; Can this '57' value be calculated from the element instead of a constant?
https://codereview.chromium.org/1642323004/diff/180001/chrome/browser/resourc... File chrome/browser/resources/md_user_manager/user_manager.css (right): https://codereview.chromium.org/1642323004/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:6: background-color: #eee; On 2016/03/11 18:59:14, tommycli wrote: > can this also be a paper-grey? Done. https://codereview.chromium.org/1642323004/diff/180001/chrome/browser/resourc... File chrome/browser/resources/md_user_manager/user_manager.js (right): https://codereview.chromium.org/1642323004/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.js:33: * @type {{width:string, height:string}} On 2016/03/11 18:59:14, tommycli wrote: > I think it should be {width: string, height: string} (spaces) Done. https://codereview.chromium.org/1642323004/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.js:43: animatedPages.offsetHeight - 57; On 2016/03/11 18:59:14, tommycli wrote: > Can this '57' value be calculated from the element instead of a constant? I removed the constant from the css file. Unfortunately I can't think of a way to get rid of it here too. The #login-header-bar is not always visible when user_pod_row.js accesses this property in order to arrange the user pods.
mahmadi@chromium.org changed reviewers: + dbeam@chromium.org
+dbeam@
https://codereview.chromium.org/1642323004/diff/200001/chrome/browser/resourc... File chrome/browser/resources/md_user_manager/user_manager.css (right): https://codereview.chromium.org/1642323004/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:26: #outer-container::-webkit-scrollbar { i'm confused, why are you encouraging scroll but hiding the scrollbar? https://codereview.chromium.org/1642323004/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:41: 0 0 2px 0 rgba(0, 0, 0, .12); this fits in one line https://codereview.chromium.org/1642323004/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:57: 0 14px 28px 0 rgba(0, 0, 0, .25) !important; why do you need !important? https://codereview.chromium.org/1642323004/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:148: background-image: linear-gradient(rgba(0, 0, 0, .4), rgba(0, 0, 0, 0) 100%); rgba(0, 0, 0, 0) could also just be transparent i think https://codereview.chromium.org/1642323004/diff/200001/chrome/browser/resourc... File chrome/browser/resources/md_user_manager/user_manager.js (right): https://codereview.chromium.org/1642323004/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.js:41: var height = animatedPages.offsetHeight - 57; why the magic 57? can you get this dynamically or otherwise document (instead of in your comment, var MAX_LOGIN_HEADER_BAR_HEIGHT = 57;) https://codereview.chromium.org/1642323004/diff/200001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/1642323004/diff/200001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1554: message = message.replace('$1', '<span class="total-count">'+ count + space between end of string and + https://codereview.chromium.org/1642323004/diff/200001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:2313: CROS_POD_WIDTH; ident funky
https://codereview.chromium.org/1642323004/diff/200001/chrome/browser/resourc... File chrome/browser/resources/md_user_manager/user_manager.css (right): https://codereview.chromium.org/1642323004/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:26: #outer-container::-webkit-scrollbar { On 2016/03/25 02:48:23, Dan Beam wrote: > i'm confused, why are you encouraging scroll but hiding the scrollbar? This is definitely by design: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... Normally there are not enough profiles for scrolling to even be possible (need 9+ profiles for that). https://codereview.chromium.org/1642323004/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:41: 0 0 2px 0 rgba(0, 0, 0, .12); On 2016/03/25 02:48:23, Dan Beam wrote: > this fits in one line Done. https://codereview.chromium.org/1642323004/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:57: 0 14px 28px 0 rgba(0, 0, 0, .25) !important; On 2016/03/25 02:48:23, Dan Beam wrote: > why do you need !important? turns out I don't anymore. https://codereview.chromium.org/1642323004/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.css:148: background-image: linear-gradient(rgba(0, 0, 0, .4), rgba(0, 0, 0, 0) 100%); On 2016/03/25 02:48:23, Dan Beam wrote: > rgba(0, 0, 0, 0) > > could also just be > > transparent > > i think Done. https://codereview.chromium.org/1642323004/diff/200001/chrome/browser/resourc... File chrome/browser/resources/md_user_manager/user_manager.js (right): https://codereview.chromium.org/1642323004/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.js:41: var height = animatedPages.offsetHeight - 57; On 2016/03/25 02:48:23, Dan Beam wrote: > why the magic 57? can you get this dynamically or otherwise document (instead > of in your comment, var MAX_LOGIN_HEADER_BAR_HEIGHT = 57;) Done. Unfortunately, can't get this dynamically, since the user pods page may not visible yet when this getter is called to calculate the space available for placing the user pods. https://codereview.chromium.org/1642323004/diff/200001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/1642323004/diff/200001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1554: message = message.replace('$1', '<span class="total-count">'+ count + On 2016/03/25 02:48:23, Dan Beam wrote: > space between end of string and + Done. https://codereview.chromium.org/1642323004/diff/200001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:2313: CROS_POD_WIDTH; On 2016/03/25 02:48:23, Dan Beam wrote: > ident funky Done.
https://codereview.chromium.org/1642323004/diff/220001/chrome/browser/resourc... File chrome/browser/resources/md_user_manager/control_bar.css (right): https://codereview.chromium.org/1642323004/diff/220001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/control_bar.css:10: z-index: 1; why do you need this? https://codereview.chromium.org/1642323004/diff/220001/chrome/browser/resourc... File chrome/browser/resources/md_user_manager/user_manager_styles.html (right): https://codereview.chromium.org/1642323004/diff/220001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager_styles.html:32: 0 2px 2px 0 rgba(0, 0, 0, .24), 0 0 2px 0 rgba(0, 0, 0, .12); wrt wrapping: box-shadow: ..., ...; OR box-shadow: ...; https://codereview.chromium.org/1642323004/diff/220001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager_styles.html:214: /* forcing the menu to be positioned as in ltr */ nit: capitalization and . https://codereview.chromium.org/1642323004/diff/220001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/1642323004/diff/220001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1559: element.innerHTML = message; why are you moving from textContent to innerHTML?
https://codereview.chromium.org/1642323004/diff/220001/chrome/browser/resourc... File chrome/browser/resources/md_user_manager/control_bar.css (right): https://codereview.chromium.org/1642323004/diff/220001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/control_bar.css:10: z-index: 1; On 2016/04/07 18:35:00, Dan Beam wrote: > why do you need this? '#outer-container' which is an absolutely positioned sibling of 'control-bar' has a z-index of 1 and therefore partially covers the border shadow of 'control-bar'. One way to fix is was to give 'control-bar' the same z-index. A better option is probably to override the z-index of '#outer-container' to 'initial' instead. PS. I moved this rule to the user_manager_styles.html. https://codereview.chromium.org/1642323004/diff/220001/chrome/browser/resourc... File chrome/browser/resources/md_user_manager/user_manager_styles.html (right): https://codereview.chromium.org/1642323004/diff/220001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager_styles.html:32: 0 2px 2px 0 rgba(0, 0, 0, .24), 0 0 2px 0 rgba(0, 0, 0, .12); On 2016/04/07 18:35:00, Dan Beam wrote: > wrt wrapping: > > box-shadow: ..., > ...; > > OR > > box-shadow: > ...; Done. https://codereview.chromium.org/1642323004/diff/220001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager_styles.html:214: /* forcing the menu to be positioned as in ltr */ On 2016/04/07 18:35:00, Dan Beam wrote: > nit: capitalization and . Done. https://codereview.chromium.org/1642323004/diff/220001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/1642323004/diff/220001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1559: element.innerHTML = message; On 2016/04/07 18:35:00, Dan Beam wrote: > why are you moving from textContent to innerHTML? the existing logic was too complex. dzhioev@ suggested I replace it with this simpler logic.
https://codereview.chromium.org/1642323004/diff/220001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/1642323004/diff/220001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1559: element.innerHTML = message; On 2016/04/07 22:48:55, Moe wrote: > On 2016/04/07 18:35:00, Dan Beam wrote: > > why are you moving from textContent to innerHTML? > > the existing logic was too complex. dzhioev@ suggested I replace it with this > simpler logic. innerHTML = '<script>alert(/xss/)</script>' // actually executes textContent = '<script>alert(1)</script>' // doesn't
Dan, I left a comment on the patch that was setting the content differently. Do you prefer me to revert to that logic? side question: are we actually exposing some sort of vulnerability even if the HTML is not user provided? https://codereview.chromium.org/1642323004/diff/100001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (left): https://codereview.chromium.org/1642323004/diff/100001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1555: messageParts = message.split('$1'); Here's how the logic will more or less look like without setting innerHTML.
lwchkg@gmail.com changed reviewers: + lwchkg@gmail.com
Drive-in. Anyway, Moe: the email is indeed user provided. Not sure whether it can contain the '<' character though, but better not to trust it. https://codereview.chromium.org/1642323004/diff/220001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/1642323004/diff/220001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1559: element.innerHTML = message; On 2016/04/08 00:16:28, Dan Beam wrote: > On 2016/04/07 22:48:55, Moe wrote: > > On 2016/04/07 18:35:00, Dan Beam wrote: > > > why are you moving from textContent to innerHTML? > > > > the existing logic was too complex. dzhioev@ suggested I replace it with this > > simpler logic. > > innerHTML = '<script>alert(/xss/)</script>' // actually executes > textContent = '<script>alert(1)</script>' // doesn't Dan, WDYT about using innerHTML but escaping |count| and |this.user.emailAddress|?
On 2016/04/12 18:28:38, lwchkg wrote: > Anyway, Moe: the email is indeed user provided. Not sure whether it can contain > the '<' character though, but better not to trust it. Update. It CAN actually contain the '<' character if the adversary edits the file "Local State". So it's a real XSS attack vector.
https://codereview.chromium.org/1642323004/diff/220001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/1642323004/diff/220001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1559: element.innerHTML = message; On 2016/04/12 18:28:38, lwchkg wrote: > On 2016/04/08 00:16:28, Dan Beam wrote: > > On 2016/04/07 22:48:55, Moe wrote: > > > On 2016/04/07 18:35:00, Dan Beam wrote: > > > > why are you moving from textContent to innerHTML? > > > > > > the existing logic was too complex. dzhioev@ suggested I replace it with > this > > > simpler logic. > > > > innerHTML = '<script>alert(/xss/)</script>' // actually executes > > textContent = '<script>alert(1)</script>' // doesn't > > Dan, WDYT about using innerHTML but escaping |count| and > |this.user.emailAddress|? i think replacing $1 and $2 manually is silly when we've got code that does this and could be factored out: https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... i think using innerHTML is a bummer, and prefer textContent ... for text. and if we realllly needed to parse as HTML, we should use... parseHtmlSubset(): https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources...
https://codereview.chromium.org/1642323004/diff/220001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/1642323004/diff/220001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1559: element.innerHTML = message; On 2016/04/12 18:33:50, Dan Beam wrote: > On 2016/04/12 18:28:38, lwchkg wrote: > > Dan, WDYT about using innerHTML but escaping |count| and > > |this.user.emailAddress|? > > i think replacing $1 and $2 manually is silly when we've got code that does this > and could be factored out: > https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... > The sad part it the messages to process is already preprocessed with ICU. So getStringF doesn't work here. (P.S. passing to C++ (async) just to fetch an ICU formatted text is lame. WDYT about creating a ICUMessageFormat library in the Javascript code? Note: we may be able to adapt the code from AngularJS.) > i think using innerHTML is a bummer, and prefer textContent ... for text. If an real exploit is possible I prefer not to use innerHTML even if the input is filtered. Trusting a dev for proper escaping/filtering is almost as bad as trusting user inputs. Even if the code is correct at a certain time, it is too easy to have an security-ignorant dev to mess it up in the future. Now I'm inclined to continue using DOM manipulation even if the code is weird. Disclaimer to Moe: Dan has the final decision, so listen to him, not me. > > and if we realllly needed to parse as HTML, we should use... parseHtmlSubset(): > https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... parseHtmlSubset(): Why the tag 'A' is the default set allowed tags? No good. The adversary can craft a malicious URL for users to click, so proper escaping is still needed.
https://codereview.chromium.org/1642323004/diff/220001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/1642323004/diff/220001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1559: element.innerHTML = message; On 2016/04/13 17:46:54, lwchkg wrote: > On 2016/04/12 18:33:50, Dan Beam wrote: > > On 2016/04/12 18:28:38, lwchkg wrote: > > > Dan, WDYT about using innerHTML but escaping |count| and > > > |this.user.emailAddress|? > > > > i think replacing $1 and $2 manually is silly when we've got code that does > this > > and could be factored out: > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... > > > > The sad part it the messages to process is already preprocessed with ICU. So > getStringF doesn't work here. > (P.S. passing to C++ (async) just to fetch an ICU formatted text is lame. WDYT > about creating a ICUMessageFormat library in the Javascript code? Note: we may > be able to adapt the code from AngularJS.) > > > i think using innerHTML is a bummer, and prefer textContent ... for text. > > If an real exploit is possible I prefer not to use innerHTML even if the input > is filtered. Trusting a dev for proper escaping/filtering is almost as bad as > trusting user inputs. Even if the code is correct at a certain time, it is too > easy to have an security-ignorant dev to mess it up in the future. > > Now I'm inclined to continue using DOM manipulation even if the code is weird. > Disclaimer to Moe: Dan has the final decision, so listen to him, not me. > > > > > and if we realllly needed to parse as HTML, we should use... > parseHtmlSubset(): > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... > > parseHtmlSubset(): Why the tag 'A' is the default set allowed tags? No good. The > adversary can craft a malicious URL for users to click, so proper escaping is > still needed. read harder: https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... I have no idea what the rest of your text said
Dan, based on the conversation thread, I think DOM manipulation is the right thing to do here. I reverted that section to avoid innerHTML.
Respond to Dan's comments. https://codereview.chromium.org/1642323004/diff/220001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/1642323004/diff/220001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1559: element.innerHTML = message; On 2016/04/13 23:00:31, Dan Beam wrote: > read harder: > https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... > Thanks for pointing out. I'm still concerned about allowing "https://" in the URL. This allows linking to attacker sites, or possibly also sending sensitive information in the URL. > I have no idea what the rest of your text said Sorry, I though quite many of us knows about ICU for processing strings, but turns out it is not after checking Git Blame results. ICU message is something that deals with singular/plurals and gender in translations. There's an C++ implementation used in Chromium, and so far the only users are jshin, msramek and me. (Git Blame also shows Dan because he touched jshin's strings.) Here's some documents ICU MessageFormat: http://userguide.icu-project.org/formatparse/messages. (Note: the use of $1 and $2 in the strings is this CL is not related to ICU. The braces are though.) Anyway, in user_pod_row.js, some C++ code is called async to process a ICU message. (see line 1514, which calls https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/..., which calls l10n_util::GetPluralStringFUTF16 which executes some underlying code in ICU4C library. Actually I hoped that the library can be called directly from JavaScript, but I'm out of luck.
do you have before and after screenshots for this work? https://codereview.chromium.org/1642323004/diff/260001/chrome/browser/resourc... File chrome/browser/resources/md_user_manager/user_manager_styles.html (right): https://codereview.chromium.org/1642323004/diff/260001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager_styles.html:38: display: none; i'm still confused as to how this is better than overflow: hidden? https://codereview.chromium.org/1642323004/diff/260001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager_styles.html:231: right: 8px; i'm confused, what are you actually overriding here? https://codereview.chromium.org/1642323004/diff/260001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/1642323004/diff/260001/ui/login/account_picke... ui/login/account_picker/user_pod_row.css:584: background: rgb(197,57,41); nit: spaces between commas https://codereview.chromium.org/1642323004/diff/260001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/1642323004/diff/260001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1557: messageParts = message.split(/(\$1|\$2)/); nit: /\$[12]/
On 2016/04/19 03:44:06, Dan Beam wrote: > do you have before and after screenshots for this work? > > https://codereview.chromium.org/1642323004/diff/260001/chrome/browser/resourc... > File chrome/browser/resources/md_user_manager/user_manager_styles.html (right): > > https://codereview.chromium.org/1642323004/diff/260001/chrome/browser/resourc... > chrome/browser/resources/md_user_manager/user_manager_styles.html:38: display: > none; > i'm still confused as to how this is better than overflow: hidden? > > https://codereview.chromium.org/1642323004/diff/260001/chrome/browser/resourc... > chrome/browser/resources/md_user_manager/user_manager_styles.html:231: right: > 8px; > i'm confused, what are you actually overriding here? > > https://codereview.chromium.org/1642323004/diff/260001/ui/login/account_picke... > File ui/login/account_picker/user_pod_row.css (right): > > https://codereview.chromium.org/1642323004/diff/260001/ui/login/account_picke... > ui/login/account_picker/user_pod_row.css:584: background: rgb(197,57,41); > nit: spaces between commas > > https://codereview.chromium.org/1642323004/diff/260001/ui/login/account_picke... > File ui/login/account_picker/user_pod_row.js (right): > > https://codereview.chromium.org/1642323004/diff/260001/ui/login/account_picke... > ui/login/account_picker/user_pod_row.js:1557: messageParts = > message.split(/(\$1|\$2)/); > nit: /\$[12]/ Dan, here are the before and after screenshots: 1) https://screenshot.googleplex.com/1h2QS8vYacQ vs https://screenshot.googleplex.com/fEgZhGS8U0d 2) https://screenshot.googleplex.com/ugKz9qTjR15 vs https://screenshot.googleplex.com/YjrE6ScGsDt dzhioev@, it's been a while since your lgtm and there's been a few changes to /ui/login/. could you please take a second look at it? 2)
https://codereview.chromium.org/1642323004/diff/260001/chrome/browser/resourc... File chrome/browser/resources/md_user_manager/user_manager_styles.html (right): https://codereview.chromium.org/1642323004/diff/260001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager_styles.html:38: display: none; On 2016/04/19 03:44:06, Dan Beam wrote: > i'm still confused as to how this is better than overflow: hidden? with overflow: hidden, the overflowing content will just be chopped. We want the user to be able to use the mouse wheel etc in order to scroll to the hidden content, however (for design reasons I don't know) we don't want the scroll bar to show. instead the top border shadow on the bottom control bar should signify that the content is scrollable. https://codereview.chromium.org/1642323004/diff/260001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager_styles.html:231: right: 8px; On 2016/04/19 03:44:06, Dan Beam wrote: > i'm confused, what are you actually overriding here? I was trying to force the ".action-box-menu" to appear in the same way as it does when dir=ltr. This would have helped fix an existing bug in dir=rtl where the menu is briefly rendered under the adjacent user pod. However, I tested this further and it seems that the bug appears in dir=ltr under some circumstances as well (Thanks for bringing this up!). Also, I think this is the wrong way to fix it from a design perspective. I'll file and fix that bug separately and change this rule to "left: 8px;" (overriding left: 5px; in the user_pod_row.css) https://codereview.chromium.org/1642323004/diff/260001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/1642323004/diff/260001/ui/login/account_picke... ui/login/account_picker/user_pod_row.css:584: background: rgb(197,57,41); On 2016/04/19 03:44:06, Dan Beam wrote: > nit: spaces between commas Done. https://codereview.chromium.org/1642323004/diff/260001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/1642323004/diff/260001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1557: messageParts = message.split(/(\$1|\$2)/); On 2016/04/19 03:44:06, Dan Beam wrote: > nit: /\$[12]/ Done. /(\$[12])/ to keep the delimiters.
https://codereview.chromium.org/1642323004/diff/260001/chrome/browser/resourc... File chrome/browser/resources/md_user_manager/user_manager_styles.html (right): https://codereview.chromium.org/1642323004/diff/260001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager_styles.html:38: display: none; On 2016/04/19 15:20:08, Moe wrote: > On 2016/04/19 03:44:06, Dan Beam wrote: > > i'm still confused as to how this is better than overflow: hidden? > > with overflow: hidden, the overflowing content will just be chopped. We want the > user to be able to use the mouse wheel etc in order to scroll to the hidden > content, however (for design reasons I don't know) we don't want the scroll bar > to show. instead the top border shadow on the bottom control bar should signify > that the content is scrollable. let the scrollbars slide freely https://codereview.chromium.org/1642323004/diff/280001/chrome/browser/resourc... File chrome/browser/resources/md_user_manager/user_manager_styles.html (right): https://codereview.chromium.org/1642323004/diff/280001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager_styles.html:266: font-weight: bold; bold -> 500
https://codereview.chromium.org/1642323004/diff/280001/chrome/browser/resourc... File chrome/browser/resources/md_user_manager/user_manager_styles.html (right): https://codereview.chromium.org/1642323004/diff/280001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager_styles.html:266: font-weight: bold; On 2016/04/19 18:00:57, Dan Beam wrote: > bold -> 500 Done.
On 2016/04/19 18:41:08, Moe wrote: > https://codereview.chromium.org/1642323004/diff/280001/chrome/browser/resourc... > File chrome/browser/resources/md_user_manager/user_manager_styles.html (right): > > https://codereview.chromium.org/1642323004/diff/280001/chrome/browser/resourc... > chrome/browser/resources/md_user_manager/user_manager_styles.html:266: > font-weight: bold; > On 2016/04/19 18:00:57, Dan Beam wrote: > > bold -> 500 > > Done. also fixed the scrollbar issue: https://screenshot.googleplex.com/tUOHLvfhg9T
lgtm https://codereview.chromium.org/1642323004/diff/300001/chrome/browser/resourc... File chrome/browser/resources/md_user_manager/user_manager.js (right): https://codereview.chromium.org/1642323004/diff/300001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.js:42: }, could this be just: newDesktopUserManager: true, ? https://codereview.chromium.org/1642323004/diff/300001/chrome/browser/resourc... File chrome/browser/resources/md_user_manager/user_manager_styles.html (right): https://codereview.chromium.org/1642323004/diff/300001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager_styles.html:163: -webkit-transform: none; nit: transform: none; https://codereview.chromium.org/1642323004/diff/300001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager_styles.html:226: left: 8px; don't you also need a: right: auto; here?
Patchset #12 (id:320001) has been deleted
Thank you! https://codereview.chromium.org/1642323004/diff/300001/chrome/browser/resourc... File chrome/browser/resources/md_user_manager/user_manager.js (right): https://codereview.chromium.org/1642323004/diff/300001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager.js:42: }, On 2016/04/19 21:15:09, Dan Beam wrote: > could this be just: > > newDesktopUserManager: true, > > ? yes. My bad. No need for this to be a computed property. https://codereview.chromium.org/1642323004/diff/300001/chrome/browser/resourc... File chrome/browser/resources/md_user_manager/user_manager_styles.html (right): https://codereview.chromium.org/1642323004/diff/300001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager_styles.html:163: -webkit-transform: none; On 2016/04/19 21:15:09, Dan Beam wrote: > nit: transform: none; Done. https://codereview.chromium.org/1642323004/diff/300001/chrome/browser/resourc... chrome/browser/resources/md_user_manager/user_manager_styles.html:226: left: 8px; On 2016/04/19 21:15:09, Dan Beam wrote: > don't you also need a: > > right: auto; > > here? it's not necessary since user_pod_row.css already has right: auto; I just need to override left: 5px to left: 8px; but I'll include it for better readability. thanks!
The CQ bit was checked by mahmadi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rogerta@chromium.org, dzhioev@chromium.org, tommycli@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/1642323004/#ps340001 (title: "Addressed comment, rebase, and fix for browser test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1642323004/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1642323004/340001
Message was sent while issue was closed.
Description was changed from ========== User Manager MD User Pods BUG=563722 based on=https://codereview.chromium.org/1630903002/ ========== to ========== User Manager MD User Pods BUG=563722 based on=https://codereview.chromium.org/1630903002/ ==========
Message was sent while issue was closed.
Committed patchset #12 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== User Manager MD User Pods BUG=563722 based on=https://codereview.chromium.org/1630903002/ ========== to ========== User Manager MD User Pods BUG=563722 based on=https://codereview.chromium.org/1630903002/ Committed: https://crrev.com/ec4fcd8d8cc2b5935cbc1e79a691251feaa53dae Cr-Commit-Position: refs/heads/master@{#388503} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/ec4fcd8d8cc2b5935cbc1e79a691251feaa53dae Cr-Commit-Position: refs/heads/master@{#388503} |
