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

Intrinsic height not updated on resizing children #16

Closed
erickok opened this issue Dec 1, 2020 · 21 comments · Fixed by #19
Closed

Intrinsic height not updated on resizing children #16

erickok opened this issue Dec 1, 2020 · 21 comments · Fixed by #19
Assignees
Labels
bug Something isn't working

Comments

@erickok
Copy link

erickok commented Dec 1, 2020

When using IntrinsicContentTrackSizes the intrinsic height of a row is calculated based on its children, but seemingly only once. When a child resizes, the intrinsic height is not updated I believe. This is especially onbious when using image widgets inside such rows:

        SingleChildScrollView(
          child: Container(
            color: Colors.yellow,
            child: LayoutGrid(
              gridFit: GridFit.loose,
              templateColumnSizes: [
                FlexibleTrackSize(1),
              ],
              templateRowSizes: [
                IntrinsicContentTrackSize(),
              ],
              children: [
                Image.network(
                  'https://images.vrt.be/height40/2018/04/26/2644a887-4938-11e8-abcc-02b7b76bf47f.png',
                  // height: 40,
                ).withGridPlacement(columnStart: 0, rowStart: 0),
              ],
            ),
          ),
        )

Screenshot 2020-12-01 at 09 58 21

The height of this row is incorrect when the image is shown. Adding a pre-specified size (height) to the image will cause the row to size correctly, but it is not always viable to known the size upfront.

Screenshot 2020-12-01 at 09 58 50

Another way to see this problem is by replacing the SingleChildScrollView in the above example to be an IntrinsicHeight. The row will be 0px high, as the height is not updated.

Any solution or workaround is greatly appreciated! Or even hint on where to fix this in code, as I have tried but don't understand yet when a recalculation should and can happen.

cf Sub6Resources/flutter_html#456

@shyndman
Copy link
Owner

shyndman commented Dec 1, 2020

Hi Eric,

Thanks for the report, and yeah, not good. I want to get this fixed.

I won't have a chance to take a look at this until maybe tomorrow night, but definitely by the end of next day.

As far as where to look, it will have to do with the render objects. The parent-child dance all takes place in here: https://github.com/madewithfelt/flutter_layout_grid/blob/master/lib/src/rendering/layout_grid.dart.

The one thing I can think of is that when a parent asks a child to layout, the parent can indicate if its (the parent's) layout should be invalidated if the child changes size in the future. I suspect that I'm saying that I'm not interested indiscriminately, but obviously intrinsic size is an instance where we should be.

If you poke at this before I do, any of your findings would be much appreciated. A simple repro repo would also be great, but I understand the steps you've provided in any case.

@shyndman
Copy link
Owner

shyndman commented Dec 1, 2020

Yeah, took a quick peek, and that's exactly what's happening:
https://github.com/madewithfelt/flutter_layout_grid/blob/master/lib/src/rendering/layout_grid.dart#L241

parentUsesSize: true is never being provided to child.layout, but should be if the child contributes to an intrinsically sized row/column. This probably isn't the tightest condition we can give it (because this kind of resizing is potentially expensive), but it's a good enough first step.

Good quick test might be to set parentUsesSize: true all the time, and see what happens. I would hope that things would work...but you never know. :)

Anyway, take a look if you've got cycles, but in any case I'll look soon.

@shyndman
Copy link
Owner

shyndman commented Dec 1, 2020

@erickok, the new changes to Dart/Flutter are giving me some issues. What version of Flutter are you on?

@erickok
Copy link
Author

erickok commented Dec 1, 2020

Just changing that line into

      child.layout(BoxConstraints.loose(gridSizing.sizeForArea(area)), parentUsesSize: true);

doesn't do anything yet. But I'l see if I understand what is going on.

I am on Flutter 1.22. To run you code I did have to set up a minimal mobile Flutter target as I have no macosx target working in my Flutter install.

@erickok
Copy link
Author

erickok commented Dec 1, 2020

Oh and loads of thanks to helping out!

@shyndman
Copy link
Owner

shyndman commented Dec 1, 2020

Happy to.

I ran a few initial tests, and I'm not hitting the problem yet. Here's what I'm working with:
https://gist.github.com/shyndman/541de2cdaf5b22b77be6ad54e21d2048

