778413 - Invalid minimum cell width calculation for box-sizing:border-box

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Tal Aloni]

Attachment: Border-Box sizing causing text wrapping in small table

When using the newly implemented box-sizing:border-box and not specifying width,
minimum cell width will be calculated incorrectly (less than the correct minimum), this may cause wrapping issues or may cause cell elements to appear outside the cell border.

Comment 1

[Tal Aloni]

Attachment: Table Cell Border-Box Auto Width Patch v1

Comment 2

[Tal Aloni]

Attachment: Border-Box Overflow Test Case

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 646847 [details] [diff] [review]
Table Cell Border-Box Auto Width Patch v1

The documentation of GetMinWidth and GetPrefWidth in nsIFrame.h is clear that it should always return the content-box width, and that's what other implementations (e.g., in nsBlockFrame) do.

I think we should maintain consistency with the comment in nsIFrame.h; I think the bug here is in the callers of these functions -- and in this case probably only in *some* of the callers.  (In other words, I think this patch would probably fix some things and break others.)

Comment 4

[Tal Aloni]

Attachment: Table Cell Border-Box Auto Width Patch v2

David, Thanks for the review!
I'm sorry, You are absolutely right, I had some incorrect assumptions about the role of GetMinWidth and GetPrefWidth.

Here is an updated patch.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think this looks basically correct now:  as I understand it, minCoord and prefCoord represent values that are box-sizing-based widths until near the end of the function, when they're converted to border-box widths.  It would be good to have a comment explaining that.

I also think it would probably be useful to have a boxSizing variable that condenses the quirks check and the switch across values of boxSizing, e.g., initialized with:

if (is quirks) {
  // move the comment here from near the end of the function explaining why we ignore box sizing in quirks mode
  boxSizing = NS_STYLE_BOX_SIZING_CONTENT;
} else {
  boxSizing = stylePos->mBoxSizing;
}

and then switch on that rather than having the quirks mode check duplicated in two parts of the function.

It might also be worth adding a comment near at least the first call to nsLayoutUtils::ComputeWidthValue that we're passing 0's for the fourth and fifth parameters because we're operating in box-sizing widths rather than content-box widths as ComputeWidthValue normally operates.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 646883 [details] [diff] [review]
Table Cell Border-Box Auto Width Patch v2

Mostly looks good, but could you address the comments above?

Comment 7

[Tal Aloni]

Attachment: Table Cell Border-Box Auto Width Patch v3

