layout2020 (flexbox): Implement start, end, and space-evenly content alignment
#31724
Conversation
nicoburns
force-pushed
the
content-alignment-fixes
branch
from
062a95d
to
a95bbfa
Compare
nicoburns
changed the title
start, end, and space-evenly content alignmentstart, end, and space-evenly content alignment
|
https://github.com/servo/servo/blob/main/tests/wpt/README.md#updating-test-expectations should answer your question about updating test expectations. |
|
@jdm Thanks! I've noticed that this PR is failing the "Tidy" CI check due to duplicate entries in the Cargo.lock. Is there a process for updating Cargo.lock to avoid this? (I've updated Cargo.toml to point to temporarily point to my branch of Stylo that this PR depends on - and I deleted and recreated Cargo.toml to make to work but it seems like CI is not even running most of it's checks in the current state). |
|
You can update https://github.com/servo/servo/blob/main/servo-tidy.toml to allow duplicates. |
|
It looks like you ran a general cargo update, though, which would be good to avoid unless it's actually required for these changes. |
nicoburns
force-pushed
the
content-alignment-fixes
branch
from
a95bbfa
to
ab1ed2e
Compare
|
OK, I've fixed the cargo.lock update to be just Stylo. The WPT check still seems to be skipped (does this need to be triggered manually?), but we seem to be passing the checks that have been run :) |
|
🔨 Triggering try run (#8339499036) for Linux WPT |
|
WPT tests are triggered via adding labels. They take too long to run for every push. I've triggered them here. |
|
Test results for linux-wpt-layout-2020 from try job (#8339499036): Flaky unexpected result (21)
Stable unexpected results that are known to be intermittent (17)
Stable unexpected results (4)
|
|
|
|
Hmm... the stable unexpected results (both PASS and FAIL) are all test expectations that I changed which are different on my local machine (macos m1). But I guess I should change them back. Frustrating that there are platform differences here (I'm assuming that's what's going on as the results are consistent for me). I feel like there shouldn't be! The |
nicoburns
force-pushed
the
content-alignment-fixes
branch
from
ff0950e
to
f85b80c
Compare
P.S. What are the criteria for being added to the Servo github organisation / who's decision would that be? It would be good to be able to add the CI-triggering labels myself so I don't have to take up anybody else's time |
|
🔨 Triggering try run (#8420665004) for Linux WPT |
Apologies for the delay. I've triggered a new WPT run.
Someone who understands flex layout or is willing to dive in here, will just need to review these changes. I think we also need to land servo/stylo#21 first.
It's all a bit informal now, but we should likely add a bit more more process here. |
|
Test results for linux-wpt-layout-2020 from try job (#8420665004): Flaky unexpected result (20)
Stable unexpected results that are known to be intermittent (18)
Stable unexpected results (20)
|
|
|
nicoburns
force-pushed
the
content-alignment-fixes
branch
from
f85b80c
to
38b14ef
Compare
I think these failures were due to servo/stylo#21 becoming out of date due to servo/stylo#22 being recently landed. I've rebased. I suppose we'll need to do another WPT run?
@Loirooriol would you be an appropriate person to review this and servo/stylo#21 (which is tiny btw). Also, is there anyone else active in the Servo project who is familiar enough with Flexbox to review such PRs, because I have a bunch more changes like this planned (streaming in flexbox-local changes from well-tested code in Taffy, so I'm pretty confident in the changes, but I imagine Servo will still want them reviewed). In this case servo
I suppose once 1. and 2. are completed, the PR author might be able to complete steps 3-5? |
nicoburns
force-pushed
the
content-alignment-fixes
branch
from
38b14ef
to
84da933
Compare
Loirooriol
added
T-linux-wpt-2013
github-actions
bot
removed
the
T-linux-wpt-2013
nicoburns
force-pushed
the
content-alignment-fixes
branch
from
4265905
to
f6cce8a
Compare
|
@mrobinson I think this also needs a "layout 2013" WPT run P.S. I've rebased this against latest |
|
All of the code changes here are in the layout_2020 directory so it's really unlikely that this has affected results for legacy layout. That said, before landing this, the merge queue will run the legacy layout (layout 2013) WPT suite as well so even if it did change results, it would be caught before landing. |
There is also a Stylo change which seems to have changed layout2013 parsing results. I don't really care if the tests are run or not. My main concern is trying to reduce latency with regard to test runs, such that if layout2013 needs to be run I'd rather it be run sooner than later. |
github-actions
bot
removed
the
T-linux-wpt-2013
|
🔨 Triggering try run (#8716833107) for Linux WPT |
|
Test results for linux-wpt-layout-2013 from try job (#8716833107): Flaky unexpected result (15)
Stable unexpected results that are known to be intermittent (8)
|
|
✨ Try run (#8716833107) succeeded. |
nicoburns
force-pushed
the
content-alignment-fixes
branch
from
f6cce8a
to
932156c
Compare
|
@Loirooriol Just a reminder that this is awaiting your re-review in case you have forgotten about it. |
|
|
||
| [.default, .verticalWriting 2] | ||
| expected: FAIL | ||
| expected: FAIL |
There was a problem hiding this comment.
Did you edit this file manually? I think it should end with a newline.
There was a problem hiding this comment.
Yes, the automated update mechanism isn't working very well for me due to platform-specific differences (at least I'm assuming that's the cause). Specifically, the following tests:
CI PASS [local FAIL] /css/css-flexbox/flexbox-flex-direction-column-percentage-ignored.html
CI FAIL [local PASS] /css/css-flexbox/flexbox-mbp-horiz-003-reverse.xhtml
CI FAIL [local PASS] /css/css-flexbox/flexbox-mbp-horiz-003.xhtml
CI FAIL [local PASS] /css/css-flexbox/flexbox-mbp-horiz-003v.xhtml
nicoburns
force-pushed
the
content-alignment-fixes
branch
from
932156c
to
b21bda4
Compare
nicoburns
force-pushed
the
content-alignment-fixes
branch
from
b21bda4
to
93da1c1
Compare
nicoburns
force-pushed
the
content-alignment-fixes
branch
from
93da1c1
to
ce876e6
Compare
Update test expectations for content alignment fixes Revert test expectations that are still generating the old results in CI Update layout2013 test expectation for content alignment Update content alignment fallback to use safe alignment Implement fallback alignment Update content alignment with recent spec changes
nicoburns
force-pushed
the
content-alignment-fixes
branch
from
ce876e6
to
13387b8
Compare
|
I've changed the reference in |
This is unnecessary as it can be managed via Cargo.lock and it makes it harder to update stylo as you always have to know what revision to use versus only when you are trying to pin to a specific one. We chose to use the branch name after agreeing among ourselves what the best approach should be, so I think we make changes here based on consensus. |
Hmm... ok sorry. Well whoever next bumps stylo can switch it back. Happy to go with the consensus, but I'm not personally convinced that the chosen approach is easier: specifically in this case (and it seems to me that it could easily happen in future) the required commit is not represented by a branch (because the Is there an easy way to update Cargo.lock to a specific commit of a branch without temporarily specifying it in the Cargo.toml and then changing it back or manually updating the commit ids in the Cargo.lock (which requires you to know both the old and new commit id). |
My workflow generally to switch Servo's |
IIRC that is what I tried first, but I ended up with compile errors because |
Instead of switching branch to be In the case that multiple changes land in Stylo (that need updates in Servo) and without Servo being updated in between (this has never happened for me, but can happen), you will then need to set |
|
I don't particularly mind changing the approach, but probably this conversation should happen in a https://github.com/servo/stylo/ issue. For now in #32146 I'm going back to using the branch. |
Implements support for
start,endandspace-evenlyvalues of thealign-andjustify-content properties. Also fixes alignment for some of the other values (space-between) in some cases. Note that the actual alignment logic is duplicated forAlignContentandJustifyContent. This is required as they are different types. If we switch to Gecko's alignment implementation then this could be de-duplicated as they become wrappers of a shared underling type.Depends on servo/stylo#21
Questions
Notes on WPT test changes:
align-content-wrap-003.html
align-content-wrap-005.html
directionsupport (was previously passing by coincidence)directionsupport (was previously passing by coincidence)flexbox_justifycontent-rtl-001.html
directionsupport (was previously passing by coincidence)flexbox-flex-direction-column-percentage-ignored.html
overflow-auto-008.html
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors