Fixes for aspect ratio and min/max sizes by nicoburns · Pull Request #317 · DioxusLabs/taffy

nicoburns · 2023-01-04T14:17:07Z

Objective

Fix #313 (and more generally fix support for aspect-ratio).
This PR now also fixes #167

Subtasks

Context

Taffy has an aspect_ratio style property and claims to support CSS aspect-ratio. But actual support is patchy, and it is not covered by gentests.

nicoburns · 2023-01-04T21:13:16Z

I'm not sure if this covers everything (in fact I have 4 disabled tests), but aspect ratio is working a lot better with this PR than it was before (including the use case in #313), and I'm intending to move on to other tasks now and return to aspect ratio again later if required.

adjabaev · 2023-01-04T23:14:52Z

@nicoburns hey!
thank for the quick answer/ implementation!
I just started playing with it and noticed this test isn't passing:
(the inset doesn't seem to change anything, it is just a representation of a real use case)

// aspect_ratio_leaf_inset_width
<div id="test-root" style="display: flex; width: 1280px; height: 720px;">
  <div style="position: absolute; top: 5%; left: 5%; width: 20%; aspect-ratio: 3;"></div>
</div>

nicoburns · 2023-01-05T01:11:43Z

@adjabaev Well spotted! I've fixed that case, but I think I'll need to test more cases using absolute positioning.

adjabaev · 2023-01-05T01:44:30Z

86509aa

It might be a mistake but both tests you commited (fixtures) have identical content

On a side note, I'll keep trying getting crazy with aspect ratio and keep you updated if I end up finding something behaving weirdly

nicoburns · 2023-01-05T12:16:14Z

It might be a mistake but both tests you commited (fixtures) have identical content

Ah yes, that'll be because the "rename" and "duplicate" buttons are right next to each other in my editor. I wanted to rename it "absolute" because it turns out that percentages have nothing to do with it.

… to text node content sizing

… flex items

Weibye

Some nitpicks and questions :)

src/geometry.rs

scripts/gentest/test_helper.js

src/compute/leaf.rs

It now: - Applies margins correctly - Applies min/max correctly - Applies aspect ratio correctly

…ive to the grid container

Co-authored-by: Andreas Weibye <13300393+Weibye@users.noreply.github.com>

…d item in container with border

src/compute/flexbox.rs

…space

nicoburns · 2023-01-11T00:20:15Z

@alice-i-cecile This is now ready for another round of review / merging when ready.

Weibye

Looks good, I haven't been able to spot any issues but as usual I'm relying on your expertise and the tests to vouch for the correctness of the implementation.

Weibye · 2023-01-12T19:03:55Z

src/compute/flexbox.rs

