Omnibox UI Experiments: Try rendering the title above the origin |
|||||
Issue description
,
May 23 2017
,
May 23 2017
+jdonnelly +emilyschechter Please take a a look at the first-pass screenshots (bottom is the After) for some feedback. I have some comments in c#1.
,
May 23 2017
Looks good to me. The difference between one-line and two-line results is not so strange if there's more vertical margin between the results, as on mobile. I assume you can combine this with your margin size experiment? Can you also test with chrome://flags/#omnibox-entity-suggestions enabled? (An excellent test query is "roosevelt".) I think you'll need to reverse the order of contents and description in this case (the entity name (e.g. Theodore Roosevelt) should be on top and the entity type (e.g. 26th U.S. President) should be on the bottom. Also, I prefer the layout from the mocks where "Google Search" is on top. You can switch up the order of these things with the logic used on mobile. See, e.g. the iOS implementation ([1] and [2]). But note that mobile omits "Google Search". One other thing: in the vertical layout case, can you switch the text color of page titles to match the mock[3]? (Except for the selected suggestion in that mock, which is backwards.) [1] https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/omnibox/omnibox_popup_material_view_controller.mm?l=286-294&rcl=f3c088d58adeab44de2d05269c5b92391712f292 [2] https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/omnibox/omnibox_popup_material_view_controller.mm?l=339-347&rcl=f3c088d58adeab44de2d05269c5b92391712f292 [3] https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZePKuXhPkpqK/files/MCFUJVnt41h6xosXkn0RHMVmwMSJbxAVNpw
,
May 23 2017
Hey Justin, Glad you like it. I've attached a screenshot of PS2 with the swapped text for non-search results. This one has the 12px vertical margin experiment also activated, so it looks a lot nicer. Thanks for the pointer to the iOS code, because that was very helpful for me to get this done expediently. Regarding the changed color - I see that the updated stylings for Titles are in a separate experiment in the PRD (Coloring/Styling). Should we leave that to a separate experiment so we don't start mixing up the two? Thanks, Tommy
,
May 23 2017
cc groby in case any opinions
,
May 23 2017
+Max Amazing! I also think that different coloring should be a separate experiment in an attempt to not mix up experiments. (though @jdonnelly, that mock link isn't working for me for some reason, could you let me know which # in the mock or attack a photo?)
,
May 25 2017
jdonnelly: BTW, I was unable to get the entity suggestions to work. If you have a chance we can get together in person at my desk and test the CL out against those. Or we can address that in a followup CL as well (we'll leave the bug open until Entity Suggest and Cocoa are both working) Thanks, Tommy
,
May 25 2017
I think the icons should be aligned with the first line of the two, rather than centered against the two lines. I'd also suggest making single-line entries just take one line, instead of centering the one line in two lines of space. Vertical scanning is improved by the spacing between the matches (compared to the first screenshot), but we still need to tweak weights/sizes/colors more. The existing ones feel like a mismash and my eyes have very little idea where to go.
,
May 25 2017
Hey pkasting: Icons are centered between two lines as specified by the mocks. I also think they look better that way -- but I'll defer to +maxwalker for the final decision. Regarding the single-line entries. I thought it made more sense to give a fixed height to all suggestions -- but I'm flexible and again I'll defer to Max. Regarding font sizes/weights/colors : I agree, we'll be tweaking them in followup. Thanks!
,
May 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/26317da136aba1d8f3c46cd8347d396894416c7d commit 26317da136aba1d8f3c46cd8347d396894416c7d Author: tommycli <tommycli@chromium.org> Date: Fri May 26 20:50:33 2017 Omnibox UI Experiments: Add Vertical Layout experiment (title-on-top). This is a minimal Views-only patch that renders the title above the origin. It doesn't change any styling yet, nor does it touch Cocoa. Those will be in followup CLs. BUG= 725599 Review-Url: https://codereview.chromium.org/2901953002 Cr-Commit-Position: refs/heads/master@{#475119} [modify] https://crrev.com/26317da136aba1d8f3c46cd8347d396894416c7d/chrome/browser/about_flags.cc [modify] https://crrev.com/26317da136aba1d8f3c46cd8347d396894416c7d/chrome/browser/flag_descriptions.cc [modify] https://crrev.com/26317da136aba1d8f3c46cd8347d396894416c7d/chrome/browser/flag_descriptions.h [modify] https://crrev.com/26317da136aba1d8f3c46cd8347d396894416c7d/chrome/browser/ui/views/omnibox/omnibox_result_view.cc [modify] https://crrev.com/26317da136aba1d8f3c46cd8347d396894416c7d/components/omnibox/browser/omnibox_field_trial.cc [modify] https://crrev.com/26317da136aba1d8f3c46cd8347d396894416c7d/components/omnibox/browser/omnibox_field_trial.h [modify] https://crrev.com/26317da136aba1d8f3c46cd8347d396894416c7d/tools/metrics/histograms/enums.xml
,
May 26 2017
Mac equivalent patch: https://codereview.chromium.org/2906893004 Screenshots:
,
May 26 2017
I'll change the title gray color in a separate CL for both Views and Mac. Trying to make minimal and easily reviewable CLs.
,
May 30 2017
For pkasting: Screenshots of how it looks like with icon aligned to first line... just for comparison purposes. In the screenshot, single-line items are also not centered...
,
May 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/81081694021693dc29c3ff9bde3da5003510a0ce commit 81081694021693dc29c3ff9bde3da5003510a0ce Author: tommycli <tommycli@chromium.org> Date: Wed May 31 20:33:18 2017 Omnibox UI Experiments: Port Vertical Layout experiment to Mac Cocoa. Ports https://codereview.chromium.org/2901953002 Omnibox vertical layout experiment to Mac Cocoa. BUG= 725599 Review-Url: https://codereview.chromium.org/2906893004 Cr-Commit-Position: refs/heads/master@{#476001} [modify] https://crrev.com/81081694021693dc29c3ff9bde3da5003510a0ce/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h [modify] https://crrev.com/81081694021693dc29c3ff9bde3da5003510a0ce/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm [modify] https://crrev.com/81081694021693dc29c3ff9bde3da5003510a0ce/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm
,
Jun 1 2017
Should be active on Mac, Linux, and Windows. Thanks all! Closing.
,
Jun 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8f997af7a224a5c15eece32f1dea70d5a3c676f3 commit 8f997af7a224a5c15eece32f1dea70d5a3c676f3 Author: tommycli <tommycli@chromium.org> Date: Wed Jun 07 18:25:36 2017 Omnibox UI: Confine Cocoa suggestion lines to a single-line. Now that there vertical margins and vertical layout is possible, we need to confine suggestion text to be rendered within a single line. This was never a problem before since the whole cell could only fit a single line, but this is necessary now. BUG= 725599 Review-Url: https://codereview.chromium.org/2915283003 Cr-Commit-Position: refs/heads/master@{#477709} [modify] https://crrev.com/8f997af7a224a5c15eece32f1dea70d5a3c676f3/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm
,
Oct 5 2017
I have a question. Is this supposed to change the color of the title when the experiment is NOT enabled from grey to black? Because that's the case for me. I have bisected it to 4 possible CLs, of which this is the only one related. https://chromium.googlesource.com/chromium/src/+log/4b97690be1738233d04011eee439dd58b17eb05b..26317da136aba1d8f3c46cd8347d396894416c7d
,
Oct 5 2017
I notice the patch has changed force_dim from true to false, so it's apparently intentional.
,
Feb 14 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3f4574176a93e7929b8d3490f1f7b4e98e5c0a0d commit 3f4574176a93e7929b8d3490f1f7b4e98e5c0a0d Author: Trent Apted <tapted@chromium.org> Date: Wed Feb 14 23:44:11 2018 Use multi-line omnibox results for touchable chrome. Bug: 801583, 725599 Change-Id: I502172cfaa8292a80f1ce239491a089b77281a83 Reviewed-on: https://chromium-review.googlesource.com/915641 Commit-Queue: Trent Apted <tapted@chromium.org> Reviewed-by: Tommy Li <tommycli@chromium.org> Cr-Commit-Position: refs/heads/master@{#536889} [modify] https://crrev.com/3f4574176a93e7929b8d3490f1f7b4e98e5c0a0d/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
,
Apr 20 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/162a1e0c71eb88f898027d4d3746558d9a771517 commit 162a1e0c71eb88f898027d4d3746558d9a771517 Author: Tommy C. Li <tommycli@chromium.org> Date: Fri Apr 20 20:24:37 2018 Omnibox UI Experiments: Remove Vertical Layout We aren't planning on implementing vertical layout in the near future, so we are removing this flag to simplify the code. Bug: 725599 Change-Id: I2074eb0d9cf99cdf315bbe5cdcd67ef796883865 Reviewed-on: https://chromium-review.googlesource.com/1020079 Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org> Reviewed-by: Dave Schuyler <dschuyler@chromium.org> Commit-Queue: Tommy Li <tommycli@chromium.org> Cr-Commit-Position: refs/heads/master@{#552451} [modify] https://crrev.com/162a1e0c71eb88f898027d4d3746558d9a771517/chrome/browser/about_flags.cc [modify] https://crrev.com/162a1e0c71eb88f898027d4d3746558d9a771517/chrome/browser/flag_descriptions.cc [modify] https://crrev.com/162a1e0c71eb88f898027d4d3746558d9a771517/chrome/browser/flag_descriptions.h [modify] https://crrev.com/162a1e0c71eb88f898027d4d3746558d9a771517/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.h [modify] https://crrev.com/162a1e0c71eb88f898027d4d3746558d9a771517/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell.mm [modify] https://crrev.com/162a1e0c71eb88f898027d4d3746558d9a771517/chrome/browser/ui/cocoa/omnibox/omnibox_popup_matrix.mm [modify] https://crrev.com/162a1e0c71eb88f898027d4d3746558d9a771517/chrome/browser/ui/views/omnibox/omnibox_result_view.cc [modify] https://crrev.com/162a1e0c71eb88f898027d4d3746558d9a771517/components/omnibox/browser/omnibox_field_trial.cc [modify] https://crrev.com/162a1e0c71eb88f898027d4d3746558d9a771517/components/omnibox/browser/omnibox_field_trial.h |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by tommycli@chromium.org
, May 23 2017141 KB
141 KB View Download
124 KB
124 KB View Download