Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scroll doesn't reset when switching the tabs while scrolled to top #12

Closed
chichkanov opened this issue Sep 17, 2024 · 14 comments
Closed
Labels
enhancement New feature or request

Comments

@chichkanov
Copy link

chichkanov commented Sep 17, 2024

Steps to repro:

  1. Create two tabs with a scrollable content
  2. Scroll first tab content
  3. Go to the second tab and scroll to top
  4. Go back to the first tab

Issues:

  1. I believe scroll should be reset. This is how it works in X and Instagram
  2. If I scroll to top there is some janky header animation

Was testing the latest main branch

Sample

enum Tab: Hashable {
    case first
    case second
}

struct ContentView: View {
    
    @State var selectedTab: Tab = .first
    
    var body: some View {
        MaterialTabs(
            selectedTab: $selectedTab,
            headerTitle: { context in
                VStack {
                    Text("Material Tabs")
                        .font(.title)
                    
                    Text("Some long long description bla\nsecond line of text bla bla bal")
                        .font(.callout)
                        .foregroundStyle(.secondary)
                }
                .padding()
                .headerStyle(OffsetHeaderStyle(fade: true), context: context)
            },
            headerTabBar: { context in
                MaterialTabBar(selectedTab: $selectedTab, sizing: .equalWidth, context: context)
            },
            headerBackground: { context in
                Color.white
            },
            content: {
                firstTabContent()
                secondTabContent()
            }
        )
    }
    
    @ViewBuilder private func firstTabContent() -> some View {
        MaterialTabsScroll(tab: Tab.first) { _ in
            LazyVStack {
                ForEach(0..<100) { index in
                    Text("Row \(index)")
                        .padding()
                }
            }
        }
        .materialTabItem(tab: Tab.first, label: .secondary("First"))
    }
    
    @ViewBuilder private func secondTabContent() -> some View {
        MaterialTabsScroll(tab: Tab.second) { _ in
            LazyVStack {
                ForEach(0..<100) { index in
                    Text("Row \(index)")
                        .padding()
                }
            }
        }
        .materialTabItem(tab: Tab.second, label: .secondary("Second"))
    }
}
Simulator.Screen.Recording.-.iPhone.16.Pro.Max.-.2024-09-17.at.12.02.31.mp4
@wtmoose
Copy link
Member

wtmoose commented Sep 17, 2024

Oof. Thanks for finding that. Seems this was a bug since iOS 17 that we never noticed. I'm looking into it

@wtmoose
Copy link
Member

wtmoose commented Sep 17, 2024

This one should be fixed now if you'd like to take another look

@chichkanov
Copy link
Author

Hey @wtmoose . This doesn't seem to be fixed on the latest main branch

Simulator.Screen.Recording.-.iPhone.16.Pro.Max.-.2024-09-18.at.12.56.14.mp4

@wtmoose
Copy link
Member

wtmoose commented Sep 18, 2024

@chichkanov the video you attached is behaving as designed and the header is not jumping as in the original video. Could you describe what behavior you're seeing vs what you expect?

From my perspective, when you return to the first tab, you're in an edge case where the header is full size but the scroll view is not at the top. From there, the header behavior depends on scroll direction:

  1. When you scroll down (a.k.a. swipe up), the header shrinks—the header always shrinks when you scroll down.
  2. When you scroll up (a.k.a. swipe down), the header maintains the current size until the top of the scroll view content meets the bottom of the header. If you continue to scroll up from there, the header expands.

@wtmoose wtmoose reopened this Sep 18, 2024
@chichkanov
Copy link
Author

chichkanov commented Sep 18, 2024

Hey! Sure, yeah.

Here is a video which illustrated my expected behavior.

I think that whenever user started scrolling back and header becomes slightly visible, the scroll should be reset to zero on all tabs

trim.7987FFB3-B1D2-432D-B553-B9AB98172036.MOV

@wtmoose
Copy link
Member

wtmoose commented Sep 18, 2024

I've discussed with the team internally and we think the current behavior is ideal.

What you're showing in the above video is poor UX in my experience—user's don't want the app to automatically change their scroll position. If I scroll to something on Replies, the app should not reset that if I'm on another tab.

@chichkanov
Copy link
Author

chichkanov commented Sep 18, 2024

I've discussed with the team internally and we think the current behavior is ideal.

What you're showing in the above video is poor UX in my experience—user's don't want the app to automatically change their scroll position. If I scroll to something on Replies, the app should not reset that if I'm on another tab.

Interesting, I actually expect the opposite. But maybe because I use the apps where header can become pretty large, so it's gonna be a very bad UX if the scroll would've persisted when scrolled to top.
Do you have any example of the app which doesn't have this "poor UX"?

@wtmoose
Copy link
Member

wtmoose commented Sep 18, 2024

If you think about a normal tabbed UI, such as the App Store app, each tab is logically independent from other tabs. I'm guessing that we would all agree, that scrolling on one tab would never affect scrolling on any other tab. The fact that we''re introducing an element outside of the tabs that can change size, doesn't fundamentally change the independence of the tabs. At least I can't think of a logical reason why it should.

