Closed
Bug 594933
Opened 14 years ago
Closed 9 years ago
support calc() on properties that accept <number> values (line-height)
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
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.
Reporter | ||
Updated•14 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
Updated•13 years ago
|
Blocks: css-values-3
Comment 1•13 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
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 6•12 years ago
|
||
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•12 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.
Comment 8•12 years ago
|
||
(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•12 years ago
|
||
WebKit passes this.
Reporter | ||
Comment 10•12 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•12 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•12 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•12 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•12 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.
Comment 16•12 years ago
|
||
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•12 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•12 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•11 years ago
|
||
Any ETA on this?
Comment 19•11 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•11 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)
Updated•11 years ago
|
Keywords: dev-doc-needed
Reporter | ||
Comment 21•11 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.
Comment 22•10 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
Comment hidden (me-too) |
Comment hidden (me-too) |
Comment 25•9 years ago
|
||
> 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•9 years ago
|
||
:dbaron, do you think this is right track for this issue?
Attachment #8723985 -
Flags: feedback?(dbaron)
Assignee | ||
Comment 27•9 years ago
|
||
Rebase to latest m-c.
Assignee | ||
Updated•9 years ago
|
Attachment #664104 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8723986 -
Flags: feedback?(dbaron)
Reporter | ||
Comment 28•9 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•9 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•9 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•9 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.
Comment 32•9 years ago
|
||
(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•9 years ago
|
Attachment #8723986 -
Flags: feedback- → feedback+
Assignee | ||
Comment 33•9 years ago
|
||
Addressed dbaron's comment.
Attachment #8727334 -
Flags: review?(dbaron)
Assignee | ||
Updated•9 years ago
|
Attachment #8723985 -
Attachment is obsolete: true
Assignee | ||
Comment 34•9 years ago
|
||
Addressed dbaron's comment.
Attachment #8727335 -
Flags: review?(dbaron)
Assignee | ||
Updated•9 years ago
|
Attachment #8723986 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → mtseng
Reporter | ||
Comment 35•9 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•9 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•9 years ago
|
||
Attachment #8728959 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8727334 -
Attachment is obsolete: true
Assignee | ||
Comment 38•9 years ago
|
||
Addressed dbaron's comment.
Attachment #8728960 -
Flags: review?(dbaron)
Assignee | ||
Updated•9 years ago
|
Attachment #8727335 -
Attachment is obsolete: true
Reporter | ||
Comment 39•9 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•9 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 | ||
Updated•9 years ago
|
Attachment #8728960 -
Attachment is obsolete: true
Assignee | ||
Comment 42•9 years ago
|
||
Assignee | ||
Comment 43•9 years ago
|
||
Try is good.
Comment 44•9 years ago
|
||
Comment 45•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8e35ad3b586f
https://hg.mozilla.org/mozilla-central/rev/2f3cc8818587
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•9 years 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•9 years ago
|
||
stroke-width, stroke-dashoffset, and stroke-dasharray will be supported in follow-up bugs.
Comment 47•9 years ago
|
||
(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•9 years ago
|
||
How about transform:scale()?
Comment 49•9 years ago
|
||
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•9 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•