Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2625923002:
Introduce the bottom sheet class for Chrome Home (Closed)
DescriptionIntroduce the bottom sheet class for Chrome Home
This change introduces the swipe/scroll logic for the bottom sheet
(BottomSheet.java) as well as a placeholder for its content. The sheet
can be moved to 3 states -- peeking, half expanded, and fully
expanded -- and will move to the nearest one when released in between
any of them.
The sheet will only intercept swipes in the up and down direction
until it is moving, then it will follow the user's finger until
released. If the sheet is fully expanded, upward swipes will not
be intercepted. Likewise for downward swipes when the sheet is in its
peeking state. This means that all of the current gestures that the
toolbar supports still function when the sheet is in the peeking
state.
BUG=671361
Review-Url: https://codereview.chromium.org/2625923002
Cr-Commit-Position: refs/heads/master@{#444868}
Committed: https://chromium.googlesource.com/chromium/src/+/f45d9fc526bf93ab943315b26432f101d7a1e6ed
Patch Set 1 #
Total comments: 28
Patch Set 2 : move gatherTransparentRegion work around #Patch Set 3 : address comments #Patch Set 4 : rebase #
Total comments: 16
Patch Set 5 : address comments #Patch Set 6 : javadoc #
Total comments: 8
Patch Set 7 : address comments #Patch Set 8 : fix findbugs #
Total comments: 1
Dependent Patchsets: Messages
Total messages: 27 (10 generated)
mdjones@chromium.org changed reviewers: + ianwen@chromium.org
Looking for initial thoughts. ptal
Looks impressive! https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/res/lay... File chrome/android/java/res/layout/bottom_control_container.xml (right): https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/bottom_control_container.xml:13: android:id="@+id/bottom_sheet_content_wrapper" This linear layout might not be necessary. You can simply let the view below have a top margin of control_container_height? https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:420: bottomSheet.init(mCompositorViewHolder, controlContainer.getView()); I think you shouldn't let the bottom sheet to know about CompositorViewHolder, right? The root should be the coordinator. https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarControlContainer.java (right): https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarControlContainer.java:116: region.setEmpty(); Q: what's this for? https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java (right): https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:35: private enum SheetState { PEEK, HALF, FULL } Nit: use int instead? Enums are slow on Android. https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:62: private ObjectAnimator mHeightAnimator; Nit: how about mSettleAnimator? You mentioned below that this is used to settle the sheet to a state. https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:65: private Interpolator mInterpolator; Nit: inline the assignment here. https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:92: new GestureDetector(context, new GestureDetector.SimpleOnGestureListener() { Nit: You might want to move SimpleOnGestureListener to above as a class member of below in a createOGL() method. Currently the 8 space indent looks untidy for such an important piece of code. https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:104: || Math.abs(distanceY) / Math.abs(distanceX) distanceX might be 0 here. Also I think you need to move the slope calculation to #102 to make this if clause more clear. https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:132: setSheetOffsetFromBottom(Math.max(getMinOffset(), Nit: maybe write a clamp function below, instead of nesting max and min? https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:165: // Don't trust incoming motion events for the gesture detector. Hmmm "trust" is ambiguous here. Are you trying to say that we shouldn't trust gesture detector not to change the motion event, so that we should create a copy? Or you mean the motion event might have been adjusted to reflect the coordinate of its parent view, not the entire screen? https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:194: cancel.setAction(MotionEvent.ACTION_CANCEL); You only need to send cancel once. IIUC you are sending a cancel every time you receive a move event. https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:212: mSheetContentWrapper = (ViewGroup) findViewById(R.id.bottom_sheet_content_wrapper); Let's unify the name to either wrapper or container. Having two temrs meaning the same thing is a bit confusing. https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:261: private void createYAnimation(SheetState targetState) { Similarly, let's unify the animations' name as well. How about createSettleAnimation()? https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:351: * @param yVelocity The current Y velocity of the sheet. This is only used for determining the Please document if yVolocity is positive, the direction is from bottom to top.
https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/res/lay... File chrome/android/java/res/layout/bottom_control_container.xml (right): https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/bottom_control_container.xml:13: android:id="@+id/bottom_sheet_content_wrapper" On 2017/01/11 19:46:53, Ian Wen wrote: > This linear layout might not be necessary. You can simply let the view below > have a top margin of control_container_height? Removed. https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:420: bottomSheet.init(mCompositorViewHolder, controlContainer.getView()); On 2017/01/11 19:46:53, Ian Wen wrote: > I think you shouldn't let the bottom sheet to know about CompositorViewHolder, > right? The root should be the coordinator. Already forgot about the new coordinator. Fixed. https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarControlContainer.java (right): https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarControlContainer.java:116: region.setEmpty(); On 2017/01/11 19:46:53, Ian Wen wrote: > Q: what's this for? I moved this over to BottomSheet.java. I have it set to empty so the bottom sheet is not clipped when it is swiped up. Since this function is not necessarily called when the sheet is animating, I can't update the region correctly. I need a more correct solution for this. https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java (right): https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:35: private enum SheetState { PEEK, HALF, FULL } On 2017/01/11 19:46:54, Ian Wen wrote: > Nit: use int instead? Enums are slow on Android. Done. https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:62: private ObjectAnimator mHeightAnimator; On 2017/01/11 19:46:54, Ian Wen wrote: > Nit: how about mSettleAnimator? You mentioned below that this is used to settle > the sheet to a state. Done. https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:65: private Interpolator mInterpolator; On 2017/01/11 19:46:54, Ian Wen wrote: > Nit: inline the assignment here. Done. https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:92: new GestureDetector(context, new GestureDetector.SimpleOnGestureListener() { On 2017/01/11 19:46:54, Ian Wen wrote: > Nit: You might want to move SimpleOnGestureListener to above as a class member > of below in a createOGL() method. Currently the 8 space indent looks untidy for > such an important piece of code. Done. https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:104: || Math.abs(distanceY) / Math.abs(distanceX) On 2017/01/11 19:46:54, Ian Wen wrote: > distanceX might be 0 here. Also I think you need to move the slope calculation > to #102 to make this if clause more clear. Done. https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:132: setSheetOffsetFromBottom(Math.max(getMinOffset(), On 2017/01/11 19:46:53, Ian Wen wrote: > Nit: maybe write a clamp function below, instead of nesting max and min? Done. Used MathUtils.clamp(...) https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:165: // Don't trust incoming motion events for the gesture detector. On 2017/01/11 19:46:54, Ian Wen wrote: > Hmmm "trust" is ambiguous here. > > Are you trying to say that we shouldn't trust gesture detector not to change the > motion event, so that we should create a copy? > > Or you mean the motion event might have been adjusted to reflect the coordinate > of its parent view, not the entire screen? The incoming event may have been adjusted. I updated the doc. https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:194: cancel.setAction(MotionEvent.ACTION_CANCEL); On 2017/01/11 19:46:54, Ian Wen wrote: > You only need to send cancel once. IIUC you are sending a cancel every time you > receive a move event. Removed the need for this with the code added to onInterceptTouchEvent(). https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:212: mSheetContentWrapper = (ViewGroup) findViewById(R.id.bottom_sheet_content_wrapper); On 2017/01/11 19:46:53, Ian Wen wrote: > Let's unify the name to either wrapper or container. Having two temrs meaning > the same thing is a bit confusing. Removed the need for this. https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:261: private void createYAnimation(SheetState targetState) { On 2017/01/11 19:46:53, Ian Wen wrote: > Similarly, let's unify the animations' name as well. How about > createSettleAnimation()? Done. https://codereview.chromium.org/2625923002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:351: * @param yVelocity The current Y velocity of the sheet. This is only used for determining the On 2017/01/11 19:46:53, Ian Wen wrote: > Please document if yVolocity is positive, the direction is from bottom to top. Done.
mdjones@chromium.org changed reviewers: + dfalcantara@chromium.org
+dfalcantara
I've some small suggestions for this CL but overall this is a great change. lgtm. https://codereview.chromium.org/2625923002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2625923002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:359: ControlContainer controlContainer = null; Why this line? IIUC you are not using the variable until in the #387 try block. https://codereview.chromium.org/2625923002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java (right): https://codereview.chromium.org/2625923002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:24: * This class defines the behavior of a bottom sheet that has multiple states and a persistently Nit: I would say this class defines the bottom sheet, not the behavior of it. :) https://codereview.chromium.org/2625923002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:109: float currentShownRatio = getSheetOffsetFromBottom() / mContainerHeight; This code will crash if container has height of 0, which might happen if the view is detached or the view is set to visibility gone. You might want to early return, if such case happens. https://codereview.chromium.org/2625923002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:204: region.setEmpty(); Q: what will happen if you don't have this line? https://codereview.chromium.org/2625923002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:245: private MotionEvent getRawMotionEvent(MotionEvent e) { This method creates a new object. I would rename it to createRawMotionEvent() so that people are more cautious when using it. https://codereview.chromium.org/2625923002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:252: * Update the bottom sheet's peeking height. s/Update/Updates. Same with all java docs for this class. This is what newt@ told me. Javadocs of a method describes what the method does, not commands it to do it.
https://codereview.chromium.org/2625923002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2625923002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:359: ControlContainer controlContainer = null; On 2017/01/17 18:39:47, Ian Wen wrote: > Why this line? IIUC you are not using the variable until in the #387 try block. Code was previously more spread out, fixed. https://codereview.chromium.org/2625923002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java (right): https://codereview.chromium.org/2625923002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:24: * This class defines the behavior of a bottom sheet that has multiple states and a persistently On 2017/01/17 18:39:48, Ian Wen wrote: > Nit: I would say this class defines the bottom sheet, not the behavior of it. :) Done. https://codereview.chromium.org/2625923002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:109: float currentShownRatio = getSheetOffsetFromBottom() / mContainerHeight; On 2017/01/17 18:39:48, Ian Wen wrote: > This code will crash if container has height of 0, which might happen if the > view is detached or the view is set to visibility gone. > > You might want to early return, if such case happens. Changed to handle the 0 case. I think the code should run to completion though. https://codereview.chromium.org/2625923002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:204: region.setEmpty(); On 2017/01/17 18:39:48, Ian Wen wrote: > Q: what will happen if you don't have this line? The sheet will be visually clipped to the height of the toolbar. Everything will still work except nothing will be drawn above the height of the toolbar. https://codereview.chromium.org/2625923002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:245: private MotionEvent getRawMotionEvent(MotionEvent e) { On 2017/01/17 18:39:48, Ian Wen wrote: > This method creates a new object. I would rename it to createRawMotionEvent() so > that people are more cautious when using it. Done. https://codereview.chromium.org/2625923002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:252: * Update the bottom sheet's peeking height. On 2017/01/17 18:39:48, Ian Wen wrote: > s/Update/Updates. Same with all java docs for this class. > > This is what newt@ told me. Javadocs of a method describes what the method does, > not commands it to do it. Done.
lgtm % questions https://codereview.chromium.org/2625923002/diff/60001/chrome/android/java/res... File chrome/android/java/res/layout/bottom_control_container.xml (right): https://codereview.chromium.org/2625923002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/bottom_control_container.xml:11: android:orientation="vertical" > Given that BottomSheet is your own thing and everything is forced to be vertical, this should be put in the class' constructor so that no one can use it incorrectly in the future. https://codereview.chromium.org/2625923002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java (right): https://codereview.chromium.org/2625923002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:103: // Cancel the settling animation if it is running so it doesn't conflict with where the double space before with https://codereview.chromium.org/2625923002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java (right): https://codereview.chromium.org/2625923002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:35: public static final int STATE_PEEK = 0; nit: Use @IntDef? https://codereview.chromium.org/2625923002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:40: private static final long BASE_ANIMATION_DURATION_MS = 218; This number seems entirely arbitrary. Why not pick a round number for clarity? Even if 218ms works for your phone, it won't on others. https://codereview.chromium.org/2625923002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:55: private final int[] mStates = new int[] {STATE_PEEK, STATE_HALF, STATE_FULL}; Why aren't these static? They can't change. https://codereview.chromium.org/2625923002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:257: private void updateSheetPeekHeight(float toolbarHeight, float containerHeight) { You always pass in member fields when calling this. Can you just remove the arguments?
https://codereview.chromium.org/2625923002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java (right): https://codereview.chromium.org/2625923002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:35: public static final int STATE_PEEK = 0; On 2017/01/19 18:00:42, dfalcantara (load balance plz) wrote: > nit: Use @IntDef? Done. https://codereview.chromium.org/2625923002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:40: private static final long BASE_ANIMATION_DURATION_MS = 218; On 2017/01/19 18:00:42, dfalcantara (load balance plz) wrote: > This number seems entirely arbitrary. Why not pick a round number for clarity? > Even if 218ms works for your phone, it won't on others. It's the same number used by the overlay panels; it's a material design spec. I updated the doc. https://codereview.chromium.org/2625923002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:55: private final int[] mStates = new int[] {STATE_PEEK, STATE_HALF, STATE_FULL}; On 2017/01/19 18:00:42, dfalcantara (load balance plz) wrote: > Why aren't these static? They can't change. Done. https://codereview.chromium.org/2625923002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:257: private void updateSheetPeekHeight(float toolbarHeight, float containerHeight) { On 2017/01/19 18:00:42, dfalcantara (load balance plz) wrote: > You always pass in member fields when calling this. Can you just remove the > arguments? Done.
The CQ bit was checked by mdjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ianwen@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2625923002/#ps120001 (title: "address comments")
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
Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
https://codereview.chromium.org/2625923002/diff/60001/chrome/android/java/res... File chrome/android/java/res/layout/bottom_control_container.xml (right): https://codereview.chromium.org/2625923002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/bottom_control_container.xml:11: android:orientation="vertical" > On 2017/01/19 18:00:41, dfalcantara (load balance plz) wrote: > Given that BottomSheet is your own thing and everything is forced to be > vertical, this should be put in the class' constructor so that no one can use it > incorrectly in the future. Done. https://codereview.chromium.org/2625923002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java (right): https://codereview.chromium.org/2625923002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:103: // Cancel the settling animation if it is running so it doesn't conflict with where the On 2017/01/19 18:00:41, dfalcantara (load balance plz) wrote: > double space before with Done.
The CQ bit was checked by mdjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ianwen@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2625923002/#ps140001 (title: "fix findbugs")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484862554713130,
"parent_rev": "231e833f8aab516b124544a01e8a4e8441ac47f0", "commit_rev":
"f45d9fc526bf93ab943315b26432f101d7a1e6ed"}
Message was sent while issue was closed.
Description was changed from ========== Introduce the bottom sheet class for Chrome Home This change introduces the swipe/scroll logic for the bottom sheet (BottomSheet.java) as well as a placeholder for its content. The sheet can be moved to 3 states -- peeking, half expanded, and fully expanded -- and will move to the nearest one when released in between any of them. The sheet will only intercept swipes in the up and down direction until it is moving, then it will follow the user's finger until released. If the sheet is fully expanded, upward swipes will not be intercepted. Likewise for downward swipes when the sheet is in its peeking state. This means that all of the current gestures that the toolbar supports still function when the sheet is in the peeking state. BUG=671361 ========== to ========== Introduce the bottom sheet class for Chrome Home This change introduces the swipe/scroll logic for the bottom sheet (BottomSheet.java) as well as a placeholder for its content. The sheet can be moved to 3 states -- peeking, half expanded, and fully expanded -- and will move to the nearest one when released in between any of them. The sheet will only intercept swipes in the up and down direction until it is moving, then it will follow the user's finger until released. If the sheet is fully expanded, upward swipes will not be intercepted. Likewise for downward swipes when the sheet is in its peeking state. This means that all of the current gestures that the toolbar supports still function when the sheet is in the peeking state. BUG=671361 Review-Url: https://codereview.chromium.org/2625923002 Cr-Commit-Position: refs/heads/master@{#444868} Committed: https://chromium.googlesource.com/chromium/src/+/f45d9fc526bf93ab943315b26432... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/f45d9fc526bf93ab943315b26432...
Message was sent while issue was closed.
bauerb@chromium.org changed reviewers: + bauerb@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2625923002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java (right): https://codereview.chromium.org/2625923002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:218: region.setEmpty(); Drive-by: I would be very careful with this -- the region passed here is not used in isolation, but instead the method is called for every view in sequence with the same region, which means you can clobber some other view's transparent region. In particular, you are saying here that nothing in the whole region (which means everything the activity draws on the screen) is transparent, which is I think not the case if the web content SurfaceView is visible.
Message was sent while issue was closed.
On 2017/01/25 12:02:28, Bernhard Bauer wrote: > https://codereview.chromium.org/2625923002/diff/140001/chrome/android/java/sr... > File chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java > (right): > > https://codereview.chromium.org/2625923002/diff/140001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:218: > region.setEmpty(); > Drive-by: I would be very careful with this -- the region passed here is not > used in isolation, but instead the method is called for every view in sequence > with the same region, which means you can clobber some other view's transparent > region. In particular, you are saying here that nothing in the whole region > (which means everything the activity draws on the screen) is transparent, which > is I think not the case if the web content SurfaceView is visible. I realize that the implemented solution is not correct, the problem is that function is not called at the same rate as the sheet is animating causing the it to be clipped. I need to find out how to trigger that call and have it run as part of the animation update. This is definitely something I am coming back to.
Message was sent while issue was closed.
On 2017/01/25 16:31:20, mdjones wrote: > On 2017/01/25 12:02:28, Bernhard Bauer wrote: > > > https://codereview.chromium.org/2625923002/diff/140001/chrome/android/java/sr... > > File > chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java > > (right): > > > > > https://codereview.chromium.org/2625923002/diff/140001/chrome/android/java/sr... > > > chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:218: > > region.setEmpty(); > > Drive-by: I would be very careful with this -- the region passed here is not > > used in isolation, but instead the method is called for every view in sequence > > with the same region, which means you can clobber some other view's > transparent > > region. In particular, you are saying here that nothing in the whole region > > (which means everything the activity draws on the screen) is transparent, > which > > is I think not the case if the web content SurfaceView is visible. > > I realize that the implemented solution is not correct, the problem is that > function is not called at the same rate as the sheet is animating causing the it > to be clipped. I need to find out how to trigger that call and have it run as > part of the animation update. This is definitely something I am coming back to. OK, I see. Given that the whole point of gatherTransparentRegion is to find out when _everything_ is transparent (so the compositor can remove the non-surface layer), would it be sufficient to trigger a relayout when showing / hiding the toolbar?
Message was sent while issue was closed.
On 2017/01/25 17:59:27, Bernhard Bauer wrote: > On 2017/01/25 16:31:20, mdjones wrote: > > On 2017/01/25 12:02:28, Bernhard Bauer wrote: > > > > > > https://codereview.chromium.org/2625923002/diff/140001/chrome/android/java/sr... > > > File > > chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2625923002/diff/140001/chrome/android/java/sr... > > > > > > chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:218: > > > region.setEmpty(); > > > Drive-by: I would be very careful with this -- the region passed here is not > > > used in isolation, but instead the method is called for every view in > sequence > > > with the same region, which means you can clobber some other view's > > transparent > > > region. In particular, you are saying here that nothing in the whole region > > > (which means everything the activity draws on the screen) is transparent, > > which > > > is I think not the case if the web content SurfaceView is visible. > > > > I realize that the implemented solution is not correct, the problem is that > > function is not called at the same rate as the sheet is animating causing the > it > > to be clipped. I need to find out how to trigger that call and have it run as > > part of the animation update. This is definitely something I am coming back > to. > > OK, I see. Given that the whole point of gatherTransparentRegion is to find out > when _everything_ is transparent (so the compositor can remove the non-surface > layer), would it be sufficient to trigger a relayout when showing / hiding the > toolbar? If that is the case, I may take a different approach to this problem; I didn't realize it only mattered when the whole screen is transparent. I'll try that solution as one of my next patches. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
