Closed Bug 1166728 Opened 9 years ago Closed 8 years ago

remove box-sizing: padding-box

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox41 --- affected
firefox50 --- fixed

People

(Reporter: dbaron, Assigned: zentner.kyle)

References

Details

(Keywords: addon-compat, dev-doc-complete, site-compat)

Attachments

(2 files, 4 obsolete files)

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.
http://caniuse.com/#search=box-sizing currently shows that we're literally the only browser to support "padding-box".
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 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...)
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?
Flags: needinfo?(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.
Flags: needinfo?(florian)
Attached patch RemoveBoxSizingPaddingBox (obsolete) — Splinter Review
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.
Assignee: nobody → kzentner
Status: NEW → ASSIGNED
Attachment #8609071 - Flags: review?(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 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())"
Attached patch RemoveBoxSizingPaddingBox (obsolete) — Splinter Review
This version is what was tested in the most recent try run. I think I made all of the changes you suggested.
Attachment #8609071 - Attachment is obsolete: true
Attachment #8609071 - Flags: review?(dholbert)
Attachment #8610661 - Flags: review?(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!
Attachment #8610661 - Flags: review?(dholbert) → review+
(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.)
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.
Attachment #8610661 - Attachment is obsolete: true
Attachment #8610857 - Flags: review+
Attachment #8610857 - Flags: checkin?
I'll be submitting a second patch to clean up some of the logic, but this patch is ready now.
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 ?
Flags: needinfo?(kzentner)
Keywords: checkin-needed
Attachment #8610857 - Flags: checkin?
Sorry about that. This version should apply cleanly. Only one of the context lines in the patch needed to be updated.
Attachment #8610857 - Attachment is obsolete: true
Flags: needinfo?(kzentner)
Attachment #8611290 - Flags: review+
Attachment #8611290 - Flags: checkin?
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.
Attachment #8611290 - Flags: checkin? → checkin+
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
Attachment #8611290 - Flags: checkin+ → checkin-
Failed on Android too.
Given the result of the last test run, I think I've fixed everything.
Keywords: checkin-needed
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.
Attachment #8611290 - Attachment is obsolete: true
Attachment #8612937 - Flags: review+
Attachment #8612937 - Flags: checkin?
(I can backout & reland in a few minutes; I've got something else to land anyway.)
(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!)
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
Attachment #8612937 - Flags: checkin?
Depends on: 1169839
Depends on: 1169837
Keywords: site-compat
What's the plan for background-clip: padding-box?
https://developer.mozilla.org/en-US/docs/Web/CSS/background-clip
Keywords: addon-compat
That's not changing, as far as I'm aware.
See Also: → 1178168
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".
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
Flags: needinfo?(zentner.kyle)
Flags: needinfo?(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.
Flags: needinfo?(zentner.kyle)
Flags: needinfo?(dholbert)
Attachment #8763059 - Flags: review?(dholbert) → review+
(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.)
Depends on: 1280422
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.
Attachment #8763059 - Flags: review?(mats) → review+
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/
Attachment #8763059 - Flags: review+ → review?(dholbert)
Attachment #8763059 - Flags: review?(dholbert) → review+
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
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea578e2813f4
Remove support for "box-sizing: padding-box", per CSS WG resolution. r=dholbert r=mats
https://hg.mozilla.org/mozilla-central/rev/ea578e2813f4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: