floor alignment offset #549
Conversation
There was a problem hiding this comment.
Hi, and thanks for the PR!
Good catch - that value definitely shouldn't ever be negative! My intuition is that it is free_space that ought to be floored at zero, as the concept of "free space" is such that it should always be non-negative. I suspect gap may also need to be floored at zero. But that should happen elsewhere (when the layout algorithm first accesses the gap style) as gap is set by the user and a negative value is never valid. I think it is fine for this code to assume that gap is positive.
Having said that, we usually try not to rely on intuition but on generated tests that assert that our output is identical to Chrome (unless we determine that Chrome's output is incorrect). As such I would like to request the following changes:
-
Change the code to floor
free_space, either at the top of this function, or in the callers (this function is used by both the flexbox and grid algorithms). I'd be tempted to put the floor where thefree_spaceis first being computed. -
Add a line to the changelog (
RELEASES.md) under "Fixes" -
Would it be possible for you to turn the HTML you have been testing Chrome/Firefox's output with into Taffy "generated tests". This would involve:
- Adding HTML file(s) to
test_fixtures/flex(duplicate one of the existing ones as a template, and modify the "test root" with your HTML). - Running
cargo gentest(you will need Chrome andchromedriverinstalled) to generate the tests - You can then actually run the new tests with
cargo test
Failing that, could you post a sample of the HTML that triggers this issue as a comment on this issue so that we can we create the tests ourselves?
- Adding HTML file(s) to
i think this would be incorrect - chrome only floors non-first items (see the way the lines go above the boxes in the evenly and around screenshots in cols 6-7. also just to point out - i made this patch to the 0.3.x branch, not sure if that's what you want. the code i used is below (can provide the bevy code too but i guess you don't need that?), just changing the justify-content for each case. i'm not really keen to add the tests, i think it will take me much longer than someone with more familiarity ... css .screen {
display: flex;
flex-direction: row;
width: 1280px;
height: 100px;
background-color: red;
position: absolute;
top: 10%;
}
.col {
display: flex;
flex-direction: column;
height: 100%;
width: 10%;
border: 1px solid blue;
background-color: #404040;
}
.box {
display: flex;
flex-direction: column;
justify-content: space-evenly;
border: 1px solid red;
color: white;
font-family: 'Fira Mono', monospace;
font-size: 7pt;
}html <!DOCTYPE html>
<html>
<head>
<link rel="stylesheet" href="styles2.css">
</head>
<body>
<div class="screen">
<div class="col">
<div class="box" style="height: 75px;">
<div>text line one</div>
<div>text line two</div>
</div>
</div>
<div class="col">
<div class="box" style="height: 65px;">
<div>text line one</div>
<div>text line two</div>
</div>
</div>
<div class="col">
<div class="box" style="height: 55px;">
<div>text line one</div>
<div>text line two</div>
</div>
</div>
<div class="col">
<div class="box" style="height: 45px;">
<div>text line one</div>
<div>text line two</div>
</div>
</div>
<div class="col">
<div class="box" style="height: 35px;">
<div>text line one</div>
<div>text line two</div>
</div>
</div>
<div class="col">
<div class="box" style="height: 25px;">
<div>text line one</div>
<div>text line two</div>
</div>
</div>
<div class="col">
<div class="box" style="height: 15px;">
<div>text line one</div>
<div>text line two</div>
</div>
</div>
<div class="col">
<div class="box" style="height: 5px;">
<div>text line one</div>
<div>text line two</div>
</div>
</div>
<div class="col">
<div class="box" style="height: -5px;">
<div>text line one</div>
<div>text line two</div>
</div>
</div>
<div class="col">
<div class="box" style="height: -15px;">
<div>text line one</div>
<div>text line two</div>
</div>
</div>
</div>
</body>
</html> |
Yeah, no worries. I'll take a look later. The HTML you provided is really helpful.
We normally try to land fixes on
Ah ok. That's good to know. I'll play around with the tests and try to pin down the behaviour. |
|
We probably ought to implement the "safe" alignment variants too. With 
|
|
@robtfm Taffy |
|
awesome, thank you :) |
Objective
correct space-xxx behaviour for cramped containers.
Context
i noticed a difference in layout between chrome and firefox vs taffy (via bevy) when the computed alignment for non-first items returns a negative value. it seems to be corrected by flooring the offset at zero.
i don't know for sure if the floor should be with the
gapor without, i assumed with.screenshots below for containers with
justify-contentset to each space-xxx modenote the "modified" screenshots still show differences in the last two columns where the height is a negative number of px - i am not sure if there's a good spec for that, but i can work around it by just supplying
Autoif the input is negative.space-between original
modified
space-evenly original
modified
space-around original
modified