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

RESOLVED FIXED in Firefox 62

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: dholbert, Assigned: mats)

Tracking

(Blocks 2 bugs, {dev-doc-complete})

Trunk
mozilla62
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

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

Comment 1

3 years ago
(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,
                 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.
Is this a duplicate of Bug 1264520?
Flags: needinfo?(dholbert)
Reporter

Comment 4

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

Comment 5

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

Comment 6

2 years ago
grid-column and grid-row must also support.
Assignee

Comment 7

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

Comment 10

2 years ago
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.
Flags: needinfo?(dholbert) → in-testsuite?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Reporter

Comment 13

11 months ago
mozreview-review
Comment on attachment 8985689 [details]
Bug 1296209: A couple of calc() tests.

https://reviewboard.mozilla.org/r/251236/#review257648
Attachment #8985689 - Flags: review?(dholbert) → review+

Comment 14

11 months ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/89e5afeea62a
A couple of calc() tests. r=dholbert

Comment 15

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/89e5afeea62a
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
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.
You need to log in before you can comment on or make changes to this bug.