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

Incorrect layout for custom MeasureFunction nodes caused by stale available width information #1784

Open
Hexlord opened this issue Feb 6, 2025 · 2 comments

Comments

@Hexlord
Copy link

Hexlord commented Feb 6, 2025

Sometimes in our app when the panel is resize slowly from width 140 to width 180 the text with wrapping enabled gets MeasureFunction invoked with incorrect width (e.g. for panel width 140 it is called with 120 (correct), 160 is called with 140 (correct), 161 is called with 121 (incorrect). There are other elements inbetween, and it is inside a 35 percent width container, so I could imagine it is a difficult scenario to support for the algorithm.

It is fixed by setting bool needToVisitNode to true unconditionally (but obviously that destroys performance).
It is also fixed by marking dirty all the descendant wrapping text elements of the resized panel (but it is annoying as it means iterating whole tree below them to find all the wrapping texts).

Finally we were able to fix it with what feels a bit more proper of a solution: we propagate bool sizeChange through the function calls starting at calculateLayoutInternal, it starts with false, but if we meet any node along the recursion that has its style's dimensions changed, we set it to true, and from there on always visit nodes in that recursion chain. We update the layout's LastWidth and LastHeight in the same place LastOwnerDirection is currently getting updated.
We have not noticed noticeable performance degradation, however we do not have many resizing scenarios (no animations yet). So we remain sceptical to that fix, and would love if someone from the team would take a look and evaluate it.

We are sorry for no minimal repro, we tried using the Yoga playground, however it does not allow usage of percentage dimensions, which we believe is crucial for the repro which involves a container X with percentage width dimension, and inside it are fixed width spacer followed by wrapping text, and the parent of X gets resized (and gets marked dirty), however it is not sufficient to cause Yoga without this new fix to update the wrapping text correctly every time (as mentioned, rare times the measure function is called with outdated available width, also known as inner width). We tried disabling caching for custom measure function node, always vising wrapping texts and other cheaper fixes, but none of them are sufficient.

Image

@NickGerleman
Copy link
Contributor

A unit test here causing the failure might also be helpful, asserting what measure function sees. Off the top of my head though, I know:

  1. There is some really buggy caching behavior around flexBasis, that we have never gotten around to nicely fixing the right way (historically attempts have broken a lot of code, and it has been hard as of recent to dedicate time to fixing some Yoga conformance issues).
  2. Measurement cache uses available size and measure mode to know whether measurement can be reused, but I think does not include containing block dimensions, which % use as reference, which feels like a bug., https://github.com/facebook/yoga/blob/main/yoga/node/CachedMeasurement.h

@Hexlord
Copy link
Author

Hexlord commented Feb 7, 2025

A unit test here causing the failure might also be helpful, asserting what measure function sees. Off the top of my head though, I know:

  1. There is some really buggy caching behavior around flexBasis, that we have never gotten around to nicely fixing the right way (historically attempts have broken a lot of code, and it has been hard as of recent to dedicate time to fixing some Yoga conformance issues).
  2. Measurement cache uses available size and measure mode to know whether measurement can be reused, but I think does not include containing block dimensions, which % use as reference, which feels like a bug., https://github.com/facebook/yoga/blob/main/yoga/node/CachedMeasurement.h

We tried disabling canUseCachedMeasurement (always return false) and forcing flex basis to always be recomputed in computeFlexBasisForChild at the same time before reporting this issue here, unfortunately it was not sufficient to stop the repro.
It did feel like it still was using some form of cache, as it felt like it was pulling old values from previous iterations. Visually it looked as if text snapped sometimes to just ignore the paddings altogether, but only on the right side (towards the flex direction), and other times would snap back to correct state.
I think the fix we came up on the image above required both force visiting nodes under resized node AND resetting computedFlexBasis at the same time. Or alternatively, like mentioned, after resizing the panel marking dirty all wrapping texts below and when marking an entity dirty also resetting its computedFlexBasis. So it must be a combination of outdated computedFlexBasis and inability to recompute it correctly without marking nodes leading to the wrapping text as dirty (so that they are visited).

Perhaps a good unit test would be to make sure that computed innerWidth (the one passed to custom MeasureFunction) is consistent when an ancestor node is resized before and after the node below the ancestor is marked dirty. That could be the current issue - it is not sufficient to mark just the resized ancestor node dirty for all information to be available for correct recomputation of e.g. flexBasis of a descendant node, maybe an extra condition is that between that ancestor and descendant are multiple nodes with % width dimensions and fixed paddings

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants