Closed Bug 594933 Opened 14 years ago Closed 8 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: dbaron, Assigned: mtseng)

References

(Blocks 3 open bugs)

Details

(Keywords: css3, dev-doc-complete)

Attachments

(4 files, 7 obsolete files)

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
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.
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
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
The first two block should look identical and the last two block should also look identical.
Assignee: nobody → thomas
According to  testcase line-height.htm the line-height 300% is incorrect.
(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 :)
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
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
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.
(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 think I would have possible been able to center some divs if I had attr() and calc(). sure would have been nice. :-)
(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)
(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.
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)
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-
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)
Any ETA on this?
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)
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)
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.
Blocks: 737785
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
Assignee: thomas → nobody
> 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/
:dbaron, do you think this is right track for this issue?
Attachment #8723985 - Flags: feedback?(dbaron)
Attachment #664104 - Attachment is obsolete: true
Attachment #8723986 - Flags: feedback?(dbaron)
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+
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-
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?
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');
Addressed dbaron's comment.
Attachment #8727334 - Flags: review?(dbaron)
Attachment #8723985 - Attachment is obsolete: true
Addressed dbaron's comment.
Attachment #8727335 - Flags: review?(dbaron)
Attachment #8723986 - Attachment is obsolete: true
Assignee: nobody → mtseng
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+
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-
Attachment #8727334 - Attachment is obsolete: true
Addressed dbaron's comment.
Attachment #8728960 - Flags: review?(dbaron)
Attachment #8727335 - Attachment is obsolete: true
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+
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).
Attachment #8728960 - Attachment is obsolete: true
Try is good.
https://hg.mozilla.org/mozilla-central/rev/8e35ad3b586f
https://hg.mozilla.org/mozilla-central/rev/2f3cc8818587
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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)
stroke-width, stroke-dashoffset, and stroke-dasharray will be supported in follow-up bugs.
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.
How about transform:scale()?
Blocks: 1264520
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: