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 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.)

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.

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 on attachment 646883 [details] [diff] [review] Table Cell Border-Box Auto Width Patch v2 Mostly looks good, but could you address the comments above?

(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.

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

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".

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.

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.

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

There you go, equivalent to V3 with merged logic.

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.

> 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.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ebe042fd624 Should this have a test?

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.

(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.

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.

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.

Thanks!

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 :-)

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

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

(In reply to Ryan VanderMeulen from comment #16) > Should this have a test? Yes, it should. Sorry; should have caught that during review.

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?)

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

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

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

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.

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

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

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.

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