+            match (align_self, constants.is_wrap_reverse) {
+                // Stretch alignment does not apply to absolutely positioned items
+                // See "Example 3" at https://www.w3.org/TR/css-flexbox-1/#abspos-items
+                // Note: Stretch should be FlexStart not Start when we support both
+                (AlignSelf::Baseline | AlignSelf::Stretch | AlignSelf::Start, false) | (AlignSelf::End, true) => {
+                    constants.padding_border.cross_start(constants.dir) + resolved_margin.cross_start(constants.dir)
                }
-                AlignSelf::End => {
-                    if constants.is_wrap_reverse {
-                        constants.padding_border.cross_start(constants.dir)
-                    } else {
-                        free_cross_space - constants.padding_border.cross_end(constants.dir)
-                    }
+                (AlignSelf::Baseline | AlignSelf::Stretch | AlignSelf::Start, true) | (AlignSelf::End, false) => {
+                    constants.container_size.cross(constants.dir)
+                        - constants.padding_border.cross_end(constants.dir)
+                        - final_size.cross(constants.dir)
+                        - resolved_margin.cross_end(constants.dir)
                }
-                AlignSelf::Center => free_cross_space / 2.0,
-                AlignSelf::Baseline => free_cross_space / 2.0, // Treat as center for now until we have baseline support
-                AlignSelf::Stretch => {
-                    if constants.is_wrap_reverse {
-                        free_cross_space - constants.padding_border.cross_end(constants.dir)
-                    } else {
-                        constants.padding_border.cross_start(constants.dir)
-                    }
+                (AlignSelf::Center, _) => {
+                    (constants.container_size.cross(constants.dir)
+                        + constants.padding_border.cross_start(constants.dir)
+                        - constants.padding_border.cross_end(constants.dir)
+                        - final_size.cross(constants.dir)
+                        + resolved_margin.cross_start(constants.dir)
+                        - resolved_margin.cross_end(constants.dir))
+                        / 2.0


I particularly like this way of structuring this code!

alice-i-cecile · 2023-01-12T19:41:18Z

src/geometry.rs

    }
 }

+impl<U, T: Add<U>> Add<Rect<U>> for Rect<T> {


This deserves a note in the release notes; it's surprisingly useful.

nicoburns force-pushed the fix-aspect-ratio branch 3 times, most recently from 015934e to b28deaa Compare January 4, 2023 19:38

nicoburns marked this pull request as ready for review January 4, 2023 21:11

nicoburns changed the title ~~WIP: Fix and test support for aspect ratio~~ Fix and test support for aspect ratio Jan 4, 2023

nicoburns requested review from Weibye and alice-i-cecile January 4, 2023 21:11

nicoburns mentioned this pull request Jan 4, 2023

Support CSS Grid #204

Closed

87 tasks

nicoburns force-pushed the fix-aspect-ratio branch 2 times, most recently from 7ff739f to fd02d7f Compare January 5, 2023 01:16

nicoburns marked this pull request as draft January 5, 2023 01:16

nicoburns force-pushed the fix-aspect-ratio branch from fd02d7f to 86509aa Compare January 5, 2023 01:17

nicoburns added the bug Something isn't working label Jan 5, 2023

nicoburns added 12 commits January 5, 2023 18:21

Add aspect ratio support to gentest script + add aspect ratio support…

0bff4e2

… to text node content sizing

Add maybe_apply_aspect_ratio helper to Size<Option<f32>>

2801c19

Apply aspect ratio to leaf node style resolution

1dea4d0

Apply aspect ratio when doing final placement of grid items

fd88cf4

Make cargo gentest compile generator in release mode

da30c98

Add tests for aspect ratio of leaf nodes in grid containers

cd7a563

Apply aspect ratio to grid container styles

d74c903

Apply aspect ratio to automatic minimum size

796cf13

Add tests for aspect ratio of content-sized leaf nodes

fd57d84

Use main size for flex-basis if flex-basis is not set

fbc92dc

Resolve size, min_size, max_size against aspect ratio when generating…

807624e

… flex items

Add gentests for aspect-ratio in flex-row (align-start)

b2f0905

Weibye reviewed Jan 8, 2023

View reviewed changes

src/geometry.rs Outdated Show resolved Hide resolved

src/geometry.rs Show resolved Hide resolved

scripts/gentest/test_helper.js Outdated Show resolved Hide resolved

src/compute/leaf.rs Show resolved Hide resolved

nicoburns added 4 commits January 8, 2023 14:28

Always apply margins to absolutely positioned grid items

a2ef38c

Rewrite flex item absolute positioning

befdef1

It now: - Applies margins correctly - Applies min/max correctly - Applies aspect ratio correctly

Take border into account when absolutely positioning grid items relat…

e9568cf

…ive to the grid container

Add absolute positioning margin test

fad8919

nicoburns force-pushed the fix-aspect-ratio branch from 022a092 to fad8919 Compare January 8, 2023 14:41

nicoburns and others added 6 commits January 8, 2023 14:52

Apply suggestions from code review

f063cc0

Co-authored-by: Andreas Weibye <13300393+Weibye@users.noreply.github.com>

Update release notes

01fadec

Add test for absolutely positioned fallback to static position of gri…

29161f0

…d item in container with border

Implement Add for Rect where the inner type implements Add

db1bc72

Apply aspect ratio and min/max size to flex container size

dec6bc0

Cleanup TODOs in the flexbox algorithm

0319a30

nicoburns mentioned this pull request Jan 8, 2023

Identify or implement step 4 of the flex layout algorithm #119

Closed

Weibye reviewed Jan 8, 2023

View reviewed changes

src/compute/flexbox.rs Outdated Show resolved Hide resolved

nicoburns added 5 commits January 8, 2023 21:12

Remove one more TODO

e1ef9d8

Add min/max size tests for absolutely positioned flex items

1419e31

Move absolute flex positioning aspect ratio tests under absolute name…

94df9c5

…space

Clamp available_space by min/max size for final layout

1ded18f

Add tests for applying aspect ratio to min/max size of flex items

355477e

nicoburns marked this pull request as ready for review January 11, 2023 00:16

nicoburns changed the title ~~Fix and test support for aspect ratio~~ Fixes for aspect ratio and min/max sizes Jan 11, 2023

nicoburns changed the title ~~Fixes for aspect ratio and min/max sizes~~ Fixes for aspect ratio, min/max sizes and margins of absolutely positioned flex items Jan 11, 2023

Weibye self-requested a review January 11, 2023 13:11

nicoburns changed the title ~~Fixes for aspect ratio, min/max sizes and margins of absolutely positioned flex items~~ Fixes for aspect ratio and min/max sizes Jan 11, 2023

nicoburns mentioned this pull request Jan 12, 2023

Fix flexbox baseline alignment #325

Merged

3 tasks

Weibye approved these changes Jan 12, 2023

View reviewed changes

alice-i-cecile reviewed Jan 12, 2023

View reviewed changes

alice-i-cecile approved these changes Jan 12, 2023

View reviewed changes

alice-i-cecile merged commit 610c92f into DioxusLabs:main Jan 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes for aspect ratio and min/max sizes#317

Fixes for aspect ratio and min/max sizes#317
alice-i-cecile merged 40 commits intoDioxusLabs:mainfrom
nicoburns:fix-aspect-ratio

nicoburns commented Jan 4, 2023 •

edited

Loading

Uh oh!

nicoburns commented Jan 4, 2023

Uh oh!

adjabaev commented Jan 4, 2023 •

edited

Loading

Uh oh!

nicoburns commented Jan 5, 2023

Uh oh!

adjabaev commented Jan 5, 2023 •

edited

Loading

Uh oh!

nicoburns commented Jan 5, 2023

Uh oh!

Weibye left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nicoburns commented Jan 11, 2023

Uh oh!

Weibye left a comment

Uh oh!

Weibye Jan 12, 2023

Uh oh!

alice-i-cecile Jan 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

nicoburns commented Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Subtasks

Context

Uh oh!

nicoburns commented Jan 4, 2023

Uh oh!

adjabaev commented Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicoburns commented Jan 5, 2023

Uh oh!

adjabaev commented Jan 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicoburns commented Jan 5, 2023

Uh oh!

Weibye left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nicoburns commented Jan 11, 2023

Uh oh!

Weibye left a comment

Choose a reason for hiding this comment

Uh oh!

Weibye Jan 12, 2023

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile Jan 12, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nicoburns commented Jan 4, 2023 •

edited

Loading

adjabaev commented Jan 4, 2023 •

edited

Loading

adjabaev commented Jan 5, 2023 •

edited

Loading