New issue
Advanced search Search tips

Issue 849779 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocking:
issue 823535



Sign in to add a comment

Alignment - Suggestions and omnibox URL

Project Member Reported by emilyschechter@chromium.org, Jun 5 2018

Issue description

We should align the text in the suggestions and the omnibox URL. See attached
 
omnialign.jpeg
54.4 KB View Download
Cc: -tommycli@chromium.org jdonnelly@chromium.org
Owner: tommycli@chromium.org
Status: Assigned (was: Untriaged)
I believe tommycli is planning to work on this (correct me if I'm wrong).
Project Member

Comment 2 by bugdroid1@chromium.org, Jun 8 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1e07aadc746e5d902fdd314fbb2ba0f692a58e43

commit 1e07aadc746e5d902fdd314fbb2ba0f692a58e43
Author: Tommy C. Li <tommycli@chromium.org>
Date: Fri Jun 08 20:09:02 2018

Omnibox Refresh: Update Icons to 32x24 rounded rect shape

Previously they were 24x24. But in the new Material Refresh spec, they
take up 32x24.

Bug:  849779 ,  823535 
Change-Id: Ie1f5dfe9e9f2ae88bcf07643fb6b33f6472bdcdf
Reviewed-on: https://chromium-review.googlesource.com/1090073
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565723}
[modify] https://crrev.com/1e07aadc746e5d902fdd314fbb2ba0f692a58e43/chrome/browser/ui/layout_constants.cc
[modify] https://crrev.com/1e07aadc746e5d902fdd314fbb2ba0f692a58e43/chrome/browser/ui/layout_constants.h
[modify] https://crrev.com/1e07aadc746e5d902fdd314fbb2ba0f692a58e43/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc
[modify] https://crrev.com/1e07aadc746e5d902fdd314fbb2ba0f692a58e43/chrome/browser/ui/views/location_bar/icon_label_bubble_view_unittest.cc
[modify] https://crrev.com/1e07aadc746e5d902fdd314fbb2ba0f692a58e43/chrome/browser/ui/views/location_bar/keyword_hint_view.cc
[modify] https://crrev.com/1e07aadc746e5d902fdd314fbb2ba0f692a58e43/chrome/browser/ui/views/location_bar/location_bar_view.cc
[modify] https://crrev.com/1e07aadc746e5d902fdd314fbb2ba0f692a58e43/chrome/browser/ui/views/omnibox/omnibox_match_cell_view.cc
[modify] https://crrev.com/1e07aadc746e5d902fdd314fbb2ba0f692a58e43/chrome/browser/ui/views/page_action/page_action_icon_view.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Jun 13 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0830cdf01de84c9af5f575add7b511beeba301a2

commit 0830cdf01de84c9af5f575add7b511beeba301a2
Author: Tommy C. Li <tommycli@chromium.org>
Date: Wed Jun 13 15:12:44 2018

Omnibox UI Refresh: Align Omnibox text to new Suggestion text

The goal of the CL is to re-align the Omnibox text to match the new
Refresh spec.

In the new Refresh spec, the text has a different indentation depending
on whether or not the popup is open, so this CL resets the insets
of the OmniboxViewViews whenever the popup opens or closes.

It also updates LocationBarView to no longer ignore the left-inset
value of the OmniboxViewViews. We used to also ignore the right-inset
value, but we stopped doing that recently [1].

After no longer ignoring the left-inset value, we update the
non-Refresh value to the correct value of 0 (to keep things looking
the same), and update the Refresh value to 2, which matches mocks.

We are updating the inset of the OmniboxViewViews textfield instead of
the bounds of the textfield because we want the whole area to remain a
clickable I-beam, even if we indent the text a bit.

[1] https://codereview.chromium.org/2642893002/diff/100001/chrome/browser/ui/views/location_bar/location_bar_view.cc

Bug:  849779 ,  823535 
Change-Id: I8c2a9e4934a78efec0d3d6cd60c63181be2d92c4
Reviewed-on: https://chromium-review.googlesource.com/1096491
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566834}
[modify] https://crrev.com/0830cdf01de84c9af5f575add7b511beeba301a2/chrome/browser/ui/views/harmony/chrome_layout_provider.cc
[modify] https://crrev.com/0830cdf01de84c9af5f575add7b511beeba301a2/chrome/browser/ui/views/location_bar/location_bar_view.cc
[modify] https://crrev.com/0830cdf01de84c9af5f575add7b511beeba301a2/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
[modify] https://crrev.com/0830cdf01de84c9af5f575add7b511beeba301a2/chrome/browser/ui/views/omnibox/omnibox_view_views.h

Status: Fixed (was: Assigned)
Should be lined up now after last CL.
Project Member

Comment 5 by bugdroid1@chromium.org, Jun 27 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fe55ead60654aebd0f12bd53f0f6785e1c3741a7

commit fe55ead60654aebd0f12bd53f0f6785e1c3741a7
Author: Tommy C. Li <tommycli@chromium.org>
Date: Wed Jun 27 18:27:54 2018

Revert "Omnibox UI Refresh: Align Omnibox text to new Suggestion text"

This reverts commit 0830cdf01de84c9af5f575add7b511beeba301a2.

Reason for revert: UX direction change.

Original change's description:
> Omnibox UI Refresh: Align Omnibox text to new Suggestion text
> 
> The goal of the CL is to re-align the Omnibox text to match the new
> Refresh spec.
> 
> In the new Refresh spec, the text has a different indentation depending
> on whether or not the popup is open, so this CL resets the insets
> of the OmniboxViewViews whenever the popup opens or closes.
> 
> It also updates LocationBarView to no longer ignore the left-inset
> value of the OmniboxViewViews. We used to also ignore the right-inset
> value, but we stopped doing that recently [1].
> 
> After no longer ignoring the left-inset value, we update the
> non-Refresh value to the correct value of 0 (to keep things looking
> the same), and update the Refresh value to 2, which matches mocks.
> 
> We are updating the inset of the OmniboxViewViews textfield instead of
> the bounds of the textfield because we want the whole area to remain a
> clickable I-beam, even if we indent the text a bit.
> 
> [1] https://codereview.chromium.org/2642893002/diff/100001/chrome/browser/ui/views/location_bar/location_bar_view.cc
> 
> Bug:  849779 ,  823535 
> Change-Id: I8c2a9e4934a78efec0d3d6cd60c63181be2d92c4
> Reviewed-on: https://chromium-review.googlesource.com/1096491
> Reviewed-by: Scott Violet <sky@chromium.org>
> Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
> Commit-Queue: Tommy Li <tommycli@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#566834}

TBR=sky@chromium.org,tommycli@chromium.org,jdonnelly@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  849779 ,  823535 
Change-Id: Icf9e2f004042be6f25b9a86d23f269f5ebf0fd90
Reviewed-on: https://chromium-review.googlesource.com/1117218
Commit-Queue: Tommy Li <tommycli@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570847}
[modify] https://crrev.com/fe55ead60654aebd0f12bd53f0f6785e1c3741a7/chrome/browser/ui/views/harmony/chrome_layout_provider.cc
[modify] https://crrev.com/fe55ead60654aebd0f12bd53f0f6785e1c3741a7/chrome/browser/ui/views/location_bar/location_bar_view.cc
[modify] https://crrev.com/fe55ead60654aebd0f12bd53f0f6785e1c3741a7/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
[modify] https://crrev.com/fe55ead60654aebd0f12bd53f0f6785e1c3741a7/chrome/browser/ui/views/omnibox/omnibox_view_views.h

Project Member

Comment 6 by bugdroid1@chromium.org, Jun 28 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b815244a60fd4419607e50a3725ade3b53b1b480

commit b815244a60fd4419607e50a3725ade3b53b1b480
Author: Tommy Li <tommycli@chromium.org>
Date: Thu Jun 28 17:04:33 2018

Reland "Omnibox UI Refresh: Align Omnibox text to new Suggestion text"

This reverts commit fe55ead60654aebd0f12bd53f0f6785e1c3741a7.

Reason for revert: UX direction change.

Original change's description:
> Revert "Omnibox UI Refresh: Align Omnibox text to new Suggestion text"
> 
> This reverts commit 0830cdf01de84c9af5f575add7b511beeba301a2.
> 
> Reason for revert: UX direction change.
> 
> Original change's description:
> > Omnibox UI Refresh: Align Omnibox text to new Suggestion text
> > 
> > The goal of the CL is to re-align the Omnibox text to match the new
> > Refresh spec.
> > 
> > In the new Refresh spec, the text has a different indentation depending
> > on whether or not the popup is open, so this CL resets the insets
> > of the OmniboxViewViews whenever the popup opens or closes.
> > 
> > It also updates LocationBarView to no longer ignore the left-inset
> > value of the OmniboxViewViews. We used to also ignore the right-inset
> > value, but we stopped doing that recently [1].
> > 
> > After no longer ignoring the left-inset value, we update the
> > non-Refresh value to the correct value of 0 (to keep things looking
> > the same), and update the Refresh value to 2, which matches mocks.
> > 
> > We are updating the inset of the OmniboxViewViews textfield instead of
> > the bounds of the textfield because we want the whole area to remain a
> > clickable I-beam, even if we indent the text a bit.
> > 
> > [1] https://codereview.chromium.org/2642893002/diff/100001/chrome/browser/ui/views/location_bar/location_bar_view.cc
> > 
> > Bug:  849779 ,  823535 
> > Change-Id: I8c2a9e4934a78efec0d3d6cd60c63181be2d92c4
> > Reviewed-on: https://chromium-review.googlesource.com/1096491
> > Reviewed-by: Scott Violet <sky@chromium.org>
> > Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
> > Commit-Queue: Tommy Li <tommycli@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#566834}
> 
> TBR=sky@chromium.org,tommycli@chromium.org,jdonnelly@chromium.org
> 
> # Not skipping CQ checks because original CL landed > 1 day ago.
> 
> Bug:  849779 ,  823535 
> Change-Id: Icf9e2f004042be6f25b9a86d23f269f5ebf0fd90
> Reviewed-on: https://chromium-review.googlesource.com/1117218
> Commit-Queue: Tommy Li <tommycli@chromium.org>
> Reviewed-by: Tommy Li <tommycli@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#570847}

TBR=sky@chromium.org,tommycli@chromium.org,jdonnelly@chromium.org

Change-Id: Ib5afb6cc4e892594e3cfdd0c775ae6ef1e4f862f
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  849779 ,  823535 
Reviewed-on: https://chromium-review.googlesource.com/1118785
Reviewed-by: Tommy Li <tommycli@chromium.org>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571168}
[modify] https://crrev.com/b815244a60fd4419607e50a3725ade3b53b1b480/chrome/browser/ui/views/harmony/chrome_layout_provider.cc
[modify] https://crrev.com/b815244a60fd4419607e50a3725ade3b53b1b480/chrome/browser/ui/views/location_bar/location_bar_view.cc
[modify] https://crrev.com/b815244a60fd4419607e50a3725ade3b53b1b480/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
[modify] https://crrev.com/b815244a60fd4419607e50a3725ade3b53b1b480/chrome/browser/ui/views/omnibox/omnibox_view_views.h

Sign in to add a comment