I'm splitting this out of bug 363249, since bug 363249 has gotten quite long. This bug is about adding support for calc(), min(), and max() CSS expressions to properties for which percentage values depend on layout. On bug 363249 I created an infrastructure for doing this, and I even used that infrastructure in a limited way in bug 531344, but the main uses still need to be written, and that's what this bug is about. I plan to start with 'width' (which is likely the hardest property) and then do the rest from there. Patches for 'width' to come shortly...

We weren't really testing a lot of width code in the style system mochitests; this fixes that by adding a display:block prerequisite for width and fixing the fallout by: * being consistent about what the size of the container is * adding a little hack for -moz-available

Presently, there's no need for this function to be distinct, so make it inline-equivalent to another one.

We'll need more than one set of calc ops with nsStyleCoord input, so factor those parts into a common base class.

It's easier to maintain when it's in one place.

This fixes an earlier design error; we need to distinguish 50% and calc(50%) in computed style since there are cases where we need to treat calc() differently. In those cases, we should treat calc() expressions containing only a leaf just like other ones.

Note that really only patches 1 and 6 are specific to 'width' and will need to be repeated for additional properties.

Comment on attachment 464219 [details] [diff] [review] width patch 1: Add a 'display: block' prerequisite for 'width' property tests so that calc() tests actually test the code Should the text for the xfail_value thing still be "should not get initial ..." even if swapping?

Comment on attachment 464220 [details] [diff] [review] width patch 2: remove ComputeCoordPercentCalc r=bzbarsky

Comment on attachment 464221 [details] [diff] [review] width patch 3: common base class for calc ops with nsStyleCoord input r=bzbarsky

Comment on attachment 464222 [details] [diff] [review] width patch 4: consolidate code for determining when widths and heights depend on a container r=bzbarsky

Comment on attachment 464224 [details] [diff] [review] width patch 5: distingish between 50% and calc(50%) in computed style Seems ok, but I assume there's a reason that SpecifiedToComputedCalcOps couldn't handle this? Just a general issue with how ComputeCalc works?

Comment on attachment 464225 [details] [diff] [review] width patch 6: support width: calc() >+++ b/layout/generic/nsImageFrame.cpp The return x ? false : y; pattern this uses can just be: return !x && y; but either way. It's hard to read no matter what. :( >+++ b/layout/reftests/css-calc/width-block-1.html >+<p style="width: -moz-calc(30% + 20%)">max(30% + 20%)</p> >+<p style="width: -moz-calc(30% + max(20%, 1px))">max(30% + max(20%, 1px))</p> For these two the description string doesn't really match the style. Fix it? It looks like you should also fix DependsOnIntrinsicSize in nsSVGOuterSVGFrame to handle %-less calc, right? r=me with that.

(In reply to comment #12) > Comment on attachment 464224 [details] [diff] [review] > width patch 5: distingish between 50% and calc(50%) in computed style > > Seems ok, but I assume there's a reason that SpecifiedToComputedCalcOps > couldn't handle this? Just a general issue with how ComputeCalc works? Yeah, ComputeCalc just skips over calc() nodes, and I'd sort of rather leave it that way. This also only creates the extra node in some very simple cases, rather than all the time, as a ComputeCalc solution would. Or maybe it's just that this way was easier since I wouldn't have to add to all the other calc ops. (In reply to comment #13) > Comment on attachment 464225 [details] [diff] [review] > width patch 6: support width: calc() > > >+++ b/layout/generic/nsImageFrame.cpp > > The > > return x ? false : y; > > pattern this uses can just be: > > return !x && y; > > but either way. It's hard to read no matter what. :( I think in this case I find it slightly more comprehensible as is. But I added an extra set of parens in my changes to avoid doing a (correct) a || bb && c. > It looks like you should also fix DependsOnIntrinsicSize in nsSVGOuterSVGFrame > to handle %-less calc, right? I changed it to: return (width.GetUnit() != eStyleUnit_Coord && (!width.IsCalcUnit() || width.CalcHasPercent())) || (height.GetUnit() != eStyleUnit_Coord && (!height.IsCalcUnit() || height.CalcHasPercent())); It would be nice if you double-checked that.

> It would be nice if you double-checked that. That looks correct.

http://hg.mozilla.org/mozilla-central/rev/17316f38926e http://hg.mozilla.org/mozilla-central/rev/40d85fe94a90 http://hg.mozilla.org/mozilla-central/rev/cf5092534a44 http://hg.mozilla.org/mozilla-central/rev/b50d19ed3449 http://hg.mozilla.org/mozilla-central/rev/bffe7ef73b3a http://hg.mozilla.org/mozilla-central/rev/d6326ce2ea4c

Note that this also fixes a regression from the 'width' patch where I changed floor to round.

Also note that height patch 3 fixes a clamping error in width patch 6, in GetAbsoluteCoord.

I noticed this while working on the offsets patch.

Comment on attachment 466759 [details] [diff] [review] height patch 1: fix prerequisites, as in width patch 1 r=bzbarsky

Comment on attachment 466760 [details] [diff] [review] min/max-width patch 1: fix prerequisites r=bzbarsky

Comment on attachment 466761 [details] [diff] [review] offsets patch 1: fix prerequisites r=me, but file those bugs and get the bug numbers in the comments?

Comment on attachment 466762 [details] [diff] [review] offsets patch 2: fix bug exposed by offsets patch 1 r=bzbarsky

Comment on attachment 466763 [details] [diff] [review] height and min/max-width patch 2: fix computation of width- and height-dependent values r=me

Comment on attachment 466765 [details] [diff] [review] height patch 3: support calc() on height, min-height, and max-height I wonder whether it's worth adding a method on nsStyleCoord (perhaps HasPercent() or something?) that returns mUnit == eStyleUnitPercent || (IsCalcUnit() && CalcHasPercent()); seems like it would simplify a number of these callsites... >+++ b/layout/generic/nsBlockFrame.cpp >+ // length, percent, or combination thereof. Test > 0 so we clamp >+ // calc() results to 0. I'm not sure what that last sentence means.... Should there be a "negative" after "clamp"? >+++ b/layout/generic/nsFrame.cpp > IsAutoHeight(const nsStyleCoord &aCoord, nscoord aCBHeight) Could we just factor this out into nsLayoutUtils instead of having at least two copies of it around? >+++ b/layout/generic/nsHTMLReflowState.cpp >+ (mStylePosition->HeightDependsOnContainer() && >+ // FIXME: condition this on not-abspos? >+ mStylePosition->mHeight.GetUnit() != eStyleUnit_Auto) || Why is this preferable to a HasPercent() check? Same for the other stuff here. >@@ -2272,38 +2284,48 @@ nsHTMLReflowState::ComputeMinMaxValues(n >+ if (((NS_AUTOHEIGHT == aContainingBlockHeight) && Remove the innermost parens around the == there? r=me with the above nits and my question about the nsHTMLReflowState code around that FIXME comment answered.

Comment on attachment 466766 [details] [diff] [review] min/max-width patch 3: support calc() on min-width and max-width r=bzbarsky

Comment on attachment 466887 [details] [diff] [review] serialize to appunits more often r=me

Comment on attachment 466888 [details] [diff] [review] offsets patch 3: unbreak nsStyleUnion on 64-bit r=me

Comment on attachment 466892 [details] [diff] [review] offsets patch 4: support calc() on 'top', 'right', 'bottom', and 'left' >+++ b/layout/generic/nsAbsoluteContainingBlock.cpp >+static inline PRBool IsFixedOffset(const nsStyleCoord& aCoord) { >+ return aCoord.GetUnit() == eStyleUnit_Coord || >+ (aCoord.IsCalcUnit() && !aCoord.CalcHasPercent()); If you put this on nsStyleCoord, some of the earlier XUL patches in this bug could share the code. >+++ b/layout/style/nsStyleStruct.h >+ PRBool OffsetHasPercent(mozilla::css::Side aSide) const >+ { >+ const nsStyleCoord coord = mOffset.Get(aSide); >+ return coord.GetUnit() == eStyleUnit_Percent || >+ (coord.IsCalcUnit() && coord.CalcHasPercent()); And this could become |return mOffset.Get(aSide).HasPercent();| if we add that HasPercent thing. r=me with those nits.

(In reply to comment #33) > I wonder whether it's worth adding a method on nsStyleCoord (perhaps > HasPercent() or something?) that returns > > mUnit == eStyleUnitPercent || (IsCalcUnit() && CalcHasPercent()); I did exactly that in my patch for calc() on margins and padding; I can move it up in my patch queue. > >+++ b/layout/generic/nsBlockFrame.cpp > >+ // length, percent, or combination thereof. Test > 0 so we clamp > >+ // calc() results to 0. > > I'm not sure what that last sentence means.... Should there be a "negative" > after "clamp"? yes. > >+++ b/layout/generic/nsHTMLReflowState.cpp > >+ (mStylePosition->HeightDependsOnContainer() && > >+ // FIXME: condition this on not-abspos? > >+ mStylePosition->mHeight.GetUnit() != eStyleUnit_Auto) || > > Why is this preferable to a HasPercent() check? Same for the other stuff here. In this case we actually should be checking for 'auto' heights for abs-pos elements. More later...

(In reply to comment #37) > >+++ b/layout/generic/nsAbsoluteContainingBlock.cpp > >+static inline PRBool IsFixedOffset(const nsStyleCoord& aCoord) { > >+ return aCoord.GetUnit() == eStyleUnit_Coord || > >+ (aCoord.IsCalcUnit() && !aCoord.CalcHasPercent()); > > If you put this on nsStyleCoord, some of the earlier XUL patches in this bug > could share the code. I sort of agree, except I can't think of a good name that wouldn't force anybody reading the code to go look at nsStyleCoord.h to see what the method does. Well, I suppose it could be IsCoordOrNonPercentCalc().

Revised patches: http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/45fa8bc23621/... where "..." is one of: height-prerequisites min-max-prerequisites offset-prerequisites fix-relative-offset-no-frame calc-compute-width-height-dependent-values coord-has-percent coord-has-coord-or-np-calc calc-height calc-min-max-width calc-serialize-to-appunits-more nsstyleunion-assignment calc-offsets coord-has-percent and coord-has-coord-or-np-calc are new patches to address the review comments.

or, more accurately: http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/45fa8bc23621/height-prerequisites http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/45fa8bc23621/min-max-prerequisites http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/45fa8bc23621/offset-prerequisites http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/45fa8bc23621/fix-relative-offset-no-frame http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/45fa8bc23621/calc-compute-width-height-dependent-values http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/45fa8bc23621/coord-has-percent http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/45fa8bc23621/coord-has-coord-or-np-calc http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/45fa8bc23621/calc-height http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/45fa8bc23621/calc-min-max-width http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/45fa8bc23621/calc-serialize-to-appunits-more http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/45fa8bc23621/nsstyleunion-assignment http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/45fa8bc23621/calc-offsets

> except I can't think of a good name How about IsAbsoluteLength? Or is that not really what's being tested for with this check?

That doesn't sound like something that might be IsCalcUnit().

Well, the key is that this is testing for a value that: 1) Can be converted to a length. 2) Is guaranteed to have that length not depend on geometry. Right?

Maybe ConvertsToLength() ?

<dbaron> bz, does ConvertsToLength() sound ok? <bz> dbaron: yes <bz> dbaron: sounds good

Might still be good to factor out IsAutoHeight. Followup bug if you don't want to do it here? You still need to update calc-offsets and the XUL bits to use ConvertsToLength, right?

(In reply to comment #47) > Might still be good to factor out IsAutoHeight. Followup bug if you don't want > to do it here? I'll do an additional patch on top. > You still need to update calc-offsets and the XUL bits to use ConvertsToLength, > right? That's done in the above patches, except I didn't update the min-height/min-width bits in nsBox.cpp intentionally because it needs to do an explicit check against 0-coord.

Ah, indeed. Sounds good!

http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=5a32f9f00418 http://hg.mozilla.org/mozilla-central/rev/403160bb0801 http://hg.mozilla.org/mozilla-central/rev/86c9d9e4ad36 http://hg.mozilla.org/mozilla-central/rev/b2b38f9c5430 http://hg.mozilla.org/mozilla-central/rev/4d4f8ffaa60e http://hg.mozilla.org/mozilla-central/rev/6c9f878d44ab http://hg.mozilla.org/mozilla-central/rev/a533af3f2efc http://hg.mozilla.org/mozilla-central/rev/fb7ecc5f447c http://hg.mozilla.org/mozilla-central/rev/fcc2aa4bd451 http://hg.mozilla.org/mozilla-central/rev/4cc74816505e http://hg.mozilla.org/mozilla-central/rev/a9ab3d82ec5f http://hg.mozilla.org/mozilla-central/rev/2285b8926740 http://hg.mozilla.org/mozilla-central/rev/e8d5a27d4918 http://hg.mozilla.org/mozilla-central/rev/5a32f9f00418

I should have done this earlier. I think it's untestable except in performance tests, since it just causes us to do more restyling. gcc compiles the new code to a jump table. I'm not sure whether that's a good thing or a bad thing (though I'd guess bad, but hopefully not significantly so), but I think doing it this way is more maintainable.

... because we shouldn't have unconstrained containing block widths anymore

Comment on attachment 469492 [details] [diff] [review] make nsStyleCoord::operator== do deep array comparison Would be nice if NS_ABORT took a message argument... Can we perhaps make that happen in a followup? We only have 4 existing consumers of it, I think.

Comment on attachment 469494 [details] [diff] [review] remove checks for unconstrained containing block widths r=me

It seems silly to have this code repeated twice. I found it while auditing all users of mVerticalAlign for my next patch (for vertical-align and text-indent). I even checked that the XXX comment being removed is false by printing the values.

(In reply to comment #54) > Would be nice if NS_ABORT took a message argument... Can we perhaps make that > happen in a followup? We only have 4 existing consumers of it, I think. I considered doing this back in May; Benjamin didn't seem too happy with the idea because he wants to make a whole bunch of other changes, and doesn't like the NS_ABORT_IF_FALSE name in the first place.

Things on which we might want to support calc() that are not supported based on what's in the tree and the above patches, are, I think: * border-radius and outline-radius (I want to wait until after bug 471643 lands) * stroke-width, stroke-dashoffset, stroke-dasharray * background-position (including in gradient positions), background-size, and transform-origin * gradient stop positions

Comment on attachment 470172 [details] [diff] [review] consolidate table-cell vertical-align getter r=bzbarsky

Moving to beta6+ - bz, dbaron, if either of you thinks we should stretch b5 to get this in, please move it back.

Fine with beta6, now that beta6 is feature freeze.

Comment on attachment 470170 [details] [diff] [review] support calc() for margins and padding >+++ b/layout/generic/nsContainerFrame.cpp > static nscoord GetCoord(const nsStyleCoord& aCoord, nscoord aIfNotCoord) > { >+ if (aCoord.GetUnit() == eStyleUnit_Coord) { >+ return aCoord.GetCoordValue(); >+ } >+ if (aCoord.IsCalcUnit() && !aCoord.CalcHasPercent()) { >+ return nsRuleNode::ComputeCoordPercentCalc(aCoord, 0); >+ } >+ return aIfNotCoord; > } I assume the reason to not do: if (aCoord.ConvertsToLength()) { return nsRuleNode::ComputeCoordPercentCalc(aCoord, 0); } return aIfNotCoord; is just efficiency, right? I think we've had this sort of pattern before too; not sure whether it's worth factoring out into a method on nsStyleCoord that does the check for coord first and does the simple thing in that case or something... >+++ b/layout/generic/nsInlineFrame.cpp >+IsPaddingZero(const nsStyleCoord &aCoord) We already had this code in nsBlockFrame.... Can we share them somehow? >+++ b/layout/style/nsStyleStruct.cpp >+ // FIXME: We could cache calc() with percents here if we changed >+ // IsFixedData and CalcCoord. File a bug and put the bug# here? r=bzbarsky

Comment on attachment 470177 [details] [diff] [review] support calc() on vertical-align and text-indent Don't forget to address the FIXME in the checkin comment, right? Or do the tests in here handle that? >+++ b/layout/generic/nsLineLayout.cpp >@@ -1746,186 +1742,184 @@ nsLineLayout::VerticalAlignFrames(PerSpa >+ case NS_STYLE_VERTICAL_ALIGN_BASELINE: >+ { >+ // The elements baseline is aligned with the baseline of "element's" >+ if (verticalAlign.HasPercent()) { >+ // Percentages are like lengths, except treated as a percentage >+ // of the elements line-height value. "element's" r=bzbarsky

(In reply to comment #65) > Comment on attachment 470170 [details] [diff] [review] > support calc() for margins and padding > > >+++ b/layout/generic/nsContainerFrame.cpp > > static nscoord GetCoord(const nsStyleCoord& aCoord, nscoord aIfNotCoord) > > { > >+ if (aCoord.GetUnit() == eStyleUnit_Coord) { > >+ return aCoord.GetCoordValue(); > >+ } > >+ if (aCoord.IsCalcUnit() && !aCoord.CalcHasPercent()) { > >+ return nsRuleNode::ComputeCoordPercentCalc(aCoord, 0); > >+ } > >+ return aIfNotCoord; > > } > > I assume the reason to not do: > > if (aCoord.ConvertsToLength()) { > return nsRuleNode::ComputeCoordPercentCalc(aCoord, 0); > } > return aIfNotCoord; > > is just efficiency, right? Actually, I think I should just change it to that. > >+++ b/layout/generic/nsInlineFrame.cpp > >+IsPaddingZero(const nsStyleCoord &aCoord) > > We already had this code in nsBlockFrame.... Can we share them somehow? I'll move it to nsLayoutUtils (in a separate patch on top). > >+++ b/layout/style/nsStyleStruct.cpp > >+ // FIXME: We could cache calc() with percents here if we changed > >+ // IsFixedData and CalcCoord. > > File a bug and put the bug# here? Actually, I'll do another patch on top right here.

(In reply to comment #66) > Don't forget to address the FIXME in the checkin comment, right? Or do the > tests in here handle that? I just forgot to remove the FIXME when I wrote the reftests. And I'll fix the grammar in the reindented comments.

Comment on attachment 470754 [details] [diff] [review] cache non-percent calc() in style structs r=bzbarsky

Comment on attachment 470170 [details] [diff] [review] support calc() for margins and padding >diff --git a/layout/generic/nsFrame.cpp b/layout/generic/nsFrame.cpp > static void > AddCoord(const nsStyleCoord& aStyle, > nsIRenderingContext* aRenderingContext, > nsIFrame* aFrame, > nscoord* aCoord, float* aPercent) > { >- switch (aStyle.GetUnit()) { >- case eStyleUnit_Coord: >- *aCoord += aStyle.GetCoordValue(); >- break; >- case eStyleUnit_Percent: >- *aPercent += aStyle.GetPercentValue(); >- break; >- default: >- break; >- } >+ LengthPercentPairWithMinMaxCalcOps ops; >+ LengthPercentPairWithMinMaxCalcOps::result_type pair = >+ css::ComputeCalc(aStyle, ops); >+ *aCoord += pair.mLength; >+ *aPercent += pair.mPercent; > } I had to make one tweak here to fix an assertion: I added, to the beginning of the function: if (!aStyle.IsCoordPercentCalcUnit()) { return; } since it is passed eStyleUnit_Auto margins rather often, which asserts (and potentially does the wrong thing) without that check.

http://hg.mozilla.org/mozilla-central/rev/400bc943fcf7 http://hg.mozilla.org/mozilla-central/rev/9dc831f46e4c http://hg.mozilla.org/mozilla-central/rev/253d994413d9 http://hg.mozilla.org/mozilla-central/rev/7bb992392d3a http://hg.mozilla.org/mozilla-central/rev/4744aeff506a http://hg.mozilla.org/mozilla-central/rev/4b05a762af72 http://hg.mozilla.org/mozilla-central/rev/4edcf6c4cd03

This depends on patches in bug 459144 (though only since the code it's patching in nsFrame.cpp was moved in those patches).

>+ return nsRuleNode::ComputeCoordPercentCalc(aCoord, nscoord_MAX) == 0 && >+ nsRuleNode::ComputeCoordPercentCalc(aCoord, 0) == 0; That should use ||, and > 0 tests, no? Still need to read the rest of the patch.

er, right. But probably != rather than >. And, now that you mention it, I realize I need to clamp these values to 0, and the same for margins and padding.

er, just padding (but I think there are also some issues with outline-width, perhaps)

Well, that's the question. Why do we want to claim a nonzero corner for cases we'll clamp to 0 anyway? Maybe it's not a big deal...

Comment on attachment 472322 [details] [diff] [review] support calc() on -moz-border-radius and -moz-outline-radius And we might want to consider refactoring things so this "0 and MAX" trick lives on nsStyleCoord... OK for now, though. r=me.

Ah, right, I'll change it to >.

There were some test failures that always puzzled me. Since the next patch would have significantly added to them, I sort of figured out what was going on (though not fully). Here's a workaround. Something makes the frame for elementf go away and not come back. (I'm guessing it's -moz-binding-related.)

It turns out I missed a whole bunch of places where I needed to clamp negative calc() expressions. This fixes them, and adds better tests.

Without this, the previous patch will assert. The spec says negative values in stroke-dasharray are in error, so this makes it so.

Comment on attachment 472760 [details] [diff] [review] fix issues with missing frame in test_value_computation.html Do you need to flush again after resetting style back from "none"? Seems like the safe thing to do... r=me with that.

Comment on attachment 472761 [details] [diff] [review] fix issues with clamping of negative calc()s Duplicating all this info on which props are nonnegative in computed style saddens me, but yeah.... r=me

Comment on attachment 472762 [details] [diff] [review] reject negative values in stroke-dasharray r=me

http://hg.mozilla.org/mozilla-central/rev/d7e5bc1bbb7b http://hg.mozilla.org/mozilla-central/rev/d8e37eb0c77c http://hg.mozilla.org/mozilla-central/rev/3f2ae0cc2cb8 http://hg.mozilla.org/mozilla-central/rev/1ad67ed26a45 I filed followups for remaining properties to be done: * bug 594933 for properties that have numbers in them * bug 594934 for the background-position-like properties * bug 594935 for gradient stop positions Marking this bug as fixed.