Closed Bug 1473044 Opened 6 years ago Closed 6 years ago

[css-flexbox] Flex container's auto-height (intrinsic cross size) should take column/row gaps into consideration

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: dholbert, Assigned: mihir)

References

Details

Attachments

(1 file)

Bug 1473044 - Make flexbox cross size take row/column gap into account.
59 bytes, text/x-review-board-request
dholbert
: review+
Details 
STR:
 1. Load https://jsfiddle.net/6kwz1m58/1

EXPECTED RESULTS:
Black border should stretch to include the magenta box (which is pushed down due to flex-wrap and a "row-gap").

ACTUAL RESULTS:
Magenta box is way outside the black border.  Black border only stretches down far enough such that it would've included the magenta box if there were no gap.
No longer blocks: 1398483
Depends on: 1398483
Assignee: nobody → miyer
Summary: Flex container's auto-height (intrinsic cross size) should take grid gaps into consideration → Flex container's auto-height (intrinsic cross size) should take column/row gaps into consideration
Priority: -- → P3
Summary: Flex container's auto-height (intrinsic cross size) should take column/row gaps into consideration → [css-flexbox] Flex container's auto-height (intrinsic cross size) should take column/row gaps into consideration
Attachment #8991066 - Flags: review?(dholbert)
Comment on attachment 8991066 [details]
Bug 1473044 - Make flexbox cross size take row/column gap into account.

https://reviewboard.mozilla.org/r/256062/#review262938

Looks good! This needs a test, though.

Please make sure you're testing both the basic behavior, and also testing "max-height" clamping (which I think should Just Work based on how ComputeCrossSize uses the value that you're tweaking here).
Attachment #8991066 - Flags: review?(dholbert) → review-
Comment on attachment 8991066 [details]
Bug 1473044 - Make flexbox cross size take row/column gap into account.

https://reviewboard.mozilla.org/r/256062/#review262938

Just to clarify, I should file the test under this bug (1473044) and not the original flexbox gap bug, right?
Correct -- just include the test as part of this bug's patch (like you did in bug 1398483 for that bug's test).

(To write the test, I'd suggest just making a modified copy of one of your reftests from bug 1398483.  That's perhaps better than starting with the jsfiddle testcase here, because the jsfiddle here depends on the size of its text, and text sizing/rendering can be a bit flaky for reftesting purposes.)
Comment on attachment 8991066 [details]
Bug 1473044 - Make flexbox cross size take row/column gap into account.

https://reviewboard.mozilla.org/r/256062/#review263216

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-column-row-gap-003-ref.html:6
(Diff revision 2)
> +<!--
> +     Reference for the correctness of gaps in horizontal and vertical multi-line
> +     flex containers.
> +-->
> +<html>
> +<head>
> +  <title>Reference: Testing cross size computation with row and column gaps in horizontal multi-line flex containers</title>

Drop the code-comment here, since it just repeats what the title already says and hence doesn't add any information.

(Plus it has a stale mention of "vertical" which is incorrect I think.)

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-column-row-gap-003-ref.html:29
(Diff revision 2)
> +    .columnWrap {
> +      flex-flow: column wrap;
> +    }

Looks like this class is unused, so let's get rid of it (in both files).

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-column-row-gap-003.html:6
(Diff revision 2)
> +<!--
> +     Testcase for the correctness of gaps in horizontal and vertical multi-line
> +     flex containers.
> +-->
> +<html>
> +<head>
> +  <title>CSS Test: Testing cross size computation with row and column gaps in horizontal and vertical multi-line flex containers</title>

Drop "and vertical" from the `<title>` here.  And drop the whole code-comment above it ("Testcase for..."), same as in the reference case.

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-column-row-gap-003.html:25
(Diff revision 2)
> +      border: 1px solid black;
> +      column-gap: 20px;
> +      row-gap: 40px;
> +      align-content: space-around;
> +      justify-content: space-around;
> +      float: left;

No need for these to be floated -- let's drop the "float:left" here, so that these flex containers just stack vertically.

(float:left adds complexity, which can be worth it if the sizes are such that you need to stack things horizontally due to the sizes of the things involed; and here, we don't really need that.)

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-column-row-gap-003.html:43
(Diff revision 2)
> +    .w200 {
> +      width: 200px;
> +      max-height: 900px;
> +    }

Probably drop the "max-height:900px" from the w200 scenario here, so that you're testing a truly-unconstrained height somewhere.

(If you want to test a large max-height, that's fine too, but you could maybe do that with a specific hClampLarge class rather than with a kinda-confusing max-height declaration on a w200 class.)
Attachment #8991066 - Flags: review?(dholbert) → review+
Keywords: checkin-needed
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0e53ce7e1398
Make flexbox cross size take row/column gap into account. r=dholbert
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0e53ce7e1398
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
I have reproduced this bug with Nightly 63.0a1 (2018-07-04) on Windows 10, 64 Bit!
This bug's fix is verified with latest Beta!

Build ID 	20180927143327
User Agent 	Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:63.0) Gecko/20100101 Firefox/63.0
QA Whiteboard: [good first verify] → [good first verify] [testday20180928]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: