[MdRefresh] Moving the Text in the Chip 3px to the left would align it with the new divider |
|||||
Issue descriptionChrome Version: Chromium #576471 OS: OS=All What steps will reproduce the problem? (1) Enable MdRefresh. (2) Open a tab and go to apple.com (chip appears) (3) Open a second tab and go to google.com (only lock appears) (4) Press CMD-TAB to cycle between both tabs What is the expected result? The text in the Chip could be 3px to the left to be aligned nicely with the new divider. What happens instead? The text in the chip has a lot of (uneven) space and is not aligned with the new divider. The alignment would make tab switching more smooth for your eyes when you are looking at the URL-Bar. Screenshots are attached. Thanks :) Mehmet
,
Jul 19
(BTW: As you can see in the 2nd screenshot, I also increased the space by 4px (to 11px) in the chip between the right side of the text and the divider. That matches the space between the divider and the URL, which is also 11px. Maybe this can be considered in this bug too - otherwise please let me know, so that I can open a new report. Thanks.)
,
Jul 19
SGTM
,
Jul 24
,
Aug 1
tommycli: can you try making this change, or ask manukh to? I was planning to do it but I've got a bunch of analysis and launch tasks to do this week and I'm afraid I won't get to it.
,
Aug 1
Sure I'll take a look.
,
Aug 2
Hey tommycli@, Thanks for looking into this. I have one question. Since we are using the jog again, it would be also nice, when the label of chip could be 2px to the left in this case. I have attached a screencast to demonstrate it by switching two tabs (one with chip and one without). WDYT? Thanks :)
,
Aug 2
Hey Mehmet, Thanks for all the feeback. I ended up adjusting the label 2dp to the left, as you suggested. I didn't mess with the after-label spacing, since the label and URL are already already equidistant from the separator with jog enabled. It's not perfect with jog disabled, but there's too many constraints to make that case equidistant.
,
Aug 2
Looks great 👍 Thank you very much!
,
Aug 2
In the latest patchset I also added a provision to make the Touchable Refresh ChromeOS look correct too. Touchable Refresh with Jog Disabled has additional issues that will have to be addressed in a separate CL.
,
Aug 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/066563c76de0f6639e195732c37ab280911db329 commit 066563c76de0f6639e195732c37ab280911db329 Author: Tommy C. Li <tommycli@chromium.org> Date: Thu Aug 02 22:58:15 2018 Omnibox Refresh UI: Move security chip label left by 2dp for alignment This moves the security chip label left by 2dp so it aligns with the label-less separator, as well as URLs. This improves the alignment of text with or without jog enabled. See bug for screenshot. Bug: 865546 Change-Id: I46918bc1dfe7ff43a8851206c92f2875fc1680f2 Reviewed-on: https://chromium-review.googlesource.com/1159686 Commit-Queue: Tommy Li <tommycli@chromium.org> Reviewed-by: Justin Donnelly <jdonnelly@chromium.org> Cr-Commit-Position: refs/heads/master@{#580363} [modify] https://crrev.com/066563c76de0f6639e195732c37ab280911db329/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc [modify] https://crrev.com/066563c76de0f6639e195732c37ab280911db329/chrome/browser/ui/views/location_bar/icon_label_bubble_view.h
,
Aug 3
Thank you very much, Tommy! Looks great in latest Canary Version 70.0.3511.0. I noticed one small regression. The 2dp movement to the left has been also an effect on the Search-Engine Label. As you can see in the attached screenshot the label "Search ..." is no longer aligned with PopUp suggestions and has been moved 2dp to the left. It effects both states - with jog and with divider. Is it possible to exclude the Search-Engine Label from the change? Thanks for looking into this in advance :) Mehmet
,
Aug 3
Hey Mehmet, thanks for noticing that regression. I'll work to fix it on Monday. I think the merge-ability of this set of refinements for M69 is going down, since it's clear that these alignments are fragile and regression-prone.
,
Aug 3
Okay, thank you.
,
Aug 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/479fc9bddd08df0ead50e953e8d5423f85223aa0 commit 479fc9bddd08df0ead50e953e8d5423f85223aa0 Author: Tommy C. Li <tommycli@chromium.org> Date: Tue Aug 07 16:58:18 2018 Omnibox Refresh UI: Move Search Keyword label right by 2dp for alignment In a previous CL, we moved the security chip label left by 2dp to align with URLs in the case where we have an unlabeled lock icon. However, in doing so, we accidentally un-aligned the indented search keyword label with the suggestions text. This CL fixes that. As far as I can tell, in the old pre-Refresh world, the extra internal spacing used for the search keyword provider just matched the value of GetPrefixedSeparatorWidth() as a matter of coincidence. This CL divorces those two for post-Refresh, because "something's gotta give". Bug: 865546 Change-Id: I464cd61f360d6ba1a53bca46a973cd0193ede9fc Reviewed-on: https://chromium-review.googlesource.com/1164394 Reviewed-by: Kevin Bailey <krb@chromium.org> Commit-Queue: Tommy Li <tommycli@chromium.org> Cr-Commit-Position: refs/heads/master@{#581258} [modify] https://crrev.com/479fc9bddd08df0ead50e953e8d5423f85223aa0/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc [modify] https://crrev.com/479fc9bddd08df0ead50e953e8d5423f85223aa0/chrome/browser/ui/views/location_bar/icon_label_bubble_view.h [modify] https://crrev.com/479fc9bddd08df0ead50e953e8d5423f85223aa0/chrome/browser/ui/views/location_bar/selected_keyword_view.cc [modify] https://crrev.com/479fc9bddd08df0ead50e953e8d5423f85223aa0/chrome/browser/ui/views/location_bar/selected_keyword_view.h
,
Aug 7
Thanks for fixing it :) I‘ll verify it in tomorrows Canary Build.
,
Aug 8
mehmet: Thank you for your attention to detail. The above two CLs are I don't think worth merging into M69. -- But M70 users will appreciate the polish.
,
Aug 8
👍 |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by jdonnelly@chromium.org
, Jul 19