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

Status: RESOLVED FIXED

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

Description

[Daniel Holbert [:dholbert]]

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.)

[1] https://drafts.csswg.org/css-values-3/#calc-notation

Comment 1

[Daniel Holbert [:dholbert]]

(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.)

Comment 2

[Jonathan Chan [:jyc]]

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,
                 SETCOORD_IA | SETCOORD_INITIAL_AUTO | SETCOORD_UNSET_INITIAL,
                 aContext, nullptr, conditions)) {
    if (eCSSUnit_Inherit == zIndexValue->GetUnit()) {
      // handle inherit, because it's ok to inherit 'auto' here
      conditions.SetUncacheable();
      pos->mZIndex = parentPos->mZIndex;
    }
  }

SetCoord has a bunch of code like this:

  ...
  else if (((aMask & SETCOORD_PERCENT) != 0) &&
           (aValue.GetUnit() == eCSSUnit_Percent)) {
    aCoord.SetPercentValue(aValue.GetPercentValue());
  }
  ...

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,
                           aConditions);
    *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.

Comment 3

[Jeremy Chen ([e-mail])]

Is this a duplicate of Bug 1264520?

Comment 4

[Daniel Holbert [: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.

Comment 5

[Daniel Holbert [: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.)

Comment 6

[yisibl]

grid-column and grid-row must also support.

Comment 7

[Mats Palmgren (inactive)]

I have WIP patches for this...

Comment 8

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

(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?)

Comment 10

[Daniel Holbert [:dholbert]]

Sure, sounds like we can mark this as FIXED - though we should add some calc() values to "other_values" arrays in https://dxr.mozilla.org/mozilla-central/source/layout/style/test/property_database.js 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.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1296209: A couple of calc() tests.

There's a bunch of others in WPT already, so I don't think it's worth going
through absolutely all of them...

Review commit: https://reviewboard.mozilla.org/r/251236/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251236/

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8985689 [details]
Bug 1296209: A couple of calc() tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251236/diff/1-2/

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8985689 [details]
Bug 1296209: A couple of calc() tests.

https://reviewboard.mozilla.org/r/251236/#review257648

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/89e5afeea62a
A couple of calc() tests. r=dholbert

Comment 15

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/89e5afeea62a

Comment 16

[Will Bamberg [:wbamberg]]

This is I believe already noted in the compat data for calc():
https://github.com/mdn/browser-compat-data/blob/master/css/types/calc.json#L35
https://developer.mozilla.org/en-US/docs/Web/CSS/calc#Browser_compatibility

So I'm marking it dev-doc-complete, but let me know if we need anything else here.