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

RESOLVED FIXED

Status

()

Core
Layout: Block and Inline
RESOLVED FIXED
12 years ago
7 months ago

People

(Reporter: Hixie (not reading bugmail), Assigned: dbaron)

Tracking

(Blocks: 1 bug)

Trunk
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [reflow-refactor])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
Or '-moz-shrink-wrap' or whatever -- ref:
   http://www.w3.org/Style/Group/css3-src/css3-box/#the-shrink-wrap
(Assignee)

Updated

12 years ago
Depends on: 300030
Whiteboard: [reflow-refactor]
(Assignee)

Comment 2

11 years ago
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
(Assignee)

Comment 3

11 years ago
Note that the patch in bug 163504 is working around the lack of -moz-container (not a great name, though, maybe -moz-fill?).
(Assignee)

Updated

10 years ago
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
(Assignee)

Comment 4

10 years ago
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...
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
(Assignee)

Comment 5

10 years ago
I need to make sure to ignore box-sizing for these values.
(Assignee)

Comment 6

10 years ago
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/.
Attachment #263144 - Attachment is obsolete: true
(Assignee)

Comment 7

10 years ago
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).
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+
(Assignee)

Comment 9

10 years ago
(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.
(Assignee)

Comment 11

10 years ago
(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.
(Assignee)

Comment 12

10 years ago
Checked in to trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/layout/reftests/box-properties&command=DIFF_FRAMESET&file=reftest.list&rev1=1.1&rev2=1.2&root=/cvsroot
Flags: in-testsuite+
> I just fixed it instead.

Remove the XXX comment as well?  And shouldn't _Chars count as a fixed width?

Comment 15

10 years ago
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

10 years ago
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?
Created attachment 264297 [details] [diff] [review]
Like so
Attachment #264297 - Flags: review?
Attachment #264297 - Flags: superreview?(dbaron)
Attachment #264297 - Flags: review?(dbaron)
Attachment #264297 - Flags: review?
(Assignee)

Comment 19

10 years ago
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.
(Assignee)

Comment 21

10 years ago
Specification posted at http://dbaron.org/css/intrinsic/#new-width-values

Updated

10 years ago
Depends on: 395137
(Assignee)

Updated

10 years ago
Depends on: 402706
(Assignee)

Comment 22

10 years ago
Note that the names of these values were changed in bug 402706.
(Assignee)

Updated

7 months ago
Blocks: 1312587
You need to log in before you can comment on or make changes to this bug.