1166728 - remove box-sizing: padding-box

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

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

The CSS WG just resolved to remove box-sizing: padding-box.  We should remove it.

Need to deal with:
https://mxr.mozilla.org/mozilla-central/search?string=box-sizing%3A+padding-box&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
first.

Comment 1

[Boris Zbarsky [:bzbarsky]]

And also https://mxr.mozilla.org/addons/search?string=box-sizing%3A+padding-box&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=addons

Comment 2

[Daniel Holbert [:dholbert]]

To get the property not-recognized-by-the-CSS-parser (fixing this bug), we need to:
(1) remove the line here:
 https://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSProps.cpp?rev=956a03448bbe#895

(That maps the CSS keyword "padding-box" to our internal numeric representation of this box-sizing value, which we use in layout.  We use that same keyword table when re-serializing the style for getComputedStyle and elem.style, too; so I don't think there's any extra work for that.)

That one-line change should "fix" this bug, as far as web developers can see. Beyond that, we need to:
 (2) fix the *usages* of this property in tests & our browser UI

and then:
 (3) fix the now-dead-code that checks for this value in layout:
https://mxr.mozilla.org/mozilla-central/ident?i=NS_STYLE_BOX_SIZING_PADDING

We also need to get add-ons updated, per the URL in comment 1, but that should maybe happen separately.

Comment 3

[Daniel Holbert [:dholbert]]

http://caniuse.com/#search=box-sizing currently shows that we're literally the only browser to support "padding-box".

Comment 4

[Kyle Zentner]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=391b976818d3

Comment 5

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

The searchbar.css changes are suspicious -- changing from padding-box to border-box on an element with a border is a substantive change.  Does it have padding?  (Can't tell from local examination of the CSS, but you could tell from, e.g., inspector, if you can get it to inspect chrome.)  If it doesn't have padding, you should be able to change to content-box.

Comment 6

[Daniel Holbert [:dholbert]]

(Comment 5 is about changes to a " .searchbar-engine-one-off-item:not(.last-row) {" rule.)

I believe that style is applied to this element:
> 1321           let button = document.createElementNS(kXULNS, "button");
[...]
> 1328           button.setAttribute("class", "searchbar-engine-one-off-item");
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/urlbarBindings.xml?rev=446ce38d0005&mark=1321-1321,1328-1328#1321

If XUL buttons are anything like HTML buttons, they've got nonzero padding by default. (At least, an unstyled HTML button shows that is has 6px of padding-left/padding-right, in devtools.)

So, this searchbar.css change probably does change behavior. Not sure yet what the best solution is, though. (Probably using either "border-box" or "content-box" and adjusting the width/height setting, I suppose...)

Comment 7

[Daniel Holbert [:dholbert]]

Looks like that "one-off-item" styling (with 'box-sizing:padding-box') was added in bug 1088660, by Florian:
http://hg.mozilla.org/mozilla-central/annotate/6bcf7110d616/browser/themes/linux/searchbar.css#l153

Florian, do you remember what the context of this element looks like, and do you have a suggestion as to what tweaks might be needed there, given that "box-sizing: padding-box" is going away?

Comment 8

[Florian Quèze [:florian]]

(In reply to David Baron [:dbaron] from comment #5)
> The searchbar.css changes are suspicious -- changing from padding-box to
> border-box on an element with a border is a substantive change.  Does it
> have padding?  (Can't tell from local examination of the CSS, but you could
> tell from, e.g., inspector, if you can get it to inspect chrome.)  If it
> doesn't have padding, you should be able to change to content-box.

The padding is set to 0 at:
http://hg.mozilla.org/mozilla-central/annotate/6bcf7110d616/browser/themes/linux/searchbar.css#l146

(In reply to Daniel Holbert [:dholbert] from comment #7)
> Looks like that "one-off-item" styling (with 'box-sizing:padding-box') was
> added in bug 1088660, by Florian:
> http://hg.mozilla.org/mozilla-central/annotate/6bcf7110d616/browser/themes/
> linux/searchbar.css#l153
> 
> Florian, do you remember what the context of this element looks like, and do
> you have a suggestion as to what tweaks might be needed there, given that
> "box-sizing: padding-box" is going away?

Please change this line to:
   box-sizing: content-box;

Removing the line or changing it to border-box causes some pixels of empty space at the bottom of the list of one-off engines in the search panel.

Comment 9

[Kyle Zentner]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fe43a6fcee9

Comment 10

[Kyle Zentner]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=208898e5231e

Comment 11

[Kyle Zentner]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ed345c379e8

Comment 12

[Kyle Zentner]

Attachment: RemoveBoxSizingPaddingBox

Unless there are failures in the test run above, I think that this patch is good to go. I looked carefully at the about private browsing page, so that change should be fine. I've also simplified the logic in various places due to this change. Let me know if it makes *less* sense now.

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8609071 [details] [diff] [review]
RemoveBoxSizingPaddingBox

Preliminary review nit:

>diff --git a/layout/reftests/css-blending/background-blending-background-clip-padding-box.html b/layout/reftests/css-blending/background-blending-background-clip-padding-box.html
>--- a/layout/reftests/css-blending/background-blending-background-clip-padding-box.html
>+++ /dev/null
>@@ -1,22 +0,0 @@
>-<!-- Blend a background image and a background color specifying background-clip: padding-box  -->
[...]
>-  background-clip: padding-box;

Looks like this file (and its reference case) are being removed, but they shouldn't be. ("background-clip: padding-box" still exists, independent of the "box-sizing: padding-box" removal.)

It looks like only reftest failure on the Try run is this testcase/reference case failing to run (because they still exist in the manifest file, but the files were removed in this patch).  I expect when you revert this file-removal, you'll have green reftest results.

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8609071 [details] [diff] [review]
RemoveBoxSizingPaddingBox

Haven't looked at the reftests yet, but here are my review notes on the code changes:

>+++ b/layout/base/nsLayoutUtils.cpp
>@@ -4798,22 +4780,18 @@ nsLayoutUtils::ComputeSizeWithIntrinsicD
>+  if (stylePos->mBoxSizing == NS_STYLE_BOX_SIZING_BORDER) {
>+      boxSizingAdjust += aBorder + aPadding;
>   }

De-indent "boxSizingAdjust" by 2 spaces.  (Most code in layout, including this file, uses 2-space indentation, not 4-space).

>+++ b/layout/generic/nsFrame.cpp
>+  if (stylePos->mBoxSizing == NS_STYLE_BOX_SIZING_BORDER) {
>+      boxSizingAdjust += aBorder + aPadding;
>   }

De-indent by 2 spaces.

(Also, observation: I think a lot of functions like this pass border & padding separately *so that we can support* box-sizing: padding-box vs. border-box.  Now that border/padding will always be added/subtracted as a package, we might want to consider combining these args into aBorderPadding in a lot of spots, so that we don't need to perform "aBorder + aPadding" all over the place.)

>+++ b/layout/generic/nsHTMLReflowState.cpp
> nsCSSOffsetState::ComputeWidthValue(nscoord aContainingBlockWidth,
>                                     uint8_t aBoxSizing,
>                                     const nsStyleCoord& aCoord)
> {
>   nscoord inside = 0, outside = ComputedPhysicalBorderPadding().LeftRight() +
>                                 ComputedPhysicalMargin().LeftRight();
>-  switch (aBoxSizing) {
>-    case NS_STYLE_BOX_SIZING_BORDER:
>+  if (aBoxSizing == NS_STYLE_BOX_SIZING_BORDER) {
>       inside = ComputedPhysicalBorderPadding().LeftRight();

De-indent by 2 spaces.

> nscoord
> nsCSSOffsetState::ComputeHeightValue(nscoord aContainingBlockHeight,
>                                      uint8_t aBoxSizing,
>                                      const nsStyleCoord& aCoord)
> {
>   nscoord inside = 0;
>-  switch (aBoxSizing) {
>-    case NS_STYLE_BOX_SIZING_BORDER:
>+  if (aBoxSizing == NS_STYLE_BOX_SIZING_BORDER) {
>       inside = ComputedPhysicalBorderPadding().TopBottom();

Here too.

>@@ -1091,22 +1081,18 @@ nsHTMLReflowState::CalculateHorizBorderP
>   nscoord outside =
>     padding.LeftRight() + border.LeftRight() + margin.LeftRight();
>   nscoord inside = 0;
>+  if (mStylePosition->mBoxSizing == NS_STYLE_BOX_SIZING_BORDER) {
>+    inside += border.LeftRight() + padding.LeftRight();
>   }
>   outside -= inside;

This "outside/inside" calculation feels a bit churny, now that we've only got 2 cases to consider. In particular -- the common case (content-box) feels a bit silly here; we end up performing this calculation, effectively:
  outside = padding + border + margin;
  outside -= border - padding
...which is a bit silly.

I suspect this should be simplified (arithmetic-wise at least) to something like:
 if (mStylePosition->mBoxSizing == NS_STYLE_BOX_SIZING_BORDER) {
   inside += border.LeftRight() + padding.LeftRight();
 } else {
   outside += border.LeftRight() + padding.LeftRight();
 }
 outside += margin.LeftRight();

Could you make that change, perhaps in a followup patch so that the main patch stays mostly-mechanical?

>+++ b/layout/style/nsComputedDOMStyle.cpp
> nsMargin
> nsComputedDOMStyle::GetAdjustedValuesForBoxSizing()
> {
[...]
>-  switch(stylePos->mBoxSizing) {
>-    case NS_STYLE_BOX_SIZING_BORDER:
>+  if (stylePos->mBoxSizing == NS_STYLE_BOX_SIZING_BORDER) {
>       adjustment += mInnerFrame->GetUsedBorder();
[...]
>       adjustment += mInnerFrame->GetUsedPadding();

De-indent. Also, collapse these 2 lines to one, w/ the existing convenience function:
  mInnerFrame->GetUsedBorderAndPadding();

>+++ b/layout/tables/BasicTableLayoutStrategy.cpp
>-        if (isQuirks) {
>+        if (isQuirks || stylePos->mBoxSizing == NS_STYLE_BOX_SIZING_CONTENT) {
>             boxSizingToBorderEdge = offsets.hPadding + offsets.hBorder;
>         }
>         else {
[...]
>+            // NS_STYLE_BOX_SIZING_BORDER
>+            minCoord += offsets.hPadding + offsets.hBorder;
>+            prefCoord += offsets.hPadding + offsets.hBorder;

Add "(and standards-mode)" to end of comment here.

>+++ b/layout/tables/nsTableRowFrame.cpp
>@@ -624,26 +624,18 @@ nsTableRowFrame::CalculateCellActualHeig
>       if (PresContext()->CompatibilityMode() != eCompatibility_NavQuirks) {
>-        switch (position->mBoxSizing) {
>-          case NS_STYLE_BOX_SIZING_CONTENT:
>+        if (position->mBoxSizing == NS_STYLE_BOX_SIZING_CONTENT) {
>             outsideBoxSizing = aCellFrame->GetUsedBorderAndPadding().TopBottom();

de-indent "outsideBoxSizing". (contextual code is 2-space indented)

>+++ b/layout/xul/nsResizerFrame.cpp
>-          switch (frameToResize->StylePosition()->mBoxSizing) {
>-            case NS_STYLE_BOX_SIZING_CONTENT:
>+          if (frameToResize->StylePosition()->mBoxSizing == NS_STYLE_BOX_SIZING_CONTENT) {
>               rect.Deflate(frameToResize->GetUsedPadding());
>-            case NS_STYLE_BOX_SIZING_PADDING:
>-              rect.Deflate(frameToResize->GetUsedBorder());
>-            default:
>-              break;
>           }

De-indent the "Deflate" line here.

Also, it looks like the old code had "box-sizing:content" fall through here -- so I think you really want to keep the second Deflate call. (And then they can be combined using the convenience method mentioned above -- so this should end up being "rect.Deflate(frameToResize->GetUsedBorderAndPadding())"

Comment 15

[Kyle Zentner]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2ac4181f47e

Comment 16

[Kyle Zentner]

Attachment: RemoveBoxSizingPaddingBox

This version is what was tested in the most recent try run. I think I made all of the changes you suggested.

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 8610661 [details] [diff] [review]
RemoveBoxSizingPaddingBox

>+++ b/layout/tables/BasicTableLayoutStrategy.cpp
>@@ -108,35 +108,23 @@ GetWidthInfo(nsRenderingContext *aRender
>-        if (isQuirks) {
>-            boxSizingToBorderEdge = offsets.hPadding + offsets.hBorder;
>+        if (isQuirks || stylePos->mBoxSizing == NS_STYLE_BOX_SIZING_CONTENT) {
>+          boxSizingToBorderEdge = offsets.hPadding + offsets.hBorder;
>         }
>         else {
>-            switch (stylePos->mBoxSizing) {
[...]
>-            }
>+          // NS_STYLE_BOX_SIZING_BORDER and standards-mode
>+          minCoord += offsets.hPadding + offsets.hBorder;
>+          prefCoord += offsets.hPadding + offsets.hBorder;
>         }

The contextual code here (in BasicTableLayoutStrategy) actually does use 4-space indentation, as shown by removed contextual code. :)

So, please match that & restore the indentation you had before for the new lines here. (add 2 spaces before boxSizingToBorderEdge, and the comment / minCoord / prefCoord.)

>+++ b/layout/xul/nsResizerFrame.cpp
>+          if (frameToResize->StylePosition()->mBoxSizing == NS_STYLE_BOX_SIZING_CONTENT) {
>+            rect.Deflate(frameToResize->GetUsedPadding());
>+            rect.Deflate(frameToResize->GetUsedBorder());

Per final sentence of comment 14, these should collapse to use GetUsedBorderAndPadding().

r=me with that. (I'd still like a followup / "part 2" patch to address the churny outside/inside calculation mentioned in comment 14, too.)

Thanks!

Comment 18

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #14)
> (Also, observation: I think a lot of functions like this pass border &
> padding separately *so that we can support* box-sizing: padding-box vs.
> border-box.  Now that border/padding will always be added/subtracted as a
> package, we might want to consider combining these args into aBorderPadding
> in a lot of spots, so that we don't need to perform "aBorder + aPadding" all
> over the place.)

(I filed bug 1168478 on this, FWIW.)

Comment 19

[Kyle Zentner]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=31b5ac7a6f8a

Comment 20

[Kyle Zentner]

Attachment: RemoveBoxSizingPaddingBox w/ feedback addressed

I fixed the above, and the try run looks good. More extensive cleanup will be done in a later patch on the same bug, so this patch is ready for check in.

Comment 21

[Kyle Zentner]

I'll be submitting a second patch to clean up some of the logic, but this patch is ready now.

Comment 22

[Carsten Book [:Tomcat]]

Hi, this patch seems not to apply cleanly:

applying RemoveBoxSizingPaddingBox
patching file toolkit/themes/shared/in-content/info-pages.inc.css
Hunk #1 FAILED at 0
Hunk #2 FAILED at 80
2 out of 2 hunks FAILED -- saving rejects to file toolkit/themes/shared/in-content/info-pages.inc.css.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh RemoveBoxSizingPaddingBox

can you take a look ?

Comment 23

[Kyle Zentner]

Attachment: Updated RemoveBoxSizingPaddingBox w/ feedback addressed

Sorry about that. This version should apply cleanly. Only one of the context lines in the patch needed to be updated.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8611290 [details] [diff] [review]
Updated RemoveBoxSizingPaddingBox w/ feedback addressed

In the future, the checkin-needed keyword is the preferred way of making this request. Plays more nicely with the various bug marking tools.

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd96df1da2b5

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8611290 [details] [diff] [review]
Updated RemoveBoxSizingPaddingBox w/ feedback addressed

Backed out for making test_bug320799.html permafail on Mulet.
https://treeherder.mozilla.org/logviewer.html#?job_id=10224750&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/703ace2e0414

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Failed on Android too.

Comment 28

[Kyle Zentner]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b688ec2d3f8

Comment 29

[Kyle Zentner]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a82c8a3eff90

Comment 30

[Kyle Zentner]

Given the result of the last test run, I think I've fixed everything.

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/541cd29ea626

Comment 32

[Kyle Zentner]

Attachment: RemoveBoxSizingPaddingBox

Sorry, I forgot to upload the new patch. That one will need to be backed out now. This patch is the one that passed the tests above.

Comment 33

[Daniel Holbert [:dholbert]]

(I can backout & reland in a few minutes; I've got something else to land anyway.)

Comment 34

[Daniel Holbert [:dholbert]]

(Er, never mind -- I believe RyanVM is already backing out some other stuff, so unless I hear otherwise from him I'll let him do the backout/reland here as part of that push. Thanks, Ryan!)

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/236d4caad091 (backout)
https://hg.mozilla.org/integration/mozilla-inbound/rev/4126c66c9a80

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Backed out for Gaia sound_manager_test.js failures. Unfortunately, they were present in the last Try run too and we missed them :(
https://treeherder.mozilla.org/logviewer.html#?job_id=10272944&repo=mozilla-inbound

Comment 37

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a62605b31838

Comment 38

[Kohei Yoshino]

What's the plan for background-clip: padding-box?
https://developer.mozilla.org/en-US/docs/Web/CSS/background-clip

Comment 39

[Daniel Holbert [:dholbert]]

That's not changing, as far as I'm aware.

Comment 40

[Daniel Holbert [:dholbert]]

I briefly worried that we might need to remove usages of this feature from Thunderbird/Seamonkey, but it looks like we're OK on that front -- they don't use "box-sizing:padding-box".  At least, this MXR search...
  http://mxr.mozilla.org/comm-central/search?string=box-sizing
...turns up no hits for "padding-box".

Comment 41

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/box-sizing-padding-box-will-be-removed/

Comment 42

[Daniel Holbert [:dholbert]]

Firefox OS is moving to Tier 3 support[1], which means we don't have to gate this bug's change on fixing gaia content & tests anymore (which was filed as bug 1169839).

(The gaia content & tests might already be mostly-fixed -- not sure how many of bug 1169839's pull requests were merged in.  But regardless, they don't need to hold back this platform change.)

Kyle, not sure how much time you've got for Mozilla stuff at this point -- but would you be up for getting this bug's patch unbitrotted & ready to be landable? Note that a large chunk of the patch here (~36 KB of this ~53 KB patch) already landed in bug 1169837.

If you don't have time, no worries -- I can also try to unbitrot this & shepherd it in.  I'd like to have this happen on the sooner side, since it looks like we're haven't been running tests for this feature (box-sizing:padding-box) for some time now, as of bug 1169837.

[1] https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.platform/gF-kiJV21ro

Comment 43

[Daniel Holbert [:dholbert]]

Attachment: Bug 1166728 - Remove support for "box-sizing: padding-box", per CSS WG resolution.

Review commit: https://reviewboard.mozilla.org/r/59392/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59392/

Comment 44

[Daniel Holbert [:dholbert]]

Comment on attachment 8763059 [details]
Bug 1166728 - Remove support for "box-sizing: padding-box", per CSS WG resolution.

I've unbitrotted the latest patch here. Carrying forward r=me, but I should probably have a second pair of eyes look at this, to sanity-check my unbitrotting.  Tagging mats!

Fortunately, the final patch is significantly smaller than the previous patch here, since the majority of the previous patch (fixing *usages* of box-sizing:padding-box in tests/frontend code) was landed independently in bug 1169837.  So, this patch just has the unbitrotted c++ code-changes from the most recent patch here, as well as the insertion of "padding-box" into the list of invalid_values in property_database.js, to test that support has really been removed.

This leaves us with only 3 usages of "box-sizing.*padding-box" in the tree, all of which are in tests that were added since bug 1166728 landed. I'll post a trivial followup patch to address those, separate from the main patch here.

Comment 45

[Daniel Holbert [:dholbert]]

(The only non-obvious change in this patch, IMO, is in BasicTableLayoutStrategy.cpp.  That chunk has us doing stuff for both box-sizing:content-box as well as box-sizing:border-box, and it turns out one of those clauses exactly matches another clause directly above, so the patch just merges those clauses to share code.

I'm also happy to undo that merging, though, if you prefer.)

Comment 46

[Mats Palmgren (inactive)]

Comment on attachment 8763059 [details]
Bug 1166728 - Remove support for "box-sizing: padding-box", per CSS WG resolution.

https://reviewboard.mozilla.org/r/59392/#review56644

::: layout/base/nsLayoutUtils.cpp:5452
(Diff revision 1)
>    const bool isAutoISize = inlineStyleCoord->GetUnit() == eStyleUnit_Auto;
>    bool isAutoBSize = IsAutoBSize(*blockStyleCoord, aCBSize.BSize(aWM));
>  
>    LogicalSize boxSizingAdjust(aWM);
> -  switch (stylePos->mBoxSizing) {
> -    case StyleBoxSizing::Border:
> +  if (stylePos->mBoxSizing == StyleBoxSizing::Border) {
> +    boxSizingAdjust += aBorder + aPadding;

nit: just a plain assignment might be clearer now

::: layout/generic/nsFrame.cpp:4657
(Diff revision 1)
>                                         aFlags & ComputeSizeFlags::eShrinkWrap);
>    LogicalSize boxSizingAdjust(aWM);
>    const nsStylePosition *stylePos = StylePosition();
>  
> -  switch (stylePos->mBoxSizing) {
> -    case StyleBoxSizing::Border:
> +  if (stylePos->mBoxSizing == StyleBoxSizing::Border) {
> +    boxSizingAdjust += aBorder + aPadding;

nit: same here, if you move the boxSizingAdjust declaration just before the if-statement.

::: layout/generic/nsHTMLReflowState.cpp:1169
(Diff revision 1)
>    }
>  
>    nscoord outside = paddingStartEnd + borderStartEnd + marginStartEnd;
>    nscoord inside = 0;
> -  switch (mStylePosition->mBoxSizing) {
> -    case StyleBoxSizing::Border:
> +  if (mStylePosition->mBoxSizing == StyleBoxSizing::Border) {
> +    inside += borderStartEnd + paddingStartEnd;

nit: I'd use a plain assignment here too

::: layout/style/nsComputedDOMStyle.cpp:546
(Diff revision 1)
>    // which can be different depending on the value of the 'box-sizing' property.
>    const nsStylePosition* stylePos = StylePosition();
>  
>    nsMargin adjustment;
> -  switch(stylePos->mBoxSizing) {
> -    case StyleBoxSizing::Border:
> +  if (stylePos->mBoxSizing == StyleBoxSizing::Border) {
> +    adjustment += mInnerFrame->GetUsedBorder() + mInnerFrame->GetUsedPadding();

... and here.  Also, I'd prefer mInnerFrame->GetUsedBorderAndPadding()

::: layout/tables/FixedTableLayoutStrategy.cpp:261
(Diff revision 1)
>            float pct = styleISize->GetPercentValue();
>            colISize = NSToCoordFloor(pct * float(tableISize));
>  
>            nscoord boxSizingAdjust = 0;
> -          switch (cellFrame->StylePosition()->mBoxSizing) {
> -            case StyleBoxSizing::Content:
> +          if (cellFrame->StylePosition()->mBoxSizing == StyleBoxSizing::Content) {
> +            boxSizingAdjust += offsets.hPadding + offsets.hBorder;

nit: in this case I think we should just increment 'colISize' directly
and remove the 'boxSizingAdjust' variable.  Also, the 'offsets' variable
can be moved inside the if-statement.

::: layout/tables/nsTableRowFrame.cpp:671
(Diff revision 1)
>        if (PresContext()->CompatibilityMode() != eCompatibility_NavQuirks) {
> -        switch (position->mBoxSizing) {
> +        if (position->mBoxSizing == StyleBoxSizing::Content) {

nit: I'd prefer one if-statement using &&.
Also, I think 'outsideBoxSizing' is unnecessary; just assign 'specifiedBSize' before the 'if' and then increment it inside.

Comment 47

[Daniel Holbert [:dholbert]]

Comment on attachment 8763059 [details]
Bug 1166728 - Remove support for "box-sizing: padding-box", per CSS WG resolution.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59392/diff/1-2/

Comment 48

[Daniel Holbert [:dholbert]]

All good points, thanks!  I addressed all of that feedback.

I made one additional change -- I swapped out the cellFrame->StylePosition() call in FixedTableLayoutStrategy.cpp, to share a local variable with another cellFrame->StylePosition() call a bit further up.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cb483e19aa4

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea578e2813f4
Remove support for "box-sizing: padding-box", per CSS WG resolution. r=dholbert r=mats

Comment 50

[Carsten Book [:Tomcat]]

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

Comment 51

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/box-sizing-padding-box-has-been-removed/

Comment 52

[Jean-Yves Perrier [:teoli]]

Updated compat tables in:
https://developer.mozilla.org/en-US/docs/Web/CSS/box-sizing
and
https://developer.mozilla.org/en-US/Firefox/Releases/50#CSS