594933 - support calc() on properties that accept <number> values (line-height)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

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

We might want to support calc() on line-height, stroke-width, stroke-dashoffset, and stroke-dasharray.

In order to do so, we need to figure out how to deal with the fact that these properties accept numbers as values.

This is work that I did not do in bug 363249 / bug 585715, and am not currently planning to do for Mozilla 2 / Firefox 4.

Comment 1

[Jim Michaels]

hmm. according to http://dev.w3.org/csswg/css3-values/#calc-notation
and emails in w3c css mailing list, you are supposed to also be able to have attr() in calc() as well (which is really cool!), and be able to apply it in html's css generally where property values involve numerics. see https://bugzilla.mozilla.org/show_bug.cgi?id=647488

Comment 2

[Thomasy]

Attachment: testcase for calc() line-height.htm

The first two block should look identical and the last two block should also look identical.

Comment 3

[Thomasy]

According to  testcase line-height.htm the line-height 300% is incorrect.

Comment 4

[Thomasy]

(In reply to Thomasy from comment #3)
> According to  testcase line-height.htm the line-height 300% is incorrect.

Just forget about Comment 3. I have some misunderstanding on line-height :)

Comment 5

[Thomasy]

Attachment: Part 1:Add calc to line-height parser and do calc in ComputeLineHeight.

1. Handle eStyleUnit_Calc type in ComputeLineHeight @ nsHTMLReflowState
2. Add VARIANT_CALC to ParseNonNegativeVariant @ nsCSSParser
3. Add VARIANT_CALC to nsCSSPropList
4. Add SETCOORD_STORE_CALC to SetCoord @ nsRuleNode
5. Update comment @ nsRuleNode

Comment 6

[Kang-Hao (Kenny) Lu [:kennyluck]]

Comment on attachment 663457 [details] [diff] [review]
Part 1:Add calc to line-height parser and do calc in ComputeLineHeight.

Review of attachment 663457 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsHTMLReflowState.cpp
@@ +2323,5 @@
> +    nsStyleCoord::Calc *calc;
> +    calc = lhCoord.GetCalcValue();
> +    return NSToCoordRound((calc->mLength + calc->mPercent * aStyleContext->GetStyleFont()->mFont.size) *
> +                           aFontSizeInflation);
> +  }

I *think* this is probably wrong. Ha ha!

The calc() should be resolved at computed value time (nsRuleNode.cpp) instead of here. Note that percentage value is resolved at nsRuleNode.cpp too.

Having said that, the spec is a bit unclear here[1] so I am not sure if everyone would be convinced...

ps. Thomasy is a good friend of mine so I am laughing at him in good faith :p

[1] http://dev.w3.org/csswg/css3-values/#calc-computed-value

Comment 7

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

So what solution are you proposing to the problem of distinguishing number values of these properties from the numeric terms in the calc?

I believe this patch is nowhere near enough to do this correctly.

Comment 8

[Kang-Hao (Kenny) Lu [:kennyluck]]

