An alternative to the trimming in RenderSliverPersistentHeader.showOnScreen is to confine inflatedRect so that it doesn't exceed renderEditable's paintBounds. But that breaks around 6 tests.

It seems the paint transform that does child -> this is applied twice here.

Nice catch. Did you add a test for this?

Removed. Adding this in a different PR.

It took me awhile to understand this comment, so I tried to clarify a bit. Definitely double check this and make sure I understood correctly, though.

Trims the given Rect original so that it fits within the boundaries given by top, right, bottom, and left.

This is used to prevent the case where rect or descendant specified in showOnScreen exceed the leading edge of this sliver. If this were to happen, the viewport would move towards the leading edge (reducing its scroll offset) in order to unpin the persistent header. This is almost always undesirable.

👍 I'll put the comment above the showOnScreen method instead.

Is it OK to limit the size of the caret's scroll padding instead, so that it will never exceed the leading edge (see the comment in "editable_text.dart" file)? That makes a little bit more sense to me (in case the caller of showOnScreen really expects us to respect the rect specified). Or maybe we should reduce the default scrollPadding (EdgeInsets.all(20.0)) and document this edge case?

That makes sense, but would it be a breaking change in normal situations?

This should be indented according to the style guide, if I'm not mistaken.

child must only be != null if a descendent was provided, right? You could call showOnScreen on a child-less header to bring the entire header back on screen.

nit: instead of defining this inline, this code just be a private method on this object to declutter this method, no?

same child comment as above.

why does it need to stay in the bounds of the child? Doesn't it just have to stay within my own bounds?

If that is the case, could this all be simplified to paintBounds.intersect(boundsOfDescendantInMyOwnCoordinateSystem)?

You're right we only need to override showOnScreen for pinned headers where it's always the case (I'm overriding the wrong class). But I'm not sure which approach we should take, relying on the caller to pass us the right rect, or trim the rect the caller passed down? The latter sounds a bit hacky to me.

isn't the idea of a snapping header that it will always snap to its max extend?

showInViewport already skips scrolling if the descendant is fully in view. What case does the logic above cover?

showInViewport uses getOffsetToReveal (which assumes the target sliver moves linearly) to determine whether a sliver is still visible. With pinned headers we can get false negatives (the scroll offset indicates the sliver is not in the viewport but it is), with floating headers it may take less scroll offset than getOffsetToReveal suggested to reveal.

The added logic does assume the layout is up to date with the current offset. I'll add a check to invalidate the skip logic if the assumption does not hold.

@goderbauer got rid of the visibility check because the viewport could be scrolling / about to scroll. Could you take a look?

Sorry nvm that broke some tests. Fixing.

Nice catch. Did you add a test for this?

If the header is still visible (as said below), why do you need skipOffstage: false?

same here.

Maybe I missed it, but did you have tests for when the header animates and for when it doesn't?

I'm tempted to make this a private extension method to avoid the null check. Is using method extensions not recommended? It doesn't seem to be in the style guide.

I'm curious too. I feel like we avoid this pattern because I never see it in our codebase.

Also a nit: are square brackets like [original] supposed to be used outside of public doc comments? I guess it doesn't matter if they are.