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

RESOLVED FIXED in Firefox 48

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
7 years ago
a year ago

People

(Reporter: dbaron, Assigned: mtseng)

Tracking

(Blocks: 6 bugs, {css3, dev-doc-complete})

Trunk
mozilla48
css3, dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(4 attachments, 7 obsolete attachments)

504 bytes, text/html
Details
507 bytes, text/html
Details
2.86 KB, patch
mtseng
: review+
Details | Diff | Splinter Review
15.05 KB, patch
mtseng
: review+
Details | Diff | Splinter Review
(Reporter)

Description

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

Updated

7 years ago
Summary: support calc on line-height, stroke-width, stroke-dashoffset, and stroke-dasharray → support calc() on line-height, stroke-width, stroke-dashoffset, and stroke-dasharray
Blocks: 598208
Depends on: 363249
Blocks: 741643

Comment 1

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

5 years ago
Created attachment 663048 [details]
testcase for calc() line-height.htm

The first two block should look identical and the last two block should also look identical.
Assignee: nobody → thomas

Comment 3

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

Comment 4

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

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

Comment 7

5 years ago
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.
(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
Created attachment 663673 [details]
testcase asserting that calc() for 'line-height' is resolved at computed value time

WebKit passes this.
(Reporter)

Comment 10

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

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

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

Comment 13

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

Comment 14

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

Updated

5 years ago
Duplicate of this bug: 793592

Comment 16

5 years ago
Created attachment 664104 [details] [diff] [review]
Part 1:Add calc to line-height parser and do calc in nsRuleNode

Do calc @ nsRuleNode to fix issue in comment #9
Attachment #663457 - Attachment is obsolete: true
Attachment #663457 - Flags: review?(bzbarsky)
Attachment #664104 - Flags: review?(bzbarsky)
(Reporter)

Comment 17

5 years ago
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.
Attachment #664104 - Flags: review?(bzbarsky) → review-
(Reporter)

Updated

5 years ago
Summary: support calc() on line-height, stroke-width, stroke-dashoffset, and stroke-dasharray → support calc() on properties that accept <number> values (line-height, stroke-width, stroke-dashoffset, and stroke-dasharray)

Comment 18

4 years ago
Any ETA on this?

Comment 19

4 years ago
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
Flags: needinfo?(dbaron)
(Reporter)

Comment 20

4 years ago
Certainly not as part of this bug; I'm somewhat skeptical in general, but it's possible -- but the discussion belongs elsewhere.
Flags: needinfo?(dbaron)
Keywords: dev-doc-needed
(Reporter)

Comment 21

3 years ago
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.

Updated

3 years ago
Blocks: 737785

Comment 22

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

Updated

3 years ago
Assignee: thomas → nobody
Comment hidden (me-too)
Comment hidden (me-too)
(Reporter)

Updated

2 years ago
Blocks: 1218257
> 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/
(Assignee)

Comment 26

2 years ago
Created attachment 8723985 [details] [diff] [review]
Part 1: Let calc() supports number.

:dbaron, do you think this is right track for this issue?
Attachment #8723985 - Flags: feedback?(dbaron)
(Assignee)

Comment 27

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

Rebase to latest m-c.
(Assignee)

Updated

2 years ago
Attachment #664104 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8723986 - Flags: feedback?(dbaron)
(Reporter)

Comment 28

2 years ago
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.)
Attachment #8723985 - Flags: feedback?(dbaron) → feedback+
(Reporter)

Comment 29

2 years ago
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.
Attachment #8723986 - Flags: feedback?(dbaron) → feedback-
(Reporter)

Comment 30

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

Comment 31

2 years ago
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.
(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');
(Reporter)

Updated

2 years ago
Attachment #8723986 - Flags: feedback- → feedback+
(Assignee)

Comment 33

2 years ago
Created attachment 8727334 [details] [diff] [review]
Part 1: Let calc() supports number.

Addressed dbaron's comment.
Attachment #8727334 - Flags: review?(dbaron)
(Assignee)

Updated

2 years ago
Attachment #8723985 - Attachment is obsolete: true
(Assignee)

Comment 34

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

Addressed dbaron's comment.
Attachment #8727335 - Flags: review?(dbaron)
(Assignee)

Updated

2 years ago
Attachment #8723986 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Assignee: nobody → mtseng
(Reporter)

Comment 35

2 years ago
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
Attachment #8727334 - Flags: review?(dbaron) → review+
(Reporter)

Comment 36

2 years ago
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 <thomas@thomasy.tw>

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.
Attachment #8727335 - Flags: review?(dbaron) → review-
(Assignee)

Comment 37

2 years ago
Created attachment 8728959 [details] [diff] [review]
Part 1: Let calc() supports number v2. (carry r+: dbaron)
Attachment #8728959 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8727334 - Attachment is obsolete: true
(Assignee)

Comment 38

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

Addressed dbaron's comment.
Attachment #8728960 - Flags: review?(dbaron)
(Assignee)

Updated

2 years ago
Attachment #8727335 - Attachment is obsolete: true
(Reporter)

Comment 39

2 years ago
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
Attachment #8728960 - Flags: review?(dbaron) → review+
(Reporter)

Comment 40

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

Comment 41

2 years ago
Created attachment 8730064 [details] [diff] [review]
Part 2: Add support for calc() to line-height v3. (carry r+: dbaron)

Apply dbaron's comment.
Attachment #8730064 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8728960 - Attachment is obsolete: true
(Assignee)

Comment 42

2 years ago
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=30331de10df9
(Assignee)

Comment 43

2 years ago
Try is good.

Comment 44

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

Comment 45

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8e35ad3b586f
https://hg.mozilla.org/mozilla-central/rev/2f3cc8818587
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48

Updated

a year ago
Summary: support calc() on properties that accept <number> values (line-height, stroke-width, stroke-dashoffset, and stroke-dasharray) → support calc() on properties that accept <number> values (line-height)
(Assignee)

Comment 46

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

Updated

a year ago
Blocks: 1258270
(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

a year ago
How about transform:scale()?
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
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
Keywords: dev-doc-needed → dev-doc-complete

Updated

a year ago
Blocks: 1264520
You need to log in before you can comment on or make changes to this bug.