Closed Bug 595648 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: dbaron, Assigned: dbaron)

Details

(Keywords: css3)

Attachments

(3 files)

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.
blocking2.0: --- → betaN+
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.
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.
Attached patch patchSplinter Review
Attachment #500252 - Flags: review?(bzbarsky)
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+
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.)
Attached patch patch, part 1Splinter Review
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)
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+
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Priority: -- → P2
Resolution: --- → FIXED
Whiteboard: [needs review][passed try]
Target Milestone: --- → mozilla2.0b9
(And I realize the test changes to property_database.js in the first patch really belonged in the second.)
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.

Attachment

General

Creator:
Created:
Updated:
Size: