Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2002103003:
[Material][Mac] Adjustments to the Omnibox decorations (Closed)
Description[Material][Mac] Adjustments to the Omnibox decorations
BUG=613788, 611113
Committed: https://crrev.com/7de0ca5b7f66e36c74df676b9c0fbe2dc10364dd
Cr-Commit-Position: refs/heads/master@{#397734}
Patch Set 1 #
Total comments: 2
Patch Set 2 : #Patch Set 3 : Reduce the horizontal spacing around icon by 1 pt #Patch Set 4 : Changed icon #Patch Set 5 : Added shrike's patch #Patch Set 6 : #Patch Set 7 : Shift icon to the right #
Messages
Total messages: 30 (8 generated)
spqchan@chromium.org changed reviewers: + shrike@chromium.org
PTAL
https://codereview.chromium.org/2002103003/diff/1/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/bubble_decoration.mm (right): https://codereview.chromium.org/2002103003/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/bubble_decoration.mm:35: return ui::MaterialDesignController::IsModeMaterial() ? 3.0 : 4.0; Actually, the 4pt of padding was fine. It's just that the lock icon needs to move 1pt to the left. With your change the text got closer to the lock, which is not what we want. After you move the lock icon 1pt to the left you'll probably have to increase the IconLabelPadding to 5 instead of 4. Also, please do move the lock icon up one 1pt. It definitely is sitting lower than the text baseline.
It looks like the cl with the new lock icon just landed: https://codereview.chromium.org/1935663002 I assume it will get cherry-picked back to M52. You should not worry about moving the lock icon around until you've had a chance to start using the new lock icon and we can see how it looks (you'll probably have to change the icon size for it).
https://codereview.chromium.org/2002103003/diff/1/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/bubble_decoration.mm (right): https://codereview.chromium.org/2002103003/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/bubble_decoration.mm:35: return ui::MaterialDesignController::IsModeMaterial() ? 3.0 : 4.0; On 2016/05/24 01:13:26, shrike wrote: > Actually, the 4pt of padding was fine. It's just that the lock icon needs to > move 1pt to the left. With your change the text got closer to the lock, which is > not what we want. > > After you move the lock icon 1pt to the left you'll probably have to increase > the IconLabelPadding to 5 instead of 4. > > Also, please do move the lock icon up one 1pt. It definitely is sitting lower > than the text baseline. I made the pixel pushes with the new icon. Let me know if anything looks off. I pushed the lock icon up 1 pt too, but I think it's off with the new icon
On 2016/05/24 21:05:25, spqchan wrote: > https://codereview.chromium.org/2002103003/diff/1/chrome/browser/ui/cocoa/loc... > File chrome/browser/ui/cocoa/location_bar/bubble_decoration.mm (right): > > https://codereview.chromium.org/2002103003/diff/1/chrome/browser/ui/cocoa/loc... > chrome/browser/ui/cocoa/location_bar/bubble_decoration.mm:35: return > ui::MaterialDesignController::IsModeMaterial() ? 3.0 : 4.0; > On 2016/05/24 01:13:26, shrike wrote: > > Actually, the 4pt of padding was fine. It's just that the lock icon needs to > > move 1pt to the left. With your change the text got closer to the lock, which > is > > not what we want. > > > > After you move the lock icon 1pt to the left you'll probably have to increase > > the IconLabelPadding to 5 instead of 4. > > > > Also, please do move the lock icon up one 1pt. It definitely is sitting lower > > than the text baseline. > > I made the pixel pushes with the new icon. Let me know if anything looks off. > I pushed the lock icon up 1 pt too, but I think it's off with the new icon Hello spqchan@, This looks to be as good as we're going to get it with the assets as they are. They will either have to adjust the lock icon so that it renders 1px lower on Retina, or we hard-code and render the vector ourselves. Looking at the evcert stuff now I think we do need to tweak that also. Right now there appears to be 6pts of space on either side of the lock icon - can you change that to 5pts?
On 2016/05/25 16:49:03, shrike wrote: > On 2016/05/24 21:05:25, spqchan wrote: > > > https://codereview.chromium.org/2002103003/diff/1/chrome/browser/ui/cocoa/loc... > > File chrome/browser/ui/cocoa/location_bar/bubble_decoration.mm (right): > > > > > https://codereview.chromium.org/2002103003/diff/1/chrome/browser/ui/cocoa/loc... > > chrome/browser/ui/cocoa/location_bar/bubble_decoration.mm:35: return > > ui::MaterialDesignController::IsModeMaterial() ? 3.0 : 4.0; > > On 2016/05/24 01:13:26, shrike wrote: > > > Actually, the 4pt of padding was fine. It's just that the lock icon needs to > > > move 1pt to the left. With your change the text got closer to the lock, > which > > is > > > not what we want. > > > > > > After you move the lock icon 1pt to the left you'll probably have to > increase > > > the IconLabelPadding to 5 instead of 4. > > > > > > Also, please do move the lock icon up one 1pt. It definitely is sitting > lower > > > than the text baseline. > > > > I made the pixel pushes with the new icon. Let me know if anything looks off. > > I pushed the lock icon up 1 pt too, but I think it's off with the new icon > > Hello spqchan@, > > This looks to be as good as we're going to get it with the assets as they are. > They will either have to adjust the lock icon so that it renders 1px lower on > Retina, or we hard-code and render the vector ourselves. > > Looking at the evcert stuff now I think we do need to tweak that also. Right now > there appears to be 6pts of space on either side of the lock icon - can you > change that to 5pts? Sorry, I was mostly working MacViews today (It's almost done!) and just started looking at this now. Can you elaborate how you measured 6pts of space? It's hard for me to tell
> Sorry, I was mostly working MacViews today (It's almost done!) and just started > looking at this now. Can you elaborate how you measured 6pts of space? > It's hard for me to tell Please put MacViews on the back-burner until this cl lands. We need it to get into UI review. For measuring pixels I take screenshots and look at them in Photoshop. As an alternative it looks like the Pixie app that's part of the Graphics Tools package that you can download from the ADC will let you do roughly the same. Once you zoom into the screen far enough you can see and count individual pixels. In addition to the extra spacing, the lock icon has a different shape in EV Cert than in http - maybe the icon size is different? Or maybe it's two different icons? They need to be the same. And somehow the EV cert text is 1pt higher on Retina than the rest of the URL text that follows. I'm not sure why that would be happening. I'm attaching the details to Issue 613778.
Description was changed from ========== [Material][Mac] Adjustments to the Omnibox decorations BUG=613788,613787 ========== to ========== [Material][Mac] Adjustments to the Omnibox decorations BUG=613788,613787,611113 ==========
On 2016/05/26 16:16:06, shrike wrote: > > Sorry, I was mostly working MacViews today (It's almost done!) and just > started > > looking at this now. Can you elaborate how you measured 6pts of space? > > It's hard for me to tell > > Please put MacViews on the back-burner until this cl lands. We need it to get > into UI review. > > For measuring pixels I take screenshots and look at them in Photoshop. As an > alternative it looks like the Pixie app that's part of the Graphics Tools > package that you can download from the ADC will let you do roughly the same. > Once you zoom into the screen far enough you can see and count individual > pixels. > > In addition to the extra spacing, the lock icon has a different shape in EV Cert > than in http - maybe the icon size is different? Or maybe it's two different > icons? They need to be the same. And somehow the EV cert text is 1pt higher on > Retina than the rest of the URL text that follows. I'm not sure why that would > be happening. I'm attaching the details to Issue 613778. That's not what I meant, how did you managed to get 6pts? I'm getting more than that when I measured the sides. Anyway, I reduced the left and right side spacing around the icon by 1 pt. Let me know if it looks alright
On 2016/05/26 18:21:07, spqchan wrote: > That's not what I meant, how did you managed to get 6pts? I'm getting more than > that when I measured the sides. > Anyway, I reduced the left and right side spacing around the icon by 1 pt. Let > me know if it looks alright Apologies for misunderstanding your question. Looking at the Canary again I'm not sure where I got 6pts on both sides from. There definitely are 6pts on the left but the right has 4pt, so the lock is just 1pt too far to the right. Anyways, I tried your latest patch and the lock is now perfectly centered horizontally. Do you have any idea what's going on with the different locks? Are they the same vector being rendered at different sizes, or are there two different vectors icons being used?
On 2016/05/26 19:25:00, shrike wrote: > On 2016/05/26 18:21:07, spqchan wrote: > > That's not what I meant, how did you managed to get 6pts? I'm getting more > than > > that when I measured the sides. > > Anyway, I reduced the left and right side spacing around the icon by 1 pt. Let > > me know if it looks alright > > Apologies for misunderstanding your question. > > Looking at the Canary again I'm not sure where I got 6pts on both sides from. > There definitely are 6pts on the left but the right has 4pt, so the lock is just > 1pt too far to the right. > > Anyways, I tried your latest patch and the lock is now perfectly centered > horizontally. > > Do you have any idea what's going on with the different locks? Are they the same > vector being rendered at different sizes, or are there two different vectors > icons being used? From what I see, they are different vector icons being used EV cert uses LOCATION_BAR_HTTPS_VALID_IN_CHIP while the other one uses LOCATION_BAR_HTTPS_VALID
On 2016/05/26 19:53:39, spqchan wrote: > From what I see, they are different vector icons being used > EV cert uses LOCATION_BAR_HTTPS_VALID_IN_CHIP while the other one uses > LOCATION_BAR_HTTPS_VALID These really need to be consolidated down to a single lock icon. I relayed your info to Issue 604520, one of the bugs where these issues are being discussed.
On 2016/05/26 20:28:13, shrike wrote: > On 2016/05/26 19:53:39, spqchan wrote: > > From what I see, they are different vector icons being used > > EV cert uses LOCATION_BAR_HTTPS_VALID_IN_CHIP while the other one uses > > LOCATION_BAR_HTTPS_VALID > > These really need to be consolidated down to a single lock icon. I relayed your > info to Issue 604520, one of the bugs where these issues are being discussed. Icon is updated, and I adjusted the size accordingly. I'm not sure, but the icon might now be 1pt above the text line
On 2016/05/26 23:18:59, spqchan wrote: > On 2016/05/26 20:28:13, shrike wrote: > > On 2016/05/26 19:53:39, spqchan wrote: > > > From what I see, they are different vector icons being used > > > EV cert uses LOCATION_BAR_HTTPS_VALID_IN_CHIP while the other one uses > > > LOCATION_BAR_HTTPS_VALID > > > > These really need to be consolidated down to a single lock icon. I relayed > your > > info to Issue 604520, one of the bugs where these issues are being discussed. > > Icon is updated, and I adjusted the size accordingly. I'm not sure, but the icon > might now be 1pt above the text line FYI, I'm going to move the popup animation fix into another CL
Description was changed from ========== [Material][Mac] Adjustments to the Omnibox decorations BUG=613788,613787,611113 ========== to ========== [Material][Mac] Adjustments to the Omnibox decorations BUG=613788,611113 ==========
PTAL Added your patch in!
On 2016/06/02 21:25:32, spqchan wrote: > PTAL > > Added your patch in! spqchan@ - looks great! It looks like the EV cert lock icon is 1pt too far left. Moving it 1pt to the right will place it in the same location as the http lock. Other than that, it's great, and I will be very happy to get this cl landed and move on to UI review!
Patchset #7 (id:120001) has been deleted
Sounds good, I shifted the icon to the right
lgtm
spqchan@chromium.org changed reviewers: + rsesek@chromium.org
+rsesek for OWNERS
lgtm
The CQ bit was checked by spqchan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2002103003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2002103003/140001
Message was sent while issue was closed.
Description was changed from ========== [Material][Mac] Adjustments to the Omnibox decorations BUG=613788,611113 ========== to ========== [Material][Mac] Adjustments to the Omnibox decorations BUG=613788,611113 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [Material][Mac] Adjustments to the Omnibox decorations BUG=613788,611113 ========== to ========== [Material][Mac] Adjustments to the Omnibox decorations BUG=613788,611113 Committed: https://crrev.com/7de0ca5b7f66e36c74df676b9c0fbe2dc10364dd Cr-Commit-Position: refs/heads/master@{#397734} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/7de0ca5b7f66e36c74df676b9c0fbe2dc10364dd Cr-Commit-Position: refs/heads/master@{#397734} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
