[AppBarLayout] Use a uniform way to determine the target scrolling view

[pubiqq]

The view that the AppBarLayout should use to determine whether it should be lifted is set using the setLiftOnScrollTargetView or setLiftOnScrollTargetViewId methods. If the target view is not explicitly set, the AppBarLayout should use a closest scrollable view inside CoordinatorLayout's child with appbar_scrolling_view_behavior (due to the CoordinatorLayout pattern).

Unfortunately, the target view is not determined in this way everywhere, so in some cases we may observe incorrect widget state. This PR fixes this issue and unifies the way the target scrolling view is determined.

Fixes https://github.com/material-components/material-components-android/issues/2591, fixes https://github.com/material-components/material-components-android/issues/3925, fixes https://github.com/material-components/material-components-android/issues/4184.

[dsn5ft]

note that after applying this PR, AppBarLayout in NavigationRailDemoControlsFragment will not be lifted automatically because the nested NestedScrollView doesn't match the condition above

This seems like a change/break in the behavior. I think it's reasonable to expect the developer to provide the liftOnScrollTargetViewId in cases where there is a nested scrolling view (https://github.com/material-components/material-components-android/commit/c69580296e211e4adce1f4d44881f52fc59170da).

[dsn5ft]

Also, in some cases I believe the doubly nested scrolling works without flickering via the standard CoordinatorLayout APIs (and no liftOnScrollTargetViewId), so this PR could be disabling the current lift on scroll effect for those cases if I understand correctly.

[pubiqq]

This seems like a change/break in the behavior. I think it's reasonable to expect the developer to provide the liftOnScrollTargetViewId in cases where there is a nested scrolling view (https://github.com/material-components/material-components-android/commit/c69580296e211e4adce1f4d44881f52fc59170da).

With that comment I just wanted to say that this PR fixes only the flickering issue, but does not lead to the expected lifting of AppBarLayout. Of course, the PR doesn't break the behavior of the component if the liftOnScrollTargetViewId is set.

[pubiqq]

Also, in some cases I believe the doubly nested scrolling works without flickering via the standard CoordinatorLayout APIs (and no liftOnScrollTargetViewId), so this PR could be disabling the current lift on scroll effect for those cases if I understand correctly.

Well, yes, but this is more of an accident than an intended behavior. According to the javadocs, this should only work on AppBarLayout siblings, not on any nested views:

https://github.com/material-components/material-components-android/blob/c69580296e211e4adce1f4d44881f52fc59170da/lib/java/com/google/android/material/appbar/AppBarLayout.java#L101-L103

https://github.com/material-components/material-components-android/blob/c69580296e211e4adce1f4d44881f52fc59170da/lib/java/com/google/android/material/appbar/AppBarLayout.java#L989-L990

https://github.com/material-components/material-components-android/blob/c69580296e211e4adce1f4d44881f52fc59170da/lib/java/com/google/android/material/appbar/AppBarLayout.java#L1069-L1072

[dsn5ft]

The ScrollingViewBehavior itself should definitely be set on the sibling, but technically CoordinatorLayout supports sending up nested scrolling events up the hierarchy via the behavior's APIs.

[pubiqq]

The ScrollingViewBehavior itself should definitely be set on the sibling, but technically CoordinatorLayout supports sending up nested scrolling events up the hierarchy via the behavior's APIs.

It is, but I don't know what you're getting at. setLiftOnScrollTargetView()/setLiftOnScrollTargetViewId() works for any CoordinatorLayout descendant. appbar_scrolling_view_behavior works (and should work) only for CoordinatorLayout children due to the Coordinator pattern + docs.

[dsn5ft]

I'm trying to understand whether this will cause a regression for some cases where appbar_scrolling_view_behavior is set and there is a nested scrolling view (e.g. FrameLayout with appbar_scrolling_view_behavior and NestedScrollView child), but liftOnScrollTargetViewId is not set. Since I believe in some cases that setup works well without flickering. I can also patch the PR and test a few different cases.

[pubiqq]

I'm trying to understand whether this will cause a regression for some cases where appbar_scrolling_view_behavior is set and there is a nested scrolling view (e.g. FrameLayout with appbar_scrolling_view_behavior and NestedScrollView child), but liftOnScrollTargetViewId is not set. Since I believe in some cases that setup works well without flickering. I can also patch the PR and test a few different cases.

In the current implementation you get undefined behavior. Sometimes AppBarLayout will be lifted, sometimes it won't.

After applying the PR, AppBarLayout will never be lifted, because the direct child with appbar_scrolling_view_behavior (FrameLayout) cannot be scrolled vertically (as intended).

--

I think I've started to understand what you're trying to accomplish, but I don't think that's how it's supposed to work, as it goes against the purpose of CoordinatorLayout.Behavior and also doesn't match the documentation.

[dsn5ft]

After applying the PR, AppBarLayout will never be lifted, because the direct child with appbar_scrolling_view_behavior (FrameLayout) cannot be scrolled vertically (as intended).

A FrameLayout with appbar_scrolling_view_behavior and a scrolling view inside of it can be scrolled vertically, or at least the "scroll events" get passed up through the CoordinatorLayout Behavior APIs (I believe this is intended by the CoordinatorLayout / nested scrolling architecture).

Regardless, I believe we have many pre-existing usages set up that way and it's somewhat common knowledge that appbar_scrolling_view_behavior can be placed on a wrapper <FrameLayout/<Fragment (https://stackoverflow.com/questions/59429919/why-does-my-recyclerview-not-scroll-when-placed-inside-an-appbarlayout).

[pubiqq]

Well, it seems the words of a random dude on stackoverflow mean more than the documentation, so I have no choice but to add the desired behavior 😂.

[dsn5ft]

I just linked that one post as an example of the pre-existing behavior which is somewhat like a contract at this point. I am aware of many internal and external usages that follow that pattern.

[pubiqq]

Yes, I got it so I added that behavior in https://github.com/material-components/material-components-android/pull/3926/commits/fba8a749768db775d43aee8398cf9e88430c605e.

Just in case, I rebased the branch again (https://github.com/material-components/material-components-android/commit/b3f08c2f6e282e2406e2aa34a8144d4727a158e8) and updated the description.

[pubiqq]

Bump