Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2134663002:
Use only toolbar to transition from fakebox to real omnibox. (Closed)
|
Created:
4 years, 5 months ago by Bernhard Bauer Modified:
4 years, 4 months ago CC:
chromium-reviews, ntp-dev+reviews_chromium.org, Ted C Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse 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}
Patch Set 1 #
Total comments: 4
Patch Set 2 : review #Patch Set 3 : sync #Patch Set 4 : fixes #
Total comments: 20
Patch Set 5 : review #
Total comments: 3
Patch Set 6 : fix #
Total comments: 35
Patch Set 7 : review #Patch Set 8 : review #Patch Set 9 : review #Patch Set 10 : review #
Total comments: 4
Patch Set 11 : review #Messages
Total messages: 49 (27 generated)
Description was changed from ========== 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 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. * Variables are renamed to (hopefully) make them more consistent and self-explanatory. BUG=605054,625108,618955,616728,612520 ========== to ========== 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 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. * Variables are renamed to (hopefully) make them more consistent / self-explanatory. * Remove the isInTabSwitcherMode from some methods in favor of using the member variable. BUG=605054,625108,618955,616728,612520 ==========
Description was changed from ========== 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 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. * Variables are renamed to (hopefully) make them more consistent / self-explanatory. * Remove the isInTabSwitcherMode from some methods in favor of using the member variable. BUG=605054,625108,618955,616728,612520 ========== to ========== 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. * Variables are renamed 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 isInTabSwitcherMode from some methods in favor of using the member variable. BUG=605054,625108,618955,616728,612520 ==========
Description was changed from ========== 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. * Variables are renamed 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 isInTabSwitcherMode from some methods in favor of using the member variable. BUG=605054,625108,618955,616728,612520 ========== to ========== 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. * Variables are renamed 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 |isInTabSwitcherMode| parameter from some methods in favor of using the member variable. BUG=605054,625108,618955,616728,612520 ==========
peconn@chromium.org changed reviewers: + peconn@chromium.org
Two early nits - I'm still making my way through the body of the actual changes. https://codereview.chromium.org/2134663002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2134663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:418: if (getWrapperView().getHeight() == 0) return 0; Use 0f and 1f here instead of 0 and 1? https://codereview.chromium.org/2134663002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2134663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:295: Log.e(TAG, "searchBoxTransitionLength: %d top: %d parentHeight: %d parentScrollY: %d", Was this left in by mistake?
https://codereview.chromium.org/2134663002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2134663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:418: if (getWrapperView().getHeight() == 0) return 0; On 2016/07/08 12:53:29, PEConn1 wrote: > Use 0f and 1f here instead of 0 and 1? I used 0f as the return value, but kept the 0 in the comparison, because the height is an int. https://codereview.chromium.org/2134663002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2134663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:295: Log.e(TAG, "searchBoxTransitionLength: %d top: %d parentHeight: %d parentScrollY: %d", On 2016/07/08 12:53:29, PEConn1 wrote: > Was this left in by mistake? Yes, I had missed this file in my first cleanup pass.
Description was changed from ========== 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. * Variables are renamed 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 |isInTabSwitcherMode| parameter from some methods in favor of using the member variable. BUG=605054,625108,618955,616728,612520 ========== to ========== 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. * Variables are renamed 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 ==========
Description was changed from ========== 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. * Variables are renamed 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 ========== to ========== 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 ==========
bauerb@chromium.org changed reviewers: + mcwilliams@chromium.org
I think this is now ready for a full review (I'll add toolbar OWNERS later). PTAL, thanks!
I know you have spend a fair amount of time understanding what the code is now doing - can we add more doc so it makes it easier for others at a glance https://codereview.chromium.org/2134663002/diff/60001/chrome/android/java/res... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/2134663002/diff/60001/chrome/android/java/res... chrome/android/java/res/values/dimens.xml:228: <dimen name="location_bar_vertical_margin">8dp</dimen> Group this with the other location bar dimensions https://codereview.chromium.org/2134663002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2134663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:398: Add java doc to function as to how it is getting this value https://codereview.chromium.org/2134663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:403: Add a doc for this - When search box is a the top, ... https://codereview.chromium.org/2134663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:417: // visible. Luckily, if the first item is not visible, we know the toolbar Remove 'Luckily' from doc :) https://codereview.chromium.org/2134663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:422: final int scrollY = getVerticalScroll(); Is this final? https://codereview.chromium.org/2134663002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java (right): https://codereview.chromium.org/2134663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:102: private LocationBarPhone mLocationBar; Possibly add a doc as to what the location bar is? https://codereview.chromium.org/2134663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:300: mLocationBarBackgroundCornerRadius = Do we need Background in the name here? https://codereview.chromium.org/2134663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:802: Can we add some docs to this function https://codereview.chromium.org/2134663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:874: Documentation https://codereview.chromium.org/2134663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:895: Documentation
https://codereview.chromium.org/2134663002/diff/60001/chrome/android/java/res... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/2134663002/diff/60001/chrome/android/java/res... chrome/android/java/res/values/dimens.xml:228: <dimen name="location_bar_vertical_margin">8dp</dimen> On 2016/07/12 16:20:47, mcwilliams wrote: > Group this with the other location bar dimensions Done (I've kept |toolbar_edge_padding| in the group, as it affects the positioning of the location bar as well). https://codereview.chromium.org/2134663002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2134663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:398: On 2016/07/12 16:20:47, mcwilliams wrote: > Add java doc to function as to how it is getting this value Done. https://codereview.chromium.org/2134663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:403: On 2016/07/12 16:20:47, mcwilliams wrote: > Add a doc for this - When search box is a the top, ... I've expanded the comment above, as it refers to this as well. https://codereview.chromium.org/2134663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:417: // visible. Luckily, if the first item is not visible, we know the toolbar On 2016/07/12 16:20:47, mcwilliams wrote: > Remove 'Luckily' from doc :) Done. https://codereview.chromium.org/2134663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:422: final int scrollY = getVerticalScroll(); On 2016/07/12 16:20:47, mcwilliams wrote: > Is this final? I don't understand the question... If you mean, does this have to be final: it does not, but it also doesn't hurt to make it final, and that was the old code. https://codereview.chromium.org/2134663002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java (right): https://codereview.chromium.org/2134663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:102: private LocationBarPhone mLocationBar; On 2016/07/12 16:20:47, mcwilliams wrote: > Possibly add a doc as to what the location bar is? Done. https://codereview.chromium.org/2134663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:300: mLocationBarBackgroundCornerRadius = On 2016/07/12 16:20:47, mcwilliams wrote: > Do we need Background in the name here? I've tried to name all members that are relevant for the location bar background in a similar way, and the rounded corners are an aspect of the background drawable (as in, they're purely visual and don't affect the contents of the location bar). https://codereview.chromium.org/2134663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:802: On 2016/07/12 16:20:48, mcwilliams wrote: > Can we add some docs to this function Done. https://codereview.chromium.org/2134663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:874: On 2016/07/12 16:20:47, mcwilliams wrote: > Documentation Done. https://codereview.chromium.org/2134663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:895: On 2016/07/12 16:20:48, mcwilliams wrote: > Documentation Done.
lgtm https://codereview.chromium.org/2134663002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java (right): https://codereview.chromium.org/2134663002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:105: private LocationBarPhone mLocationBar; Playing devils advocate - if this is the omnibox - why dont we call it that?
lgtm
bauerb@chromium.org changed reviewers: + mdjones@chromium.org
Matthew, could you take a look at the toolbar changes? Thanks! Also, +ted FHI once he comes back.
I pulled this patch to play with it; it looks like the compositor version of the location bar is now incorrect (try scrolling the bar off-screen).
The CQ bit was checked by bauerb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/07/14 21:19:17, mdjones wrote: > I pulled this patch to play with it; it looks like the compositor version of the > location bar is now incorrect (try scrolling the bar off-screen). Good catch! Fixed in the new patch set. https://codereview.chromium.org/2134663002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java (right): https://codereview.chromium.org/2134663002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:105: private LocationBarPhone mLocationBar; On 2016/07/13 13:57:54, mcwilliams wrote: > Playing devils advocate - if this is the omnibox - why dont we call it that? Good question! :) In this CL I went with internal consistency, i.e. using the term "location bar" everywhere, because that was already being used. if you think we should rename it and owners agree, I could do that in a followup?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
tedchoc@chromium.org changed reviewers: + tedchoc@chromium.org
https://codereview.chromium.org/2134663002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java (right): https://codereview.chromium.org/2134663002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:105: private LocationBarPhone mLocationBar; On 2016/07/19 15:59:43, Bernhard Bauer wrote: > On 2016/07/13 13:57:54, mcwilliams wrote: > > Playing devils advocate - if this is the omnibox - why dont we call it that? > > Good question! :) In this CL I went with internal consistency, i.e. using the > term "location bar" everywhere, because that was already being used. if you > think we should rename it and owners agree, I could do that in a followup? FWIW, LocationBar is something that exists on desktop, so I don't think this is terribly out of line with that. There is an omnibox_view on desktop, but I'm not sure if that corresponds to UrlBar more so than LocationBar.*.java in Android. https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/re... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/re... chrome/android/java/res/values/dimens.xml:232: <dimen name="location_bar_icon_width">32dp</dimen> this is used in places like custom tabs, does this change have no visual change on the resting state of both the chrome omnibox as well as the CCT toolbar? https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:661: * to the NewTabPage view. align to "The translation ..." above https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:700: * to the NewTabPage view. same alignment nit https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarPhone.java (left): https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarPhone.java:176: mMicButton.setAlpha(percent); Hmm...this changes the behavior if you're focusing the omnibox and not just the NTP. What happens if you're on something like the recent tabs page where the is no text in the omnibox so the mic icon will be present during the transition. maybe the mic is hidden enough that this has no effect, but it seems odd that we'd be animating the alpha of the delete button and not the mic when one or the other is shown in the same place interchangeably https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java (right): https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:103: * The location bar (aka omnibox). I honestly don't think this comment is useful. we shouldn't need to comment the type of objects. the documentation on the object itself should be sufficient https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:336: mLocationBar.setPadding(mLocationBarBackgroundPadding.left, nit of nits: I think the two per line style that was there previously is nicer to look at than this https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:758: * {@code rect}. the @code isn't needed. the param name is out and not rect https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:763: float expansion = visualState == VisualState.NEW_TAB_NORMAL ? 1 : mUrlExpansionPercent; why the change here? what was wrong about using mUrlExpansionPercent here instead of snapping to 1 for NEW_TAB_NORMAL? https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:771: out.set((int) MathUtils.interpolate(getViewBoundsLeftOfLocationBar(visualState), yikes...this is getting pretty hard to read. let's pull out this to separate local variables https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:886: mLocationBarBackgroundAlpha = this is also getting a bit unwieldy, can we do: mLocationBarBackgroundAlpha = 255; if (isIncognito() || (....)) { mLocationBarBackgroundAlpha = LOCATION_BAR_TRANSPARENT_BACKGROUND_ALPHA; } https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:1140: : mLocationBarBackgroundBounds.left indenting should be 8 from the start of the previous line https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:1145: + mLocationBarBackgroundPadding.right; same indenting nit https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:1180: mLocationBarBackground.setBounds(mLocationBarBackgroundBounds.left i'd move this down on the next line to make it match the other params lines https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:1480: finishAnimations(); previously, finishAnimations was using the previous value of mIsInTabSwitcherMode. Now you are setting it earlier, was it a bug previously and this fixes that? https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:1492: updateUrlExpansionAnimation(); Doesn't this in the bowels use mIsInTabSwitcherMode as well...want to make sure this doesn't have more side effects https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:2109: mLocationBar.setPadding(mLocationBarBackgroundPadding.left, same formatting comment
The CQ bit was checked by bauerb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by bauerb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the detailed review! https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/re... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/re... chrome/android/java/res/values/dimens.xml:232: <dimen name="location_bar_icon_width">32dp</dimen> On 2016/07/19 19:11:31, Ted C wrote: > this is used in places like custom tabs, does this change have no visual change > on the resting state of both the chrome omnibox as well as the CCT toolbar? The CCT toolbar is unchanged. The padding for a regular tab is increased by 2dp, but that's intended to make the padding 12dp everywhere. I've uploaded some screenshots at https://imgur.com/a/uJglP. https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:661: * to the NewTabPage view. On 2016/07/19 19:11:31, Ted C wrote: > align to "The translation ..." above Done. https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:700: * to the NewTabPage view. On 2016/07/19 19:11:31, Ted C wrote: > same alignment nit Done. Sorry, I think Eclipse does that automatically. https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarPhone.java (left): https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarPhone.java:176: mMicButton.setAlpha(percent); On 2016/07/19 19:11:31, Ted C wrote: > Hmm...this changes the behavior if you're focusing the omnibox and not just the > NTP. What happens if you're on something like the recent tabs page where the is > no text in the omnibox so the mic icon will be present during the transition. > > maybe the mic is hidden enough that this has no effect, but it seems odd that > we'd be animating the alpha of the delete button and not the mic when one or the > other is shown in the same place interchangeably Well, this will keep the mic icon always visible in this case, so it will appear behind the toolbar buttons when they slide out of view. I haven't been able to see a difference to the behavior on trunk, probably because at the point when the mic icon is uncovered the transition is already so far along that the alpha value is almost 1 anyway. If you want consistency with the delete button, the easiest thing to do would be to remove that call as well :) https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java (right): https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:103: * The location bar (aka omnibox). On 2016/07/19 19:11:32, Ted C wrote: > I honestly don't think this comment is useful. we shouldn't need to comment the > type of objects. the documentation on the object itself should be sufficient Removed. (I added this comment mostly to point out to a reader who might be new to the code but familiar with Chrome that the location bar is the same thing as the omnibox, because the latter term is more user-visible.) https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:336: mLocationBar.setPadding(mLocationBarBackgroundPadding.left, On 2016/07/19 19:11:32, Ted C wrote: > nit of nits: I think the two per line style that was there previously is nicer > to look at than this Done. https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:763: float expansion = visualState == VisualState.NEW_TAB_NORMAL ? 1 : mUrlExpansionPercent; On 2016/07/19 19:11:32, Ted C wrote: > why the change here? what was wrong about using mUrlExpansionPercent here > instead of snapping to 1 for NEW_TAB_NORMAL? mUrlExpansionPercent is the maximum of mUrlFocusChangePercent and mNtpSearchBoxScrollPercent, so while scrolling the NTP, this would interpolate linearly between the unfocused omnibox state and the expanded state, based on the scrolling position. This is wrong for several reasons: The unfocused omnibox state is never actually seen on the NTP, so the starting value would be wrong, and the omnibox expansion while scrolling is not fully linear (it would only start at 40% for the old UI, and for the new UI we want a fast out, slow in transition), so this would end up with completely wrong values. We would then compensate for that in updateNtpTransitionAnimation(), but only for the horizontal bounds. This is why you can see a very faint double line when scrolling on the NTP on trunk: The fakebox and the omnibox are diverging slightly in height. https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:771: out.set((int) MathUtils.interpolate(getViewBoundsLeftOfLocationBar(visualState), On 2016/07/19 19:11:31, Ted C wrote: > yikes...this is getting pretty hard to read. let's pull out this to separate > local variables Done. https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:886: mLocationBarBackgroundAlpha = On 2016/07/19 19:11:32, Ted C wrote: > this is also getting a bit unwieldy, can we do: > > mLocationBarBackgroundAlpha = 255; > if (isIncognito() || (....)) { > mLocationBarBackgroundAlpha = LOCATION_BAR_TRANSPARENT_BACKGROUND_ALPHA; > } Done. https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:1140: : mLocationBarBackgroundBounds.left On 2016/07/19 19:11:32, Ted C wrote: > indenting should be 8 from the start of the previous line Done. https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:1145: + mLocationBarBackgroundPadding.right; On 2016/07/19 19:11:32, Ted C wrote: > same indenting nit Done. https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:1180: mLocationBarBackground.setBounds(mLocationBarBackgroundBounds.left On 2016/07/19 19:11:32, Ted C wrote: > i'd move this down on the next line to make it match the other params lines Done. https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:1480: finishAnimations(); On 2016/07/19 19:11:32, Ted C wrote: > previously, finishAnimations was using the previous value of > mIsInTabSwitcherMode. Now you are setting it earlier, was it a bug previously > and this fixes that? I didn't see a bug, I just changed it to avoid cases where we implicitly relied on using the old value. In this particular method, only updateProgressBarVisibility() (called from within finishAnimations()) uses mIsInTabSwitcherMode, and using the new value seems correct there. https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:1492: updateUrlExpansionAnimation(); On 2016/07/19 19:11:32, Ted C wrote: > Doesn't this in the bowels use mIsInTabSwitcherMode as well...want to make sure > this doesn't have more side effects I have to admit don't remember exactly, but I think that fixed a bug (at least with this CL) -- updateUrlExpansionAnimation() will return early if we're in tab switcher mode, and I ran into some problems where state wasn't updated properly. https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:2109: mLocationBar.setPadding(mLocationBarBackgroundPadding.left, On 2016/07/19 19:11:32, Ted C wrote: > same formatting comment Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm (with a bit of hesitation, but can't blame you for logic that isn't easy explainable) https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarPhone.java (left): https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarPhone.java:176: mMicButton.setAlpha(percent); On 2016/07/21 13:10:03, Bernhard Bauer wrote: > On 2016/07/19 19:11:31, Ted C wrote: > > Hmm...this changes the behavior if you're focusing the omnibox and not just > the > > NTP. What happens if you're on something like the recent tabs page where the > is > > no text in the omnibox so the mic icon will be present during the transition. > > > > maybe the mic is hidden enough that this has no effect, but it seems odd that > > we'd be animating the alpha of the delete button and not the mic when one or > the > > other is shown in the same place interchangeably > > Well, this will keep the mic icon always visible in this case, so it will appear > behind the toolbar buttons when they slide out of view. I haven't been able to > see a difference to the behavior on trunk, probably because at the point when > the mic icon is uncovered the transition is already so far along that the alpha > value is almost 1 anyway. If you want consistency with the delete button, the > easiest thing to do would be to remove that call as well :) Yeah, I thought so as much. Please check in RTL if that holds true as well. https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java (right): https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:763: float expansion = visualState == VisualState.NEW_TAB_NORMAL ? 1 : mUrlExpansionPercent; On 2016/07/21 13:10:04, Bernhard Bauer wrote: > On 2016/07/19 19:11:32, Ted C wrote: > > why the change here? what was wrong about using mUrlExpansionPercent here > > instead of snapping to 1 for NEW_TAB_NORMAL? > > mUrlExpansionPercent is the maximum of mUrlFocusChangePercent and > mNtpSearchBoxScrollPercent, so while scrolling the NTP, this would interpolate > linearly between the unfocused omnibox state and the expanded state, based on > the scrolling position. This is wrong for several reasons: The unfocused omnibox > state is never actually seen on the NTP, so the starting value would be wrong, > and the omnibox expansion while scrolling is not fully linear (it would only > start at 40% for the old UI, and for the new UI we want a fast out, slow in > transition), so this would end up with completely wrong values. We would then > compensate for that in updateNtpTransitionAnimation(), but only for the > horizontal bounds. This is why you can see a very faint double line when > scrolling on the NTP on trunk: The fakebox and the omnibox are diverging > slightly in height. Hmm...I guess this must have been a carry over from when you scrolled the NTP and it would become the unfocused ntp at the top and would only expand once you tapped. This looks ok if you scroll half way and then release and tap the omnibox right (before it has time to animate)? Or if you just tap on the fakebox? https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:1480: finishAnimations(); On 2016/07/21 13:10:03, Bernhard Bauer wrote: > On 2016/07/19 19:11:32, Ted C wrote: > > previously, finishAnimations was using the previous value of > > mIsInTabSwitcherMode. Now you are setting it earlier, was it a bug previously > > and this fixes that? > > I didn't see a bug, I just changed it to avoid cases where we implicitly relied > on using the old value. In this particular method, only > updateProgressBarVisibility() (called from within finishAnimations()) uses > mIsInTabSwitcherMode, and using the new value seems correct there. Hmm...i'd like to think it was done before for a reason. While I don't have any great ideas for testing, I'm not very confident this won't cause other weird side effects from various tab selections. Please try all types of tab selections before submitting (different models, closing, undoing, reselecting, entering/exiting for the same tab, etc..).
On 2016/07/22 01:04:15, Ted C wrote: > Yeah, I thought so as much. Please check in RTL if that holds true as well. Argh, turns out the *alignment* of the url action button in RTL is different in the fakebox than in the omnibox. I'll have to ask Alan which one the desired layout is.
OK, I've updated the expansion animation to work correctly in RTL, and checked with Alan about the padding. Would you mind taking another look at ToolbarPhone.updateUrlExpansionAnimation()? https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java (right): https://codereview.chromium.org/2134663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:763: float expansion = visualState == VisualState.NEW_TAB_NORMAL ? 1 : mUrlExpansionPercent; On 2016/07/22 01:04:15, Ted C wrote: > On 2016/07/21 13:10:04, Bernhard Bauer wrote: > > On 2016/07/19 19:11:32, Ted C wrote: > > > why the change here? what was wrong about using mUrlExpansionPercent here > > > instead of snapping to 1 for NEW_TAB_NORMAL? > > > > mUrlExpansionPercent is the maximum of mUrlFocusChangePercent and > > mNtpSearchBoxScrollPercent, so while scrolling the NTP, this would interpolate > > linearly between the unfocused omnibox state and the expanded state, based on > > the scrolling position. This is wrong for several reasons: The unfocused > omnibox > > state is never actually seen on the NTP, so the starting value would be wrong, > > and the omnibox expansion while scrolling is not fully linear (it would only > > start at 40% for the old UI, and for the new UI we want a fast out, slow in > > transition), so this would end up with completely wrong values. We would then > > compensate for that in updateNtpTransitionAnimation(), but only for the > > horizontal bounds. This is why you can see a very faint double line when > > scrolling on the NTP on trunk: The fakebox and the omnibox are diverging > > slightly in height. > > Hmm...I guess this must have been a carry over from when you scrolled the > NTP and it would become the unfocused ntp at the top and would only expand > once you tapped. > > This looks ok if you scroll half way and then release and tap the omnibox > right (before it has time to animate)? That doesn't work, because the omnibox doesn't accept touch events while it's animating. > Or if you just tap on the fakebox? Yes, that looks okay.
lgtm still w/ a couple small nits https://codereview.chromium.org/2134663002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java (right): https://codereview.chromium.org/2134663002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:848: boolean isThisRtl = ApiCompatibilityUtils.isLayoutRtl(this); we use just isRtl other places https://codereview.chromium.org/2134663002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:952: // shrinkage = 1; remove
The CQ bit was checked by bauerb@chromium.org to run a CQ dry run
Thanks! https://codereview.chromium.org/2134663002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java (right): https://codereview.chromium.org/2134663002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:848: boolean isThisRtl = ApiCompatibilityUtils.isLayoutRtl(this); On 2016/07/29 17:29:34, Ted C wrote: > we use just isRtl other places Done. https://codereview.chromium.org/2134663002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:952: // shrinkage = 1; On 2016/07/29 17:29:34, Ted C wrote: > remove Done. Sorry!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bauerb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mcwilliams@chromium.org, mdjones@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2134663002/#ps200001 (title: "review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/7a676cfc5291809bf2a95fd16ba1d57907f7961a Cr-Commit-Position: refs/heads/master@{#408885}
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:200001) has been created in https://codereview.chromium.org/2198993002/ by gab@chromium.org. The reason for reverting is: Suspect for http://crbug.com/633321, will re-land if not.. |
