[Home] Show bottom nav above keyboard

[Home] Show bottom nav above keyboard

This change allows the bottom navigation to be shown above the
keyboard when the bottom sheet is open. An onLayoutChange event is
triggered when the keyboard is shown or hidden. The height of the
window and keyboard are determined and sent as a new event,
"onSheetLayout" to any BottomSheetObservers.

When the keyboard is focused while viewing any of the bottom sheet
contents, extra padding is added to the content container. This
shrinks the view so it remains visible while allowing the bottom
sheet to retain it's original height. The result of this is seeing
the default tab theme color behind the keyboard when it appears
instead of seeing black flashes.

Since the navigation is now visible when the omnibox is focused from
the peeking state (thus showing the placeholder sheet content) the
bottom nav shows none of the tabs as selected. This allows users to
view suggestions from this state if they wish to.

BUG=706249
Review-Url: https://codereview.chromium.org/2892953005
Cr-Commit-Position: refs/heads/master@{#474363}
Committed: https://chromium.googlesource.com/chromium/src/+/e713cd4c5b8995854329af72ea0ec3be7eb945f8

[mdjones, 2017-05-20 00:12:33]

mdjones@chromium.org changed reviewers:
+ twellington@chromium.org

[mdjones, 2017-05-20 00:12:34]

Adding another observer method might be a bit heavy-handed; I'm open to
suggestions. ptal

[Theresa, 2017-05-22 15:26:43]

https://codereview.chromium.org/2892953005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java
(right):

https://codereview.chromium.org/2892953005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:576:
int navHeight = getResources().getDimensionPixelSize(R.dimen.bottom_nav_height);
nit: Let's store this dimension in a variable rather than retrieving it from
Android each time.

https://codereview.chromium.org/2892953005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java
(left):

https://codereview.chromium.org/2892953005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java:225:
showBottomSheetContent(R.id.action_home);
With this removed, what's transitioning the bottom sheet back to
R.id.action_home when the omnibox is unfocused?

https://codereview.chromium.org/2892953005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java
(right):

https://codereview.chromium.org/2892953005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java:102:
getMenu().findItem(mSelectedItemId).setChecked(true);
showBottomSheetContent() sets msSelecedItemId and sets the menu item to checked,
so I think this line and 101 are not needed. Also, if the user has
history/downloads/bookmarks selected and mSelectedItemId is set before
showBottomSheetContent is called, then the previously selected pane won't get
unchecked.

https://codereview.chromium.org/2892953005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java:134:
setTranslationY(containerHeight - windowHeight);
This observer method is called on any sheet layout, correct? Does it work as
expected on rotation or window resize?

[mdjones, 2017-05-22 18:34:10]

https://codereview.chromium.org/2892953005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java
(right):

https://codereview.chromium.org/2892953005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:576:
int navHeight = getResources().getDimensionPixelSize(R.dimen.bottom_nav_height);
On 2017/05/22 15:26:42, Theresa wrote:
> nit: Let's store this dimension in a variable rather than retrieving it from
> Android each time.

Done.

https://codereview.chromium.org/2892953005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java
(left):

https://codereview.chromium.org/2892953005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java:225:
showBottomSheetContent(R.id.action_home);
On 2017/05/22 15:26:42, Theresa wrote:
> With this removed, what's transitioning the bottom sheet back to
> R.id.action_home when the omnibox is unfocused?

Nothing. Recent changes will not allow a user to get into a state where the
sheet is expanded, the placeholder content is showing, and the omnibox is not
selected (namely hitting back once will close the sheet).

https://codereview.chromium.org/2892953005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java
(right):

https://codereview.chromium.org/2892953005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java:102:
getMenu().findItem(mSelectedItemId).setChecked(true);
On 2017/05/22 15:26:42, Theresa wrote:
> showBottomSheetContent() sets msSelecedItemId and sets the menu item to
checked,
> so I think this line and 101 are not needed. Also, if the user has
> history/downloads/bookmarks selected and mSelectedItemId is set before
> showBottomSheetContent is called, then the previously selected pane won't get
> unchecked.

Done. Added a check before the "setSelected" code that crashed if I didn't have
this.

https://codereview.chromium.org/2892953005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java:134:
setTranslationY(containerHeight - windowHeight);
On 2017/05/22 15:26:42, Theresa wrote:
> This observer method is called on any sheet layout, correct? Does it work as
> expected on rotation or window resize?

Yes, the values are computed after any updates to the sheet have already
occurred. We always want the negative difference between the window and
container heights. In the case where the keyboard is not showing, the values
will be the same and produce 0 for the Y translation.

[Theresa, 2017-05-23 14:46:43]

lgtm % comment on BottomSheetContentController.java

https://codereview.chromium.org/2892953005/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java
(right):

https://codereview.chromium.org/2892953005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java:86:
if (mBottomSheet.getTargetSheetState() != BottomSheet.SHEET_STATE_PEEK) {
We should check that the placeholder view is showing  so that history, et. al.,
don't transition back to the home tab when the sheet is moved.

[mdjones, 2017-05-23 18:22:20]

https://codereview.chromium.org/2892953005/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java
(right):

https://codereview.chromium.org/2892953005/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java:86:
if (mBottomSheet.getTargetSheetState() != BottomSheet.SHEET_STATE_PEEK) {
On 2017/05/23 14:46:43, Theresa wrote:
> We should check that the placeholder view is showing  so that history, et.
al.,
> don't transition back to the home tab when the sheet is moved.

Done.

[mdjones, 2017-05-23 18:22:49]

mdjones@chromium.org changed reviewers:
+ tedchoc@chromium.org

[mdjones, 2017-05-23 18:22:50]

+tedchoc for owners

[Ted C, 2017-05-23 20:18:42]

https://codereview.chromium.org/2892953005/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/2892953005/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:464: new
WindowDelegate(this.getWindow()));
no need for "this." right?

https://codereview.chromium.org/2892953005/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java
(right):

https://codereview.chromium.org/2892953005/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:553:
int decorHeight = windowDelegate.getDecorViewHeight();
If chrome home isn't visible, does this function still get called?  Ideally, we
wouldn't be doing a bunch of work for the panels if chrome home is at the bottom
or hidden.

https://codereview.chromium.org/2892953005/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:558:
boolean keyboardHeightChanged = heightMinusKeyboard != mHeightMinusKeyboard;
I wonder if we should enhance the InsetObserverView to have an observer to
provide this information to you (to avoid having you to calculate it yourself).

[mdjones, 2017-05-23 23:26:27]

https://codereview.chromium.org/2892953005/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/2892953005/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:464: new
WindowDelegate(this.getWindow()));
On 2017/05/23 20:18:41, Ted C wrote:
> no need for "this." right?

It just didn't seem verbose enough. Done.

https://codereview.chromium.org/2892953005/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java
(right):

https://codereview.chromium.org/2892953005/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:553:
int decorHeight = windowDelegate.getDecorViewHeight();
On 2017/05/23 20:18:41, Ted C wrote:
> If chrome home isn't visible, does this function still get called?  Ideally,
we
> wouldn't be doing a bunch of work for the panels if chrome home is at the
bottom
> or hidden.

Yes, in the future we can rework this to only apply width and height once the
sheet has been opened. For now I have added a check around where the padding is
set so we don't resize content unnecessarily.

https://codereview.chromium.org/2892953005/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:558:
boolean keyboardHeightChanged = heightMinusKeyboard != mHeightMinusKeyboard;
On 2017/05/23 20:18:41, Ted C wrote:
> I wonder if we should enhance the InsetObserverView to have an observer to
> provide this information to you (to avoid having you to calculate it
yourself).

Inset observer needs some work, it's not currently doing what we think it is.
I'll follow up with a change where we share the keyboard sizing logic.

[Ted C, 2017-05-23 23:41:16]

lgtm

https://codereview.chromium.org/2892953005/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java
(right):

https://codereview.chromium.org/2892953005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:553:
int decorHeight = windowDelegate.getDecorViewHeight();
maybe a TODO to share this with LocationBarLayout somehow?

https://codereview.chromium.org/2892953005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:568:
mHeightMinusKeyboard = heightMinusKeyboard;
another thing to check is whether this path is triggered when entering
fullscreen videos...probably wouldn't hurt but just a thing to be aware

[mdjones, 2017-05-24 00:54:50]

https://codereview.chromium.org/2892953005/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java
(right):

https://codereview.chromium.org/2892953005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:553:
int decorHeight = windowDelegate.getDecorViewHeight();
On 2017/05/23 23:41:16, Ted C wrote:
> maybe a TODO to share this with LocationBarLayout somehow?

Done.

https://codereview.chromium.org/2892953005/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:568:
mHeightMinusKeyboard = heightMinusKeyboard;
On 2017/05/23 23:41:16, Ted C wrote:
> another thing to check is whether this path is triggered when entering
> fullscreen videos...probably wouldn't hurt but just a thing to be aware

It is; I filed a bug for restricting the number of times this code block is run.
crbug.com/725730

[mdjones, 2017-05-24 16:09:31]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2017-05-24 16:09:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org, tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2892953005/#ps160001
(title: "address comments")

[commit-bot: I haz the power, 2017-05-24 16:09:54]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 16:12:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-24 16:12:33]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mdjones, 2017-05-24 17:24:02]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2017-05-24 17:24:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, twellington@chromium.org
Link to the patchset: https://codereview.chromium.org/2892953005/#ps180001
(title: "rebase; remove window delegate")

[commit-bot: I haz the power, 2017-05-24 17:24:33]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 18:31:14]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1495646642257420,
"parent_rev": "ba6281e4333f2faeedff5a9d10e96a24e6b91fa4", "commit_rev":
"e713cd4c5b8995854329af72ea0ec3be7eb945f8"}

[commit-bot: I haz the power, 2017-05-24 18:31:29]

Description was changed from

==========
[Home] Show bottom nav above keyboard

This change allows the bottom navigation to be shown above the
keyboard when the bottom sheet is open. An onLayoutChange event is
triggered when the keyboard is shown or hidden. The height of the
window and keyboard are determined and sent as a new event,
"onSheetLayout" to any BottomSheetObservers.

When the keyboard is focused while viewing any of the bottom sheet
contents, extra padding is added to the content container. This
shrinks the view so it remains visible while allowing the bottom
sheet to retain it's original height. The result of this is seeing
the default tab theme color behind the keyboard when it appears
instead of seeing black flashes.

Since the navigation is now visible when the omnibox is focused from
the peeking state (thus showing the placeholder sheet content) the
bottom nav shows none of the tabs as selected. This allows users to
view suggestions from this state if they wish to.

BUG=706249
==========

to

==========
[Home] Show bottom nav above keyboard

This change allows the bottom navigation to be shown above the
keyboard when the bottom sheet is open. An onLayoutChange event is
triggered when the keyboard is shown or hidden. The height of the
window and keyboard are determined and sent as a new event,
"onSheetLayout" to any BottomSheetObservers.

When the keyboard is focused while viewing any of the bottom sheet
contents, extra padding is added to the content container. This
shrinks the view so it remains visible while allowing the bottom
sheet to retain it's original height. The result of this is seeing
the default tab theme color behind the keyboard when it appears
instead of seeing black flashes.

Since the navigation is now visible when the omnibox is focused from
the peeking state (thus showing the placeholder sheet content) the
bottom nav shows none of the tabs as selected. This allows users to
view suggestions from this state if they wish to.

BUG=706249

Review-Url: https://codereview.chromium.org/2892953005
Cr-Commit-Position: refs/heads/master@{#474363}
Committed:
https://chromium.googlesource.com/chromium/src/+/e713cd4c5b8995854329af72ea0e...
==========

[commit-bot: I haz the power, 2017-05-24 18:31:31]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/e713cd4c5b8995854329af72ea0e...