Last Comment Bug 311415 - implement -moz-intrinsic, -moz-min-intrinsic, -moz-shrink-wrap, -moz-fill for width, min-width, and max-width
: implement -moz-intrinsic, -moz-min-intrinsic, -moz-shrink-wrap, -moz-fill for...
Status: RESOLVED FIXED
[reflow-refactor]
:
Product: Core
Classification: Components
Component: Layout: Block and Inline (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: ---
Assigned To: David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch)
:
Mentors:
Depends on: reflow-refactor 395137 402706
Blocks:
  Show dependency treegraph
 
Reported: 2005-10-06 15:00 PDT by Hixie (not reading bugmail)
Modified: 2007-12-06 14:42 PST (History)
14 users (show)
jwalden+bmo: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
work in progress (24.60 KB, patch)
2007-04-28 10:27 PDT, David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch)
no flags Details | Diff | Splinter Review
patch (113.16 KB, patch)
2007-05-03 00:19 PDT, David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch)
no flags Details | Diff | Splinter Review
patch (113.44 KB, patch)
2007-05-03 08:37 PDT, David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
Like so (2.09 KB, patch)
2007-05-09 17:16 PDT, Boris Zbarsky [:bz]
dbaron: review+
dbaron: superreview+
Details | Diff | Splinter Review

Description Hixie (not reading bugmail) 2005-10-06 15:00:18 PDT
Mozilla should implement the 'intrinsic' keyword for the 'width' property (and
'max-width' and 'min-width') to see how useful it would be to have in CSS3.
Comment 1 Hixie (not reading bugmail) 2005-10-06 15:01:49 PDT
Or '-moz-shrink-wrap' or whatever -- ref:
   http://www.w3.org/Style/Group/css3-src/css3-box/#the-shrink-wrap
Comment 2 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2007-01-25 12:46:41 PST
I think we should implement:
 -moz-intrinsic (pref width)
 -moz-min-intrinsic (min width)
 -moz-shrink-wrap (what auto does for inline-blocks, floats, abs-pos)
 -moz-container (what auto does for blocks)