Looks like the grid is changing size when the child does, which doesn't make sense really...so I'm definitely missing something.

What I need is a clear test failure.

@shyndman
Copy link
Owner

shyndman commented Dec 1, 2020

Better than a gist, here's the branch: https://github.com/madewithfelt/flutter_layout_grid/tree/intrinsic-size-problems

I'm off to bed. I'll go deeper tomorrow.

@erickok
Copy link
Author

erickok commented Dec 1, 2020

Thanks for trying!

I just tried myself with a widget very similar to yours - a ResizableContainer with a height propery. Upon updating the state (by setting the height) indeed the layout is adjusted correctly. It is even weirder if I use an image like this:

        SingleChildScrollView(
          child: Container(
            color: Colors.yellow,
            child: LayoutGrid(
              gridFit: GridFit.loose,
              templateColumnSizes: [
                FlexibleTrackSize(1),
              ],
              templateRowSizes: [
                IntrinsicContentTrackSize(),
              ],
              children: [
                FadeInImage.memoryNetwork(
                  placeholder: kTransparentImage,
                  image: 'https://images.vrt.be/height40/2018/04/26/2644a887-4938-11e8-abcc-02b7b76bf47f.png',
                  // height: height,
                ).withGridPlacement(columnStart: 0, rowStart: 0),
              ],
            ),
          ),
        )

You see it first getting a height equal to the (available) width, and when the fade animation is done the height adjusts to something smaller (yet still incorrect).

ezgif-7-e34f7c5e726e

@shyndman
Copy link
Owner

shyndman commented Dec 2, 2020

OK, going to figure this out today.

@shyndman
Copy link
Owner

Hey @erickok,

Sorry for dropping off. Been really busy with work. I'm hoping to have a chance to look at this very soon. I'll keep you posted.

@shyndman
Copy link
Owner

Hi @erickok,

Sorry for the long long wait, but I have the rest of the day to work on all things flutter_layout_grid, and I'm looking forward to solving this, so here we go.

@shyndman shyndman self-assigned this Jan 12, 2021
@shyndman shyndman added the bug Something isn't working label Jan 12, 2021
@shyndman
Copy link
Owner

OK, so I got that code sample you gave me running, confirmed that the weirdness is grid's fault, and I think I see what I'm doing wrong.

Still thinking through what "right" would look like. I need to do some testing with the CSS equivalent.

@shyndman
Copy link
Owner

https://codepen.io/shyndman/pen/RwGejeQ

^ This is an approximation of what's going wrong.

@shyndman
Copy link
Owner

The grid is asking that image child, "how tall would you be if you filled the entire column", and it's saying "hella tall!!". Then the grid accommodates by changing its height.

But then when the grid actually asks the image to render, the image renders at its intrinsic size instead of filling the column, and we're left with a big gap on the bottom.

The same goes for the placeholder image, which has a different aspect ratio than the image we're loading, so we see odd scaling effects on that bottom gap.

@shyndman
Copy link
Owner

So I'm clearly missing a piece here. Going to dive into Flutter source and find it.

@shyndman
Copy link
Owner

shyndman commented Jan 13, 2021

OK, I have something working that feels reasonable.

It deviates from CSS Layout Grid somewhat, because on closer reading the sizing concepts differ between the two systems. I have to figure out if the differences can be reconciled, or otherwise refactor the codebase to be more Flutter-y.

I'm leaning towards Flutter-y, but need to sleep on it.

Edit: Turns out it's not quite there. I'm finding edge cases. The core sizing algorithm definitely needs a bit of a Flutter-specific rethink, which should actually simplify it quite a bit.

But I'm free all day tomorrow too, so this is getting done!!!

@shyndman
Copy link
Owner

Still on it. Going to get there. Built up a bunch of diagnostics today, so I can get this thing as close to CSS as possible.

@shyndman
Copy link
Owner

Keeping with it tomorrow. I'm excited to get this thing humming. :)

@shyndman
Copy link
Owner

The latest:
https://user-images.githubusercontent.com/42326/104807437-1b112800-57ad-11eb-8006-661d618954d2.mov

Getting there. :)

@shyndman
Copy link
Owner

OK, here we go. Last day.

@shyndman
Copy link
Owner

So @erickok, I think this works properly now. Feel free to reopen if there are issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants