Oops - sent too soon

The fake omnibox is 48dp
The omnibox is 56dp

What height should we be using. I feel 56dp is the correct height.

This is was is causing the slight pixel disparity on scroll:
https://screenshot.googleplex.com/Vjmi1GYgcoP

Alan, I'll defer to you on this. :) 

Context is that the omnibox and fake omnibox (selected state of the omnibox) have always been different sizes. This hasn't been a problem, but with our new transition it does feel a little strange. Not sure how you feel about it. 

You can check it out in Dev or Beta by enabling the flag for "Show content snippets on the New Tab Page". 

This is actually happening with or without the snippets enabled. Harder to test as you need the ability to scroll but if you go to landscape mode - you will see the issue. Scroll slowly

56dp for fake and real omnibox sounds fine to me. 

Unrelated: Is it possible to remove the corner radius in the real omnibox state? In the attachment, you can still see a peak of the corners, however not really noticeable in actual use. 

Deleted: Screen Shot 2016-04-20 at 12.25.10 PM.png 6.2 KB

	Screen Shot 2016-04-20 at 12.25.10 PM.png 6.2 KB View Download

Thanks Alan!

Do you mean the search box above the fold? Or the search box below the fold?

Been asked to unassigned this as on leave. It is currently an issue on live so not sure why it is marked as MVP

Thanks Nicole. Unfortunately, our changes (with the transition) make this significantly more visible. The rounded corners below the fold also remain an issue.

We'll need to clear a certain bar WRT polish for the UI review and fixing this problem will help a lot with that.

Note that the rounded corners are coming from the nine-patch file that we are using (textbox.9.png). If we want to get rid of this, we'll need a different nine-patch file (and we have to think about how to do the transition between rounded corners and no sharp corners).

Ping :)

Actually, I'm wondering whether we could just expand the omnibox a bit further horizontally, so the rounded corners wouldn't show anymore. In the top state, we only show the bottom shadow anyway.

Agree that this is a product excellence issue worth solving and I'll leave the MVP label on it because this is now visible in 90+% of NTP scrolls.

Moving this nonessential bug to the next milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

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

commit 7a676cfc5291809bf2a95fd16ba1d57907f7961a
Author: bauerb <bauerb@chromium.org>
Date: Sun Jul 31 15:23:11 2016

Use only toolbar to transition from fakebox to real omnibox.

Previously, we would fade out the fakebox and fade in the real omnibox during
the scroll transition, which created discrepancies. With this change, the
toolbar omnibox does the full transition. The search box on the NTP is still
kept, both to get the starting point of the transition, and for the tablet
toolbar, where the omnibox stays at the top, so there are two boxes.

Because the two boxes now have to match, their dimensions and padding are
slightly updated. The height is 56dp, and the padding on the sides is 12dp.

Also, in the focused state the omnibox now has 1dp bleed at the sides,
which moves the rounded corners out of view.

The remaining changes are not user-visible:
* Opacity of the fakebox is now driven by the toolbar. This lets us centralize
  the logic (the toolbar can decide its own opacity and the opacity of the
  fakebox) and use different behavior on tablets, where the fakebox will simply
  fade out as before.
* Rename variables to (hopefully) make them more consistent / self-explanatory.
* |mLocationBarBackgroundBounds| (formerly |mUrlViewportBounds|) more accurately
  reflects the visible omnibox bounds in the absence of the NTP / when the NTP
  is scrolled so that the omnibox is at the top. To do that, we no longer
  include the Y translation we apply to position it where the fakebox is (it's
  instead applied to the |mLocationBarBackgroundNtpOffset|), and we always use
  an expansion value of 1 if the current tab is an NTP.
* Remove the |inset_textbox| drawable -- it was the same as |textbox|, but with
  a margin added at the top and bottom (but *not* left or right). Since we
  calculate the omnibox bounds manually anyway, this allows us to account for
  the omnibox padding in the same way in either dimension.
* Remove the |isInTabSwitcherMode| parameter from some methods in favor of using
  the member variable.

BUG= 605054 , 625108 , 618955 ,616728,612520

