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

RESOLVED FIXED in mozilla2.0b7

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
7 years ago
3 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
Details | Diff | Splinter Review
2.83 KB, patch
Details | Diff | Splinter Review
6.28 KB, patch
Details | Diff | Splinter Review
8.58 KB, patch
Details | Diff | Splinter Review
5.26 KB, patch
Details | Diff | Splinter Review
36.88 KB, patch
Details | Diff | Splinter Review
1.02 KB, patch
Details | Diff | Splinter Review
1.87 KB, patch
Details | Diff | Splinter Review
3.69 KB, patch
Details | Diff | Splinter Review
3.18 KB, patch
Details | Diff | Splinter Review
4.39 KB, patch
Details | Diff | Splinter Review
51.28 KB, patch
Details | Diff | Splinter Review
24.99 KB, patch
Details | Diff | Splinter Review
1.37 KB, patch
Details | Diff | Splinter Review
789 bytes, patch
Details | Diff | Splinter Review
29.34 KB, patch
Details | Diff | Splinter Review
1.77 KB, patch
Details | Diff | Splinter Review
3.34 KB, patch
Details | Diff | Splinter Review
37.19 KB, patch
Details | Diff | Splinter Review
4.78 KB, patch
Details | Diff | Splinter Review
35.77 KB, patch
Details | Diff | Splinter Review
5.32 KB, patch
Details | Diff | Splinter Review
4.43 KB, patch
Details | Diff | Splinter Review
18.23 KB, patch
Details | Diff | Splinter Review
10.94 KB, patch
Details | Diff | Splinter Review
56.45 KB, patch
Details | Diff | Splinter Review
2.80 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

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

Updated

7 years ago
blocking2.0: --- → beta5+
(Assignee)

Comment 1

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

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

Comment 2

7 years ago
Created attachment 464220 [details] [diff] [review]
width patch 2: remove ComputeCoordPercentCalc

Presently, there's no need for this function to be distinct, so make it inline-equivalent to another one.
Attachment #464220 - Flags: review?(bzbarsky)
(Assignee)

Comment 3

7 years ago
Created attachment 464221 [details] [diff] [review]
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.
Attachment #464221 - Flags: review?(bzbarsky)
(Assignee)

Comment 4

7 years ago
Created attachment 464222 [details] [diff] [review]
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.
Attachment #464222 - Flags: review?(bzbarsky)
(Assignee)

Comment 5

7 years ago
Created attachment 464224 [details] [diff] [review]
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.
Attachment #464224 - Flags: review?(bzbarsky)
(Assignee)

Comment 6

7 years ago
Created attachment 464225 [details] [diff] [review]
width patch 6: support width: calc()
Attachment #464225 - Flags: review?(bzbarsky)
(Assignee)

Comment 7

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

Comment 14

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

Comment 16

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

Comment 17

7 years ago
Created attachment 466759 [details] [diff] [review]
height patch 1: fix prerequisites, as in width patch 1
Attachment #466759 - Flags: review?(bzbarsky)
(Assignee)

Comment 18

7 years ago
Created attachment 466760 [details] [diff] [review]
min/max-width patch 1: fix prerequisites
Attachment #466760 - Flags: review?(bzbarsky)
(Assignee)

Comment 19

7 years ago
Created attachment 466761 [details] [diff] [review]
offsets patch 1: fix prerequisites
Attachment #466761 - Flags: review?(bzbarsky)
(Assignee)

Comment 20

7 years ago
Created attachment 466762 [details] [diff] [review]
offsets patch 2: fix bug exposed by offsets patch 1
Attachment #466762 - Flags: review?(bzbarsky)
(Assignee)

Comment 21

7 years ago
Created attachment 466763 [details] [diff] [review]
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.
Attachment #466763 - Flags: review?(bzbarsky)
(Assignee)

Comment 22

7 years ago
Created attachment 466765 [details] [diff] [review]
height patch 3: support calc() on height, min-height, and max-height
Attachment #466765 - Flags: review?(bzbarsky)
(Assignee)

Comment 23

7 years ago
Created attachment 466766 [details] [diff] [review]
min/max-width patch 3: support calc() on min-width and max-width
Attachment #466766 - Flags: review?(bzbarsky)
(Assignee)

Comment 24

7 years ago
Also note that height patch 3 fixes a clamping error in width patch 6, in GetAbsoluteCoord.
(Assignee)

Comment 25

7 years ago
Created attachment 466887 [details] [diff] [review]
serialize to appunits more often

I noticed this while working on the offsets patch.
Attachment #466887 - Flags: review?(bzbarsky)
(Assignee)

Comment 26

7 years ago
Created attachment 466888 [details] [diff] [review]
offsets patch 3: unbreak nsStyleUnion on 64-bit
Attachment #466888 - Flags: review?(bzbarsky)
(Assignee)

Comment 27

7 years ago
Created attachment 466892 [details] [diff] [review]
offsets patch 4: support calc() on 'top', 'right', 'bottom', and 'left'
Attachment #466892 - 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+
(Assignee)

Comment 38

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

Comment 39

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

Comment 40

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

Comment 41

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

Comment 43

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

Comment 45

7 years ago
Maybe ConvertsToLength() ?
(Assignee)

Comment 46

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

Comment 48

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

7 years ago
Depends on: 590417
(Assignee)

Comment 50

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

Comment 51

7 years ago
Created attachment 469492 [details] [diff] [review]
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.
Attachment #469492 - Flags: review?(bzbarsky)
(Assignee)

Comment 52

7 years ago
Created attachment 469494 [details] [diff] [review]
remove checks for unconstrained containing block widths
Attachment #469494 - Flags: review?(bzbarsky)
(Assignee)

Comment 53

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

Comment 56

7 years ago
Created attachment 470170 [details] [diff] [review]
support calc() for margins and padding
Attachment #470170 - Flags: review?(bzbarsky)
(Assignee)

Comment 57

7 years ago
Created attachment 470172 [details] [diff] [review]
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.
Attachment #470172 - Flags: review?(bzbarsky)
(Assignee)

Comment 58

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

Comment 59

7 years ago
Created attachment 470177 [details] [diff] [review]
support calc() on vertical-align and text-indent
Attachment #470177 - Flags: review?(bzbarsky)
(Assignee)

Comment 60

7 years ago
Created attachment 470179 [details] [diff] [review]
diff -w of VerticalAlignFrames changes in previous attachment
(Assignee)

Comment 61

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

Comment 64

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

Comment 67

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

Comment 68

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

Comment 69

7 years ago
Created attachment 470754 [details] [diff] [review]
cache non-percent calc() in style structs
Attachment #470754 - Flags: review?(bzbarsky)
Comment on attachment 470754 [details] [diff] [review]
cache non-percent calc() in style structs

r=bzbarsky
Attachment #470754 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 71

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

Comment 72

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

Comment 73

7 years ago
Created attachment 472274 [details] [diff] [review]
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).
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.
(Assignee)

Comment 75

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

Comment 76

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

Comment 77

7 years ago
Created attachment 472322 [details] [diff] [review]
support calc() on -moz-border-radius and -moz-outline-radius
Attachment #472322 - Flags: review?(bzbarsky)
(Assignee)

Updated

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

Comment 80

7 years ago
Ah, right, I'll change it to >.
(Assignee)

Comment 81

7 years ago
Created attachment 472760 [details] [diff] [review]
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.)
Attachment #472760 - Flags: review?(bzbarsky)
(Assignee)

Comment 82

7 years ago
Created attachment 472761 [details] [diff] [review]
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.
Attachment #472761 - Flags: review?(bzbarsky)
(Assignee)

Comment 83

7 years ago
Created attachment 472762 [details] [diff] [review]
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.
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+
(Assignee)

Comment 87

7 years ago
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: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b6
Blocks: 777519
(Assignee)

Updated

3 years ago
Depends on: 1099448
You need to log in before you can comment on or make changes to this bug.