Fix flexbox baseline alignment by nicoburns · Pull Request #325 · DioxusLabs/taffy

nicoburns · 2023-01-12T17:59:52Z

Objective

Fix flexbox baseline alignment implementation (fixes baseline alignment tests imported from yoga)
Refactor flexbox baseline alignment implementation so that it doesn't require directly accessing the layout of descendant nodes other than direct children (required to make it's baseline alignment interoperate with Grid and other layout modes properly)
Optimise baseline alignment computation so that child baseline are only calculated when necessary

Context

~~Note: This PR depends on #320 which in turn depends on #317~~ both now merged!

I did a quick run of the benchmarks vs. main to check for regressions, and actually I'm seeing mild performance wins (10% - 30% on some benchmarks) on quite a few of the benchmarks. There is also a ~10% regression on a couple, but those are the very smallest ones that are completing in single-digit microseconds anyway, so I'm not going to worry too much about those (we probably ought to remove those super small ones at some point - I don't think they're very representative of real-world performance).

Feedback wanted

Does changing each algorithm's perform_layout function to return:

struct SizeAndBaselines {
   size: Size<f32>,
   first_baseline: Point<Option<f32>>,
}

instead of Size<f32> as I have done in this PR seem reasonable? This is instead of the 3rd layout method proposed in #28 (comment)

- Introduce LayoutAlgorithm trait

nicoburns · 2023-01-15T23:55:30Z

This still doesn't implement baseline alignment for CSS Grid, but I now think I'd like to merge this without that to unblock #326. These changes (fixing baseline alignment for flexbox, and refactoring the flexbox implementation so that it doesn't require accessing the layout of descendant nodes other than direct children) are worthwhile on their own, and baseline alignment for CSS Grid can follow in another PR.

src/compute/flexbox.rs

alice-i-cecile · 2023-01-16T00:01:06Z

src/compute/flexbox.rs


 /// Computes the layout of [`LayoutTree`] according to the flexbox algorithm
-pub fn compute(
+pub fn compute_generic(


This name isn't very clear.

alice-i-cecile · 2023-01-16T00:03:32Z

src/compute/grid/mod.rs

 ///   - Track (row/column) sizing
 ///   - Alignment & Final item placement
-pub fn compute(
+pub fn compute_generic(


Ditto on the name here.

src/compute/grid/mod.rs

src/compute/leaf.rs

alice-i-cecile · 2023-01-16T00:05:47Z

src/compute/mod.rs

+
+/// The public interface to a generic algorithm that abstracts over all of Taffy's algorithms
+/// and applies the correct one based on the `Display` style
+pub struct GenericAlgorithm;


Hmm. I'm not very happy with this naming / docs. Maybe something like UnifyingLayoutAlgorithm?

This struct actually disappears in my latest version of #326, as I've made the "generic" algorithm and the "leaf" algorithm operate on &mut Taffy rather than &mut impl LayoutTree (so they don't fit the LayoutAlgorithm trait anymore). This allows us to remove measure_funcs from the LayoutTree trait and make them an implementation detail of our Taffy storage implementation.

Good I'm fine to wait.

src/layout.rs

alice-i-cecile

Some feedback on structure and docs. I would like to get this merged quickly though, so feel free to defer some of the work (e.g. the trait refactor) to a follow-up PR.

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

nicoburns · 2023-01-16T00:27:12Z

@alice-i-cecile I've renamed the compute_generic functions back to compute, which is what they were already called. So I think it can now safely be said that the interface is no worse than it was. In any case, as these traits/functions are currently private, I'm not going to worry about too much (we can easily change this later if we want to). I suggest we bikeshed this in #326 (which does make the LayoutAlgorithm trait public) and also makes major changes to the public interface exposed through the LayoutTree trait.

Is there anything else blocking this?

alice-i-cecile · 2023-01-16T00:47:40Z

Nope, this now LGTM.

nicoburns mentioned this pull request Jan 12, 2023

Support CSS Grid #204

Closed

87 tasks

nicoburns force-pushed the baseline-alignment branch 3 times, most recently from 1dc8683 to ef8e73d Compare January 12, 2023 21:23

nicoburns changed the title ~~WIP: Fix baseline alignment + Add layout methods to LayoutTree trait~~ WIP: Fix baseline alignment Jan 12, 2023

nicoburns removed usability Make the library more comfortable to use breaking-change A change that breaks our public interface controversial This work requires a heightened standard of review due to implementation or design complexity labels Jan 12, 2023

This was referenced Jan 12, 2023

WIP: Refactor the LayoutTree trait #326

Closed

Import yoga tests #320

Merged

nicoburns added 5 commits January 12, 2023 23:11

Fix baseline alignment with margins and padding

62270ae

Don't run baseline alignment on flex columns

2b14163

Enable baseline child multiline tests

c9e4346

Refactor baseline alignment to be returned by perform_layout function

ca302c2

- Introduce LayoutAlgorithm trait

Only calculate child baseline for baseline alignment when necessary

2cc99ee

nicoburns force-pushed the baseline-alignment branch from ef8e73d to 2cc99ee Compare January 12, 2023 23:11

nicoburns removed the blocked Cannot be advanced until something else changes label Jan 13, 2023

nicoburns changed the title ~~WIP: Fix baseline alignment~~ Fix flexbox baseline alignment Jan 14, 2023

Add disabled test for grid item baseline alignment

31ece5c

nicoburns marked this pull request as ready for review January 15, 2023 23:53

nicoburns requested review from Weibye and alice-i-cecile January 15, 2023 23:53