Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2065493007:
[Android] Use NTPRecyclerView for peeking card viewport height (Closed)
|
Created:
4 years, 6 months ago by Philipp Keck Modified:
4 years, 6 months ago CC:
chromium-reviews, zine-eng+reviews_google.com Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Android] Use NTPRecyclerView for peeking card viewport height
Instead of using the height of NewTabPageView, use the height of
NewTabPageRecyclerView as the viewport height for the computation
of the peeking card layout animation.
The view structure is: NTPView -> NTPRecyclerView -> Card.
The NTPView has the dimensions of the entire screen (excluding
Android's status bars). The NTPRecyclerView has the dimensions of
the page content. On phones the two coincide, whereas on tablets
the tabs and address bar take additional space from the NTPView,
which is not included in the NTPRecyclerView. To be visible at the
lower edge of the screen, the peeking card (viewholder) needs the
distance of the lower edge from the top of the parent it is rendered
in. The parent is NTPRecyclerView (not NTPView) and the distance is
the height of the parent (aka the height of the viewport).
BUG=617577
Committed: https://crrev.com/e6d15de0bd5f2527dec694f4a72a15182ba289f4
Cr-Commit-Position: refs/heads/master@{#400997}
Patch Set 1 #Patch Set 2 : [Android] Refactor and Use NTPRecyclerView for peeking card viewport height #
Total comments: 15
Patch Set 3 : Comment and code structure improvements #
Total comments: 2
Patch Set 4 : Inline if-return statements #
Messages
Total messages: 25 (7 generated)
pke@google.com changed reviewers: + dgn@chromium.org, peconn@chromium.org
bauerb@chromium.org changed reviewers: + bauerb@chromium.org
The CL subject should be in the imperative mood (see http://chris.beams.io/posts/git-commit/). Also, could you explain in the description _why_ this fixes the bug?
Description was changed from ========== Peeking card shows border on tablets and phones BUG=617577 ========== to ========== [Android] Use NTPRecyclerView for peeking card viewport height Instead of using the height of NewTabPageView, use the height of NewTabPageRecyclerView as the viewport height for the computation of the peeking card layout animation. The view structure is: NTPView -> NTPRecyclerView -> Card. The NTPView has the dimensions of the entire screen (excluding Android's status bars). The NTPRecyclerView has the dimensions of the page content. On phones the two coincide, whereas on tablets the tabs and address bar take additional space from the NTPView, which is not included in the NTPRecyclerView. To be visible at the lower edge of the screen, the peeking card (viewholder) needs the distance of the lower edge from the top of the parent it is rendered in. The parent is NTPRecyclerView (not NTPView) and the distance is the height of the parent (aka the height of the viewport). BUG=617577 ==========
Ok, thank you. I hope it is better now. While the current code works (I tested phone and tablet in both portrait and landscape), it still passes an unnecessary viewportHeight parameter. To me, "viewport" means the screen area that the entire app uses. But the card wants its peek position within its parent, so viewport minus all status or tab bars. For example, if you added a fixed button bar at the bottom (e.g. history and bookmarks buttons), the scrollable part (RecyclerView) would shrink and you'd expect the peeking card to move up (over the new button bar). So the peek position is always calculated using the height of the direct container. The JavaDoc for CardViewHolder#updatePeek(viewportHeight) also says that it's the height of the containing view. But this doesn't need to be passed in (through two functions). To me, the code would be more understandable if it just used "((View)getParent()).getHeight()" instead of the viewportHeight parameter.
Description was changed from ========== [Android] Use NTPRecyclerView for peeking card viewport height Instead of using the height of NewTabPageView, use the height of NewTabPageRecyclerView as the viewport height for the computation of the peeking card layout animation. The view structure is: NTPView -> NTPRecyclerView -> Card. The NTPView has the dimensions of the entire screen (excluding Android's status bars). The NTPRecyclerView has the dimensions of the page content. On phones the two coincide, whereas on tablets the tabs and address bar take additional space from the NTPView, which is not included in the NTPRecyclerView. To be visible at the lower edge of the screen, the peeking card (viewholder) needs the distance of the lower edge from the top of the parent it is rendered in. The parent is NTPRecyclerView (not NTPView) and the distance is the height of the parent (aka the height of the viewport). BUG=617577 ========== to ========== [Android] Use NTPRecyclerView for peeking card viewport height Instead of using the height of NewTabPageView, use the height of NewTabPageRecyclerView as the viewport height for the computation of the peeking card layout animation. The view structure is: NTPView -> NTPRecyclerView -> Card. The NTPView has the dimensions of the entire screen (excluding Android's status bars). The NTPRecyclerView has the dimensions of the page content. On phones the two coincide, whereas on tablets the tabs and address bar take additional space from the NTPView, which is not included in the NTPRecyclerView. To be visible at the lower edge of the screen, the peeking card (viewholder) needs the distance of the lower edge from the top of the parent it is rendered in. The parent is NTPRecyclerView (not NTPView) and the distance is the height of the parent (aka the height of the viewport). BUG=617577 ==========
On 2016/06/15 14:26:12, pke wrote: > Ok, thank you. I hope it is better now. Nice explanation, thanks! > While the current code works (I tested phone and tablet in both portrait and > landscape), it still passes an unnecessary viewportHeight parameter. To me, > "viewport" means the screen area that the entire app uses. But the card wants > its peek position within its parent, so viewport minus all status or tab bars. > For example, if you added a fixed button bar at the bottom (e.g. history and > bookmarks buttons), the scrollable part (RecyclerView) would shrink and you'd > expect the peeking card to move up (over the new button bar). So the peek > position is always calculated using the height of the direct container. > The JavaDoc for CardViewHolder#updatePeek(viewportHeight) also says that it's > the height of the containing view. But this doesn't need to be passed in > (through two functions). To me, the code would be more understandable if it just > used "((View)getParent()).getHeight()" instead of the viewportHeight parameter. I think using "viewport" to mean the area inside which a view could be drawn is fine -- it doesn't have to be the full app area (for example, for a web page the viewport isn't the app area either). I'm not sure we can always assume the the viewport will be the size of the parent (it could be a smaller area, e.g. if the parent itself would contain the bottom toolbar), and in addition the ViewHolder is not actually a child of the RecyclerView. I'm wondering whether we should move this to the RecyclerView though, because we now only look at the RecyclerView anyway.
On 2016/06/15 15:51:40, Bernhard Bauer wrote: > On 2016/06/15 14:26:12, pke wrote: > > Ok, thank you. I hope it is better now. > > Nice explanation, thanks! > > > While the current code works (I tested phone and tablet in both portrait and > > landscape), it still passes an unnecessary viewportHeight parameter. To me, > > "viewport" means the screen area that the entire app uses. But the card wants > > its peek position within its parent, so viewport minus all status or tab bars. > > For example, if you added a fixed button bar at the bottom (e.g. history and > > bookmarks buttons), the scrollable part (RecyclerView) would shrink and you'd > > expect the peeking card to move up (over the new button bar). So the peek > > position is always calculated using the height of the direct container. > > The JavaDoc for CardViewHolder#updatePeek(viewportHeight) also says that it's > > the height of the containing view. But this doesn't need to be passed in > > (through two functions). To me, the code would be more understandable if it > just > > used "((View)getParent()).getHeight()" instead of the viewportHeight > parameter. > > I think using "viewport" to mean the area inside which a view could be drawn is > fine -- it doesn't have to be the full app area (for example, for a web page the > viewport isn't the app area either). > > I'm not sure we can always assume the the viewport will be the size of the > parent (it could be a smaller area, e.g. if the parent itself would contain the > bottom toolbar), and in addition the ViewHolder is not actually a child of the > RecyclerView. I'm wondering whether we should move this to the RecyclerView > though, because we now only look at the RecyclerView anyway. Ok. Yes, sorry, it's not a child. The could would need to be "((View) itemView.getParent()).getHeight()". I see what you mean. Also when the RecyclerView gets a bottom padding, just taking its height would be wrong. I don't understand what you mean by "move this to the RecyclerView". Move the code there? I guess, all the code in CardsLayoutOperations could be moved into the common ancestor of the operations it performs. But probably it's in there to group all the difficult layouting code? Removing the viewportHeight parameter from CardsLayoutOperations#updatePeekingCard() would be reasonable, and then let this layouting code determine that recyclerView.getHeight() is the right measure (until this possibly changes in the future, then the layouting class is the obvious place to make the adjustment).
CardLayoutOperations is just a temporary place for us to store all the layout operations until we can find them somewhere better to go. Do feel free to get rid of updatePeekingCard if there's some place where it would make more sense for it to be (previously they were all in the NewTabPageView which was growing a bit too big).
On 2016/06/15 16:05:57, pke wrote: > On 2016/06/15 15:51:40, Bernhard Bauer wrote: > > On 2016/06/15 14:26:12, pke wrote: > > > Ok, thank you. I hope it is better now. > > > > Nice explanation, thanks! > > > > > While the current code works (I tested phone and tablet in both portrait and > > > landscape), it still passes an unnecessary viewportHeight parameter. To me, > > > "viewport" means the screen area that the entire app uses. But the card > wants > > > its peek position within its parent, so viewport minus all status or tab > bars. > > > For example, if you added a fixed button bar at the bottom (e.g. history and > > > bookmarks buttons), the scrollable part (RecyclerView) would shrink and > you'd > > > expect the peeking card to move up (over the new button bar). So the peek > > > position is always calculated using the height of the direct container. > > > The JavaDoc for CardViewHolder#updatePeek(viewportHeight) also says that > it's > > > the height of the containing view. But this doesn't need to be passed in > > > (through two functions). To me, the code would be more understandable if it > > just > > > used "((View)getParent()).getHeight()" instead of the viewportHeight > > parameter. > > > > I think using "viewport" to mean the area inside which a view could be drawn > is > > fine -- it doesn't have to be the full app area (for example, for a web page > the > > viewport isn't the app area either). > > > > I'm not sure we can always assume the the viewport will be the size of the > > parent (it could be a smaller area, e.g. if the parent itself would contain > the > > bottom toolbar), and in addition the ViewHolder is not actually a child of the > > RecyclerView. I'm wondering whether we should move this to the RecyclerView > > though, because we now only look at the RecyclerView anyway. > > Ok. Yes, sorry, it's not a child. The could would need to be "((View) > itemView.getParent()).getHeight()". > > I see what you mean. Also when the RecyclerView gets a bottom padding, just > taking its height would be wrong. > I don't understand what you mean by "move this to the RecyclerView". Move the > code there? I guess, all the code in CardsLayoutOperations could be moved into > the common ancestor of the operations it performs. But probably it's in there to > group all the difficult layouting code? > Removing the viewportHeight parameter from > CardsLayoutOperations#updatePeekingCard() would be reasonable, and then let this > layouting code determine that recyclerView.getHeight() is the right measure > (until this possibly changes in the future, then the layouting class is the > obvious place to make the adjustment). On 2016/06/15 16:09:27, PEConn1 wrote: > CardLayoutOperations is just a temporary place for us to store all the layout > operations until we can find them somewhere better to go. Do feel free to get > rid of updatePeekingCard if there's some place where it would make more sense > for it to be (previously they were all in the NewTabPageView which was growing a > bit too big). Basically, that. I think now that we're not depending on a property of the NTPView anymore, it makes much more sense for this to live purely in the RecyclerView.
I moved all the code to the recycler view. Is it good like that? The real change (aside from that refactoring) should still be the "ntpView.getHeight()" -> "ntpRecyclerView.getHeight()".
Thanks! https://codereview.chromium.org/2065493007/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java (right): https://codereview.chromium.org/2065493007/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:97: * value of this card) equivalent to no peeking, but top=(viewPortHeight-1) would be I don't really understand this comment. https://codereview.chromium.org/2065493007/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2065493007/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:201: if (viewHolder != null && viewHolder instanceof CardViewHolder I would split this check up and return early if any of the conditions fails. https://codereview.chromium.org/2065493007/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:218: if (header != null) { Same here, return early if it is null. https://codereview.chromium.org/2065493007/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:232: if (viewHolder instanceof SnippetHeaderViewHolder) { And here as well.
https://codereview.chromium.org/2065493007/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java (right): https://codereview.chromium.org/2065493007/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:97: * value of this card) equivalent to no peeking, but top=(viewPortHeight-1) would be On 2016/06/16 12:40:12, Bernhard Bauer wrote: > I don't really understand this comment. I agree that this comment is confusing. I was trying to reflect the results of our previous discussion: We don't simply use getParent().getHeight() because there might be other stuff at the bottom edge (included in the parent's height) that only the parent knows about. Therefore, through this parameter, the parent announces how much space it gives to the viewport that the card is rendered in (possibly less than the parent's height). Maybe I should remove the additional comment here and leave it as is? It's still applicable but the meaning of "viewport" might be blurry. https://codereview.chromium.org/2065493007/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2065493007/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:201: if (viewHolder != null && viewHolder instanceof CardViewHolder On 2016/06/16 12:40:12, Bernhard Bauer wrote: > I would split this check up and return early if any of the conditions fails. I'm not sure I fully understand what you mean by split. Do you mean (structurally): 1) if (!A && B && C) x; else return; should become if (A || !B || !C) { return; } x; or 2) if (!A && B && C) x; else return; should become if (A) return; if (!B) return; if (!C) return; x; or something else? If it's (1) or (2), please explain what the advantage would be. I don't find them to be more readable and they compile to the same code in Java because "&&" is used instead of "&" and Java optimizes for early returns. https://codereview.chromium.org/2065493007/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:218: if (header != null) { On 2016/06/16 12:40:12, Bernhard Bauer wrote: > Same here, return early if it is null. Acknowledged. https://codereview.chromium.org/2065493007/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:232: if (viewHolder instanceof SnippetHeaderViewHolder) { On 2016/06/16 12:40:12, Bernhard Bauer wrote: > And here as well. Acknowledged.
https://codereview.chromium.org/2065493007/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java (right): https://codereview.chromium.org/2065493007/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:97: * value of this card) equivalent to no peeking, but top=(viewPortHeight-1) would be On 2016/06/16 13:06:01, pke wrote: > On 2016/06/16 12:40:12, Bernhard Bauer wrote: > > I don't really understand this comment. > > I agree that this comment is confusing. I was trying to reflect the results of > our previous discussion: We don't simply use getParent().getHeight() because > there might be other stuff at the bottom edge (included in the parent's height) > that only the parent knows about. Therefore, through this parameter, the parent > announces how much space it gives to the viewport that the card is rendered in > (possibly less than the parent's height). > Maybe I should remove the additional comment here and leave it as is? It's still > applicable but the meaning of "viewport" might be blurry. Hm, how about "the height of the containing viewport, i.e. the area inside the containing view that is available for drawing"? https://codereview.chromium.org/2065493007/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2065493007/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:201: if (viewHolder != null && viewHolder instanceof CardViewHolder On 2016/06/16 13:06:01, pke wrote: > On 2016/06/16 12:40:12, Bernhard Bauer wrote: > > I would split this check up and return early if any of the conditions fails. > > I'm not sure I fully understand what you mean by split. Do you mean > (structurally): > > 1) if (!A && B && C) x; else return; > should become > if (A || !B || !C) { return; } x; > > or > > 2) if (!A && B && C) x; else return; > should become > if (A) return; > if (!B) return; > if (!C) return; > x; > > or something else? > > If it's (1) or (2), please explain what the advantage would be. I don't find > them to be more readable and they compile to the same code in Java because "&&" > is used instead of "&" and Java optimizes for early returns. I meant (2). It allows spreading the different conditions out, and it removes one level of indentation from the `x;` part (which is more often than not the "interesting" part of the code). Otherwise you'll very often end up with an indentation "pyramid". https://codereview.chromium.org/2065493007/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:220: // Update the space at the bottom, which needs to know about the height of the header. Nit: Add an empty line before a comment.
https://codereview.chromium.org/2065493007/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java (right): https://codereview.chromium.org/2065493007/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:97: * value of this card) equivalent to no peeking, but top=(viewPortHeight-1) would be On 2016/06/16 13:54:35, Bernhard Bauer wrote: > On 2016/06/16 13:06:01, pke wrote: > > On 2016/06/16 12:40:12, Bernhard Bauer wrote: > > > I don't really understand this comment. > > > > I agree that this comment is confusing. I was trying to reflect the results of > > our previous discussion: We don't simply use getParent().getHeight() because > > there might be other stuff at the bottom edge (included in the parent's > height) > > that only the parent knows about. Therefore, through this parameter, the > parent > > announces how much space it gives to the viewport that the card is rendered in > > (possibly less than the parent's height). > > Maybe I should remove the additional comment here and leave it as is? It's > still > > applicable but the meaning of "viewport" might be blurry. > > Hm, how about "the height of the containing viewport, i.e. the area inside the > containing view that is available for drawing"? Done. https://codereview.chromium.org/2065493007/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2065493007/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:201: if (viewHolder != null && viewHolder instanceof CardViewHolder On 2016/06/16 13:54:35, Bernhard Bauer wrote: > On 2016/06/16 13:06:01, pke wrote: > > On 2016/06/16 12:40:12, Bernhard Bauer wrote: > > > I would split this check up and return early if any of the conditions fails. > > > > I'm not sure I fully understand what you mean by split. Do you mean > > (structurally): > > > > 1) if (!A && B && C) x; else return; > > should become > > if (A || !B || !C) { return; } x; > > > > or > > > > 2) if (!A && B && C) x; else return; > > should become > > if (A) return; > > if (!B) return; > > if (!C) return; > > x; > > > > or something else? > > > > If it's (1) or (2), please explain what the advantage would be. I don't find > > them to be more readable and they compile to the same code in Java because > "&&" > > is used instead of "&" and Java optimizes for early returns. > > I meant (2). It allows spreading the different conditions out, and it removes > one level of indentation from the `x;` part (which is more often than not the > "interesting" part of the code). Otherwise you'll very often end up with an > indentation "pyramid". Ok. Actually the null-check can be dropped because null isn't an instance of CardViewHolder anyway. I noticed that the JavaDoc I wrote is wrong. It doesn't return the first visible card, it just returns the first card if that happens to be visible. So if the first is hidden and the second is visible, it returns null. To make the comment and code simpler, I moved the visibility check out of that method, now the structure of both new find-Methods is the same and also conforms to the structure you suggested. https://codereview.chromium.org/2065493007/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:220: // Update the space at the bottom, which needs to know about the height of the header. On 2016/06/16 13:54:35, Bernhard Bauer wrote: > Nit: Add an empty line before a comment. Done. And I assume this rule doesn't apply elsewhere when the comment is the first line in a block.
https://codereview.chromium.org/2065493007/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2065493007/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:220: // Update the space at the bottom, which needs to know about the height of the header. On 2016/06/16 15:28:28, Philipp Keck wrote: > On 2016/06/16 13:54:35, Bernhard Bauer wrote: > > Nit: Add an empty line before a comment. > > Done. And I assume this rule doesn't apply elsewhere when the comment is the > first line in a block. Exactly :) It's for the case where the comment follows code. https://codereview.chromium.org/2065493007/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2065493007/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:189: return; Nit: You can put the return statement on the previous line and leave the braces out.
Ok I changed that - only for return statements. Btw the android-formatter.xml for Eclipse does not conform to the Android style guide (style guide requires same-line "then statements", formatter puts them on the next line). https://codereview.chromium.org/2065493007/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2065493007/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:189: return; On 2016/06/16 16:39:28, Bernhard Bauer wrote: > Nit: You can put the return statement on the previous line and leave the braces > out. Done.
LGTM, thanks!
The CQ bit was checked by pke@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2065493007/60001
Message was sent while issue was closed.
Description was changed from ========== [Android] Use NTPRecyclerView for peeking card viewport height Instead of using the height of NewTabPageView, use the height of NewTabPageRecyclerView as the viewport height for the computation of the peeking card layout animation. The view structure is: NTPView -> NTPRecyclerView -> Card. The NTPView has the dimensions of the entire screen (excluding Android's status bars). The NTPRecyclerView has the dimensions of the page content. On phones the two coincide, whereas on tablets the tabs and address bar take additional space from the NTPView, which is not included in the NTPRecyclerView. To be visible at the lower edge of the screen, the peeking card (viewholder) needs the distance of the lower edge from the top of the parent it is rendered in. The parent is NTPRecyclerView (not NTPView) and the distance is the height of the parent (aka the height of the viewport). BUG=617577 ========== to ========== [Android] Use NTPRecyclerView for peeking card viewport height Instead of using the height of NewTabPageView, use the height of NewTabPageRecyclerView as the viewport height for the computation of the peeking card layout animation. The view structure is: NTPView -> NTPRecyclerView -> Card. The NTPView has the dimensions of the entire screen (excluding Android's status bars). The NTPRecyclerView has the dimensions of the page content. On phones the two coincide, whereas on tablets the tabs and address bar take additional space from the NTPView, which is not included in the NTPRecyclerView. To be visible at the lower edge of the screen, the peeking card (viewholder) needs the distance of the lower edge from the top of the parent it is rendered in. The parent is NTPRecyclerView (not NTPView) and the distance is the height of the parent (aka the height of the viewport). BUG=617577 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Android] Use NTPRecyclerView for peeking card viewport height Instead of using the height of NewTabPageView, use the height of NewTabPageRecyclerView as the viewport height for the computation of the peeking card layout animation. The view structure is: NTPView -> NTPRecyclerView -> Card. The NTPView has the dimensions of the entire screen (excluding Android's status bars). The NTPRecyclerView has the dimensions of the page content. On phones the two coincide, whereas on tablets the tabs and address bar take additional space from the NTPView, which is not included in the NTPRecyclerView. To be visible at the lower edge of the screen, the peeking card (viewholder) needs the distance of the lower edge from the top of the parent it is rendered in. The parent is NTPRecyclerView (not NTPView) and the distance is the height of the parent (aka the height of the viewport). BUG=617577 ========== to ========== [Android] Use NTPRecyclerView for peeking card viewport height Instead of using the height of NewTabPageView, use the height of NewTabPageRecyclerView as the viewport height for the computation of the peeking card layout animation. The view structure is: NTPView -> NTPRecyclerView -> Card. The NTPView has the dimensions of the entire screen (excluding Android's status bars). The NTPRecyclerView has the dimensions of the page content. On phones the two coincide, whereas on tablets the tabs and address bar take additional space from the NTPView, which is not included in the NTPRecyclerView. To be visible at the lower edge of the screen, the peeking card (viewholder) needs the distance of the lower edge from the top of the parent it is rendered in. The parent is NTPRecyclerView (not NTPView) and the distance is the height of the parent (aka the height of the viewport). BUG=617577 Committed: https://crrev.com/e6d15de0bd5f2527dec694f4a72a15182ba289f4 Cr-Commit-Position: refs/heads/master@{#400997} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e6d15de0bd5f2527dec694f4a72a15182ba289f4 Cr-Commit-Position: refs/heads/master@{#400997} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
