Issue metadata
Sign in to add a comment
|
visD tweaks for new sign-in warning dialogue
Reported by
jshan...@etouch.net,
Nov 22 2016
|
||||||||||||||||||||||
Issue descriptionChrome Version: 57.0.2926.0 (Official Build)Revision da59d418f54604ba2451cd0ef3a9cd42c05ca530-refs/heads/master@{#433437} (32/64-bit) OS:Windows (7,8,10), Mac (10.11.6, 10.12.1), Linux (14.04 LTS) What steps will reproduce the problem? (1)Launch chrome and click on avatar icon , sign in with valid credentials. (2)Navigate to chrome://md-settings and click on 'sign out' button. (3)Again sign in with different credentials and press tab key, observe the focus. Actual: Overlapping of text and focus ring is observed after pressing tab key in confirmation dialog box. Expected: No such overlapping should be seen after pressing tab key in confirmation dialog box. This is a regression issue broken in 'M57' and below is the manual regression range: Good Build: 56.0.2924.0 Bad Build: 57.0.2926.0
,
Nov 22 2016
Using the per-revision bisect providing the bisect results, Good build: 56.0.2924.0 (Revision: 433059). Bad build: 57.0.2926.0 (Revision: 433437). You are probably looking for a change made after 433176 (known good), but no later than 433177 (first known bad). CHANGELOG URL: https://chromium.googlesource.com/chromium/src/+log/09cceadb789bf9836f6f3bb6bd342df94e34b15c..8c24599eb8d95b70cf0a7ed017ca61c843326324 @msarda -- Could you please look into the issue, pardon me if it has nothing to do with your changes and if possible please assign it to concern owner. Thank You.
,
Nov 22 2016
This bisect is correct - this UI changed with my CL. This UI was redone in that CL (there were no radio buttons before). I guess I need to increase the spacing between the buttons and the text.
,
Nov 22 2016
Unfortunately, all of our UXers are out of office right now. I will bring it up with them when they're back next week (post-Thanksgiving). How were you getting the values for the padding right now, Mihai?
,
Nov 22 2016
To my knowledge, I have not changed the spacing at all. While looking at the mocks, I can see there is more spacing between the radio button and its label.
,
Nov 22 2016
Interesting. And you're just using the default spacing? I'll ask a UXer for specific padding measurements when they're back in office next week.
,
Dec 1 2016
+Alan to help with the spec for the padding between the radio buttons and the text
,
Dec 5 2016
Specs attached. All these components are live on dev-channel settings for reference. - Ripple diameter is 40px, and black with alpha (not colored) - components should be painted with Chrome blue: #4285F4 (not polymer purple)
,
Dec 5 2016
+dbeam@ who can point you to the code
,
Dec 5 2016
,
Dec 13 2016
,
Dec 13 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/052b535c63488892a84df2ffa0b230627885a05e commit 052b535c63488892a84df2ffa0b230627885a05e Author: msarda <msarda@chromium.org> Date: Tue Dec 13 22:25:02 2016 Change the radio button styling to the sign-in confirmation dialog This CL does the following changes to the radio button: * Ripple diameter is now 40px, and black with alpha (not colored) * Radio button is painted with Chrome blue: #4285F4 (not polymer purple) BUG= 667665 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2569243002 Cr-Commit-Position: refs/heads/master@{#438312} [modify] https://crrev.com/052b535c63488892a84df2ffa0b230627885a05e/chrome/browser/resources/signin/signin_shared_css.html
,
Dec 14 2016
,
Dec 15 2016
,
Jan 10 2017
Re-opening to address a few overlooked specifications: 1. Let's expand the dialog width from 448px > 512px to accommodate text-length 2. When the header runs over to 2 lines, ensure there's 16px padding top and bottom 3. header line-height: 22px 4. body line-height: 20px 5. radio_off color: #757575 6. header and body text: #333333
,
Jan 10 2017
,
Jan 10 2017
I have CL https://codereview.chromium.org/2625663003 in flight that will address there requirements as explained below: > 1. Let's expand the dialog width from 448px > 512px to accommodate text-length Done. Note that this makes it bigger than all the other sign-in dialogs (like sign-in, sync confirmation and sign-in error dialogs that are all 448px) > 2. When the header runs over to 2 lines, ensure there's 16px padding top and bottom In CSS, it is pretty impossible to make it be 52 pixels in a case and 32+2*line height in another case. So, I've basically coded it as being 2*16+nb_lines*line_height in all cases. This means that in the case of a single line, this will be 2*16+22=54 and for 2 lines it will be 2*16+2*22=76px. > 3. header line-height: 22px Done. > 4. body line-height: 20px The paper radio button does not support a custom line height for its label - the only parameters we can configure on it are listed here https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chromium/paper-radio-button/paper-radio-button.html?rcl=0&l=35 > 5. radio_off color: #757575 Done > 6. header and body text: #333333 Done See attached screenshots for how this dialog looks on Windows. Please LMK if something still looks off in any way.
,
Jan 10 2017
Re-assigning to Alan to answer your questions above and give an LGTM before you land the CL. Alan, please re-assign to Mihai after you've given feedback.
,
Jan 10 2017
,
Jan 10 2017
Sorry Mihai, I didn't explicitly call out the additional 16px between rule line and 1st radio button label. See attached. Comments below... > 1. Let's expand the dialog width from 448px > 512px to accommodate text-length Done. Note that this makes it bigger than all the other sign-in dialogs (like sign-in, sync confirmation and sign-in error dialogs that are all 448px) Noted :) > 2. When the header runs over to 2 lines, ensure there's 16px padding top and bottom In CSS, it is pretty impossible to make it be 52 pixels in a case and 32+2*line height in another case. So, I've basically coded it as being 2*16+nb_lines*line_height in all cases. This means that in the case of a single line, this will be 2*16+22=54 and for 2 lines it will be 2*16+2*22=76px. SGTM. Thanks for improvising. > 4. body line-height: 20px The paper radio button does not support a custom line height for its label - the only parameters we can configure on it are listed here https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chromium/paper-radio-button/paper-radio-button.html?rcl=0&l=35 I believe we've done something similar in Settings so I asked dbeam@ and he said he commented in the review and told me to look at this: https://jsfiddle.net/rtvqztbr/ Hopefully this answers your questions. Let me know. > 5. radio_off color: #757575 Done Your screenshots don't reflect this at the moment. Intentional?
,
Jan 11 2017
6. 16px between title bar and the first radio button: Done 7. Radio button color #757575 Done (it looks like --paper-grey-600 works for --paper-radio-button-unchecked-colo but --google-grey-600 done not ) 8. Radio button line height of 20px Done (thanks for the tip) See attached screenshot.
,
Jan 11 2017
Re-assigning to Alan to sign-off on latest implementation. Alan, if it looks good to you, please re-assign back to Mihai to land the CL.
,
Jan 12 2017
Perfecto! Thanks Mihai!
,
Jan 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a0b7a374a98a7aa4902e922ad7b7279ade10c3da commit a0b7a374a98a7aa4902e922ad7b7279ade10c3da Author: msarda <msarda@chromium.org> Date: Fri Jan 13 21:03:44 2017 Layout nits for the signin email confirmation dialog. This CL does the following layout changes for the sign-in email confirmation dialog: 1. Expands the dialog width from 448px > 512px to accommodate text-length 2. Change the height og the title bar to have a 16px padding at the top and at the bottom. 3. Change title line-height to 22px 4. Chaneg the title font color to: #333333 5. Change radio_off color: #757575 (this is google-grey-600) BUG= 667665 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2625663003 Cr-Commit-Position: refs/heads/master@{#443665} [modify] https://crrev.com/a0b7a374a98a7aa4902e922ad7b7279ade10c3da/chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html [modify] https://crrev.com/a0b7a374a98a7aa4902e922ad7b7279ade10c3da/chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.js [modify] https://crrev.com/a0b7a374a98a7aa4902e922ad7b7279ade10c3da/chrome/browser/resources/signin/signin_shared_css.html [modify] https://crrev.com/a0b7a374a98a7aa4902e922ad7b7279ade10c3da/chrome/browser/ui/webui/signin/signin_email_confirmation_dialog.cc
,
Jan 14 2017
,
Jan 17 2017
Tested the issue on Windows-7 and Mac OS 10.12 using chrome latest Beta M57-57.0.2984.0 by following steps mentioned in the original comment. Observed that No overlapping should be seen after pressing tab key in confirmation dialog box. Hence adding TE-Verified label. Please fins the screen shot for reference. Thank you! |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by jshan...@etouch.net
, Nov 22 2016