(In reply to David Baron [:dbaron] from comment #7)
> So what solution are you proposing to the problem of distinguishing number
> values of these properties from the numeric terms in the calc?

Is this really a problem? For 'line-height', there are only two cases as far as I can tell:

* mixing <number> and <length> was discussed and rejected[1]. (If we really think it could be useful, we should introduce a unit for line height.)
* mixing <number> and <number> is indeed not addressed in Thomasy's patch, but doing so isn't really worthwhile at this point because we don't have attr() or var() yet, and also it should be handled along with 'counter-increment: calc(1 + 1)' and such.

For stroke-*, it seems that the only sane way is to disallow unitless <length> in calc().

[1] http://dev.w3.org/csswg/css3-values/issues-lc-2012#issue-26

Comment 9

[Kang-Hao (Kenny) Lu [:kennyluck]]

Attachment: testcase asserting that calc() for 'line-height' is resolved at computed value time

WebKit passes this.

Comment 10

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

So the patch only supports non-<number> values of 'line-height' inside of calc()?

I'd be opposed to doing that, since <number> values of 'line-height' are preferable in quite a few ways that authors tend not to realize, so I don't want to do anything to discourage authors from using <number> values.

Comment 11

[Thomasy]

(In reply to David Baron [:dbaron] from comment #10)
> So the patch only supports non-<number> values of 'line-height' inside of
> calc()?
> 
> I'd be opposed to doing that, since <number> values of 'line-height' are
> preferable in quite a few ways that authors tend not to realize, so I don't
> want to do anything to discourage authors from using <number> values.

I am not quite understand your points.
But I did realize case like "25% - 3px + 2 * 12.5%"
Can you give me more examples ?

Comment 12

[Jim Michaels]

I think I would have possible been able to center some divs if I had attr() and calc(). sure would have been nice. :-)

Comment 13

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

(In reply to Thomasy from comment #11)
> (In reply to David Baron [:dbaron] from comment #10)
> > So the patch only supports non-<number> values of 'line-height' inside of
> > calc()?
> > 
> > I'd be opposed to doing that, since <number> values of 'line-height' are
> > preferable in quite a few ways that authors tend not to realize, so I don't
> > want to do anything to discourage authors from using <number> values.
> 
> I am not quite understand your points.
> But I did realize case like "25% - 3px + 2 * 12.5%"
> Can you give me more examples ?

I'm talking about <number> values like
  line-height: 1.2
so used in calc() they would look like:
  line-height: calc(0.5 * 1.2 + 3 * 0.1)
or perhaps be combined with other values:
  line-height: calc(1.2 - 1px)

Comment 14

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

(In reply to David Baron [:dbaron] from comment #0)
> In order to do so, we need to figure out how to deal with the fact that
> these properties accept numbers as values.

I'd note that I think doing this is quite hard; it requires deferring the decision on whether something is the number-part or factor-part inside a calc() expression later than the current code makes that decision.  If my memory is correct, I originally tried to write the calc() parsing code to support this, and gave up when I realized how much additional work it would be.

Comment 16

[Thomasy]

Attachment: Part 1:Add calc to line-height parser and do calc in nsRuleNode

Do calc @ nsRuleNode to fix issue in comment #9

Comment 17

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

Comment on attachment 664104 [details] [diff] [review]
Part 1:Add calc to line-height parser and do calc in nsRuleNode

We should not do this until we've fixed ParseCalc and the functions it calls to deal with numbers (which, as I said, is hard) and fixed ParseVariant to pass VARIANT_NUMBER through to ParseCalc.

Comment 18

[Florian Bender]

Any ETA on this?

Comment 19

[WM]

Will you try to support using counter() as number in calc()?

that will make it easy to create such transition effect like this:

http://img.svbtle.com/ekfniysvvg4chw.gif

Comment 20

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

Certainly not as part of this bug; I'm somewhat skeptical in general, but it's possible -- but the discussion belongs elsewhere.

Comment 21

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

This has probably become slightly higher priority now that it works in both Chrome and IE.

It may well make sense to do other work at the same time, in particular implementing the agreements about unit algebra in http://lists.w3.org/Archives/Public/www-style/2014Apr/0373.html .  I think the two things require a bunch of the same infrastructure.

Comment 22

[Julio Becerra]

Too bad that this doesn't work in FF. Demo link. Works in Chrome and IE

http://jsbin.com/wacodikawe/1/edit?html,css,output

Comment 23

[Michael]

How is this still an issue? Logged in 2012?

Comment 24

[Ian Yang]

Please add the support.

Comment 25

[Karl Dubost💡 :karlcow]

> So, the line-height for the title would be that minus twice 
> the slide’s border-width:
>
>    line-height: calc(#{$b/$a*100vw} - #{2*$slide-border-width});
>
> This was my initial idea, which, in theory, should work just 
> fine. And it does in WebKit browsers and IE. However, it 
> turns out that calc() values don’t work for line-height (and 
> some other properties) in Firefox; so, calc() is not the best 
> solution there. Luckily, there are a lot of other ways to 
> solve this problem (flexbox, absolute positioning and more).
http://www.smashingmagazine.com/2015/12/getting-started-css-calc-techniques/

Comment 26

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 1: Let calc() supports number.

:dbaron, do you think this is right track for this issue?

Comment 27

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 2: Add calc to line-height parser and do calc in nsRuleNode.

Rebase to latest m-c.

Comment 28

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

Comment on attachment 8723985 [details] [diff] [review]
Part 1: Let calc() supports number.

I think much of this looks ok, but I don't think it's sufficient.

I think the unit checking in the parsing code needs additional changes.  For example, I don't think the code will accept calc(1.2 + 1px), which calc() ought to accept for line-height.  There is various code in the functions inside of ParseCalc that needs updating, and some comments also need updating.

There's no need to add mHasNumber to either CalcValue or ComputedCalc; I don't think anything depends on the presence of number like things depend on the presence of percentages.

(I'd probably rather have switched to doing full unit algebra at the same time, but given that, despite the CSS Working Group agreeing to it, there's currently no spec for it, I guess this is ok.)

Comment 29

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

Comment on attachment 8723986 [details] [diff] [review]
Part 2: Add calc to line-height parser and do calc in nsRuleNode.

>+      if (calc->mHasNumber) {
>+        text->mLineHeight.SetCoordValue(
>+            NSToCoordRound(float(aContext->StyleFont()->mFont.size) *
>+                           calc->mNumber));
>+      } else {
>+        text->mLineHeight.SetCoordValue(
>+            NSToCoordRound(float(aContext->StyleFont()->mFont.size) *
>+                           calc->mPercent + calc->mLength));
>+      }

This doesn't make sense.  You need to be able to merge number and length/percent values, and produce a computed calc() result, and then make the code that looks at nsStyleText::mLineHeight be able to handle a computed calc value.

However, you should still be computing percentages to lengths in this code; the computed calc values for line-height should contain only lengths and numbers.


You also need to add reftests for cases that combine number with other units.

>+  nsStyleCoord mLineHeight;             // [inherited] coord, factor, normal, calc

You need to actually make this comment true (as I said in the previous comment); it's not true in the current patch, since you never created computed calc values.

>+    other_values: [ "1.0", "1", "1em", "47px", "-moz-block-height", "calc(2px)", "calc(50%)", "calc(3*25px)", "calc(25px*3)", "calc(3*25px + 50%)", "calc(1 + 

In addition to reftests, you also need to add tests here for combinations of numbers with other types of values.  Please apply considerable creativity to getting all the interesting combinations.

Comment 30

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

Then again, the spec doesn't seem to allow mixing of <number> values with non-<number> values in + and - expressions, which makes most of my comments moot.

I tend to think that's a mistake in the spec, though.

What do other implementations do?

Comment 31

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

I guess it looks like at least Chromium follows that bit of the spec, although I think it's silly, since numbers are (in the case of line-height) unit-equivalent to lengths and percentages.

However, given that, I guess you can ignore all of my comments about supporting things like calc(1.2px + 1).

However, it also means that you added a bunch of extra code that you don't need, since you never have a need for storing numbers in computed calc() expressions at all.  So ComputedCalc can remain just length and percent, the line-height nsStyleCoord should keep its existing comment that says it doesn't have calc().  (You still don't need mHasNumber; you can just check for nonzero values.)  At least I think you can do that -- you'd need to undo the hack where you have the the nsRuleNode.cpp code for line-height use SETCOORD_STORE_CALC and then replace all of the expressions that it stores.

You should add a number of tests to the invalid_values line for line-height in property_database.js, though, since you should test that the things we don't want to accept are, in fact, rejected.

Comment 32

[Karl Dubost💡 :karlcow]

(In reply to David Baron [:dbaron] ⌚️UTC+8 from comment #31)
> I guess it looks like at least Chromium follows that bit of the spec,
> although I think it's silly, since numbers are (in the case of line-height)
> unit-equivalent to lengths and percentages.
> 
> However, given that, I guess you can ignore all of my comments about
> supporting things like calc(1.2px + 1).

The test cases for Chromium are 
https://src.chromium.org/viewvc/blink/trunk/LayoutTests/css3/calc/line-height.html
https://src.chromium.org/viewvc/blink/trunk/LayoutTests/css3/calc/line-height-expected.txt

One of their tests is 
#calc-percent { line-height: calc(100% * 2); }

They also have tests with line-height rounding using calc.
https://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/css/line-height-rounding.html
https://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/css/line-height-rounding-expected.txt

 var message = checkLineHeight('calc(1.05 + 0.01px)', '10.51px');

Comment 33

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 1: Let calc() supports number.

Addressed dbaron's comment.

Comment 34

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 2: Add calc to line-height parser and do calc in nsRuleNode.

Addressed dbaron's comment.

Comment 35

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

Comment on attachment 8727334 [details] [diff] [review]
Part 1: Let calc() supports number.

>diff --git a/layout/style/nsCSSParser.cpp b/layout/style/nsCSSParser.cpp
>-    // calc() currently allows only lengths and percents inside it.
>-    if (!ParseCalc(aValue, aVariantMask & VARIANT_LP)) {
>+    // calc() currently allows only lengths and percents and number inside it.
>+    if (!ParseCalc(aValue, aVariantMask & VARIANT_LPN)) {

Could you also mention in this comment that number cannot be mixed with length and percent.

r=dbaron with that

Comment 36

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

Comment on attachment 8727335 [details] [diff] [review]
Part 2: Add calc to line-height parser and do calc in nsRuleNode.

># User Yu-Sian (Thomasy) Liu <[e-mail]>

You should add yourself as an author of the patch.  (IS any of Thomasy's
patch left?  If not, you should replace his name; if so, you should
add your name, separated by a comma.)

Could you add a test with calc() in the line-height value to the
other_values line for the "font" shorthand property in
property_database.js?


>-  nsStyleCoord mLineHeight;             // [inherited] coord, factor, normal
>+  nsStyleCoord mLineHeight;             // [inherited] coord, factor, normal, calc

Please remove this change.  mLineHeight doesn't store calc() values.


The nsRuleNode changes are incorrect; you're incorrectly treating
line-height: calc(1.2) as calc(120%).  The values are different, since
they inherit differently.  You do need to treat it as 1.2, and store
it as such.

This means you probably want the intermediate values to be nsStyleCoord
rather than nscoord, which also has the advantage of being able to round
at the end rather than do intermediate rounding.  (And you should do
that rounding with NSToCoordRound.)

Instead of inheriting from BasicCoordCalcOps, you probably want to
implement something rather like BasicFloatCalcOps, except also carrying
the unit (coord vs. number) and making assertions about it in
MergeAdditive.

In nsCSSPropList.h, you shouldn't add
+        CSS_PROPERTY_STORES_CALC |


Marking as review- because of the nsRuleNode issues, for which I'd
like to look at the revised patch.

Comment 37

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 1: Let calc() supports number v2. (carry r+: dbaron)

Comment 38

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 2: Add calc to line-height parser and do calc in nsRuleNode v2.

Addressed dbaron's comment.

Comment 39

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

Comment on attachment 8728960 [details] [diff] [review]
Part 2: Add calc to line-height parser and do calc in nsRuleNode v2.

>Bug 594933 - Part 2: Add calc to line-height parser and do calc in nsRuleNode. r=dbaron

This commit message should just be:

Bug 594933 - Part 2: Add support for calc() to line-height. r=dbaron

>+struct SetLineHeightCalcOps : public CalcLengthCalcOps::NumbersAlreadyNormalizedOps

The base class should be clerade as css::NumbersAlreadyNormalizedOps,
like it was in the previous version of the patch.

(I'm actually surprised that this declaration works.)


>+  result_type ComputeLeafValue(const nsCSSValue& aValue)
>+  {
>+    LineHeightCalcObj result;
>+    nscoord fontSize = mStyleContext->StyleFont()->mFont.size;

Please move the declaration of fontSize into the eCSSUnit_Percent case,
since that's the only place where it used.  This avoids doing
unnecessary work, and also makes it clearer that it's related to the
call to SetUncacheable.

>+    if (aValue.IsLengthUnit()) {
>+      result.mIsNumber = false;
>+      result.mLineHeight = CalcLength(aValue, mStyleContext,
>+                                      mPresContext, mConditions);
>+    }
>+    else if (eCSSUnit_Percent == aValue.GetUnit()) {
>+      mConditions.SetUncacheable();
>+      result.mIsNumber = false;
>+      result.mLineHeight = fontSize * aValue.GetPercentValue();
>+    }
>+    else if (eCSSUnit_Number == aValue.GetUnit()) {
>+      mConditions.SetUncacheable();

Please remove this SetUncacheable.  It de-optimizes for no good reason.

>+      result.mIsNumber = true;
>+      result.mLineHeight = aValue.GetFloatValue();
>+    } else {
>+      MOZ_ASSERT(false, "unexpected value");
>+      result.mIsNumber = false;
>+      result.mLineHeight = fontSize;
>+    }




>+      text->mLineHeight.SetCoordValue(NSToCoordRound(obj.mLineHeight));

This should actually be NSToCoordRoundWithClamp rather than just
NSToCoordRound, to avoid producing out-of-range nscoords.

r=dbaron with those changes

Comment 40

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

Oh, and you should make this else:
>+    } else {
>+      MOZ_ASSERT(false, "unexpected value");
>+      result.mIsNumber = false;
>+      result.mLineHeight = fontSize;
>+    }

set mIsNumber to true and mLineHeight to 1.0f, so that you don't need fontSize.  Numbers are better than lengths as a default for line-height anyway (although it shouldn't actually matter).

Comment 41

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Part 2: Add support for calc() to line-height v3. (carry r+: dbaron)

Apply dbaron's comment.

Comment 42

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=30331de10df9

Comment 43

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Try is good.

Comment 44

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e35ad3b586f
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f3cc8818587

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8e35ad3b586f
https://hg.mozilla.org/mozilla-central/rev/2f3cc8818587

Comment 46

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

stroke-width, stroke-dashoffset, and stroke-dasharray will be supported in follow-up bugs.

Comment 47

[Boris Chiou [:boris]]

(In reply to Morris Tseng [:mtseng] from comment #46)
> stroke-width, stroke-dashoffset, and stroke-dasharray will be supported in
> follow-up bugs.

File Bug 1258270 for stroke-width and stroke-dasharray. I think stroke-dashoffset would be supported in Bug 1218257.

Comment 48

[lmlb_lmlb]

How about transform:scale()?

Comment 49

[Boris Chiou [:boris]]

Looks like we missed adding tests in test_transitions_per_property.html. [1]

[1] https://dxr.mozilla.org/mozilla-central/rev/b942c98f56c4c2926b8b81b98425072a091bbf7b/layout/style/test/test_transitions_per_property.html#155

Comment 50

[Jean-Yves Perrier [:teoli]]

Updated compat table on: https://developer.mozilla.org/en-US/docs/Web/CSS/calc#Browser_compatibility
and there is a mention in:
https://developer.mozilla.org/en-US/Firefox/Releases/48#CSS