investigate and test handling of '0' in calc() expressions

RESOLVED FIXED

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

({css3})

Trunk
Points:
---

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(3 attachments)

(Assignee)

Description

7 years ago
Since '0' is both a length and a number, our calc() code might do strange things with it.  I should figure out what it does currently, figure out what the right behavior is, and write tests.
(Assignee)

Updated

7 years ago
blocking2.0: --- → betaN+

Comment 1

7 years ago
Created attachment 493470 [details]
multiplication by 0 number and length

I came across one strangeness:

If I multiply anything with the number 0 I get 0;
If I multiply anything with the length 0, calc() breaks.
(Assignee)

Comment 2

7 years ago
I think the current spec is clear, when read carefully, that unitless zero is not allowed to be treated as a length.  I proposed adding a note to make this a bit more obvious:
http://lists.w3.org/Archives/Public/www-style/2010Dec/0478.html

I should audit the code and add tests that we do not allow unitless zero as a length.
(Assignee)

Comment 3

7 years ago
Created attachment 500252 [details] [diff] [review]
patch
Attachment #500252 - Flags: review?(bzbarsky)
(Assignee)

Updated

7 years ago
Whiteboard: [needs review]
Comment on attachment 500252 [details] [diff] [review]
patch

Looks good, assuming calc is the only way VARIANT_NUMBER|VARIANT_LENGTH can end up in ParseVariant...  I wish we had a way to assert that.  :(

Or is the point that SVG does just that in some cases, but then treats "number" as that number of pixels, which for 0 doesn't matter?
Attachment #500252 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 5

7 years ago
SVG does that in a bunch of cases, as does line-height.  But in those cases it seems a good bit more consistent to treat '0' as a number.  I don't think anything other than calc() does it in contexts where the number and length have different meanings at zero.  (But if something else does in the future, I think this is the right change to ParseVariant.)
(Assignee)

Comment 6

7 years ago
Created attachment 500332 [details] [diff] [review]
patch, part 1

It turns out I also need to change some SMIL tests.  The fact that I did is slightly disturbing since it suggests that we can't interpolate between units that are actually supposed to be equivalent.  (I suspect transitions have the same bug.)  

So I'm wondering both (a) if you're ok with these test changes, (b) if we have a bug on that underlying problem, and (c) if we have any idea what other browsers do here (both for animation and for computed style).


I also split the patch in half to make it clearer which test changes are related to which patch.  Part 2 is http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/1900328119c2/calc-unitless-zero
Attachment #500332 - Flags: review?(dholbert)
(Assignee)

Updated

7 years ago
Whiteboard: [needs review] → [needs review][passed try]
Comment on attachment 500332 [details] [diff] [review]
patch, part 1

>diff --git a/content/smil/test/db_smilCSSFromBy.js b/content/smil/test/db_smilCSSFromBy.js
>   lengthPx: [
>-    new AnimTestcaseFromBy("0", "8px", { fromComp: "0px", // 0 acts like 0px
>+    new AnimTestcaseFromBy("0px", "8px", { fromComp: "0px",
>                                          midComp: "4px",
>                                          toComp: "8px"}),

Nit: Fix indentation on the last 2 lines here.

>diff --git a/content/smil/test/db_smilCSSFromTo.js b/content/smil/test/db_smilCSSFromTo.js
>   lengthPx: [
>-    new AnimTestcaseFromTo("0", "12px", { fromComp: "0px",  // 0 acts like 0px
>+    new AnimTestcaseFromTo("0px", "12px", { fromComp: "0px",
>                                           midComp:  "6px"}),

and on the last line here

>-    new AnimTestcaseFromTo("16px", "0", { midComp: "8px",
>-                                          toComp:  "0px"}), // 0 acts like 0px
>+    new AnimTestcaseFromTo("16px", "0px", { midComp: "8px",
>+                                          toComp:  "0px"}),

and here.

(In reply to comment #6)
> So I'm wondering both (a) if you're ok with these test changes,

Yup! The tests are the way they are just because I'm comparing against computed value, and a length of 0 has always resulted in a computed value of 0px, so that's why most of those values are there.

> (b) if we have a bug on that underlying problem

I don't think we do.  Bug 594198 was sort of related -- it was about e.g. <animate from="0" to="20" ...> not working *because* 0 is specially treated as 0px there.

I took a conservative approach in that bug and just added some special handling of '0' values in nsSMILCSSValueType, to make them more tolerant -- see bug 594198 comment 4 in particular.

> (c) if we have any idea what other browsers do here (both for animation and for computed style).

No, I'm not sure.
Attachment #500332 - Flags: review?(dholbert) → review+
(Assignee)

Comment 8

7 years ago
http://hg.mozilla.org/mozilla-central/rev/fe4489fb36ab
http://hg.mozilla.org/mozilla-central/rev/97e0776740d3
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Priority: -- → P2
Resolution: --- → FIXED
Whiteboard: [needs review][passed try]
Target Milestone: --- → mozilla2.0b9
(Assignee)

Comment 9

7 years ago
(And I realize the test changes to property_database.js in the first patch really belonged in the second.)
(Assignee)

Comment 10

7 years ago
I think there is something wrong with SMIL, though, since 5 and 5px should really be the same thing.  I tend to think we should make that change at a lower level in the style system, though, given that it's really odd that computed style differs too.  I'll try to file a bug later...
(In reply to comment #10)
> I think there is something wrong with SMIL, though, since 5 and 5px should
> really be the same thing.

Well, the main problem is that 5px computes to a nsStyleAnimation::Value of type eUnit_Coord, whereas 5 gets type eUnit_Float.  And right now, nsStyleAnimation doesn't allow addition/interpolation/distance-computation between those types.

> I tend to think we should make that change at a
> lower level in the style system, though, given that it's really odd that
> computed style differs too.

I agree.
Priority: P2 → --
Target Milestone: mozilla2.0b9 → ---
You need to log in before you can comment on or make changes to this bug.