[Tabs] Give MDCTabBarDelegate pass through methods for "willDisplayCell"/"didEndDisplayingCell"

[andrewoverton]

One of the solutions proposed in #6275 is to provide pass through methods for the UICollectionViewDelegate methods -collectionView:willDisplayCell:forItemAtIndexPath: and -collectionView:didEndDisplayingCell:forItemAtIndexPath:. This is the one I chose, mainly because it seemed the most straightforward. One comment said we should be careful about this approach, and another said we needed to look more into the "pre-fetching" behavior of these methods. It seems like a good approach to me... Scrolling through the tabs in the TabBarTextOnlyExample with print statements seems to work how I would expect it to. 👍

[material-automation]

Based on the changes, the title has been prefixed with [Tabs].

[material-automation]

bazel detected changes to the following targets:

//components/AppBar:SwiftExamples
//components/Tabs:ColorThemer
//components/Tabs:FontThemer
//components/Tabs:ObjcExamples
//components/Tabs:SwiftExamples
//components/Tabs:Tabs
//components/Tabs:Theming
//components/Tabs:TypographyThemer
//components/Tabs:snapshot_test_lib
//components/Tabs:snapshot_tests
//components/Tabs:snapshot_tests.swift_runtime_linkopts
//components/Tabs:snapshot_tests_test_binary
//components/Tabs:snapshot_tests_test_bundle
//components/Tabs:unit_test_sources
//components/Tabs:unit_test_theming_sources
//components/Tabs:unit_tests
//components/Tabs:unit_tests_IPAD_PRO_12_9_IN_9_3
//components/Tabs:unit_tests_IPAD_PRO_12_9_IN_9_3.swift_runtime_linkopts
//components/Tabs:unit_tests_IPAD_PRO_12_9_IN_9_3_test_binary
//components/Tabs:unit_tests_IPAD_PRO_12_9_IN_9_3_test_bundle
//components/Tabs:unit_tests_IPHONE_7_PLUS_IN_10_3
//components/Tabs:unit_tests_IPHONE_7_PLUS_IN_10_3.swift_runtime_linkopts
//components/Tabs:unit_tests_IPHONE_7_PLUS_IN_10_3_test_binary
//components/Tabs:unit_tests_IPHONE_7_PLUS_IN_10_3_test_bundle
//components/Tabs:unit_tests_IPHONE_X_IN_11_0
//components/Tabs:unit_tests_IPHONE_X_IN_11_0.swift_runtime_linkopts
//components/Tabs:unit_tests_IPHONE_X_IN_11_0_test_binary
//components/Tabs:unit_tests_IPHONE_X_IN_11_0_test_bundle

[romoore]

Please be careful about choosing this solution. It was discouraged over the recommended alternative because it potentially binds the implementation to UICollectionView.

[romoore]

Please add tests (preferred) or provide testing steps in the commit message.

[mrhappyasthma]

Is there any difference between this PR and the one we closed out a while ago #6276? They look quite similar on the surface.

Or are we just moving forward with this approach as opposed to the alternative suggestion on the GitHub Issue, which would not rely on the underlying UICollectionView delegate methods on the item bar. (Which, if I recall, was one of the more major concerns with the original approach. Since the dependency on UICollectionView is not exposed.)

---

Tangential to this, one caveat with the UICollecitonViewDelegate displaying methods that may be a concern (but is largely unavoidable) is when it's nested in a view that's preloaded off screen.

For example with a scroll view or collection view with paging enabled (https://developer.apple.com/documentation/uikit/uiscrollview/1619432-pagingenabled?language=objc), UIKit seems to create the page before/after the current page in advance, such that the page change animation can take place without requiring any data fetching during the animation.

However these preloads will trigger willDisplayCell: calls on any nested collection views within that page, even if they are outside the bounds of the current window. (For instance, this would occur if you had an MDCTabBar/MDCItemBar on one of those pages before/after the current page.)

I'm not sure there's really a good way to do anything about this, and it's an unlikely thing to occur since most tab bars are used as top-level elements rather than nested elements, but it is something that occurs in my team's project today. So it's worth mentioning here just as an FYI.

Note: It's not a blocking issue since it occurs for any nested collection view and isn't an inherent problem to do with any of the MDC* classes. We have a workaround to handle this in our project in a more general way for any offscreen preloaded collection view.

[romoore]

@mrhappyasthma @andrewoverton I believe Mark's concerns are the reason we were leaning toward exploiting UIScrollView APIs and expect the client to inspect the tab item's view's frame to determine whether it is visible (and how much of it). The major drawback to that approach would potentially be performance-based, so an evaluation of the performance impact of that solution is justified. If the expense of the UIScrollView-based is such that it prohibits our using it, then we'll have to stick with UICollectionView for now.

[andrewoverton]

Hi @mrhappyasthma, I either didn't know or forgot about #6276! The differences look very minimal to me :D

I don't think I share the concern about binding the implementation with UICollectionView. These APIs don't seem any more bound to UICollectionView than -itemBar:shouldSelectItem:, for example. As far as I know, the talk of removing UICollectionView from MDCTabs is hypothetical.

As for the paging/fetching stuff--your comment helps clarify these concerns for me. I'm glad to hear you have a way of dealing with the "nested collection view in a scrollview with paging enabled" behavior.

This approach seemed fine to me initially, and I still wouldn't say I'm against it, but I'm happy to tinker with the other approach if everybody prefers it.

[mrhappyasthma]

I'm perfectly happy with this approach too, as it would unblock my team. These same underlying delegate calls were how things were done for the old tab bar implementation internally. (Hence my original PR to port over that behavior to mirror the behavior of the deprecated Google tab bar).

So if the project stakeholders are fine with this PR, then I'm all for it.

I just recall there being valid concerns before, so figured I'd add some context to explain where issues can (and will) occur in the nuanced usage of the tab bar with the delegate methods as proposed.

For my team, we handle these issues automatically as a result of having used the old tab bar implementation so it wouldn't be an issue. But other clients of the API might be subjected to this edge case. On the other hand, the approach that Rob mentioned would make it more transparent, since it would be on the client of the tab bar to determine visibility, but requires more work for the general case.

[codeman7]

This PR has been reverted due to internal failures.

[jverkoey]

@andrewoverton Is there an issue we can reopen for this PR?

[jverkoey]

Looks like https://github.com/material-components/material-components-ios/issues/6275 is the one.