If Android works as you're proposing (I don't have an Android device handy) or if this is common other iOS apps, I would say that this is a bad choice being propagated unless you can provide a compelling logical argument why the behavior is better.

@chichkanov
Copy link
Author

chichkanov commented Sep 18, 2024

If Android works as you're proposing (I don't have an Android device handy) or if this is common other iOS apps, I would say that this is a bad choice being propagated unless you can provide a compelling logical argument why the behavior is better.

I didn't say Android works this way. I am using iOS and I am used to this experience. I got your point, sounds good

If you think about a normal tabbed UI, such as the App Store app, each tab is logically independent from other tabs

I think this is a very wrong example since top tabs are very different from the TabBar

At least I can't think of a logical reason why it should.

In my point of view if the user scrolled to the very top of the page, it's not an expected behavior to preserve the scroll position for all the other tabs

@wtmoose
Copy link
Member

wtmoose commented Sep 18, 2024

I saw that you posted and deleted a response. That response had a valid counter-argument that you didn't include in your revised response.

Specifically, if the header is unusually tall, you can get in a situation where the user is stuck with a small scroll region. I'm going to label this ticket as an enhancement. I don't intend to change the default behavior, but I'm considering adding a configuration option.

If other people are reading this and disagree, chime in and make your case.

@wtmoose wtmoose added the enhancement New feature or request label Sep 18, 2024
@chichkanov
Copy link
Author

chichkanov commented Sep 18, 2024

That response had a valid counter-argument that you didn't include in your revised response.

It's not a counter-argument. I deleted it because it wasn't making sense since if the header is too tall, user wouldn't be able to scroll properly in the first place.

@wtmoose
Copy link
Member

wtmoose commented Sep 18, 2024

It's not a counter-argument. I deleted it because it wasn't making sense since if the header is too tall, user wouldn't be able to scroll properly in the first place.

I'd say don't make headers so tall. But I do think it makes sense as a counter argument. In that scenario:

  1. You can freely scroll down (swipe up) because the header will shrink. Not a problem. Ideally, you could scroll by swiping in the header. However, SwiftUIMaterialTabs doesn't support this due to SwiftUI limitations. I can support this by dipping into UIKit (which I've tried to to avoid) so I added Support swiping in the header to scroll #16 for it.
  2. You can't freely scroll up (swipe down), because you're stuck scrolling in a small space. This is a legit problem if the header usually tall.

@wtmoose
Copy link
Member

wtmoose commented Sep 19, 2024

OK this is implemented. I added a MaterialTabsConfig struct that you can optionally supply when constructing your MaterialTabs. I added two new behaviors and changed the default behavior. The behavior you requested is not the default, so you'll need to set crossTabSyncMode = .resetScrollPosition. If you want to take a look, I'm interested in your feedback.

public struct MaterialTabsConfig: Equatable {

    /// Specifies how the scroll view and header adjust to maintain continuity when switching tabs. These options affect one specific scenario. Suppose
    /// we have two tabs A and B and a collapsible title view:
    ///
    ///   1. User scrolls down (a.k.a swipes up) in tab A such that the title view is collapsed.
    ///   2. User switches to tab B and scrolls up (a.k.a swipes down) until the title view is expanded
    ///   3. User switches back to tab A
    ///
    /// Since the title view is expanded on tab A, but it would have naturally been collapsed, there are multiple strategies for how continuitity is preserved.
    public enum CrossTabSyncMode: Equatable {
        /// Preserves the scroll position relative to the header. If the header has moved up, the scroll view is moved up. If the header is moved down, the scroll
        /// view is moved down. The benefit of this approach is preserving the user's scroll position. The down side is that the header is expanded when it
        /// should be collapsed and will remain expanded if the user scrolls up, which could severely limit the space for scroll view content if the header
        /// is unusually tall.
        case preserveScrollPosition
        /// Resets the scroll position to align the top of the scroll view content is aligned with the bottom of the header. This is how many apps behave and
        /// it ensures that scroll position and title collapse state are always in sync. The down side is that the user's previous scroll position is lost.
        case resetScrollPosition
        /// Initially preserves the scroll position the same as `preserveContentOffset`. However, if the user scrolls, the title collapse state is
        /// animated to where match the scroll position. This option introduces a title view animation, but eliminates the down sides of other options.
        /// This is the default behavior.
        case resetTitleOnScroll(_ animation: Animation = .snappy(duration: 0.3))
    }

    public var crossTabSyncMode: CrossTabSyncMode = .resetTitleOnScroll()

    public init(crossTabSyncMode: CrossTabSyncMode = .resetTitleOnScroll()) {
        self.crossTabSyncMode = crossTabSyncMode
    }
}

@wtmoose wtmoose closed this as completed Sep 22, 2024
@wtmoose
Copy link
Member

wtmoose commented Sep 22, 2024

2.0.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants