Last Comment Bug 778413 - Invalid minimum cell width calculation for box-sizing:border-box
: Invalid minimum cell width calculation for box-sizing:border-box
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: Layout: Tables (show other bugs)
: 16 Branch
: All All
: -- minor with 1 vote (vote)
: mozilla17
Assigned To: Tal Aloni
: Tal Aloni
Mentors:
Depends on:
Blocks: 243412 338554
  Show dependency treegraph
 
Reported: 2012-07-28 02:41 PDT by Tal Aloni
Modified: 2012-09-19 10:26 PDT (History)
8 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
Border-Box sizing causing text wrapping in small table (223 bytes, text/html)
2012-07-28 02:41 PDT, Tal Aloni
no flags Details
Table Cell Border-Box Auto Width Patch v1 (1.91 KB, patch)
2012-07-28 06:46 PDT, Tal Aloni
dbaron: review-
Details | Diff | Review
Border-Box Overflow Test Case (325 bytes, text/html)
2012-07-28 06:54 PDT, Tal Aloni
no flags Details
Table Cell Border-Box Auto Width Patch v2 (1.98 KB, patch)
2012-07-28 11:20 PDT, Tal Aloni
dbaron: review-
Details | Diff | Review
Table Cell Border-Box Auto Width Patch v3 (3.90 KB, patch)
2012-07-28 14:53 PDT, Tal Aloni
no flags Details | Diff | Review
Table Cell Border-Box Auto Width Patch v4 (5.82 KB, patch)
2012-08-01 23:49 PDT, Tal Aloni
dbaron: review+
Details | Diff | Review
Table Cell Border-Box Auto Width Patch v5 (5.97 KB, patch)
2012-08-03 00:36 PDT, Tal Aloni
tal.aloni.il: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Review
Minimum cell width calculation reftest (2.63 KB, patch)
2012-08-22 12:12 PDT, Tal Aloni
dbaron: review+
Details | Diff | Review
Minimum cell width calculation reftest v2 (2.54 KB, patch)
2012-08-25 03:38 PDT, Tal Aloni
tal.aloni.il: review+
Details | Diff | Review

Description Tal Aloni 2012-07-28 02:41:08 PDT
Created attachment 646830 [details]
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 2012-07-28 06:46:09 PDT
Created attachment 646847 [details] [diff] [review]
Table Cell Border-Box Auto Width Patch v1
Comment 2 Tal Aloni 2012-07-28 06:54:46 PDT
Created attachment 646848 [details]
Border-Box Overflow Test Case
Comment 3 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-28 09:39:16 PDT
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 2012-07-28 11:20:17 PDT
Created attachment 646883 [details] [diff] [review]
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-7 (review requests must explain patch) 2012-07-28 12:38:47 PDT
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-7 (review requests must explain patch) 2012-07-28 12:42:51 PDT
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 2012-07-28 14:53:01 PDT
Created attachment 646907 [details] [diff] [review]
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-7 (review requests must explain patch) 2012-07-28 14:58:28 PDT
I don't think it would be confusing in the future if you called it adjustedBoxSizing or something like that.
Comment 9 Tal Aloni 2012-07-28 15:10:45 PDT
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-7 (review requests must explain patch) 2012-08-01 14:59:35 PDT
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 2012-08-01 22:42:39 PDT
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 2012-08-01 22:59:57 PDT
On second thought, I could push the calculation of the "add" variable upward, and reuse the logic.
Comment 13 Tal Aloni 2012-08-01 23:49:09 PDT
Created attachment 648236 [details] [diff] [review]
Table Cell Border-Box Auto Width Patch v4

There you go, equivalent to V3 with merged logic.
Comment 14 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-08-02 16:43:28 PDT
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 2012-08-03 00:36:41 PDT
Created attachment 648639 [details] [diff] [review]
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] 2012-08-04 08:32:02 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ebe042fd624

Should this have a test?
Comment 17 Tal Aloni 2012-08-04 08:50:24 PDT
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 2012-08-04 09:06:15 PDT
(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] 2012-08-04 09:08:13 PDT
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] 2012-08-04 09:09:55 PDT
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 2012-08-04 09:23:06 PDT
Thanks!
Comment 22 Ed Morley [:emorley] 2012-08-04 10:12:27 PDT
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] 2012-08-04 12:06:13 PDT
Re-landed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d802da5c4ed2
Comment 24 Ryan VanderMeulen [:RyanVM] 2012-08-04 18:42:10 PDT
https://hg.mozilla.org/mozilla-central/rev/d802da5c4ed2
Comment 25 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-06 16:10:55 PDT
approving for Aurora since this is a fix to a new feature in 16.
Comment 26 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-08-06 17:31:13 PDT
(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] 2012-08-08 11:52:07 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/7f9f10e43508
Comment 28 Tal Aloni 2012-08-22 12:12:18 PDT
Created attachment 654306 [details] [diff] [review]
Minimum cell width calculation reftest
Comment 29 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-08-24 18:14:32 PDT
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 2012-08-25 02:59:00 PDT
> (You tested that the test fails without the patch, right?)
of course.
Comment 31 Tal Aloni 2012-08-25 03:38:02 PDT
Created attachment 655294 [details] [diff] [review]
Minimum cell width calculation reftest v2

NOTE TO CHECK-IN BUDDY:
Only the reftest needs to be checked in!
Thanks!
Comment 33 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-08-25 12:55:40 PDT
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 36 Tal Aloni 2012-08-25 23:08:51 PDT
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-7 (review requests must explain patch) 2012-08-25 23:53:02 PDT
Thet table-bordercollapse folder is for tests that test border collapsing.  This test tests width computation.

Note You need to log in before you can comment on or make changes to this bug.