See also:
http://lists.w3.org/Archives/Public/www-style/2003Mar/0109
(I can't find where I proposed intrinsic and min-intrinsic)
Comment 3 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2007-01-25 12:48:04 PST
Note that the patch in bug 163504 is working around the lack of -moz-container (not a great name, though, maybe -moz-fill?).
Comment 4 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2007-04-28 10:27:40 PDT
Created attachment 263144 [details] [diff] [review]
work in progress

This has the style system half done.  I want to do this, and add some thorough tests, before I do bug 364066, so that I don't accidentally write in some infinite loops...
Comment 5 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2007-04-30 11:24:26 PDT
I need to make sure to ignore box-sizing for these values.
Comment 6 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2007-05-03 00:19:30 PDT
Created attachment 263565 [details] [diff] [review]
patch

This should be complete, but I still need to look into some reftest failures; most of them seem like problems showing up only on my system rather than things caused by the patch.

It does pass all the mochitests in layout/style/test/.
Comment 7 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2007-05-03 08:37:35 PDT
Created attachment 263605 [details] [diff] [review]
patch

I tried to get hg to give me a diff -b, but it didn't want to.

The reftest failures I was seeing yesterday happen without the patch, so it's something else in my tree or something wrong with my system.  (I'm also seeing failures on two of the new tests, since I'm seeing failures for all tests with scaled images in them (rather than just the ones where we expect scaled images to match nonscaled ones).  But since the tinderboxes don't see that I didn't mark them failing.)

This patch is mostly pretty straightforward.  Some of the behavior about how these values should behave for intrinsic widths and for table cells/columns requires a little thought, and I probably should document that behavior at some point.  But for now it's documented in the code and the tests.  Note that this patch has a lot of reftests in it (as well as additions to property_database.js for the style system mochitests).
Comment 8 Boris Zbarsky [:bz] 2007-05-03 12:49:53 PDT
Comment on attachment 263605 [details] [diff] [review]
patch

>+++ b/layout/base/nsLayoutUtils.cpp
>+GetIntrinsicCoord(const nsStyleCoord& aStyle,
>+  if (val == NS_STYLE_WIDTH_INTRINSIC)
>+    aResult = aFrame->GetPrefWidth(aRenderingContext);
>+  else
>+    aResult = aFrame->GetMinWidth(aRenderingContext);

Assert right before that that val is NS_STYLE_WIDTH_INTRINSIC or NS_STYLE_WIDTH_MIN_INTRINSIC?  I know it's pretty clear from the current code, but still.

>+++ b/layout/base/nsLayoutUtils.h
>+   * on aContentEdgeToBoxSiing and aBoxSizingToMarginEdge 

Missing "z" in "Sizing".

It might also be worth clearly documenting what aContentEdgeToBoxSizing and aBoxSizingToMarginEdge mean.

Document that this function can return a negative number and callers should handle that?

>+++ b/layout/generic/nsAbsoluteContainingBlock.cpp

>+    // XXX All of the enumerated values except -moz-fill are ok too.

Followup bug?  And cite in comment?

>+++ b/layout/generic/nsFrame.cpp

I wonder whether we could somehow refactor the min/max/pref width code between nsFrame::ComputeSize and nsLayoutUtils::ComputeSizeWithIntrinsicDimensions... not sure how.

>+++ b/layout/generic/nsHTMLReflowState.cpp
>+nsHTMLReflowState::CalculateHorizBorderPaddingMargin(
>+                       nscoord aContainingBlockWidth,
>+                       nscoord* aInsideBoxSizing,
>+                       nscoord* aOutsideBoxSizing)

Document the new args in the header?

>@@ -1611,8 +1641,20 @@ nsHTMLReflowState::InitConstraints(nsPre

>+        nscoord inside = 0, outside = mComputedBorderPadding.LeftRight() +
>+                                      mComputedMargin.LeftRight();
>+        switch (mStylePosition->mBoxSizing) {

Could you refactor this code?  We have it in at least two places now...  It might make sense to have a struct for the inside and outside pair instead of passing them around separately all over, but either way.

>+++ b/layout/generic/nsHTMLReflowState.h

>+  // XXX Make aResult a return value

File a followup?  And cite in comment?

>+  inline nscoord ComputeWidthValue(nscoord aContainingBlockWidth,

Document that this doesn't work for auto widths?

>+++ b/layout/generic/nsSpacerFrame.cpp
>+  // XXX Should use containing block size!

Followup bug?  And cite in comment?

>+  // XXX This code doesn't handle some value types for width and height,
...

Followup bug if desired... I can live either way.  ;)

>@@ -164,7 +170,7 @@ SpacerFrame::GetDesiredSize(nsHTMLReflow
>-      aMetrics.width = NSToCoordRound(factor * aPercentBase.height);
>+      aMetrics.height = NSToCoordRound(factor * aPercentBase.height);

Right then.... ;)

I basically skipped over the reftests; let me know if you do want me to review them.

>+++ b/layout/style/test/property_database.js

Could you add some comments explaining what the various fields are?  I _think_ I've figured it out, butjust for future reference.

>+++ b/layout/tables/BasicTableLayoutStrategy.cpp
>+    } else if (unit == eStyleUnit_Enumerated && aIsCell) {
>+        switch (aStylePos->mWidth.GetIntValue()) {

Add a default NS_NOTREACHED case?  debug-only if desired.

>+        if (!aIsCell || maxWidth.GetIntValue() == NS_STYLE_WIDTH_FILL)
>+            maxWidth.SetAutoValue();

Would Reset() make more sense than SetAutoValue()?  A max-width of "auto" isn't something we usually have, while just not having a max-width is represented by the null unit.

>+        if (!aIsCell || minWidth.GetIntValue() == NS_STYLE_WIDTH_FILL)
>+            minWidth.SetAutoValue();

And here, would setting to 0 make more sense?

>+++ b/layout/tables/FixedTableLayoutStrategy.cpp

>@@ -227,24 +242,30 @@ FixedTableLayoutStrategy::ComputeColumnW
>+                    colWidth = nsLayoutUtils::IntrinsicForContainer(
>+                                 aReflowState.rendContext,
>+                                 cellFrame, nsLayoutUtils::MIN_WIDTH);

Why MIN_WIDTH and not PREF_WIDTH?  If there's a reason, document?

>+++ b/layout/tables/nsTableFrame.cpp

File followup bugs as needed on the comments in here?

r+sr=bzbarsky with those nits picked.
Comment 9 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2007-05-03 14:36:37 PDT
(In reply to comment #8)
> Document that this function can return a negative number and callers should
> handle that?

I think I'll make it not return negative numbers.
Comment 10 Boris Zbarsky [:bz] 2007-05-03 14:51:44 PDT
> I think I'll make it not return negative numbers.

Sounds code.  Then the replaced-element-sizing code can probably nix some checks.
Comment 11 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2007-05-03 15:19:55 PDT
(In reply to comment #8)
> >+++ b/layout/generic/nsAbsoluteContainingBlock.cpp
> 
> >+    // XXX All of the enumerated values except -moz-fill are ok too.
> 
> Followup bug?  And cite in comment?

I just fixed it instead.

> >+++ b/layout/generic/nsFrame.cpp
> 
> I wonder whether we could somehow refactor the min/max/pref width code between
> nsFrame::ComputeSize and nsLayoutUtils::ComputeSizeWithIntrinsicDimensions...
> not sure how.

Bug 364066, and changes for scaling SVG, will make them a bit more different, perhaps.

> >+++ b/layout/generic/nsHTMLReflowState.cpp
> >+nsHTMLReflowState::CalculateHorizBorderPaddingMargin(
> >+                       nscoord aContainingBlockWidth,
> >+                       nscoord* aInsideBoxSizing,
> >+                       nscoord* aOutsideBoxSizing)
> 
> Document the new args in the header?

I'll do it in the C++ file, since the method is private.



> Could you refactor this code?  We have it in at least two places now...  It
> might make sense to have a struct for the inside and outside pair instead of
> passing them around separately all over, but either way.

I just added another variant of ComputeWidthValue that has the common chunk inside it (taking aBoxSizing instead of the inside/outside pair).

> >+  // XXX Make aResult a return value
> 
> File a followup?  And cite in comment?

Not worth a followup bug.

> >+++ b/layout/generic/nsSpacerFrame.cpp
> Followup bug?  And cite in comment?
> Followup bug if desired... I can live either way.  ;)

Filed bug 379654 for the whole mess.

> >@@ -227,24 +242,30 @@ FixedTableLayoutStrategy::ComputeColumnW
> >+                    colWidth = nsLayoutUtils::IntrinsicForContainer(
> >+                                 aReflowState.rendContext,
> >+                                 cellFrame, nsLayoutUtils::MIN_WIDTH);
> 
> Why MIN_WIDTH and not PREF_WIDTH?  If there's a reason, document?

                    // Note that the difference between MIN_WIDTH and
                    // PREF_WIDTH shouldn't matter for any of these
                    // values of styleWidth; use MIN_WIDTH for symmetry
                    // with GetMinWidth above, just in case there is a
                    // difference.

> >+++ b/layout/tables/nsTableFrame.cpp
> 
> File followup bugs as needed on the comments in here?

Fixed instead, by forbidding fixed-layout when width: -moz-intrinsic.
Comment 12 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2007-05-03 16:13:09 PDT
Checked in to trunk.
Comment 14 Boris Zbarsky [:bz] 2007-05-03 23:28:10 PDT
> I just fixed it instead.

Remove the XXX comment as well?  And shouldn't _Chars count as a fixed width?
Comment 15 Nickolay_Ponomarev 2007-05-04 00:20:05 PDT
I mentioned this bug on http://developer.mozilla.org/en/docs/CSS_Reference:Mozilla_Extensions, might be worth documenting them or linking to the proposed spec if there is any.
Comment 16 neil@parkwaycc.co.uk 2007-05-09 12:42:34 PDT
Opening Chatzilla triggers an assert in nsStyleCoord::GetIntValue called from IsFixedWidth called from nsAbsoluteContainingBlock::FrameDependsOnContainer when the style coord for the width in question is in fact eStyleUnit_Auto.
Comment 17 Boris Zbarsky [:bz] 2007-05-09 17:03:43 PDT
Oh, hmm...  Parentheses.

I just went over them, and WidthDependsOnContainer in nsLineLayout.cpp in the original patch has an overparenthesized last equality comparison, and the IsFixedWidth check in nsAbsoluteContainingBlock has:

(aCoord.GetUnit() == eStyleUnit_Enumerated &&
 aCoord.GetIntValue() == NS_STYLE_WIDTH_INTRINSIC ||
 aCoord.GetIntValue() == NS_STYLE_WIDTH_MIN_INTRINSIC);

we need parens around the latter two checks, right?
Comment 18 Boris Zbarsky [:bz] 2007-05-09 17:16:31 PDT
Created attachment 264297 [details] [diff] [review]
Like so
Comment 19 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2007-05-09 18:14:15 PDT
Comment on attachment 264297 [details] [diff] [review]
Like so

Oops.  r+sr=dbaron
Comment 20 Boris Zbarsky [:bz] 2007-05-09 19:54:58 PDT
Checked in that change.
Comment 21 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2007-05-11 08:20:48 PDT
Specification posted at http://dbaron.org/css/intrinsic/#new-width-values
Comment 22 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2007-12-06 14:42:26 PST
Note that the names of these values were changed in bug 402706.

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