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

RESOLVED FIXED in mozilla2.0b7

Status

()

defect
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

Trunk
mozilla2.0b7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

Attachments

(27 attachments, 1 obsolete attachment)

7.25 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.83 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
6.28 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
8.58 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
5.26 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
36.88 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.02 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.87 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.69 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.18 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
4.39 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
51.28 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
24.99 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.37 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
789 bytes, patch
bzbarsky
: review+
Details | Diff | Splinter Review
29.34 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.77 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.34 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
37.19 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
4.78 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
35.77 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
5.32 KB, patch
Details | Diff | Splinter Review
4.43 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
18.23 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
10.94 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
56.45 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.80 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
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...
blocking2.0: --- → beta5+
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
Attachment #464219 - Flags: review?(bzbarsky)
Presently, there's no need for this function to be distinct, so make it inline-equivalent to another one.
Attachment #464220 - Flags: review?(bzbarsky)
We'll need more than one set of calc ops with nsStyleCoord input, so factor those parts into a common base class.
Attachment #464221 - Flags: review?(bzbarsky)
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.
Attachment #464224 - Flags: review?(bzbarsky)
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?
Attachment #464219 - Flags: review?(bzbarsky) → review+
Comment on attachment 464220 [details] [diff] [review]
width patch 2: remove ComputeCoordPercentCalc

r=bzbarsky
Attachment #464220 - Flags: review?(bzbarsky) → review+
Comment on attachment 464221 [details] [diff] [review]
width patch 3: common base class for calc ops with nsStyleCoord input

r=bzbarsky
Attachment #464221 - Flags: review?(bzbarsky) → review+
Comment on attachment 464222 [details] [diff] [review]
width patch 4: consolidate code for determining when widths and heights depend on a container

r=bzbarsky
Attachment #464222 - Flags: review?(bzbarsky) → review+
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?
Attachment #464224 - Flags: review?(bzbarsky) → review+
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.
Attachment #464225 - Flags: review?(bzbarsky) → review+
(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.
Note that this also fixes a regression from the 'width' patch where I changed floor to round.
Attachment #466763 - Flags: review?(bzbarsky)
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.
Attachment #466887 - Flags: review?(bzbarsky)
Comment on attachment 466759 [details] [diff] [review]
height patch 1: fix prerequisites, as in width patch 1

r=bzbarsky
Attachment #466759 - Flags: review?(bzbarsky) → review+
Comment on attachment 466760 [details] [diff] [review]
min/max-width patch 1: fix prerequisites

r=bzbarsky
Attachment #466760 - Flags: review?(bzbarsky) → review+
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?
Attachment #466761 - Flags: review?(bzbarsky) → review+
Comment on attachment 466762 [details] [diff] [review]
offsets patch 2: fix bug exposed by offsets patch 1

r=bzbarsky
Attachment #466762 - Flags: review?(bzbarsky) → review+
Comment on attachment 466763 [details] [diff] [review]
height and min/max-width patch 2: fix computation of width- and height-dependent values

r=me
Attachment #466763 - Flags: review?(bzbarsky) → review+
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.
Attachment #466765 - Flags: review?(bzbarsky) → review+
Comment on attachment 466766 [details] [diff] [review]
min/max-width patch 3: support calc() on min-width and max-width

r=bzbarsky
Attachment #466766 - Flags: review?(bzbarsky) → review+
Comment on attachment 466887 [details] [diff] [review]
serialize to appunits more often

r=me
Attachment #466887 - Flags: review?(bzbarsky) → review+
Comment on attachment 466888 [details] [diff] [review]
offsets patch 3: unbreak nsStyleUnion on 64-bit

r=me
Attachment #466888 - Flags: review?(bzbarsky) → review+
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.
Attachment #466892 - Flags: review?(bzbarsky) → review+
(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.
> 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!

Updated

9 years ago
Depends on: 590417
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.
Attachment #469492 - Flags: review?(bzbarsky)
... 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.
Attachment #469492 - Flags: review?(bzbarsky) → review+
Comment on attachment 469494 [details] [diff] [review]
remove checks for unconstrained containing block widths

r=me
Attachment #469494 - Flags: review?(bzbarsky) → review+
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.
Attachment #470172 - Flags: review?(bzbarsky)
(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
Attachment #470172 - Flags: review?(bzbarsky) → review+
Moving to beta6+ - bz, dbaron, if either of you thinks we should stretch b5 to get this in, please move it back.
blocking2.0: beta5+ → beta6+
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
Attachment #470170 - Flags: review?(bzbarsky) → review+
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
Attachment #470177 - Flags: review?(bzbarsky) → review+
(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
Attachment #470754 - Flags: review?(bzbarsky) → review+
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.
This depends on patches in bug 459144 (though only since the code it's patching in nsFrame.cpp was moved in those patches).
Attachment #472274 - Flags: review?(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.
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)
Attachment #472274 - Attachment is obsolete: true
Attachment #472274 - Flags: review?(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 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.
Attachment #472322 - Flags: review?(bzbarsky) → review+
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.)
Attachment #472760 - Flags: review?(bzbarsky)
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.
Attachment #472761 - Flags: review?(bzbarsky)
Without this, the previous patch will assert.  The spec says negative values in stroke-dasharray are in error, so this makes it so.
Attachment #472762 - Flags: review?(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.
Attachment #472760 - Flags: review?(bzbarsky) → review+
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
Attachment #472761 - Flags: review?(bzbarsky) → review+
Comment on attachment 472762 [details] [diff] [review]
reject negative values in stroke-dasharray

r=me
Attachment #472762 - Flags: review?(bzbarsky) → review+
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b6
You need to log in before you can comment on or make changes to this bug.