Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
99543005:
[rAc] [OSX] Reduce gap between "shipping address" label and the shipping address. (Closed)
|
Created:
7 years ago by Ilya Sherman Modified:
7 years ago Reviewers:
groby-ooo-7-16 CC:
chromium-reviews, benquan, Dane Wallinga, dyu1, estade+watch_chromium.org, Ilya Sherman, rouslan+autofillwatch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[rAc] [OSX] Reduce gap between "shipping address" label and the shipping address.
BUG=325484
TEST=(see bug for screenshots)
R=groby@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=241082
Patch Set 1 #
Total comments: 4
Patch Set 2 : Center w.r.t. CVV input field #
Total comments: 4
Patch Set 3 : Restore manual futz factor #Messages
Total messages: 15 (0 generated)
Before and after screenshots are posted on the bug: https://code.google.com/p/chromium/issues/detail?id=325484
https://codereview.chromium.org/99543005/diff/1/chrome/browser/ui/cocoa/autof... File chrome/browser/ui/cocoa/autofill/autofill_suggestion_container.mm (left): https://codereview.chromium.org/99543005/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_suggestion_container.mm:40: const CGFloat kLabelTopPadding = 5.0; So, what happens with a CVV input? Is that not centered any more? https://codereview.chromium.org/99543005/diff/1/chrome/browser/ui/cocoa/autof... File chrome/browser/ui/cocoa/autofill/autofill_suggestion_container.mm (right): https://codereview.chromium.org/99543005/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_suggestion_container.mm:108: [paragraphStyle setLineSpacing:0.5 * [[label_ font] pointSize]]; Why the change from lineHeightMultiple to lineSpacing?
https://codereview.chromium.org/99543005/diff/1/chrome/browser/ui/cocoa/autof... File chrome/browser/ui/cocoa/autofill/autofill_suggestion_container.mm (left): https://codereview.chromium.org/99543005/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_suggestion_container.mm:40: const CGFloat kLabelTopPadding = 5.0; On 2013/12/13 07:19:39, groby wrote: > So, what happens with a CVV input? Is that not centered any more? Good call, that was very broken. Fixed. https://codereview.chromium.org/99543005/diff/1/chrome/browser/ui/cocoa/autof... File chrome/browser/ui/cocoa/autofill/autofill_suggestion_container.mm (right): https://codereview.chromium.org/99543005/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_suggestion_container.mm:108: [paragraphStyle setLineSpacing:0.5 * [[label_ font] pointSize]]; On 2013/12/13 07:19:39, groby wrote: > Why the change from lineHeightMultiple to lineSpacing? Because lineHeightMultiple reserves space above the first line, whereas lineSpacing only affects intra-line spacing.
https://codereview.chromium.org/99543005/diff/20001/chrome/browser/ui/cocoa/a... File chrome/browser/ui/cocoa/autofill/autofill_suggestion_container.mm (right): https://codereview.chromium.org/99543005/diff/20001/chrome/browser/ui/cocoa/a... chrome/browser/ui/cocoa/autofill/autofill_suggestion_container.mm:204: roundf((NSHeight(inputFieldFrame) - NSHeight(labelFrame)) / 2.0); I'm still not sure this works - if the label is multi-line, this will produce odd results :) I used the manual futz factor since it's a rather painful process to get the first line's size. If you want to do it: Get text container, get layout manager Get range for first line of text ([textContainer lineRangeForRange:...]) Get glyph range ([layoutManager getGlyphRangeForCharacterRange:...]) Get the bounding rect ([layoutManager boundingRectForGlyphRange:...]) Or, just add 5 :)
https://codereview.chromium.org/99543005/diff/20001/chrome/browser/ui/cocoa/a... File chrome/browser/ui/cocoa/autofill/autofill_suggestion_container.mm (right): https://codereview.chromium.org/99543005/diff/20001/chrome/browser/ui/cocoa/a... chrome/browser/ui/cocoa/autofill/autofill_suggestion_container.mm:204: roundf((NSHeight(inputFieldFrame) - NSHeight(labelFrame)) / 2.0); On 2013/12/13 19:25:20, groby wrote: > I'm still not sure this works - if the label is multi-line, this will produce > odd results :) > > I used the manual futz factor since it's a rather painful process to get the > first line's size. If you want to do it: > > Get text container, get layout manager > Get range for first line of text ([textContainer lineRangeForRange:...]) > Get glyph range ([layoutManager getGlyphRangeForCharacterRange:...]) > Get the bounding rect ([layoutManager boundingRectForGlyphRange:...]) > > Or, just add 5 :) Is there ever a case where the label is multi-line? (Does that happen with Wallet's CVV challenges? I couldn't get Wallet to work enough to test this... :/) I got rid of the manual futz factor because it was adding too much padding for lines that don't have an inputField. I suppose I could add it back just for cases when an input field is present. Does that sound reasonable to you? :)
lgtm https://codereview.chromium.org/99543005/diff/20001/chrome/browser/ui/cocoa/a... File chrome/browser/ui/cocoa/autofill/autofill_suggestion_container.mm (right): https://codereview.chromium.org/99543005/diff/20001/chrome/browser/ui/cocoa/a... chrome/browser/ui/cocoa/autofill/autofill_suggestion_container.mm:204: roundf((NSHeight(inputFieldFrame) - NSHeight(labelFrame)) / 2.0); On 2013/12/14 00:03:06, Ilya Sherman wrote: > On 2013/12/13 19:25:20, groby wrote: > > I'm still not sure this works - if the label is multi-line, this will produce > > odd results :) > > > > I used the manual futz factor since it's a rather painful process to get the > > first line's size. If you want to do it: > > > > Get text container, get layout manager > > Get range for first line of text ([textContainer lineRangeForRange:...]) > > Get glyph range ([layoutManager getGlyphRangeForCharacterRange:...]) > > Get the bounding rect ([layoutManager boundingRectForGlyphRange:...]) > > > > Or, just add 5 :) > > Is there ever a case where the label is multi-line? (Does that happen with > Wallet's CVV challenges? I couldn't get Wallet to work enough to test this... > :/) > > I got rid of the manual futz factor because it was adding too much padding for > lines that don't have an inputField. I suppose I could add it back just for > cases when an input field is present. Does that sound reasonable to you? :) Hm. Looking at the mocks, it seems the CVV challenge is only ever a single line - so maybe this is good "as is". I'd add the futz factor if there is an input field, just in case...
https://codereview.chromium.org/99543005/diff/20001/chrome/browser/ui/cocoa/a... File chrome/browser/ui/cocoa/autofill/autofill_suggestion_container.mm (right): https://codereview.chromium.org/99543005/diff/20001/chrome/browser/ui/cocoa/a... chrome/browser/ui/cocoa/autofill/autofill_suggestion_container.mm:204: roundf((NSHeight(inputFieldFrame) - NSHeight(labelFrame)) / 2.0); On 2013/12/14 00:08:40, groby wrote: > On 2013/12/14 00:03:06, Ilya Sherman wrote: > > On 2013/12/13 19:25:20, groby wrote: > > > I'm still not sure this works - if the label is multi-line, this will > produce > > > odd results :) > > > > > > I used the manual futz factor since it's a rather painful process to get the > > > first line's size. If you want to do it: > > > > > > Get text container, get layout manager > > > Get range for first line of text ([textContainer lineRangeForRange:...]) > > > Get glyph range ([layoutManager getGlyphRangeForCharacterRange:...]) > > > Get the bounding rect ([layoutManager boundingRectForGlyphRange:...]) > > > > > > Or, just add 5 :) > > > > Is there ever a case where the label is multi-line? (Does that happen with > > Wallet's CVV challenges? I couldn't get Wallet to work enough to test this... > > :/) > > > > I got rid of the manual futz factor because it was adding too much padding for > > lines that don't have an inputField. I suppose I could add it back just for > > cases when an input field is present. Does that sound reasonable to you? :) > > Hm. Looking at the mocks, it seems the CVV challenge is only ever a single line > - so maybe this is good "as is". I'd add the futz factor if there is an input > field, just in case... Alright, added the manual futz factor back in, but only when there is an input field.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/isherman@chromium.org/99543005/40001
Retried try job too often on linux_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/isherman@chromium.org/99543005/40001
Retried try job too often on mac_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/isherman@chromium.org/99543005/40001
Retried try job too often on mac_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/isherman@chromium.org/99543005/40001
Message was sent while issue was closed.
Change committed as 241082 |