Review-Url: https://codereview.chromium.org/2134663002
Cr-Commit-Position: refs/heads/master@{#408885}

[delete] https://crrev.com/78c093f7cae68751ea9321f2d5c526cfdea62a5a/chrome/android/java/res/drawable/inset_textbox.xml
[modify] https://crrev.com/7a676cfc5291809bf2a95fd16ba1d57907f7961a/chrome/android/java/res/layout/new_tab_page_layout.xml
[modify] https://crrev.com/7a676cfc5291809bf2a95fd16ba1d57907f7961a/chrome/android/java/res/values/dimens.xml
[modify] https://crrev.com/7a676cfc5291809bf2a95fd16ba1d57907f7961a/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
[modify] https://crrev.com/7a676cfc5291809bf2a95fd16ba1d57907f7961a/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
[modify] https://crrev.com/7a676cfc5291809bf2a95fd16ba1d57907f7961a/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java
[modify] https://crrev.com/7a676cfc5291809bf2a95fd16ba1d57907f7961a/chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarPhone.java
[modify] https://crrev.com/7a676cfc5291809bf2a95fd16ba1d57907f7961a/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java
[modify] https://crrev.com/7a676cfc5291809bf2a95fd16ba1d57907f7961a/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarTablet.java

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

commit ed720fdd749d46e0934267bd8f935bbeb1ec0991
Author: gab <gab@chromium.org>
Date: Mon Aug 01 20:35:41 2016

Revert of Use only toolbar to transition from fakebox to real omnibox. (patchset #11 id:200001 of https://codereview.chromium.org/2134663002/ )

Reason for revert:
Suspect for  http://crbug.com/633321 , will re-land if not.

Original issue's description:
> Use only toolbar to transition from fakebox to real omnibox.
>
> Previously, we would fade out the fakebox and fade in the real omnibox during
> the scroll transition, which created discrepancies. With this change, the
> toolbar omnibox does the full transition. The search box on the NTP is still
> kept, both to get the starting point of the transition, and for the tablet
> toolbar, where the omnibox stays at the top, so there are two boxes.
>
> Because the two boxes now have to match, their dimensions and padding are
> slightly updated. The height is 56dp, and the padding on the sides is 12dp.
>
> Also, in the focused state the omnibox now has 1dp bleed at the sides,
> which moves the rounded corners out of view.
>
> The remaining changes are not user-visible:
> * Opacity of the fakebox is now driven by the toolbar. This lets us centralize
>   the logic (the toolbar can decide its own opacity and the opacity of the
>   fakebox) and use different behavior on tablets, where the fakebox will simply
>   fade out as before.
> * Rename variables to (hopefully) make them more consistent / self-explanatory.
> * |mLocationBarBackgroundBounds| (formerly |mUrlViewportBounds|) more accurately
>   reflects the visible omnibox bounds in the absence of the NTP / when the NTP
>   is scrolled so that the omnibox is at the top. To do that, we no longer
>   include the Y translation we apply to position it where the fakebox is (it's
>   instead applied to the |mLocationBarBackgroundNtpOffset|), and we always use
>   an expansion value of 1 if the current tab is an NTP.
> * Remove the |inset_textbox| drawable -- it was the same as |textbox|, but with
>   a margin added at the top and bottom (but *not* left or right). Since we
>   calculate the omnibox bounds manually anyway, this allows us to account for
>   the omnibox padding in the same way in either dimension.
> * Remove the |isInTabSwitcherMode| parameter from some methods in favor of using
>   the member variable.
>
> BUG= 605054 , 625108 , 618955 ,616728,612520
>
> Committed: https://crrev.com/7a676cfc5291809bf2a95fd16ba1d57907f7961a
> Cr-Commit-Position: refs/heads/master@{#408885}

TBR=peconn@chromium.org,mcwilliams@chromium.org,mdjones@chromium.org,tedchoc@chromium.org,bauerb@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 605054 , 625108 , 618955 ,616728,612520

Review-Url: https://codereview.chromium.org/2198993002
Cr-Commit-Position: refs/heads/master@{#409045}

[add] https://crrev.com/ed720fdd749d46e0934267bd8f935bbeb1ec0991/chrome/android/java/res/drawable/inset_textbox.xml
[modify] https://crrev.com/ed720fdd749d46e0934267bd8f935bbeb1ec0991/chrome/android/java/res/layout/new_tab_page_layout.xml
[modify] https://crrev.com/ed720fdd749d46e0934267bd8f935bbeb1ec0991/chrome/android/java/res/values/dimens.xml
[modify] https://crrev.com/ed720fdd749d46e0934267bd8f935bbeb1ec0991/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
[modify] https://crrev.com/ed720fdd749d46e0934267bd8f935bbeb1ec0991/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
[modify] https://crrev.com/ed720fdd749d46e0934267bd8f935bbeb1ec0991/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java
[modify] https://crrev.com/ed720fdd749d46e0934267bd8f935bbeb1ec0991/chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarPhone.java
[modify] https://crrev.com/ed720fdd749d46e0934267bd8f935bbeb1ec0991/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java
[modify] https://crrev.com/ed720fdd749d46e0934267bd8f935bbeb1ec0991/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarTablet.java

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

commit 62c1818efd6793549a21f744746a94b1a55dd2c1
Author: mathp <mathp@chromium.org>
Date: Mon Aug 01 21:41:23 2016

Reland of Use only toolbar to transition from fakebox to real omnibox. (patchset #1 id:1 of https://codereview.chromium.org/2198993002/ )

Reason for revert:
Was not the culprit for webkit_unit_tests failing.. relanding

Original issue's description:
> Revert of Use only toolbar to transition from fakebox to real omnibox. (patchset #11 id:200001 of https://codereview.chromium.org/2134663002/ )
>
> Reason for revert:
> Suspect for  http://crbug.com/633321 , will re-land if not.
>
> Original issue's description:
> > Use only toolbar to transition from fakebox to real omnibox.
> >
> > Previously, we would fade out the fakebox and fade in the real omnibox during
> > the scroll transition, which created discrepancies. With this change, the
> > toolbar omnibox does the full transition. The search box on the NTP is still
> > kept, both to get the starting point of the transition, and for the tablet
> > toolbar, where the omnibox stays at the top, so there are two boxes.
> >
> > Because the two boxes now have to match, their dimensions and padding are
> > slightly updated. The height is 56dp, and the padding on the sides is 12dp.
> >
> > Also, in the focused state the omnibox now has 1dp bleed at the sides,
> > which moves the rounded corners out of view.
> >
> > The remaining changes are not user-visible:
> > * Opacity of the fakebox is now driven by the toolbar. This lets us centralize
> >   the logic (the toolbar can decide its own opacity and the opacity of the
> >   fakebox) and use different behavior on tablets, where the fakebox will simply
> >   fade out as before.
> > * Rename variables to (hopefully) make them more consistent / self-explanatory.
> > * |mLocationBarBackgroundBounds| (formerly |mUrlViewportBounds|) more accurately
> >   reflects the visible omnibox bounds in the absence of the NTP / when the NTP
> >   is scrolled so that the omnibox is at the top. To do that, we no longer
> >   include the Y translation we apply to position it where the fakebox is (it's
> >   instead applied to the |mLocationBarBackgroundNtpOffset|), and we always use
> >   an expansion value of 1 if the current tab is an NTP.
> > * Remove the |inset_textbox| drawable -- it was the same as |textbox|, but with
> >   a margin added at the top and bottom (but *not* left or right). Since we
> >   calculate the omnibox bounds manually anyway, this allows us to account for
> >   the omnibox padding in the same way in either dimension.
> > * Remove the |isInTabSwitcherMode| parameter from some methods in favor of using
> >   the member variable.
> >
> > BUG= 605054 , 625108 , 618955 ,616728,612520
> >
> > Committed: https://crrev.com/7a676cfc5291809bf2a95fd16ba1d57907f7961a
> > Cr-Commit-Position: refs/heads/master@{#408885}
>
> TBR=peconn@chromium.org,mcwilliams@chromium.org,mdjones@chromium.org,tedchoc@chromium.org,bauerb@chromium.org
> # Not skipping CQ checks because original CL landed more than 1 days ago.
> BUG= 605054 , 625108 , 618955 ,616728,612520
>
> Committed: https://crrev.com/ed720fdd749d46e0934267bd8f935bbeb1ec0991
> Cr-Commit-Position: refs/heads/master@{#409045}

TBR=peconn@chromium.org,mcwilliams@chromium.org,mdjones@chromium.org,tedchoc@chromium.org,bauerb@chromium.org,gab@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 605054 , 625108 , 618955 ,616728,612520

Review-Url: https://codereview.chromium.org/2204613002
Cr-Commit-Position: refs/heads/master@{#409064}

[delete] https://crrev.com/bd828670eaef26966373521fa23624d5b7b3ddd6/chrome/android/java/res/drawable/inset_textbox.xml
[modify] https://crrev.com/62c1818efd6793549a21f744746a94b1a55dd2c1/chrome/android/java/res/layout/new_tab_page_layout.xml
[modify] https://crrev.com/62c1818efd6793549a21f744746a94b1a55dd2c1/chrome/android/java/res/values/dimens.xml
[modify] https://crrev.com/62c1818efd6793549a21f744746a94b1a55dd2c1/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
[modify] https://crrev.com/62c1818efd6793549a21f744746a94b1a55dd2c1/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
[modify] https://crrev.com/62c1818efd6793549a21f744746a94b1a55dd2c1/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java
[modify] https://crrev.com/62c1818efd6793549a21f744746a94b1a55dd2c1/chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarPhone.java
[modify] https://crrev.com/62c1818efd6793549a21f744746a94b1a55dd2c1/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java
[modify] https://crrev.com/62c1818efd6793549a21f744746a94b1a55dd2c1/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarTablet.java