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)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: dbaron, Assigned: dbaron)
Details
(Keywords: css3)
Attachments
(3 files)
638 bytes,
text/html
|
Details | |
13.90 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
16.37 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
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•14 years ago
|
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.
Assignee | ||
Comment 2•14 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•14 years ago
|
||
Attachment #500252 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•14 years ago
|
Whiteboard: [needs review]
Comment 4•14 years ago
|
||
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•14 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•14 years ago
|
||
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•14 years ago
|
Whiteboard: [needs review] → [needs review][passed try]
Comment 7•14 years ago
|
||
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•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/fe4489fb36ab
http://hg.mozilla.org/mozilla-central/rev/97e0776740d3
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Priority: -- → P2
Resolution: --- → FIXED
Whiteboard: [needs review][passed try]
Target Milestone: --- → mozilla2.0b9
Assignee | ||
Comment 9•14 years ago
|
||
(And I realize the test changes to property_database.js in the first patch really belonged in the second.)
Assignee | ||
Comment 10•14 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...
Comment 11•14 years ago
|
||
(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.
Description
•