Import yoga tests #320

nicoburns · 2023-01-05T00:44:13Z

Objective

Fixes #285.

Changes made

Add a re-runnable diff script that also does some munging of the test names to make them match up when they have diverged.
Add an import script. This is pretty rough, but I feel like standards need not be so high for something that is run "offline" and rarely. It compiles, runs, it's formatted and linted.
Import ALL yoga tests (including the ones from Add newer fixtures facebook/yoga#1194 that they haven't landed on master yet)

Notes

I have imported all tests, but I have not made any attempt to fix failing ones. I feel like this would be best tackled in follow up PRs that tackle a group of related failing tests without trying to fix everything at once, as I imagine some will be a lot more difficult than other.
I have also not made any attempt to diff the tests to check for differences in the specification of tests that we both have. This would also be useful thing to do, but it's a much more complex task (diffing HTML is non-trivial).

alice-i-cecile

I think we should throw together a quick README.md for the impoort-yoga-tests crate, describing its purpose and instructions for getting it working.

The convention where tests that begin with an x are ignored should also be documented: at a glance, I can't figure out where this is being done.

alice-i-cecile · 2023-01-05T17:52:34Z

scripts/import-yoga-tests/src/main.rs

+        .collect();
+
+    for fixture in yoga_fixtures {
+        if !taffy_fixture_names.contains(&fixture.name) {


What happens if the fixtures are updated but not renamed? Won't they get out of sync with this strategy?

Won't they get out of sync with this strategy?

Yes. They likely already are. This is at least a quick way to get us tests we were entirely missing though. I guess we'll need to actually parse the HTML/CSS to sync updates.

Makes sense. What's the cost to rebuilding them completely every time?

(We should document this conversation in a comment if we decide to keep this behavior).

What's the cost to rebuilding them completely every time?

As in treating Yoga's version as the source of truth and overwriting any changes we might have made? There's no technical blocker on that. I think the main problem with that would be that it would make it hard for us to maintain any changes that we actually need. As an example: I removed whitespace from a couple of tests when I added support for sizing text nodes. It would be nice if there was a single source of truth though, and I suppose that could be managed by just being selective with when we sync and which changes we commit when we sync.

Sounds reasonable: I'm fine with whatever you pick here.

NickGerleman · 2023-01-06T08:28:59Z

test_fixtures/android_news_feed.html

+    <div layout="width: 1080; height: 444; top: 0; left: 0;" style="flex-shrink: 0; " >
+      <div layout="width: 1080; height: 444; top: 0; left: 0;" style="align-content: stretch; " >
+        <div layout="width: 1080; height: 226; top: 0; left: 0;" style="align-content: stretch; " >
+          <div layout="width: 1044; height: 202; top: 24; left: 36;" style="flex-direction: row; align-items: flex-start; align-content: stretch; margin-top: 24px; margin-start: 36px; " >


The "margin-start" here is not a valid CSS property, and Yoga's test generator did some funky things with it. But it's effectively the same as "margin-inline-start" for any case that isn't a vertical writing direction (which I don't think Yoga or Taffy support).

That should be cleaned up soon in the Yoga source, there is a PR I had cleaned up some of these in the newer PR this imports from, but I discovered I needed to update the test generator to properly translate it.

Ah, that's useful to know. Taffy actually doesn't support margin-inline-start either (it has margin-left). I'm guessing Yoga also doesn't support this and the test generator just converts it to left/right?

I did a test run overwriting all of Taffy's test with Yoga's (where there as an equivalent) to see what the diff would be like, and there were quite a few differences. In addition to the start/end vs. left/right thing, the main one's were due to Yoga defaulting to flex-direciton: column while Taffy defaults to flex-direction: row. Many of a Taffy's tests had an explicit flex-direction added to account for this. And some of them had width/height swapped to account for it in a different way. I suspect we'll need to add explicit flex-direction everywhere if we want to unify the test suites.

Ah, that's useful to know. Taffy actually doesn't support margin-inline-start either (it has margin-left). I'm guessing Yoga also doesn't support this and the test generator just converts it to left/right?

Yoga does have support for flow-relative margin, padding, etc. The API at the Yoga level looks like YGNodeStyleSetMargin, accepting a YGEdge, which can be (in order of precedence) a cardinal direction (e.g. YGEdgeLeft), a flow-relative direction (e.g. YGEdgeStart), an axis (e.g. YGEdgeHorizontal), or YGEdgeAll.

React Native historically historically mapped YGEdgeStart to marginStart, etc, but we recently added marginInlineStart, marginBlockStart, etc to be conformant with browsers.

Wouldn't you need a flow-relative direciton AND an axis to fully specify an edge? (YGEdgeStart by itself is ambiguous. You'd need YGEdgeInlineStart, no?).

Start/End only ever refer to the horizontal axis, and just picks Left/Right based on RTL. But yeah, the style has distinct inputs for padding, margin, etc per YGEdge, and a specific precedence when multiple inputs are present which may overlap the same physical edge. It would be reevaluated if laid out as RTL. Yoga also weirdly treats both YGEdgeLeft and YGEdgeStart as effecting the right side of a box with flexDirection: "row-reverse" which is inconsistent with browsers.

Yoga's precedence logic based on specificity is one difference from browsers, where CSS properties are ordered and if there is overlap, the last assignment wins (e.g. "margin" takes precedence over "marginLeft" if it comes after).

Perhaps it would make sense to make this the default specifically for the test suite? (updating the test fixtures accordingly so that the actual tests don't change).

I think updating that would probably make sense. If you're having to add column direction, was Yoga's test generator doing that in the outer HTML too?

If you're having to add column direction, was Yoga's test generator doing that in the outer HTML too?

I haven't actually checked that yet. I've gone through and added all the flex-direction: columns. But actually, all of those tests were already passing. So it's possible that the missing flex-directions weren't actually making a meaningful difference to the tests. I'd still be more comfortable if the tests are unambiguously specified though.

Start/End only ever refer to the horizontal axis, and just picks Left/Right based on RTL.

Ah, I guess that makes sense given that IIRC Yoga supports RTL but not vertical writing modes. The variants are confusingly named though! I'm guessing that might the sort of thing it might be more possible to make breaking changes to (as it will turn into a compile error)?

Yoga also weirdly treats both YGEdgeLeft and YGEdgeStart as effecting the right side of a box with flexDirection: "row-reverse" which is inconsistent with browsers.

That is weird. As documented in #307, it does work that way for the flex-start alignment value, but there is also a separate start which isn't affected by the the flex-direction, and it definitely shouldn't do that for left!

If you're having to add column direction, was Yoga's test generator doing that in the outer HTML too?

@NickGerleman I looked into this, and the stylesheet that Yoga uses when generating tests applies this default. See https://github.com/facebook/yoga/blob/main/gentest/test-template.html#L28

…stead

nicoburns · 2023-01-12T21:57:33Z

@alice-i-cecile Requested changes made. Filtering out tests beginning with x happens here https://github.com/DioxusLabs/taffy/blob/main/scripts/gentest/src/main.rs#L27. And the gentest script completely overwrites the generated test directory when it runs so that will also delete any already existing generated tests.

Weibye

Good work as usual! The increased test coverage is surely going to be valuable in guiding rework and new features.

scripts/import-yoga-tests/Cargo.toml

test_fixtures/align_baseline_multiline_column.html

nicoburns mentioned this pull request Jan 5, 2023

Sync generated test suite with Yoga #285

Closed

nicoburns marked this pull request as draft January 5, 2023 00:54

alice-i-cecile added the build system Make continuous integration do the tedious things label Jan 5, 2023

nicoburns changed the title ~~WIP: Import yoga tests~~ Import yoga tests Jan 5, 2023

nicoburns force-pushed the test/import-yoga-tests branch from 7866d71 to 4b879d0 Compare January 5, 2023 14:01

nicoburns marked this pull request as ready for review January 5, 2023 14:01

nicoburns added code quality Make the code cleaner or prettier. testing Additional tests or improvements to the testing infrastructure and removed code quality Make the code cleaner or prettier. labels Jan 5, 2023

alice-i-cecile requested changes Jan 5, 2023

View reviewed changes

alice-i-cecile reviewed Jan 5, 2023

View reviewed changes

nicoburns force-pushed the test/import-yoga-tests branch from 4b879d0 to 2e8f922 Compare January 5, 2023 18:21

nicoburns added this to the 0.3 milestone Jan 5, 2023

NickGerleman reviewed Jan 6, 2023

View reviewed changes

nicoburns added the blocked Cannot be advanced until something else changes label Jan 6, 2023

nicoburns force-pushed the test/import-yoga-tests branch 2 times, most recently from 9507561 to bac71f1 Compare January 7, 2023 17:54

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

nicoburns force-pushed the test/import-yoga-tests branch 2 times, most recently from 89c8647 to 99b56ca Compare January 12, 2023 12:55

nicoburns added the blocked Cannot be advanced until something else changes label Jan 12, 2023

nicoburns mentioned this pull request Jan 12, 2023

Fix flexbox baseline alignment #325

Merged

3 tasks

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

nicoburns force-pushed the test/import-yoga-tests branch from f5421b1 to 33b6737 Compare January 12, 2023 21:05

nicoburns added 6 commits January 12, 2023 21:06

Add script to diff taffy tests against yoga tests by name

0f653d5

Add fixture template

5163ba8

Modify diff script to display disabled tests next to enabled ones

eccb48d

Make diff script work using environment variable

0719d53

Add script to import test fixtures from yoga

997b4bc

Fix diff script for column_row_gap test

0378c38

nicoburns added 8 commits January 12, 2023 21:06

Remove commented code

c7dc4c9

Add cargo command for importing yoga tests

5c38ff8

Import all generated tests from yoga (failing tests disabled)

68a1183

Import tests with explicit flex direction

cebb18e

Convert tests not specifically testing start/end to use left/right in…

493a740

…stead

Replace margin-start with margin-left in android_news_feed tests

ec6f651

Document convention of tests starting with an x being disabled

7c72cc0

Enable absolute child with margin tests

7f7b0c6

nicoburns force-pushed the test/import-yoga-tests branch from 33b6737 to 7f7b0c6 Compare January 12, 2023 21:06

Fix align content not stretch with align items stretch

b687024

nicoburns mentioned this pull request Jan 12, 2023

WIP: Refactor the LayoutTree trait #326

Closed

13 tasks

nicoburns requested a review from alice-i-cecile January 12, 2023 21:57

Add README for import-yoga-tests package

8a139da

nicoburns force-pushed the test/import-yoga-tests branch from cb6733d to 8a139da Compare January 12, 2023 22:00

Weibye approved these changes Jan 12, 2023

View reviewed changes

scripts/import-yoga-tests/Cargo.toml Show resolved Hide resolved

test_fixtures/align_baseline_multiline_column.html Outdated Show resolved Hide resolved

Remove broken after chrome behaviour change comment

5a9f4cb

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

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import yoga tests #320

Import yoga tests #320

nicoburns commented Jan 5, 2023 •

edited

Loading

alice-i-cecile left a comment

alice-i-cecile Jan 5, 2023

nicoburns Jan 5, 2023

alice-i-cecile Jan 5, 2023 •

edited

Loading

nicoburns Jan 5, 2023

alice-i-cecile Jan 6, 2023

NickGerleman Jan 6, 2023

NickGerleman Jan 6, 2023 •

edited

Loading

nicoburns Jan 6, 2023

nicoburns Jan 6, 2023

NickGerleman Jan 6, 2023

NickGerleman Jan 8, 2023

NickGerleman Jan 8, 2023 •

edited

Loading

nicoburns Jan 8, 2023

nicoburns Jan 8, 2023

nicoburns Jan 12, 2023

nicoburns commented Jan 12, 2023

Weibye left a comment

Import yoga tests #320

Import yoga tests #320

Conversation

nicoburns commented Jan 5, 2023 • edited Loading

Objective

Changes made

Notes

alice-i-cecile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alice-i-cecile Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickGerleman Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickGerleman Jan 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoburns commented Jan 12, 2023

Weibye left a comment

Choose a reason for hiding this comment

nicoburns commented Jan 5, 2023 •

edited

Loading

alice-i-cecile Jan 5, 2023 •

edited

Loading

NickGerleman Jan 6, 2023 •

edited

Loading

NickGerleman Jan 8, 2023 •

edited

Loading