Closed Bug 311415 Opened 19 years ago Closed 17 years ago

implement -moz-intrinsic, -moz-min-intrinsic, -moz-shrink-wrap, -moz-fill for width, min-width, and max-width

Categories

(Core :: Layout: Block and Inline, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: ian, Assigned: dbaron)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [reflow-refactor])

Attachments

(2 files, 2 obsolete files)

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.
Or '-moz-shrink-wrap' or whatever -- ref:
   http://www.w3.org/Style/Group/css3-src/css3-box/#the-shrink-wrap
Whiteboard: [reflow-refactor]
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)
Summary: width: -moz-intrinsic (and max-width: -moz-intrinsic, etc) → implement -moz-intrinsic, -moz-min-intrinsic, -moz-shrink-wrap, -moz-container for width, min-width, and max-width
Note that the patch in bug 163504 is working around the lack of -moz-container (not a great name, though, maybe -moz-fill?).
Summary: implement -moz-intrinsic, -moz-min-intrinsic, -moz-shrink-wrap, -moz-container for width, min-width, and max-width → implement -moz-intrinsic, -moz-min-intrinsic, -moz-shrink-wrap, -moz-fill for width, min-width, and max-width
Attached patch work in progress (obsolete) — Splinter Review
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...
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
I need to make sure to ignore box-sizing for these values.
Attached patch patch (obsolete) — Splinter Review
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/.
Attachment #263144 - Attachment is obsolete: true
Attached patch patchSplinter Review
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).
Attachment #263565 - Attachment is obsolete: true
Attachment #263605 - Flags: superreview?(bzbarsky)
Attachment #263605 - Flags: review?(bzbarsky)
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.
Attachment #263605 - Flags: superreview?(bzbarsky)
Attachment #263605 - Flags: superreview+
Attachment #263605 - Flags: review?(bzbarsky)
Attachment #263605 - Flags: review+
(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.
> I think I'll make it not return negative numbers.

Sounds code.  Then the replaced-element-sizing code can probably nix some checks.
(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.
Checked in to trunk.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
> I just fixed it instead.

Remove the XXX comment as well?  And shouldn't _Chars count as a fixed width?
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.
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.
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?
Attached patch Like soSplinter Review
Attachment #264297 - Flags: review?
Attachment #264297 - Flags: superreview?(dbaron)
Attachment #264297 - Flags: review?(dbaron)
Attachment #264297 - Flags: review?
Comment on attachment 264297 [details] [diff] [review]
Like so

Oops.  r+sr=dbaron
Attachment #264297 - Flags: superreview?(dbaron)
Attachment #264297 - Flags: superreview+
Attachment #264297 - Flags: review?(dbaron)
Attachment #264297 - Flags: review+
Checked in that change.
Depends on: 395137
Note that the names of these values were changed in bug 402706.
Ah, setting to dev-doc-complete, as this was documented years ago in bug 402706.
Depends on: 1482628
No longer depends on: 1482628
You need to log in before you can comment on or make changes to this bug.