Bug 38249 – Vertically shrinking flexbox has wrong height when padding is specified

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Yoshiki Hayashi]

Created attachment 54536 [details]
HTML file to reproduce bug

When a flexbox with non-zero box-flex property is resized to smaller size than the specified one, the height of the flexbox gets an unexpected value.

If you open the attached HTML file, 100x100 mostly green box rendered is the expected result.  However, current implementation renders 100x114 mostly green box.

[Yoshiki Hayashi]

Created attachment 54538 [details]
Proposed Patch

[Shinichiro Hamaji]

Comment on attachment 54538 [details]
Proposed Patch

Generally, this looks good. Putting r- for some style nitpicks.

LayoutTests/fast/flexbox/child-flexing.html:37
 +    overflow-y: auto;
Some properties are duplicated with verticalOuter. I'd write

div.outer {
  width: 100px;
  height: 100px;
  ...
}

Also, I'm not sure we are setting overflow-y. I think overflow: auto would be fine.


LayoutTests/fast/flexbox/child-flexing.html:35
 +    box-align: top;
I don't think box-align is necessary to check this behavior?

LayoutTests/fast/flexbox/child-flexing.html:43
 +    background-color: green;
We usually use greens to mean "the test passes". I think it's better to choose a different color.

LayoutTests/fast/flexbox/child-flexing.html:53
 +    border-left: 90px solid olive;
As the bug summary contains the word "padding", I think it's better to check paddings as well.

LayoutTests/fast/flexbox/child-flexing.html:124
 +  shouldBe("element.scrollHeight", "100");
I'd add debug("vertically expanding") above this line. In this way we can see which test is failing easily. Or, we can just change the shouldBe lines like

shouldBe("document.getElementById('expandVertical')", "100");



LayoutTests/fast/flexbox/child-flexing-expected.txt:9
 +  horizontally shrinking
These messages are unnecessary. It may be better to put all HTMLs above id="console" into a single <div> and remove the div if window.layoutTestController exists.

Please check printing/script-tests/pageNumerForElementById.js as an example.

[Yoshiki Hayashi]

Created attachment 54544 [details]
2010-04-28  Eric Seidel  <[e-mail]>

[Yoshiki Hayashi]

Argh, wrong command line arg ended up uploading unrelated patch.  Sorry for the noise.  Will upload updated patch shortly.

[Yoshiki Hayashi]

Created attachment 54547 [details]
Proposed Patch 2

[Shinichiro Hamaji]

Comment on attachment 54547 [details]
Proposed Patch 2

Looks good. The original C++ code was apparently wrong and the test case appropriately checks the code around the fix.

I'd land this manually so I'm setting cq-.

[Shinichiro Hamaji]

Committed r58630: <http://trac.webkit.org/changeset/58630>