(In reply to David Baron [:dbaron] from comment #5)
> I also think it would probably be useful to have a boxSizing variable that
> condenses the quirks check and the switch across values of boxSizing
I agree with the first suggestion, implemented in v3.
I don't want to switch the value of box-sizing as it may be confusing to read in the future.

I've also added comments as suggested.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I don't think it would be confusing in the future if you called it adjustedBoxSizing or something like that.

Comment 9

[Tal Aloni]

mBoxSizing is already a variable, so declaring this new variable won't help performance. it also won't decrease the lines of code.
if you are concerned with readability, than I'd rather add a comment under "if (isQuirks)" such as "// NS_STYLE_BOX_SIZING_CONTENT".

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm not concerned about performance; I'm concerned about repeating the same logic in two different places when it could be in one place.

Comment 11

[Tal Aloni]

I hear you, but I think repeating the logic twice here makes the code easier to understand and better reflect the logic behind it.
I would do the change if you insist, but I rather not.

Comment 12

[Tal Aloni]

On second thought, I could push the calculation of the "add" variable upward, and reuse the logic.

Comment 13

[Tal Aloni]

Attachment: Table Cell Border-Box Auto Width Patch v4

There you go, equivalent to V3 with merged logic.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 648236 [details] [diff] [review]
Table Cell Border-Box Auto Width Patch v4

>+    bool isQuirks = (aFrame->PresContext()->CompatibilityMode() == eCompatibility_NavQuirks);

Drop the outer (), and break the line after the == so it doesn't go over 80 columns.

>+    nscoord outerEdgesWidth = 0;

Maybe call this boxSizingToBorderEdge, since that says what it is more clearly?

>@@ -87,6 +90,42 @@
> 
>         minCoord = aFrame->GetMinWidth(aRenderingContext);
>         prefCoord = aFrame->GetPrefWidth(aRenderingContext);

Perhaps prefix this comment with "Until almost the end of this function,":

>+        // minCoord and prefCoord represents the box-sizing based width

represents -> represent

>+        if (isQuirks) {
>+          outerEdgesWidth = offsets.hPadding + offsets.hBorder;
>+        }

4-space indent here


r=dbaron with that.  Thanks for fixing this and for putting up with all my comments.

Comment 15

[Tal Aloni]

Attachment: Table Cell Border-Box Auto Width Patch v5

> Thanks for fixing this and for putting up with all my comments.
no problem, you are usually right. thanks for going forward with my patches.

> Drop the outer (), and break the line after the == so it doesn't go over 80
> columns.
I did just that, note that I truly believe the outer () improve readability, and I think limiting lines to 80 characters is a bit dated.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ebe042fd624

Should this have a test?

Comment 17

[Tal Aloni]

Comment on attachment 648639 [details] [diff] [review]
Table Cell Border-Box Auto Width Patch v5

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 338554
User impact if declined: border-box minimum cell width calculation will be incorrect.
Risk to taking this patch (and alternatives if risky): None, this change only affect border-box, which is new to aurora (Firefox 16).
String or UUID changes made by this patch: None.

Comment 18

[Tal Aloni]

(In reply to Ryan VanderMeulen from comment #16)
> Should this have a test?
I don't think so, it's a childhood illness of a new feature.
The target milestone should be Firefox 16 though, as this fixes a bug in a feature new to Firefox 16.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Target milestone tracks when the fix lands on mozilla-central. The tracking flags track landing on other branches. In other words, if it lands on Aurora, we would set status-firefox16 to fixed.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

With respect to a test, I'll defer to dbaron. But in general, all regression fixes should have tests checked in with them to avoid breaking them again in the future. Being a new feature isn't really relevant IMO.

Comment 21

[Tal Aloni]

Thanks!

Comment 22

[Ed Morley [:emorley]]

Backed out as part of the mass tree revert due to bustage caused by other landings:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c801b99d726f

Once the tree is open again, this can reland :-)

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Re-landed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d802da5c4ed2

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d802da5c4ed2

Comment 25

[Lukas Blakk [:lsblakk] use ?needinfo]

approving for Aurora since this is a fix to a new feature in 16.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Ryan VanderMeulen from comment #16)
> Should this have a test?

Yes, it should.  Sorry; should have caught that during review.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7f9f10e43508

Comment 28

[Tal Aloni]

Attachment: Minimum cell width calculation reftest

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 654306 [details] [diff] [review]
Minimum cell width calculation reftest

You should remove the table-layout:fixed from both test and reference; the bug that this is testing is explicitly about table-layout:auto tables.  In this particular case fixed falls back to auto because no width is specified, but it's still very confusing to have it there.

Also, you should include a newline at the end of the file.  (Windows treats newline characters as line separators, but other OSes treat them as line terminators.  You want things so diff doesn't show the "No newline at end of file" message, but also doesn't show blank lines at the end of the file.)

r=dbaron with that

(You tested that the test fails without the patch, right?)

Comment 30

[Tal Aloni]

> (You tested that the test fails without the patch, right?)
of course.

Comment 31

[Tal Aloni]

Attachment: Minimum cell width calculation reftest v2

NOTE TO CHECK-IN BUDDY:
Only the reftest needs to be checked in!
Thanks!

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/35fddfaef1b6
https://hg.mozilla.org/releases/mozilla-aurora/rev/aa9dbb75a716

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

In hindsight, that probably should have gone in table-width rather than table-bordercollapse, and also had a less strange filename.  But it's probably not worth changing.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/815d9b2bd355
https://hg.mozilla.org/releases/mozilla-aurora/rev/b93e2a513c8f

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/35fddfaef1b6
https://hg.mozilla.org/mozilla-central/rev/815d9b2bd355

Comment 36

[Tal Aloni]

Those tests have border-collapse:collapse, that's why I put it in the table-bordercollapse folder.
however, for the purpose of this issue, it doesn't matter if this test is using border-collapse:collapse or not.

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Thet table-bordercollapse folder is for tests that test border collapsing.  This test tests width computation.