Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2846663002:
Peek new infobars behind existing ones (Closed)
|
Created:
3 years, 8 months ago by mdjones Modified:
3 years, 7 months ago CC:
chromium-reviews, dominickn+watch_chromium.org, dfalcantara+watch_chromium.org, pkotwicz+watch_chromium.org, zpeng+watch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPeek new infobars behind existing ones
This change extends the peeking behavior when there are multiple
infobars attempting to show simultaneously. When a new infobar is
added, it will peek above the existing ones giving a brief view of
it's contents and bringing more attention to it.
BUG=721389
Review-Url: https://codereview.chromium.org/2846663002
Cr-Commit-Position: refs/heads/master@{#473719}
Committed: https://chromium.googlesource.com/chromium/src/+/58394da4f0c15dce8e8dbe2da801dd34e7e9f95c
Patch Set 1 #
Total comments: 6
Patch Set 2 : address comments #Patch Set 3 : no clip when animating #Patch Set 4 : undo tab changes #
Total comments: 4
Patch Set 5 : address comments #Patch Set 6 : fix short infobar bug #
Total comments: 4
Patch Set 7 : wrapper has height restricted mode #
Total comments: 4
Patch Set 8 : address comments #Patch Set 9 : measure happens while adding view #
Messages
Total messages: 24 (7 generated)
Description was changed from ========== Peek new infobars behind existing ones This change extends the peeking behavior when there are multiple infobars attempting to show simultaneously. When a new infobar is added, it will peek above the existing ones giving a brief view of it's contents and bringing more attention to it. BUG= ========== to ========== Peek new infobars behind existing ones This change extends the peeking behavior when there are multiple infobars attempting to show simultaneously. When a new infobar is added, it will peek above the existing ones giving a brief view of it's contents and bringing more attention to it. BUG=721389 ==========
mdjones@chromium.org changed reviewers: + twellington@chromium.org
Current implementation signed off by helene; ptal.
lgtm
mdjones@chromium.org changed reviewers: + tedchoc@chromium.org
+tedchoc ptal
https://codereview.chromium.org/2846663002/diff/1/chrome/android/java/res/lay... File chrome/android/java/res/layout/main.xml (right): https://codereview.chromium.org/2846663002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/main.xml:28: android:clipChildren="false" /> I think we should conditionally set this. I have a strong suspicion that Android would have to disable some optimizations with this enabled. https://codereview.chromium.org/2846663002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainerLayout.java (right): https://codereview.chromium.org/2846663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainerLayout.java:410: animator.setDuration(DURATION_PEEK_MS); We should be able to set the duration on the set since it is the same. Hmm...or maybe playSequentially doesn't play nicely with that https://codereview.chromium.org/2846663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainerLayout.java:416: set.addListener(new AnimatorListenerAdapter() { Do you want the view to be removed even if the set were cancelled? It is probably ok in this case.
https://codereview.chromium.org/2846663002/diff/1/chrome/android/java/res/lay... File chrome/android/java/res/layout/main.xml (right): https://codereview.chromium.org/2846663002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/main.xml:28: android:clipChildren="false" /> On 2017/05/17 17:55:54, Ted C wrote: > I think we should conditionally set this. I have a strong suspicion that > Android would have to disable some optimizations with this enabled. I changed this to be set based on whether the infobar container is running an animation. I chose to get the container class by id rather than plumbing it down; I can go this route if you prefer. https://codereview.chromium.org/2846663002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainerLayout.java (right): https://codereview.chromium.org/2846663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainerLayout.java:410: animator.setDuration(DURATION_PEEK_MS); On 2017/05/17 17:55:54, Ted C wrote: > We should be able to set the duration on the set since it is the same. > > Hmm...or maybe playSequentially doesn't play nicely with that Setting duration on the set ends up working just fine. Done. https://codereview.chromium.org/2846663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainerLayout.java:416: set.addListener(new AnimatorListenerAdapter() { On 2017/05/17 17:55:54, Ted C wrote: > Do you want the view to be removed even if the set were cancelled? It is > probably ok in this case. When cancel is called on the animator set, cancel is called on each of its constituent animations. Canceling an animation will call onAnimationEnd, so this is run in any case.
https://codereview.chromium.org/2846663002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainerLayout.java (right): https://codereview.chromium.org/2846663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainerLayout.java:439: set.addListener(new AnimatorListenerAdapter() { merge with the one below? https://codereview.chromium.org/2846663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainerLayout.java:454: mBottomContainer.setClipChildren(false); should we also set setClipChildren on the InfobarContainer here as well? Does it need to always need to be true?
I also accounted for the infobar shadow in the peeking height. https://codereview.chromium.org/2846663002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainerLayout.java (right): https://codereview.chromium.org/2846663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainerLayout.java:439: set.addListener(new AnimatorListenerAdapter() { On 2017/05/18 00:09:29, Ted C wrote: > merge with the one below? Whoops. Done. https://codereview.chromium.org/2846663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainerLayout.java:454: mBottomContainer.setClipChildren(false); On 2017/05/18 00:09:29, Ted C wrote: > should we also set setClipChildren on the InfobarContainer here as well? Does > it need to always need to be true? Added utility method that sets this value in all the relevant places.
lgtm
Discovered a bug with this patch involving tall infobars appearing behind short ones. Displaying a tall infobar would cause the entire container to grow, thus incorrectly positioning the top-most infobar. Latest patch has the fix which is to wrap the infobar content to clip it to the minimum infobar size. ptal
https://codereview.chromium.org/2846663002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainerLayout.java (right): https://codereview.chromium.org/2846663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainerLayout.java:427: mAppearingWrapper = new InfoBarWrapper(getContext(), appearingItem); I wonder if we could achieve the same if we did something like: new InfoBarWrapper(...) { void onMeasure(width, heightSpec) { heightSpec = MeasureSpec.makeMeasureSpec(Math.min(mInfoBarShadowHeight + mBackgroundPeekSize, MeasureSpec.getSize(heightSpec)), MeasureSpec.getMode(heightSpec)); super.onMeasure(widthSpec, heightSpec); } } mAppearingWrapper.setClipChildren(true);
https://codereview.chromium.org/2846663002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainerLayout.java (right): https://codereview.chromium.org/2846663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainerLayout.java:427: mAppearingWrapper = new InfoBarWrapper(getContext(), appearingItem); On 2017/05/18 23:01:33, Ted C wrote: > I wonder if we could achieve the same if we did something like: > > new InfoBarWrapper(...) { > void onMeasure(width, heightSpec) { > heightSpec = MeasureSpec.makeMeasureSpec(Math.min(mInfoBarShadowHeight + > mBackgroundPeekSize, MeasureSpec.getSize(heightSpec)), > MeasureSpec.getMode(heightSpec)); > super.onMeasure(widthSpec, heightSpec); > } > } > mAppearingWrapper.setClipChildren(true); This could work, but the wrapper persists beyond this animation; it stays visible and is reused when the infobar comes into the foreground. I could make this work but it would require adding a new flow for animation.
https://codereview.chromium.org/2846663002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainerLayout.java (right): https://codereview.chromium.org/2846663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainerLayout.java:427: mAppearingWrapper = new InfoBarWrapper(getContext(), appearingItem); On 2017/05/19 16:47:21, mdjones wrote: > On 2017/05/18 23:01:33, Ted C wrote: > > I wonder if we could achieve the same if we did something like: > > > > new InfoBarWrapper(...) { > > void onMeasure(width, heightSpec) { > > heightSpec = MeasureSpec.makeMeasureSpec(Math.min(mInfoBarShadowHeight > + > > mBackgroundPeekSize, MeasureSpec.getSize(heightSpec)), > > MeasureSpec.getMode(heightSpec)); > > super.onMeasure(widthSpec, heightSpec); > > } > > } > > mAppearingWrapper.setClipChildren(true); > > This could work, but the wrapper persists beyond this animation; it stays > visible and is reused when the infobar comes into the foreground. I could make > this work but it would require adding a new flow for animation. Maybe we should just update InfoBarWrapper to have a mode like setIsPeeking. Then this would live in this class and get set here and reset in the end of the animation. Ideally, we keep our view hierarchy as shallow as possible.
https://codereview.chromium.org/2846663002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainerLayout.java (right): https://codereview.chromium.org/2846663002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainerLayout.java:427: mAppearingWrapper = new InfoBarWrapper(getContext(), appearingItem); On 2017/05/19 16:52:13, Ted C wrote: > On 2017/05/19 16:47:21, mdjones wrote: > > On 2017/05/18 23:01:33, Ted C wrote: > > > I wonder if we could achieve the same if we did something like: > > > > > > new InfoBarWrapper(...) { > > > void onMeasure(width, heightSpec) { > > > heightSpec = > MeasureSpec.makeMeasureSpec(Math.min(mInfoBarShadowHeight > > + > > > mBackgroundPeekSize, MeasureSpec.getSize(heightSpec)), > > > MeasureSpec.getMode(heightSpec)); > > > super.onMeasure(widthSpec, heightSpec); > > > } > > > } > > > mAppearingWrapper.setClipChildren(true); > > > > This could work, but the wrapper persists beyond this animation; it stays > > visible and is reused when the infobar comes into the foreground. I could make > > this work but it would require adding a new flow for animation. > > Maybe we should just update InfoBarWrapper to have a mode like setIsPeeking. > Then this would live in this class and get set here and reset in the end of the > animation. > > Ideally, we keep our view hierarchy as shallow as possible. Done.
lgtm! https://codereview.chromium.org/2846663002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainerLayout.java (right): https://codereview.chromium.org/2846663002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainerLayout.java:455: mAppearingWrapper.removeView(mAppearingWrapper.getItem().getView()); do we need to do this anymore? https://codereview.chromium.org/2846663002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarWrapper.java (right): https://codereview.chromium.org/2846663002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarWrapper.java:29: private int mHeightForAnimation; I would throw a Px suffix on this to make it super clear lower down as well
https://codereview.chromium.org/2846663002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainerLayout.java (right): https://codereview.chromium.org/2846663002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainerLayout.java:455: mAppearingWrapper.removeView(mAppearingWrapper.getItem().getView()); On 2017/05/19 17:21:41, Ted C wrote: > do we need to do this anymore? Technically no. Prior to this change the content was not attached until the infobar came into the foreground; this was put here to return the infobar to that state. It really just depends on whether we are ok with the content view being attached while it's obscured. https://codereview.chromium.org/2846663002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarWrapper.java (right): https://codereview.chromium.org/2846663002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarWrapper.java:29: private int mHeightForAnimation; On 2017/05/19 17:21:41, Ted C wrote: > I would throw a Px suffix on this to make it super clear lower down as well Done.
The CQ bit was checked by mdjones@chromium.org
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/2846663002/#ps160001 (title: "measure happens while adding view")
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": 160001, "attempt_start_ts": 1495487911733400,
"parent_rev": "c8de6a53455b05558640d3a658fd0b874dfe8e6e", "commit_rev":
"58394da4f0c15dce8e8dbe2da801dd34e7e9f95c"}
Message was sent while issue was closed.
Description was changed from ========== Peek new infobars behind existing ones This change extends the peeking behavior when there are multiple infobars attempting to show simultaneously. When a new infobar is added, it will peek above the existing ones giving a brief view of it's contents and bringing more attention to it. BUG=721389 ========== to ========== Peek new infobars behind existing ones This change extends the peeking behavior when there are multiple infobars attempting to show simultaneously. When a new infobar is added, it will peek above the existing ones giving a brief view of it's contents and bringing more attention to it. BUG=721389 Review-Url: https://codereview.chromium.org/2846663002 Cr-Commit-Position: refs/heads/master@{#473719} Committed: https://chromium.googlesource.com/chromium/src/+/58394da4f0c15dce8e8dbe2da801... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/58394da4f0c15dce8e8dbe2da801... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
