Closed Bug 1296209 Opened 6 years ago Closed 4 years ago

Support calc() in CSS properties that take <integer> values


(Core :: CSS Parsing and Computation, defect, P3)




Tracking Status
firefox62 --- fixed


(Reporter: dholbert, Assigned: MatsPalmgren_bugz)


(Blocks 2 open bugs)


(Keywords: dev-doc-complete)


(1 file)

Bug 1293743 is adding support for representing integer-restricted calc() expressions.

(This is supposed to be allowed broadly in integer-typed CSS expressions -- the css-values-3 spec [1] says calc "can be used wherever ... <integer> values are allowed".)

So: we should broaden our code to use VARIANT_CALC for...
 - every property that takes VARIANT_NUMBER in nsCSSPropList.h, via:
   * VARIANT_AHI (e.g. "z-index", "-moz-column-count")
   * VARIANT_HI ("order")
 - various pieces of custom parsing code in nsCSSParser.cpp that uses VARIANT_NUMBER, VARIANT_HKI (e.g. in ParseFontWeight), and any other int-flavored variant mask.

(This might be as simple as just adding VARIANT_CALC to the bitmasks in most of these places, or it might be more complex -- I'm not sure.)

(Bug 984021 [about calc in rgb() expressions] seems related, too, though we should probably keep that in its own separate bug, since our color-component-parsing code (in CSSParserImpl::ParseNumberColorComponent) doesn't use nsCSSValue at all right now, and I think it might need to do so in order to benefit from Bug 1293743.)
See Also: → 984021
Talked about briefly in person w/ dholbert: we might also have to add additional logic in nsRuleNode to compute the resulting calc() expressions, in particular when they contain variables.

In CSSParserImpl::ParseProperty, we store declared values with variable references as token streams. These are then resolved in nsRuleNode::ResolveVariableReferences before parsing the resolved value as a regular nsCSSValue. With VARIANT_CALC, this can be a calc() expression too.

This is what the code in nsRuleNode for computing z-index, an integer-valued property, looks like right now:

  // z-index
  const nsCSSValue* zIndexValue = aRuleData->ValueForZIndex();
  if (! SetCoord(*zIndexValue, pos->mZIndex, parentPos->mZIndex,
                 aContext, nullptr, conditions)) {
    if (eCSSUnit_Inherit == zIndexValue->GetUnit()) {
      // handle inherit, because it's ok to inherit 'auto' here
      pos->mZIndex = parentPos->mZIndex;

SetCoord has a bunch of code like this:

  else if (((aMask & SETCOORD_PERCENT) != 0) &&
           (aValue.GetUnit() == eCSSUnit_Percent)) {

It should be as simple as adding a lot of wrapper functions that do something like this on the inside:

    SetFontSizeCalcOps ops(aParentSize, aParentFont,
                           aPresContext, aAtRoot,
    *aSize = css::ComputeCalc(*sizeValue, ops);

... before passing the values to SetCoord or wherever as before, but it would require the changes, otherwise we will probably just fail to set the values (e.g. for z-index, if SetCoord fails because the unit / mask doesn't match anywhere, ComputePositionData just carries on.
Is this a duplicate of Bug 1264520?
Flags: needinfo?(dholbert)
Not quite a duplicate, no.

 * Bug 1264520 seems to be about e.g. calc(length + number) -- mixing length with a unitless number -- for properties like "line-height" which take both types of units.

 * This bug here is about e.g. calc(number + number) -- no mixing of units -- for purely-numeric properties like "z-index" and "-moz-column-count" (which take numbers but no lengths/percentages).

This bug here is strictly simpler than Bug 1264520, and is targeted at a different set of CSS properties.  This bug might make Bug 1264520 easier, though, by laying some groundwork for handling numbers in calc.  Note that bug 1293743 has some of that infrastructure already added, too.
Flags: needinfo?(dholbert)
(Sorry, I meant to say "integer" throughout comment 4, not "number".  Though I think we need to fix this for "number" as well -- e.g. for this testcase:
 data:text/html,<div style="opacity: calc(0.1 + 0.1)">Am i semitransparent?
...but that might want to happen separately.)
Priority: -- → P3
grid-column and grid-row must also support.
I have WIP patches for this...
Assignee: nobody → mats
See Also: → 1350739
Daniel, should we just mark all these as FIXED? Stylo definitely supports calc() everywhere integers are expected, except color functions, which is bug 984021, and is under review now.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #8)
> Daniel, should we just mark all these as FIXED? Stylo definitely supports
> calc() everywhere integers are expected, except color functions, which is
> bug 984021, and is under review now.

(Meant to ni?)
Flags: needinfo?(dholbert)
Sure, sounds like we can mark this as FIXED - though we should add some calc() values to "other_values" arrays in for all of the relevant integer-value-taking properties, to be sure that this is actually fixed and to be sure it doesn't regress.

With that, I think we could consider this fixed.
Flags: needinfo?(dholbert) → in-testsuite?
Comment on attachment 8985689 [details]
Bug 1296209: A couple of calc() tests.
Attachment #8985689 - Flags: review?(dholbert) → review+
Pushed by
A couple of calc() tests. r=dholbert
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
This is I believe already noted in the compat data for calc():

So I'm marking it dev-doc-complete, but let me know if we need anything else here.
You need to log in before you can comment on or make changes to this bug.