[iOS] Fix unresponsive NTP scrolling with container [chromium/src : main]
Adam Arcaro (Gerrit)
Attention is currently required from: Scott Yoder.
Adam Arcaro would like Scott Yoder to review this change.
[iOS] Fix unresponsive NTP scrolling with container
Feed container constraints would lock when the header reached the
bottom of the NTP, preventing the NTP from being scrollable. This was
because the container had an anchor to the top of the header and to the
bottom of the view, which would eventually overlap and break.
Instead of anchoring the container to the bottom of the view, we use
a height anchor with max value to ensure it covers the remaidner of the
NTP.
Fixed: 1503675
Change-Id: Iad24068a0ecf7eeb87de3b7ab6e834d233211037
---
M ios/chrome/browser/ui/ntp/new_tab_page_view_controller.mm
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/ios/chrome/browser/ui/ntp/new_tab_page_view_controller.mm b/ios/chrome/browser/ui/ntp/new_tab_page_view_controller.mm
index 82c91d4c..3188b1a 100644
--- a/ios/chrome/browser/ui/ntp/new_tab_page_view_controller.mm
+++ b/ios/chrome/browser/ui/ntp/new_tab_page_view_controller.mm
@@ -430,7 +430,6 @@
// Reduce the zPosition so that the container appears behind the feed
// content.
_feedContainer.layer.zPosition = -1;
- _feedContainer.userInteractionEnabled = NO;
// Add corner radius to the top border.
_feedContainer.clipsToBounds = YES;
@@ -1480,8 +1479,7 @@
constraintEqualToAnchor:self.collectionView.centerXAnchor],
[_feedContainer.topAnchor
constraintEqualToAnchor:self.feedHeaderViewController.view.topAnchor],
- [_feedContainer.bottomAnchor
- constraintEqualToAnchor:self.view.bottomAnchor],
+ [_feedContainer.heightAnchor constraintEqualToConstant:CGFLOAT_MAX],
]];
}
To view, visit change 5054382. To unsubscribe, or for help writing mail filters, visit settings.
Adam Arcaro (Gerrit)
Attention is currently required from: Scott Yoder.
Patch set 1:Commit-Queue +1
2 comments:
Patchset:
Hey Scott, let me know what you think of this solution. More details in the CL description
File ios/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:
Patch Set #1, Line 433:
_feedContainer.userInteractionEnabled = NO;Removed because this was unnecessary with the zPosition above.
To view, visit change 5054382. To unsubscribe, or for help writing mail filters, visit settings.
findit-for-me@appspot.gserviceaccount.com (Gerrit)
Attention is currently required from: Scott Yoder.
Scott Yoder (Gerrit)
Attention is currently required from: Adam Arcaro.
1 comment:
File ios/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:
Patch Set #1, Line 1482:
[_feedContainer.heightAnchor constraintEqualToConstant:CGFLOAT_MAX],Is this necessary to fix the issue? It seems like removing the `userInteractionEnabled = NO` above might be the fix.
I don't know of any specific reason that constraining to CGFLOAT_MAX is bad, but I worry a little about it, and I'm not sure what it achieves here. Could you constrain the bottom to self.view.bottomAnchor + X pixels so that it looks like it extends forever?
To view, visit change 5054382. To unsubscribe, or for help writing mail filters, visit settings.
Adam Arcaro (Gerrit)
Attention is currently required from: Scott Yoder.
1 comment:
File ios/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:
Patch Set #1, Line 1482:
[_feedContainer.heightAnchor constraintEqualToConstant:CGFLOAT_MAX],
Is this necessary to fix the issue? It seems like removing the `userInteractionEnabled = NO` above m […]
Removing `userInteractionEnabled = NO` did not fix the issue, it's just something I found to be unnecessary while debugging this so I cleaned it out.
If you try to repro this locally, you'll notice the scroll gets stuck at the point where the top of feed header meets the bottom of the screen. This is because the container is anchored to the top of the feed header and the bottom of `self.view`, which overlap as you scroll. The view won't allow the bottom anchor to be above the top anchor, causing the UI to becomes unresponsive.
Since the top and bottom anchors alternate, I figured it would be best to avoid a bottom anchor in favor of a height anchor. I chose CGFLOAT_MAX to ensure that it always covers the remainder of the NTP height.
Hopefully this clears things up, lmk what you think!
To view, visit change 5054382. To unsubscribe, or for help writing mail filters, visit settings.
Scott Yoder (Gerrit)
Attention is currently required from: Adam Arcaro.
1 comment:
File ios/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:
Patch Set #1, Line 1482:
[_feedContainer.heightAnchor constraintEqualToConstant:CGFLOAT_MAX],
Removing `userInteractionEnabled = NO` did not fix the issue, it's just something I found to be unne […]
I just worry about having a very very large view. For example, does iOS allocate any sort of rendering buffers that grow in relation to the size of the view? I have no idea, but it seems like it could.
Would it be possible to constrain the height to the height of the view + X (rather than the bottom)? Something so the height isn't nearly infinite.
To view, visit change 5054382. To unsubscribe, or for help writing mail filters, visit settings.
Adam Arcaro (Gerrit)
Attention is currently required from: Scott Yoder.
1 comment:
File ios/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:
Patch Set #1, Line 1482:
[_feedContainer.heightAnchor constraintEqualToConstant:CGFLOAT_MAX],
I just worry about having a very very large view. […]
Ahh I see what you mean, agreed that a view with CGFLOAT_MAX height can be concerning then. However we can't predict the height of the feed since the content is paginated on scroll, and setting the anchor to the view's height would only cover the first ~1000px or so. I can add some arbitrary high number that will always be larger than the feed height (like 30000), but that feels a bit hacky. Any thoughts?
To view, visit change 5054382. To unsubscribe, or for help writing mail filters, visit settings.
Scott Yoder (Gerrit)
Attention is currently required from: Adam Arcaro.
1 comment:
File ios/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:
Patch Set #1, Line 1482:
[_feedContainer.heightAnchor constraintEqualToConstant:CGFLOAT_MAX],
Ahh I see what you mean, agreed that a view with CGFLOAT_MAX height can be concerning then. […]
Could you constrain the height to the height of some view that is the feed + 1000px? Or could you constrain the bottom to self.view (like before) but use a lower priority, and then add a constraint greaterThanOrEqualTo on height to set a minimum height?
To view, visit change 5054382. To unsubscribe, or for help writing mail filters, visit settings.
Adam Arcaro (Gerrit)
Attention is currently required from: Adam Arcaro.
Adam Arcaro uploaded patch set #2 to this change.
[iOS] Fix unresponsive NTP scrolling with container
Feed container constraints would lock when the header reached the
bottom of the NTP, preventing the NTP from being scrollable. This was
because the container had an anchor to the top of the header and to the
bottom of the view, which would eventually overlap and break.
Instead of anchoring the container to the bottom of the view, we use
a height anchor with max value to ensure it covers the remaidner of the
NTP.
Fixed: 1503675
Change-Id: Iad24068a0ecf7eeb87de3b7ab6e834d233211037
---
M ios/chrome/browser/ui/ntp/new_tab_page_view_controller.mm
1 file changed, 24 insertions(+), 3 deletions(-)
To view, visit change 5054382. To unsubscribe, or for help writing mail filters, visit settings.
Adam Arcaro (Gerrit)
Attention is currently required from: Scott Yoder.
Patch set 2:Commit-Queue +1
1 comment:
File ios/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:
Patch Set #1, Line 1482:
[_feedContainer.heightAnchor constraintEqualToConstant:CGFLOAT_MAX],
Could you constrain the height to the height of some view that is the feed + 1000px? Or could you co […]
I found a solution that's a little more complex than the original one, but I think is robust and will prevent any unnecessarily large container. I set a height constraint initially, and update it any time content is added to the feed (through the `feedLayoutDidEndUpdates` callback). The feed height changes dynamically on scroll, so just anchoring the container to the initial height + 1000px wouldn't cover the entire feed. Lmk what you think!
To view, visit change 5054382. To unsubscribe, or for help writing mail filters, visit settings.
Adam Arcaro (Gerrit)
Attention is currently required from: Scott Yoder.
Adam Arcaro uploaded patch set #3 to this change.
[iOS] Fix unresponsive NTP scrolling with container
Feed container constraints would lock when the header reached the
bottom of the NTP, preventing the NTP from being scrollable. This was
because the container had an anchor to the top of the header and to the
bottom of the view, which would eventually overlap and break.
Instead of anchoring the container to the bottom of the view, we use a height anchor that's updated whenever the feed's content is changed.
Fixed: 1503675
Change-Id: Iad24068a0ecf7eeb87de3b7ab6e834d233211037
---
M ios/chrome/browser/ui/ntp/new_tab_page_view_controller.mm
1 file changed, 24 insertions(+), 3 deletions(-)
To view, visit change 5054382. To unsubscribe, or for help writing mail filters, visit settings.
Adam Arcaro (Gerrit)
Attention is currently required from: Adam Arcaro, Scott Yoder.
Adam Arcaro uploaded patch set #4 to this change.
The following approvals got outdated and were removed: Commit-Queue+1 by Adam Arcaro
[iOS] Fix unresponsive NTP scrolling with container
Feed container constraints would lock when the header reached the
bottom of the NTP, preventing the NTP from being scrollable. This was
because the container had an anchor to the top of the header and to the
bottom of the view, which would eventually overlap and break.
Instead of anchoring the container to the bottom of the view, we use a height anchor that's updated whenever the feed's content is changed.
Fixed: 1503675
Change-Id: Iad24068a0ecf7eeb87de3b7ab6e834d233211037
---
M ios/chrome/browser/ui/ntp/new_tab_page_view_controller.mm
1 file changed, 30 insertions(+), 5 deletions(-)
To view, visit change 5054382. To unsubscribe, or for help writing mail filters, visit settings.
Adam Arcaro (Gerrit)
Attention is currently required from: Adam Arcaro, Scott Yoder.
Adam Arcaro uploaded patch set #5 to this change.
[iOS] Fix unresponsive NTP scrolling with container
Feed container constraints would lock when the header reached the
bottom of the NTP, preventing the NTP from being scrollable. This was
because the container had an anchor to the top of the header and to the
bottom of the view, which would eventually overlap and break.
Instead of anchoring the container to the bottom of the view, we use a height anchor that's updated whenever the feed's content is changed.
Fixed: 1503675
Change-Id: Iad24068a0ecf7eeb87de3b7ab6e834d233211037
---
M ios/chrome/browser/ui/ntp/new_tab_page_view_controller.mm
1 file changed, 26 insertions(+), 3 deletions(-)
To view, visit change 5054382. To unsubscribe, or for help writing mail filters, visit settings.
Adam Arcaro (Gerrit)
Attention is currently required from: Adam Arcaro, Scott Yoder.
Adam Arcaro uploaded patch set #6 to this change.
[iOS] Fix unresponsive NTP scrolling with container
Feed container constraints would lock when the header reached the
bottom of the NTP, preventing the NTP from being scrollable. This was
because the container had an anchor to the top of the header and to the
bottom of the view, which would eventually overlap and break.
Instead of anchoring the container to the bottom of the view, we use a height anchor that's updated whenever the feed's content is changed.
Fixed: 1503675
Change-Id: Iad24068a0ecf7eeb87de3b7ab6e834d233211037
---
M ios/chrome/browser/ui/ntp/new_tab_page_view_controller.mm
1 file changed, 33 insertions(+), 3 deletions(-)
To view, visit change 5054382. To unsubscribe, or for help writing mail filters, visit settings.
Adam Arcaro (Gerrit)
Attention is currently required from: Adam Arcaro, Scott Yoder.
Adam Arcaro uploaded patch set #7 to this change.
[iOS] Fix unresponsive NTP scrolling with container
Feed container constraints would lock when the header reached the
bottom of the NTP, preventing the NTP from being scrollable. This was
because the container had an anchor to the top of the header and to the
bottom of the view, which would eventually overlap and break.
Instead of anchoring the container to the bottom of the view, we use a height anchor that's updated whenever the feed's content is changed.
Fixed: 1503675
Change-Id: Iad24068a0ecf7eeb87de3b7ab6e834d233211037
---
M ios/chrome/browser/ui/ntp/new_tab_page_view_controller.mm
1 file changed, 33 insertions(+), 3 deletions(-)
To view, visit change 5054382. To unsubscribe, or for help writing mail filters, visit settings.
Scott Yoder (Gerrit)
Attention is currently required from: Adam Arcaro.
2 comments:
File ios/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:
if (self.feedContainerHeightConstraint) {
[NSLayoutConstraint
deactivateConstraints:@[ self.feedContainerHeightConstraint ]];
}An alternative option: if you rewrite it like below you don't need to nil check it because the call to `setActive:` will just silently not happen when `self.feedContainerHeightConstraint` is nil.
```suggestion
self.feedContainerHeightConstraint.active = NO;
```Patch Set #7, Line 1629:
[_feedContainer.heightAnchor constraintEqualToConstant:containerHeight];Is there any way to constrain to a height anchor that is the height of the content in collectionView? If you could do that and then add a constant that is greater than the size of the feed header and top section, then you could set the constraint once and never have to update it. When you constrain to a heightAnchor, the constraint updates as the view height updates.
After looking around, I wonder if you could do something like:
```
[_feedContainer.heightAnchor
constraintEqualTo:self.collectionView.contentLayoutGuide.heightAnchor
constant:kMaxFeedHeaderHeightPlusFeedTopSectionHeight];
```
To view, visit change 5054382. To unsubscribe, or for help writing mail filters, visit settings.
Adam Arcaro (Gerrit)
Attention is currently required from: Adam Arcaro.
Adam Arcaro uploaded patch set #8 to this change.
[iOS] Fix unresponsive NTP scrolling with container
Feed container constraints would lock when the header reached the
bottom of the NTP, preventing the NTP from being scrollable. This was
because the container had an anchor to the top of the header and to the
bottom of the view, which would eventually overlap and break.
Instead of anchoring the container to the bottom of the view, we use a height anchor that's updated whenever the feed's content is changed.
Fixed: 1503675
Change-Id: Iad24068a0ecf7eeb87de3b7ab6e834d233211037
---
M ios/chrome/browser/ui/ntp/new_tab_page_view_controller.mm
1 file changed, 29 insertions(+), 3 deletions(-)
To view, visit change 5054382. To unsubscribe, or for help writing mail filters, visit settings.
Adam Arcaro (Gerrit)
Attention is currently required from: Scott Yoder.
2 comments:
File ios/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:
if (self.feedContainerHeightConstraint) {
[NSLayoutConstraint
deactivateConstraints:@[ self.feedContainerHeightConstraint ]];
}
An alternative option: if you rewrite it like below you don't need to nil check it because the call […]
That's much cleaner, thanks!
Patch Set #7, Line 1629:
[_feedContainer.heightAnchor constraintEqualToConstant:containerHeight];
Is there any way to constrain to a height anchor that is the height of the content in collectionView […]
I would love for this to work, but unfortunately anchoring to the collection view's height anchor doesn't update itself when content is added. Some online research suggests that matching a UICollectionView's `contentSize.height` needs to be done manually, through callbacks like `viewDidLayoutSubviews`, or in this case, `feedLayoutDidEndUpdates`.
To view, visit change 5054382. To unsubscribe, or for help writing mail filters, visit settings.
Scott Yoder (Gerrit)
Attention is currently required from: Adam Arcaro.
Patch set 8:Code-Review +1
1 comment:
File ios/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:
Patch Set #7, Line 1629:
[_feedContainer.heightAnchor constraintEqualToConstant:containerHeight];I would love for this to work, but unfortunately anchoring to the collection view's height anchor do […]
Ack. I hate that we need this complication, but it seems like the feed somehow isn't playing nice with contentLayoutGuide on pagination.
To view, visit change 5054382. To unsubscribe, or for help writing mail filters, visit settings.
Adam Arcaro (Gerrit)
Patch set 8:Commit-Queue +2
2 comments:
Patchset:
Thanks Scott
File ios/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:
Patch Set #1, Line 1482:
[_feedContainer.heightAnchor constraintEqualToConstant:CGFLOAT_MAX],
I found a solution that's a little more complex than the original one, but I think is robust and wil […]
Done
To view, visit change 5054382. To unsubscribe, or for help writing mail filters, visit settings.
findit-for-me@appspot.gserviceaccount.com (Gerrit)
Attention is currently required from: Adam Arcaro.
This change will be blocked from submission as there are files which do not meet the coverage criteria. Following files have incremental coverage(all tests) < 60%.
- //ios/chrome/browser/ui/ntp/new_tab_page_view_controller.mm
Please add tests for uncovered lines, or add Low-Coverage-Reason:<reason> in the change description to bypass. See https://bit.ly/46jhjS9 to understand when it is okay to bypass. If you think coverage is underreported, file a bug at https://bit.ly/3ENM7Pe
Patch set 8:Code-Coverage -1
The change is no longer submittable: Code-Coverage is unsatisfied now.
Adam Arcaro (Gerrit)
Attention is currently required from: Adam Arcaro.
Adam Arcaro uploaded patch set #9 to this change.
[iOS] Fix unresponsive NTP scrolling with container
Feed container constraints would lock when the header reached the
bottom of the NTP, preventing the NTP from being scrollable. This was
because the container had an anchor to the top of the header and to the
bottom of the view, which would eventually overlap and break.
Instead of anchoring the container to the bottom of the view, we use a height anchor that's updated whenever the feed's content is changed.
Low-Coverage-Reason:This logic relies on the feed content loading, which is not possible to cover in upstream tests.
Fixed: 1503675
Change-Id: Iad24068a0ecf7eeb87de3b7ab6e834d233211037
---
M ios/chrome/browser/ui/ntp/new_tab_page_view_controller.mm
1 file changed, 29 insertions(+), 3 deletions(-)
To view, visit change 5054382. To unsubscribe, or for help writing mail filters, visit settings.
Adam Arcaro (Gerrit)
Patch set 9:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
8 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
[iOS] Fix unresponsive NTP scrolling with container
Feed container constraints would lock when the header reached the
bottom of the NTP, preventing the NTP from being scrollable. This was
because the container had an anchor to the top of the header and to the
bottom of the view, which would eventually overlap and break.
Instead of anchoring the container to the bottom of the view, we use a height anchor that's updated whenever the feed's content is changed.
Low-Coverage-Reason: This logic relies on the feed content loading, which is not possible to cover in upstream tests.
Fixed: 1503675
Change-Id: Iad24068a0ecf7eeb87de3b7ab6e834d233211037
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5054382
Reviewed-by: Scott Yoder <scott...@google.com>
Commit-Queue: Adam Arcaro <ada...@google.com>
Cr-Commit-Position: refs/heads/main@{#1230766}
---
M ios/chrome/browser/ui/ntp/new_tab_page_view_controller.mm
1 file changed, 29 insertions(+), 3 deletions(-)