implement direction property

[kane50613]

Objective

• Add support for bi-directional layout (left-to-right and right-to-left) via the direction CSS style property.
• Fixes Support direction property (right-to-left layout) #213, Scope of RTL support kane50613/takumi#330

Context

This PR is adopted from #736's work.

This change attempts to support bidi layout only for horizontal layout direction, i.e. it doesn't support vertical content such as CJK or Mongolian vertical writing systems.

Feedback wanted

• I'm new to developing taffy, how should I add & run test, I tried using blitz but seems like the main branch is targeted on a specific taffy revision.

[nicoburns]

I'm new to developing taffy, how should I add & run test, I tried using blitz but seems like the main branch is targeted on a specific taffy revision.

Blitz is currently running against #866. It's probably about time we merged that to main. I'll look into that soon. Testing Taffy with Blitz can be a good idea. You can use it to open HTML pages and inspect them interactively. You can also use it to run a subset of the WPT tests against Blitz/Taffy. I need to document the process for this properly, but process is basically:

• Clone https://github.com/web-platform-tests/wpt somewhere (locally)
• Set the WPT_DIR environment variable the directory where you cloned the WPT repo (I set this in my .zshrc)
• Run just wpt css in the Blitz repo (or cargo run -rp wpt css if you don't have just installed). The last argument is the subset of the tests to run (matching against the directory structure of the tests), so you also do just wpt css/css-flexbox, just wpt css/css-flexbox css/css-grid, etc)

The other thing you can do (and I would encourage you to do for this PR), is to use Taffy's "generated test" setup. We have some documentation for that in CONTRIBUTING.md. Basically, we have a test generator in scripts/gentest that runs the HTML snippets in text_fixtures through Google Chrome using (webdriver/chromedriver), scrapes the layout that Chrome computes, and then uses that to generates tests in tests/generated. Those tests can then be run with just cargo test.

To test the direction property with generated tests, we will need to teach the test generator about the direction property (see how #866 added support for the float and clear properties https://github.com/DioxusLabs/taffy/pull/866/changes#diff-0cbb140bc47891b73c2adc1c3d68144eb8cba70e110cbaf5ebe4dd9e6dd5ff93)

[nicoburns]

@kane50613 I tried porting this to Blitz and running the WPT tests over it. Unfortunately it doesn't seem to improve test results (in fact it makes them slightly worse). Although it's possible I made some error in the port, as there is some interaction between float and direction when computing the position of block children. Aside from the Taffy gentests, do you have any evidence that this is correct enough to be useful / an improvement not having the feature (e.g. it works in Takumi)?

Code if you want to look at it:

• Taffy branch https://github.com/DioxusLabs/taffy/tree/direction-floats
• Blitz branch https://github.com/nicoburns/blitz/tree/direction

@alice-i-cecile do you have an opinion on whether we ought to consider the gentests in this PR sufficient to merge it vs. wanting to improve on WPT tests? (the gentests look good to me, but there aren't all that many of them)

[kane50613]

I'll try to collect more tests with Takumi

[alice-i-cecile]

Let's wait on the author for more tests, then merge :)

[nicoburns]

I've also setup testing of this in Servo:

• Implement direction property for CSS Grid servo/servo#42118

That uses the branch from this PR as Servo is not yet using the version of Taffy that integrates floats.

If there are regressions in the Servo test results that may be blocking as the Servo project probably won't accept Taffy upgrades with test regressions unless there is a good reason for them (a good reason would be something like: "the implementation has actually improved, but we previously passing tests when we shouldn't have been").

To run the tests locally:

• Checkout the branch from the PR above
• If you've never run Servo before, run ./mach bootstrap
• Run ./mach build -r (-r is a release build which you definitely want to run tests)
• Run ./mach test-wpt -r --include css/css-grid (Servo only uses Taffy for CSS Grid)

More setup help is available in the Servo book https://book.servo.org/building/building.html

[nicoburns]

As far as I am aware, there are no cases where a container's direction is determined by it's parent's layout algorithm (I believe that in all cases the value for direction comes directly from a node's styles).

As such, I think this extra parameter could be removed in favour of containers retrieving their own direction from the their style object.

[kane50613]

agree, I think I misunderstood the "inheritance" here

[nicoburns]

The test results are actually not bad. A lot more newly passing tests than newly failing tests (but I think we would expect a lot more new passes for an actually correct implementation).

(it's only the "Stable unexpected results" we care about)

[nicoburns]

Hmm... a little more sleuthing into some of the failing Blitz tests get me this. This is actually the ref that it compares against not the test (http://wpt.live/css/css-flexbox/reference/justify-content-001-ref.html):

main (left) vs PR (right):

Looking at the code, I'm pretty sure it's something to do with floats that's messing this up (so essentially a merge a conflict), because there's nothing direction related here:

<!DOCTYPE html>
<meta charset="utf-8">
<title>CSS Reference</title>
<link rel="author" title="Intel" href="[http://www.intel.com](view-source:http://www.intel.com/)">
<style>
  #blue {
    background: blue;
    height: 100px;
    width: 150px;
  }
  #orange {
    background: orange;
    height: 100px;
    left: 150px;
    position: relative;
    top: -100px;
    width: 150px;
  }
</style>
<body>
  <p>Test passes if there is a single blue rectangle on the left, a single orange rectangle directly to its right, and there is no red visible on the page.</p>
  <div id="blue"></div>
  <div id="orange"></div>
</body>

[nicoburns]

I've tracked down and fixed the above issues on the direction-floats branch.

There are now only two newly failing Blitz WPT tests, and they're both subgrid tests so I don't think we need to worry about that (we don't implement subgrid, so we wouldn't expect those tests to pass).

However, there are no newly passing Blitz tests which is a bit of a shame. I suspect we may need to implement more values alignment enums to get the full benefit here. I believe the following are unimplemented:

• self-start (the start according the child's direction property)
• self-end(the end according the child's direction property)

[nicoburns]

@kane50613 I have merged the floats PR. The direction-floats branch contains a version of this branch that I have rebased on top of latest main. I would recommend switching to that branch (perhaps force-pushing it's contents to this one?).

[kane50613]

ok I'll rebase this branch

[kane50613]

Should I avoid doing 96fd6e3 that creates huge diff or that's fine

[nicoburns]

@kane50613

You could change:

let direction = match style["direction"] {
    Value::String(ref value) => match value.as_ref() {
        "rtl" => quote!(direction: taffy::style::Direction::Rtl,),
        "ltr" => quote!(direction: taffy::style::Direction::Ltr,),
        _ => quote!(),
    },
    _ => quote!(),
};

to

let direction = match style["direction"] {
    Value::String(ref value) => match value.as_ref() {
        "rtl" => quote!(direction: taffy::style::Direction::Rtl,),
        _ => quote!(),
    },
    _ => quote!(),
};

relying on the fact that we know LTR is the default. That's what we've done in the test generator for other styles (omit the style if it's the default value) to try to help keep the tests small / readable. And it would allow you to use computedStyle without the big diff.

[kane50613]

Good news, there are only a few minor issues remaining with the flex items.

EDIT: caused by non-monospaced font

[kane50613]

I'm adding new fixtures with gap to test and find out the root cause

[nicoburns]

It's an absurdly enormous diff (+177k LOC), and I'm not sure we can land it like this (becuause it's making the tests very slow to compile!). But nicoburns@66b37a6 creates direction: rtl variants of every single test like we do for box_sizing (in the "rtl" variant of the tests, the default direction for elements with no direction specified is rtl).

That gives 4020 passed; 232 failed; which is actually not too bad!

[kane50613]

ok I've fixed the test from Takumi's end and confirmed it is only because I am not using a monospaced font not direction's fault.

[kane50613]

It's an absurdly enormous diff (+177k LOC), and I'm not sure we can land it like this (becuause it's making the tests very slow to compile!). But nicoburns@66b37a6 creates direction: rtl variants of every single test like we do for box_sizing (in the "rtl" variant of the tests, the default direction for elements with no direction specified is rtl).

That gives 4020 passed; 232 failed; which is actually not too bad!

Does that mean RTL specific test cases from this PR like block_direction_rtl.html can be removed?

[nicoburns]

It's an absurdly enormous diff (+177k LOC), and I'm not sure we can land it like this (becuause it's making the tests very slow to compile!). But nicoburns@66b37a6 creates direction: rtl variants of every single test like we do for box_sizing (in the "rtl" variant of the tests, the default direction for elements with no direction specified is rtl).
That gives 4020 passed; 232 failed; which is actually not too bad!

Does that mean RTL specific test cases from this PR like block_direction_rtl.html can be removed?

We should probably leave them for now, until such time as the perf problems with the other tests can be fixed. At some point we will also want tests that test "mixed direction". As the automated setup will only generate tests where all nodes are ltr or all nodes are rtl, but there may be cases where mixing the two causes bugs (esp. once we support self-start and self-end).

[nicoburns]

(didn't meant to close)

[nicoburns]

69147b2 gets us to 4046 passed; 206 failed;

And I've noticed a big unimplemented part of direction: for CSS Grid, the placement algorithm should place items in the opposite order. i.e. when grid-auto-flow: row and direction: rtl, the first grid cell for auto placement is the top-right cell not the top-left cell like it would be for direction: ltr.

[nicoburns]

2436704 gets us to 4070 passed; 182 failed;

[nicoburns]

I believe this is wrong (and same for absolute position below). This should use the LTR values for both LTR and RTL here. Instead of this, the placement algorithm should place items in a different order. The code here happens to work when the columns are all the same size and the padding/border on either side is the same, but fails is anything is non-symmetrical.

[nicoburns]

5c1880e gets us down to 174 failing tests (note that the number is really half of this, because each test has both content box and border box variants).

We also seem to be running into #871. The fact that content_size includes half the borders is particularly problematic in a BiDi context, because the scroll_width() helper doesn't know which border to include in it's calculations.