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

Make KeepAlive work with multi-item Tab/List/Slivers #21207

Closed
wants to merge 7 commits into from

Conversation

dnfield
Copy link
Contributor

@dnfield dnfield commented Aug 30, 2018

Fixes #11895.

The SliverMultiBoxAdaptorElement tries to keep track of the child it should be inserting children after when it needs to do an insertion. This breaks if the child it thinks it wants doesn't know what siblings it should have because it got thrown into the keepAliveBucket at some point - the insertion logic expects children to be connected and in the proper indexing order.

This patch checks to see if the candidate child for _currentBeforeChild knows at least one sibling - if not, we shouldn't use it. Also adds a test that fails on master but passes on this. One doubt on the test is why I have to call pumpAndSettle and then pump again - it seems like pumpAndSettle should get me there, but it's falling short by a frame (even with a long duration parameter).

@dnfield dnfield requested a review from Hixie August 30, 2018 07:55
@dnfield dnfield changed the title Make KeepAlive work with multi-item Tab/List/Slivers (WIP) Make KeepAlive work with multi-item Tab/List/Slivers Aug 30, 2018
@dnfield
Copy link
Contributor Author

dnfield commented Aug 30, 2018

Hm - this breaks a bunch of tests that I hadn't finished running before submitting it. Will need to look at that some more.

@dnfield dnfield changed the title (WIP) Make KeepAlive work with multi-item Tab/List/Slivers Make KeepAlive work with multi-item Tab/List/Slivers Aug 30, 2018
@rmarau
Copy link

rmarau commented Aug 30, 2018

@dnfield Great someone is tracking this bug!

I have this gist that reproduces the issue you are fixing. Steps were initially documented in #18276.

It's a list with expandable items that reveal a TextField.

Prior to your patch, running in release mode, the assertion is not verified and the list gets clipped at the top.

Applying your patch fixes the assertion check, but some how the list is still being clipped. Not as hard, I must say. Before the patch (in release) all items before the first visible (on screen) were no longer accessible. After the patch, all items before the item holding the focus are removed (the one with focus included).
Steps:

  • Click the first Expandable Item on screen (Afghanistan)
  • Get the focus on the TextField
  • Scroll down a few swipes
  • Click any item to expand
  • Return to the very first. It's gone!

Is this behavior some how related to your PR?
If you think this is a coincidence and unrelated, please let me know, so I open a new issue upon merging.

Anomalies seam to reveal most when expanding an item below (not before the list) the one currently focused.
Another quirck behavior after the patch:

  • Expand and focus the last item (Transnistria)
  • Go upwards Expand Greece (no issue here... upwards!)
  • Focus Greece
  • Going downwards Expand Montenegro
    ( the list jumps down )

Note: I'm using dev v7.0.2

Hope these tests can help some how.

@dnfield
Copy link
Contributor Author

dnfield commented Aug 30, 2018

Thanks @rmarau - I had seen this but didn't appreciate how it was different from the smaller example I was working from.. I'll work on getting a test case for this in place, but I am seeing the behavior you're talking about - the last KeepAlive is getting discarded (as well as it's previousSiblings apparently).

@dnfield
Copy link
Contributor Author

dnfield commented Aug 31, 2018

Problem: after a rebuild (e.g. after calling setState()), the keptAlive widget isn't getting laid out again. Need to look at this some more.

@dnfield dnfield closed this Aug 31, 2018
@marekchen
Copy link

@dnfield When will it be fixed?

@dnfield
Copy link
Contributor Author

dnfield commented Sep 3, 2018

I believe I have a fix but I'm still working on getting some tests for it right. I'd appreciate any valdiation/feedback from test cases out there on the latest commits on this branch - I plan to reopen it when I can write some better tests to capture the scenario, which should be soon.

@marekchen
Copy link

@dnfield Thank you very much for your work

@dnfield
Copy link
Contributor Author

dnfield commented Sep 3, 2018

Ok, tests are squared away for this, should be good for review now.

@dnfield dnfield reopened this Sep 3, 2018
@dnfield
Copy link
Contributor Author

dnfield commented Sep 3, 2018

Something went wrong with the CI for this pull. Closing and will open anew.

@dnfield dnfield closed this Sep 3, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

_debugUltimatePreviousSiblingOf(after, equals: _firstChild) is not true.
4 participants