Skip to content

Conversation

@Piinks
Copy link
Contributor

@Piinks Piinks commented Feb 16, 2021

This adds support for pointer scrolling to be able to trigger the floating and snapping effects of SliverPersistentHeaders, SliverAppBars, etc.

Fixes #75059
Unblocks #71322 / #75728

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Feb 16, 2021
@google-cla google-cla bot added the cla: yes label Feb 16, 2021
@Piinks Piinks added a: desktop Running on desktop a: mouse Issues related to using a mouse or mouse support f: scrolling Viewports, list views, slivers, etc. platform-web Web applications specifically a: fidelity Matching the OEM platforms better a: quality A truly polished experience labels Feb 16, 2021
@Piinks Piinks changed the title B75059 Add support for pointer scrolling to trigger floats & snaps Feb 16, 2021
@Piinks Piinks requested a review from chunhtai February 16, 2021 20:27
@chunhtai
Copy link
Contributor

This pr does not seem to fix the repro in the original issue

import 'package:flutter/material.dart';
void main() {
  runApp(SliverPesistentHeaderScrollUpMouse());
}

class SliverPesistentHeaderScrollUpMouse extends StatelessWidget {

  @override
  Widget build(BuildContext context) {
    List<Widget> _widgets = [];
    for (int i = 0; i < 200; i++) {
      _widgets.add(
          Container(
            height: 10,
            color: Colors.blue,
            child: Text('$i'),
          )
      );
    }
    return MaterialApp(
        title: 'Default label',
        home: Scaffold(
            body: CustomScrollView(
              slivers: [
                SliverPersistentHeader(
                  floating: true,
                  delegate: Header(),
                ),
                SliverList(
                  delegate: SliverChildListDelegate(
                      _widgets
                  ),
                )
              ],
            )
        )
    );
  }
}

class Header extends SliverPersistentHeaderDelegate {

  @override
  Widget build(BuildContext context, double shrinkOffset, bool overlapsContent) {
    return Container(
      height: 50,
      color: Colors.red,
    );
  }

  @override
  double get maxExtent => 50;

  @override
  double get minExtent => 50;

  @override
  bool shouldRebuild(SliverPersistentHeaderDelegate oldDelegate) {
    return false;
  }
}

I have tried on macos and flutter for web

@Piinks
Copy link
Contributor Author

Piinks commented Feb 17, 2021

This pr does not seem to fix the repro in the original issue

Oh wow that's bizarre, sorry about that. Let me see what is going on.

@Piinks
Copy link
Contributor Author

Piinks commented Feb 17, 2021

Aah I see, I forgot SliverPersistentHeader and SliverAppBar eventually use the same RenderSliverFloatingPersistentHeader, but SliverAppBar adds the scrolling listener in _FloatingAppBar, while SliverPersistentHeader does not.
NICE catch. 🙏 💯
I'll update.

@Piinks
Copy link
Contributor Author

Piinks commented Feb 17, 2021

All set now I think. :) PTAL

}

void _isScrollingListener() {
if (_position == null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert instead?

}
}

class _FloatingHeader extends StatefulWidget {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this merge into the _SliverFloatingPersistentHeader and _SliverFloatingPinnedPersistentHeader, probably as mixin? you may need to change the two widgets to statefulwidget or somehow inject the logic into _SliverPersistentHeaderElement.

It feels a bit convoluted that how it is inserted into the widget tree and can only work with floating persistent header.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I tried that first, but then I could not find the RenderSliverFloatingPersistentHeader as an ancestor from that point. I'll look again and see if there is another way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can do something like context.findRenderObject to get the renderobject (which should be RenderSliverFloatingPersistentHeader if there is no additional renderobject in between _SliverFloatingPersistentHeader and RenderSliverFloatingPersistentHeader)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so the structure i am imagine is the _SliverFloatingPersistentHeader listens to the scrollable and updates its renderobject(assuming it is RenderSliverFloatingPersistentHeader) based on it.

In terms of how _SliverFloatingPersistentHeader can listen to scrollable, you can either change it to a stateful widget (probably overkill), or makes _SliverFloatingPersistentHeader inject some callback into _SliverPersistentHeaderElement, and figure out how _SliverPersistentHeaderElement can listen to inherited widget

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not figures out how to do this yet, not sure if I am understanding completely. _SliverFloatingPersistentHeader is a RenderObjectWidget, and _SliverPersistentHeaderElement is a RenderObjectElement.
My original plan here in introducing _FloatingHeader was to follow to same pattern we use for SliverAppBar, which applies a _FloatingAppBar in order to listen to the scroll events. Should this be rewritten? It would probably be better for maintainability if we used the same logic one way or the other.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah we have similar situation in the appbar as well. If we have it here, do we still need the logic in _FloatingAppBar? it looks the scrolling maybe listen twice for SliverAppBar?

@Piinks
Copy link
Contributor Author

Piinks commented Mar 17, 2021

I am going to close this for now while we investigate the issue further.

@Piinks Piinks closed this Mar 17, 2021
@Piinks Piinks reopened this Apr 19, 2021
@flutter-dashboard flutter-dashboard bot added the f: material design flutter/packages/flutter/material repository. label Apr 19, 2021
@Piinks
Copy link
Contributor Author

Piinks commented Apr 19, 2021

We've gotten some valid use cases reported on the bug, so I am re-opening this. :)

@Piinks Piinks requested a review from goderbauer April 19, 2021 19:20
@Piinks Piinks removed the f: material design flutter/packages/flutter/material repository. label Apr 21, 2021
@goderbauer goderbauer requested a review from chunhtai April 21, 2021 21:53
@chunhtai
Copy link
Contributor

chunhtai commented Apr 21, 2021

in case it gets buried in the thread, I replied #76145 (comment)

@gspencergoog
Copy link
Contributor

@Piinks What's the status of this PR? (Desktop Triage)

@Piinks
Copy link
Contributor Author

Piinks commented Apr 29, 2021

Thanks @gspencergoog! Just pushed the latest updates, @chunhtai PTAL :)

Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: desktop Running on desktop a: fidelity Matching the OEM platforms better a: mouse Issues related to using a mouse or mouse support a: quality A truly polished experience f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. platform-web Web applications specifically

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mouse scrolling does not trigger showing SliverPersistentHeader when floating is set to true

4 participants