585715 - support calc() on properties for which percentage values depend on layout

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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...

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: width patch 1: Add a 'display: block' prerequisite for 'width' property tests so that calc() tests actually test the code

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

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: width patch 2: remove ComputeCoordPercentCalc

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

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: width patch 3: common base class for calc ops with nsStyleCoord input

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

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: width patch 4: consolidate code for determining when widths and heights depend on a container

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

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: width patch 5: distingish between 50% and calc(50%) in computed style

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.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: width patch 6: support width: calc()

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 8

[Boris Zbarsky [:bzbarsky]]

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 9

[Boris Zbarsky [:bzbarsky]]

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

r=bzbarsky

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

r=bzbarsky

Comment 11

[Boris Zbarsky [: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 12

[Boris Zbarsky [: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 13

[Boris Zbarsky [:bzbarsky]]

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.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

That looks correct.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: height patch 1: fix prerequisites, as in width patch 1

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: min/max-width patch 1: fix prerequisites

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: offsets patch 1: fix prerequisites

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: offsets patch 2: fix bug exposed by offsets patch 1

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: height and min/max-width patch 2: fix computation of width- and height-dependent values

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

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: height patch 3: support calc() on height, min-height, and max-height

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: min/max-width patch 3: support calc() on min-width and max-width

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: serialize to appunits more often

I noticed this while working on the offsets patch.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: offsets patch 3: unbreak nsStyleUnion on 64-bit

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: offsets patch 4: support calc() on 'top', 'right', 'bottom', and 'left'

Comment 28

[Boris Zbarsky [:bzbarsky]]

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

r=bzbarsky

Comment 29

[Boris Zbarsky [:bzbarsky]]

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

r=bzbarsky

Comment 30

[Boris Zbarsky [: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 31

[Boris Zbarsky [:bzbarsky]]

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

r=bzbarsky

Comment 32

[Boris Zbarsky [: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 33

[Boris Zbarsky [:bzbarsky]]

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 34

[Boris Zbarsky [:bzbarsky]]

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

r=bzbarsky

Comment 35

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 36

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 37

[Boris Zbarsky [:bzbarsky]]

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.

Comment 38

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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...

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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().

Comment 40

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 41

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 42

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 43

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 44

[Boris Zbarsky [:bzbarsky]]

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?

Comment 45

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Maybe ConvertsToLength() ?

Comment 46

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 47

[Boris Zbarsky [:bzbarsky]]

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?

Comment 48

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 49

[Boris Zbarsky [:bzbarsky]]

Ah, indeed.  Sounds good!

Comment 50

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 51

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: make nsStyleCoord::operator== do deep array comparison

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.

Comment 52

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: remove checks for unconstrained containing block widths

Comment 53

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 54

[Boris Zbarsky [:bzbarsky]]

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 55

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 56

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: support calc() for margins and padding

Comment 57

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: consolidate table-cell vertical-align getter

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.

Comment 58

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 59

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: support calc() on vertical-align and text-indent

Comment 60

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: diff -w of VerticalAlignFrames changes in previous attachment

Comment 61

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 62

[Boris Zbarsky [:bzbarsky]]

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

r=bzbarsky

Comment 63

[Mike Beltzner [:beltzner, not reading bugmail]]

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

Comment 64

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fine with beta6, now that beta6 is feature freeze.

Comment 65

[Boris Zbarsky [:bzbarsky]]

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 66

[Boris Zbarsky [: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

Comment 67

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 68

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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 69

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: cache non-percent calc() in style structs

Comment 70

[Boris Zbarsky [:bzbarsky]]

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

r=bzbarsky

Comment 71

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 72

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 73

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: support calc() on -moz-border-radius and -moz-outline-radius

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

Comment 74

[Boris Zbarsky [:bzbarsky]]

>+    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.

Comment 75

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 76

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 77

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: support calc() on -moz-border-radius and -moz-outline-radius

Comment 78

[Boris Zbarsky [:bzbarsky]]

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 79

[Boris Zbarsky [:bzbarsky]]

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.

Comment 80

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 81

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: fix issues with missing frame in test_value_computation.html

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.)

Comment 82

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: fix issues with clamping of negative calc()s

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.

Comment 83

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: reject negative values in stroke-dasharray

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

Comment 84

[Boris Zbarsky [:bzbarsky]]

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 85

[Boris Zbarsky [:bzbarsky]]

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 86

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 87

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.