Closed Bug 1218257 Opened 9 years ago Closed 6 years ago

CSS Animation of stroke-dashoffset from 0 to 100% does not work

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox44 --- wontfix
firefox67 --- fixed

People

(Reporter: birtles, Assigned: emilio)

References

(Depends on 1 open bug, Blocks 2 open bugs,
URL
)

Details

(Keywords: parity-chrome, Whiteboard: [docs: see comment 3] [webcompat][wptsync upstream])

Attachments

(2 files, 11 obsolete files)

Bug 1218257 - Cleanup and fix interpolation of SVG lengths. r=#style,boris
(deleted), text/x-phabricator-request
Details
Bug 1218257 - Use rust lengths for the SVG lengths. r=#style,boris
(deleted), text/x-phabricator-request
Details
The attached test case contains an animation that animates the stroke-dashoffset property. The keyframes rule looks like: @keyframes trolley-path-move { to { stroke-dashoffset: 100%; } } This doesn't work in Gecko since I guess we fail to animate from the initial '0' value to '100%'. If we update the rule as follows, it works: @keyframes trolley-path-move { from { stroke-dashoffset: 0%; } to { stroke-dashoffset: 100%; } } SVG doesn't define how this property is animated.[1] It should probably be specced as animatable as "length, percentage,or calc".[2] Given that definition, I think this could be expected to work. We already have bug 594933 for making this property support calc(). As a first step, we could probably make our interpolation code here convert '0' to the appropriate unit. I'm pretty sure we do this for attribute types. [1] https://svgwg.org/svg2-draft/painting.html#StrokeDashoffsetProperty [2] https://drafts.csswg.org/css-transitions/#animtype-lpcalc
Does 0px work rather than 0? It seems like it might be an issue with how CSS_PROPERTY_NUMBERS_ARE_PIXELS interacts with eStyleAnimType_Coord.
(In reply to David Baron [:dbaron] ⌚UTC+9 [busy, returning November 2] from comment #1) > Does 0px work rather than 0? No. (For my future reference, this test is available for tweaking at https://codepen.io/tellaho/pen/PPRjbj/)
Ah, the problem is that we don't support calc() for stroke-dashoffset.
Depends on: 594933
Summary: CSS Animation of stroke-offset from 0 to 100% does not work → CSS Animation of stroke-dashoffset from 0 to 100% does not work
Assignee: nobody → boris.chiou
Attachment #8732577 - Attachment is obsolete: true
(In reply to Brian Birtles (:birtles) from comment #0) > @keyframes trolley-path-move { > from { > stroke-dashoffset: 0%; > } > to { > stroke-dashoffset: 100%; > } > } I also did some tests: 1. @keyframes anim { from { stroke-dashoffset: 0; } to { stroke-dashoffset: 100%; } } 2. @keyframes anim { from { stroke-dashoffset: 0%; } to { stroke-dashoffset: 100; } } Both 1 and 2 are failed, but if the unit is the same, (e.g. 0 and 100, or 0% and 100%), it works. stroke-width also has this problem.
When building the segment, we have to interpolate the values of keyframes [1], I found the GetCommonUnit always returns StyleAnimationValue::eUnit_Null [2] because we don't support CSS_PROPERTY_STORE_CALC for stoke-dashoffset. In this case: @keyframes trolley-path-move { to { stroke-dashoffset: 100%; } } The first unit in GetCommonUnit() is "StyleAnimationValue::eUnit_Float" (i.e. 0 or 0px), and the second unit is "StyleAnimationValue::eUnit_Percent" (i.e. 100%). Therefore, BuildSegment() returns false in this case. [1] https://dxr.mozilla.org/mozilla-central/rev/ea6298e1b4f7e22ce2311b2b6a918822f0adb112/layout/style/nsAnimationManager.cpp#853 [2] https://dxr.mozilla.org/mozilla-central/rev/ea6298e1b4f7e22ce2311b2b6a918822f0adb112/layout/style/StyleAnimationValue.cpp#59
Depends on: 556441
Keywords: dev-doc-needed
Whiteboard: [parity-chrome] → [parity-chrome][docs: see comment 3]
Status: NEW → ASSIGNED
Attachment #8734676 - Attachment is obsolete: true
Generalize SetLineHeightCalcOps, so we can use it for other attributes, such as stroke-dashoffset and stroke-width.
Attachment #8734675 - Attachment is obsolete: true
Attached patch Part 2: Remove SetLineHeightCalcOps (obsolete) (deleted) — — Splinter Review
Use the generalized CaclOps for line-height.
Attached patch Part 3: Support calc for stroke-dashoffset (obsolete) (deleted) — — Splinter Review
Use new CalcOps to calculate stroke-dashoffset and store it as nsStyleCoord::Calc. We delay the calculation of this nsStyleCoord::Calc until SVGContentUtils::GetStrokeDashData().
Attachment #8735363 - Flags: feedback?(mtseng)
Attachment #8735364 - Flags: feedback?(mtseng)
Hi Morris, I added some revisions about line-height in Part 1 and part 2. Could you please take a look at these two parts? Thanks.
We set CSS_PROPERTY_NUMBERS_ARE_PIXELS for stroke-dashoffset, so its unit could be eUnit_Percent or eUnit_Float during interpolation. Therefore, we have to add CSS_PROPERTY_STORES_CALC and let GetCommonUnit support this case.
Attachment #8735380 - Attachment is obsolete: true
Attachment #8735363 - Flags: feedback?(mtseng) → review?(dbaron)
Attachment #8735364 - Flags: feedback?(mtseng) → review?(dbaron)
Attachment #8735365 - Flags: review?(dbaron)
Attachment #8735366 - Flags: review?(dbaron)
Attachment #8735381 - Flags: review?(bbirtles)
Attachment #8735382 - Flags: review?(bbirtles)
Hi, David, Brian, Part 1 ~ Part 4: Implement calc() on stroke-dashoffset (because we didn't implement it in bug 594933). Part 5 & Part 6: Fix this animation bug. Thanks.
Comment on attachment 8735381 [details] [diff] [review] Part 5: Fix interpolation between pixel and percent on stroke-dashoffset (v2) Review of attachment 8735381 [details] [diff] [review]: ----------------------------------------------------------------- This seems fine to me but dbaron knows this code better than I do (and is already looking at the other patches in this bug).
Attachment #8735381 - Flags: review?(bbirtles) → review?(dbaron)
Comment on attachment 8735382 [details] [diff] [review] Part 6: Add tests for animations on stroke-dashoffset Review of attachment 8735382 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/test/test_bug1218257.html @@ +19,5 @@ > +.infiniteRun { > + animation: animWithoutFrom 5s linear infinite; > +} > +.run { > + animation: anim 20s linear forwards; I don't think we need classes just to trigger animations. We should just write: div.style.animation = "anim 20s linear forwards"; @@ +39,5 @@ > +function testInterpolation() { > + test(function() { > + var target = document.getElementById("target"); > + target.classList.add("infiniteRun"); > + assert_equals(document.getAnimations().length, 1, "Active animation"); What is this testing? I'm pretty sure that even before the patches for this bug, we would still get an animation, right? I think we can drop this first test? @@ +66,5 @@ > + }, "Computed value of stroke-dashoffset"); > +} > + > +SpecialPowers.pushPrefEnv( > + { "set": [["dom.animations-api.core.enabled", true]] }, testInterpolation); I don't think this works--or at least not completely: we won't have the animation-related interfaces on the global. We should just do what we do for other tests in this directory that need the animations API: use a test_... wrapper that sets the pref and then load the test in an iframe (prefixed file_...). Better still, just pause the animation and use a negative delay to seek it---then you don't need the API at all.
Attachment #8735382 - Flags: review?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #22) > What is this testing? I'm pretty sure that even before the patches for this > bug, we would still get an animation, right? > > I think we can drop this first test? Yes. You're right. I thought it'd fail before my patch. I just tested this and got an animation, so I should drop this. Thanks. > I don't think this works--or at least not completely: we won't have the > animation-related interfaces on the global. We should just do what we do for > other tests in this directory that need the animations API: use a test_... > wrapper that sets the pref and then load the test in an iframe (prefixed > file_...). Better still, just pause the animation and use a negative delay > to seek it---then you don't need the API at all. Got it. Let me try this. Thanks.
Attachment #8735382 - Attachment is obsolete: true
Attachment #8735730 - Flags: review?(bbirtles)
Comment on attachment 8735730 [details] [diff] [review] Part 6: Add tests for animations on stroke-dashoffset (v2) Review of attachment 8735730 [details] [diff] [review]: ----------------------------------------------------------------- This is fine, but shouldn't we also update this: https://dxr.mozilla.org/mozilla-central/rev/63be002b4a803df1122823841ef7633b7561d873/layout/style/test/test_transitions_per_property.html#220 In fact, maybe that's all we need? If we keep this file, should we rename it to something more descriptive? ::: layout/style/test/test_bug1218257.html @@ +20,5 @@ > + } > +</style> > +</head> > +<body> > +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1218157">Mozilla Bug1218157</a> Nit: space between Bug and the bug number. @@ +41,5 @@ > + assert_equals(window.getComputedStyle(target).strokeDashoffset, > + "5px", "Computed value at beginning"); > + > + target.style.animation = "anim 20s linear -2s forwards"; > + target.style.animationPlayState = "paused"; You can just use: target.style.animationDelay = "-2s"; No need to re-specify the animation name etc. or re-pause it. @@ +46,5 @@ > + assert_equals(window.getComputedStyle(target).strokeDashoffset, > + "calc(4.5px + 10%)", "Computed value at 1/10 of duration"); > + > + target.style.animation = "anim 20s linear -10s forwards"; > + target.style.animationPlayState = "paused"; Same here. @@ +51,5 @@ > + assert_equals(window.getComputedStyle(target).strokeDashoffset, > + "calc(2.5px + 50%)", "Computed value at half of duration"); > + > + target.style.animation = "anim 20s linear -20s forwards"; > + target.style.animationPlayState = "paused"; And here.
Attachment #8735730 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #25) > https://dxr.mozilla.org/mozilla-central/rev/ > 63be002b4a803df1122823841ef7633b7561d873/layout/style/test/ > test_transitions_per_property.html#220 > > In fact, maybe that's all we need? After checking its implementation, I think yes. However, I have a problem about "cs.getPropertyValue(prop)" [1]. It always returns a string whose format is "calc(Apx + B%)" on stroke-dashoffset (also in the test in Part 6), and for other properties, such as line-height and margin-left, the format is "Apx". any_unit_to_num() in [1] cannot convert "calc(xxx)" string into a valid number, so the tests are failed on stroke-dashoffset. I'm not sure if it is a bug. [1] https://dxr.mozilla.org/mozilla-central/rev/63be002b4a803df1122823841ef7633b7561d873/layout/style/test/test_transitions_per_property.html#1070 > If we keep this file, should we rename it to something more descriptive? If the "calc(xxx)" property value is acceptable, I think we should keep this file (Part 6) because test_transitions_per_property.html might skip this case. I'd like to rename it to "test_animation_interpolation_between_different_units.html"
(In reply to Boris Chiou [:boris] from comment #26) > If the "calc(xxx)" property value is acceptable, I think we should keep this > file (Part 6) because test_transitions_per_property.html might skip this > case. I'd like to rename it to > "test_animation_interpolation_between_different_units.html" This name doesn't mention stroke-dashoffset, so I revise it to "test_animations_interpolation_stroke-dashoffset.html". Thanks.
I probably won't have time to look at the patches until Monday, but I'm curious what approach you took to dealing with the issue in bug 1258270 comment 2.
(In reply to David Baron [:dbaron] ⌚️UTC+1 (less responsive until April 4) from comment #28) > I probably won't have time to look at the patches until Monday, but I'm > curious what approach you took to dealing with the issue in bug 1258270 > comment 2. Thanks, David. For the first comment: > the calc() code for these properties needs to treat <number> values as pixel lengths, but only in the appropriate places. I'd like to give some examples for discussion: e.g. 1. calc(5 * 2 + 3) = calc(13) = 13px 2. calc(5 * 2 + 3px) = calc(10 + 3px) = 13px For the case 1, I believe we could handle this in my patches because I will set the number as Factor into a nsStyleCoord and SVGContentUtils::CoordToFloat treats it as CSS pixel. For the case 2, the parser cannot parse this and I didn't handle this case in my current patches. I have to check this case. For the second comment: > there would be a stored calc() in the style structs. So it's possible this transformation could happen relatively late. Yes, so I store them as nsStyleUnit_Calc, and calculate the result until SVGContentUtils::GetStrokeDashData().
(In reply to Boris Chiou [:boris] from comment #30) > For the first comment: > > the calc() code for these properties needs to treat <number> values as pixel lengths, but only in the appropriate places. > I'd like to give some examples for discussion: > e.g. > 1. calc(5 * 2 + 3) = calc(13) = 13px > 2. calc(5 * 2 + 3px) = calc(10 + 3px) = 13px > > For the case 1, I believe we could handle this in my patches because I will > set the number as Factor into a nsStyleCoord and > SVGContentUtils::CoordToFloat treats it as CSS pixel. > For the case 2, the parser cannot parse this and I didn't handle this case > in my current patches. I have to check this case. Oops. I think I misunderstand your meaning. You already said we could skip the case 2 in Bug 594933 Comment 31, so I think you mean the case 1, right? We should treat "a" raw number as a pixel value in calc(). In my implementation, for case 1, I set the raw number as a |Factor| and let SVGContentUtils::CoordToFloat() to handle it [1]. Does it make sense? In other cases, I store them as a nsStyleUnit_Calc value, and also let SVGContentUtils::CoordToFloat() to convert the nsStyleUnitCalc value into a valid float value (delaying the calculation). Therefore, if we try to use getPropertyValue('stroke-dashoffset'), the returned string is a calc string, "calc(...)", instead of a pixel string, "...px". [1] https://dxr.mozilla.org/mozilla-central/rev/494289c72ba3997183e7b5beaca3e0447ecaf96d/dom/svg/SVGContentUtils.cpp#826
(In reply to Boris Chiou [:boris] from comment #30) > I'd like to give some examples for discussion: > e.g. > 1. calc(5 * 2 + 3) = calc(13) = 13px > 2. calc(5 * 2 + 3px) = calc(10 + 3px) = 13px > > For the case 1, I believe we could handle this in my patches because I will > set the number as Factor into a nsStyleCoord and > SVGContentUtils::CoordToFloat treats it as CSS pixel. > For the case 2, the parser cannot parse this and I didn't handle this case > in my current patches. I have to check this case. So it sounds like the patches won't fix the original bug reported in comment 0, then. Or will they? It seems like doing this properly will require the changes that I initially wanted in bug 594933 comment 28 and 29. (It's not clear to me that this is worth the effort of doing now, though.)
(In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #32) > So it sounds like the patches won't fix the original bug reported in comment > 0, then. Or will they? They will. Comment 0 is different from bug 594933 comment 28 and 29. The parser treats Comment 0 as two individual values (number and percent), and we convert the number into pixel implicitly, so interpolation can calculate calc(0px + 100%) in comment 0. In order to fix the problem in bug 594933 comment 28 and 29, we have to fix the calc parser (e.g. by adding a unit checker), but bug 594933 said the spec doesn't allow this case, so I didn't do the revision on the calc parser. If it is necessary, I could file another bug to fix the parser problem. For this bug and bug 1258270, could we just implement calc according to current spec? > > It seems like doing this properly will require the changes that I initially > wanted in bug 594933 comment 28 and 29. (It's not clear to me that this is > worth the effort of doing now, though.) Maybe we could file a bug to track it because I need to spend some time figuring out how the calc parser does and make sure how to do unit checker in calc parser.
So how do you know which numbers to convert to pixels and which numbers not to? For example, if you have: calc(3 * 2px) how do you know not to convert the "3" to "3px", which will then cause the expression to be invalid? Or is what you're doing here using a value as an intermediate value in animations that we don't actually support in written CSS?
(In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #34) > So how do you know which numbers to convert to pixels and which numbers not > to? For example, if you have: > > calc(3 * 2px) > > how do you know not to convert the "3" to "3px", which will then cause the > expression to be invalid? Sorry, my comment is not clear enough. I didn't do anything to the calc() parser, so if we have "calc(3 * 2px)", we parse it by the original calc() parser which returns a nsCSSValue::Array, a number "2" & a pixel "2px", so in this phrase, I still keep the number as number and don't do any conversion for it. While computing the value, I reuse(rewrite) the CalcOps we added in bug 594933, which supports number, length, and percent [1], so calc(3 * 2px) = calc(6px), which is then stored as eStyleUnit_Calc into nsStyleSVG. In addition, if I have calc(3 * 2), we parse it as "number * number", and then compute it by the CalcOps in [1], so get calc(3 * 2) = calc(6) = 6, and then set this number into nsStyleSVG as eStyleUnit_Factor, and finally convert it into pixel in SVGContentUtils::SetCoord() [2]. [1] https://dxr.mozilla.org/mozilla-central/rev/68c0b7d6f16ce5bb023e08050102b5f2fe4aacd8/layout/style/nsRuleNode.cpp#4263 [2] https://dxr.mozilla.org/mozilla-central/rev/68c0b7d6f16ce5bb023e08050102b5f2fe4aacd8/dom/svg/SVGContentUtils.cpp#828 > > Or is what you're doing here using a value as an intermediate value in > animations that we don't actually support in written CSS? If the keyframes is @keyframes { from {stroke-dashoffset: calc(3 * 2px);} to {stroke-dashoffset: 100%;} } For animation case, the intermediate value of the interpolation is between calc(6px) and 100%, and it is stored as StyleAnimationValue::eUnit_Calc. Is it accepted? If we call getPropertyValue("stroke-dashoffset"), I think we'd get a DOMString from "calc(6px + 0%)" to "calc(0px + 100%)" during the animation.
The animation for the case in comment 0 would be an animation from: stroke-dashoffset: 0 to: stroke-dashoffset: 100% At 25% of the way through that animation, the intermediate value would be: stroke-dashoffset: calc(0.75 * 0 + 0.25 * 100%); How does a value like that work with your patch?
(In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #36) > The animation for the case in comment 0 would be an animation from: > stroke-dashoffset: 0 > to: > stroke-dashoffset: 100% > > At 25% of the way through that animation, the intermediate value would be: > stroke-dashoffset: calc(0.75 * 0 + 0.25 * 100%); > > How does a value like that work with your patch? I dump the intermediate values in StyleAnimationValue::AddWeighted and the values of getComputedStyle with my patches: My output in AddWeighted (eUnit_Calc): 1. 0%: 1.0 * 0 + 0.0 * 100% == 0px 2. 10%: 0.9 * 0 + 0.1 * 100% == calc(0px + 10%) 3. 25%: 0.75 * 0 + 0.25 * 100% == calc(0px + 25%) 4. 50%: 0.5 * 0 + 0.5 * 100% == calc(0px + 50%) 5. 100%: 0.0 * 0 + 1.0 * 100% == calc(0px + 100%) My output by getComputedStyle(target).strokeDashoffset 1. 0%: getComputedStyle(target).strokeDashoffset = "0px" 2. 10%: getComputedStyle(target).strokeDashoffset = "calc(0px + 10%)" 3. 25%: getComputedStyle(target).strokeDashoffset = "calc(0px + 25%)" 4. 50%: getComputedStyle(target).strokeDashoffset = "calc(0px + 50%)" 5. 100%: getComputedStyle(target).strokeDashoffset = "calc(0px + 100%)" Therefore, I think the intermediate value is what you think, right?
Would those values be accepted by the parser if they had "0" rather than "0px"?
(In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #38) > Would those values be accepted by the parser if they had "0" rather than > "0px"? For the (calc) parser [1], we *don't* accept calc(0 + 100%). We only accept calc(0px + 100%). [1] https://dxr.mozilla.org/mozilla-central/rev/e847cfcb315f511f4928b03fd47dcf57aad05e1e/layout/style/nsCSSParser.cpp#13058 In conclusion, let see some examples: 1. [Accepted] .path { stroke-dashoffset: calc(0px + 100%); } 2. [Accepted] .path { stroke-dashoffset: calc(1 + 2); } 3. [Not Accepted] .path { stroke-dashoffset: calc(0 + 100%); // nsCSSParser::ParseCalc() doesn't support this case. I can file a bug to track this issue. } ------------------- 4. [Accepted] @keyframes anim { from { stroke-dashoffset: 0; } to { stroke-dashoffset: 100%; } } 5. [Accepted] @keyframes anim { from { stroke-dashoffset: 0px; } to { stroke-dashoffset: 100%; } } 6. [Accepted] @keyframes anim { from { stroke-dashoffset: 0%; } to { stroke-dashoffset: 100%; } } 7. [Accepted] @keyframes anim { to { stroke-dashoffset: calc(0px + 100%); } } 8. [Not Accepted] @keyframes anim { to { stroke-dashoffset: calc(0 + 100%); } }
Blocks: 1264520
(In reply to Boris Chiou [:boris] from comment #39) > (In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #38) > > Would those values be accepted by the parser if they had "0" rather than > > "0px"? > > For the (calc) parser [1], we *don't* accept calc(0 + 100%). We only accept > calc(0px + 100%). OK, so the answer to my question at the end of comment 34 is "yes". I don't think that's an acceptable approach. Animation shouldn't be able to specify values that can't be specified directly. If we want to be able to do this, we should support specifying it directly.
(In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #40) > OK, so the answer to my question at the end of comment 34 is "yes". > > I don't think that's an acceptable approach. Animation shouldn't be able to > specify values that can't be specified directly. If we want to be able to > do this, we should support specifying it directly. OK. So do you mean we have to let the ParseCalc() also supports expressions like "calc(0 + 100%)", which bug 1264520 will do, before fixing this bug?
(In reply to Boris Chiou [:boris] from comment #41) > OK. So do you mean we have to let the ParseCalc() also supports expressions > like "calc(0 + 100%)", which bug 1264520 will do, before fixing this bug? Yes.
Attachment #8735363 - Flags: review?(dbaron)
Attachment #8735364 - Flags: review?(dbaron)
Attachment #8735365 - Flags: review?(dbaron)
Attachment #8735366 - Flags: review?(dbaron)
Attachment #8735381 - Flags: review?(dbaron)
I also think that once that is done, substantially less code should be needed in this bug.
(In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #43) > I also think that once that is done, substantially less code should be > needed in this bug. Thanks for your review, David. Although this bug depends on bug 1264520, Bug 1258270 only wants to support basic calc() feature for stroke-width and stroke-array. Does it need to depend on bug 1264520 as well? If not. I think I can also move the basic calc() implementation of stroke-dashoffset into bug 1258270, and fix bug 1258270 first. Thanks.
Status: ASSIGNED → NEW
No longer blocks: 1264520
Depends on: 1264520
(In reply to Boris Chiou [:boris] from comment #44) > (In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #43) > > I also think that once that is done, substantially less code should be > > needed in this bug. > > Thanks for your review, David. Although this bug depends on bug 1264520, Bug > 1258270 only wants to support basic calc() feature for stroke-width and > stroke-array. Does it need to depend on bug 1264520 as well? If not. I think > I can also move the basic calc() implementation of stroke-dashoffset into > bug 1258270, and fix bug 1258270 first. I think that's ok. It would mean supporting calc() for these three properties only in the same partial way that it's supported for line-height. It still requires figuring out how to address bug 1258270 comment 2 in a way that's both reasonable *and* extends reasonably to what happens once we also fix bug 1264520.
I'm working on other bugs in this quarter, so remove the assignee. Please feel free to take this if you are interested in this bug.
Assignee: boris.chiou → nobody
dbaron and I discussed this bug in the afternoon, and he suggests we can check how blink (Chrome) and webkit (safari) support this, i.e. dashoffset, on CSS Animation but not on calc(...). And then we may know how to fix this bug. (By fixing the parser first or other ways?)
Priority: -- → P3
Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)
Keywords: parity-chrome
Whiteboard: [parity-chrome][docs: see comment 3] → [docs: see comment 3]

This is also hitting https://shecodes.tech/, which uses the same basic animation pattern:

  .animatedSVGElements {
    stroke-dasharray: 6%, 29%;
    stroke-dashoffset: 0;
    animation: 5.5s linear 0s infinite normal none running stroke-offset;
  }
  @keyframes stroke-offset {
    100% {
      stroke-dashoffset:-35%
    }
  }
Flags: webcompat?
See Also: → https://webcompat.com/issues/26542
Whiteboard: [docs: see comment 3] → [docs: see comment 3] [webcompat]

I guess this is:

https://searchfox.org/mozilla-central/rev/00f3836a87b844b5e4bc82f698c559b9966e4be2/servo/components/style/values/animated/svg.rs#29

I'm confused about what the SVG code is doing. It's interpretting numbers specially, but then in practice it treats it as if they were CSS pixels:

https://searchfox.org/mozilla-central/rev/00f3836a87b844b5e4bc82f698c559b9966e4be2/dom/svg/SVGContentUtils.cpp#774

Is there any reason it can't just use a plain LengthPercentage, with the unitless length quirk stuff? Then all this stuff will just work.

Flags: needinfo?(boris.chiou)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #50)

Is there any reason it can't just use a plain LengthPercentage, with the unitless length quirk stuff? Then all this stuff will just work.

I guess "unitless length quirk" was not implemented well when we worked on SVGLengthPercentageOrNumber, so Mantaroh introduced this type. That means, it's worth to try unitless length.

Let me try this in this bug. :)

Flags: needinfo?(boris.chiou)
Assignee: nobody → boris.chiou
Attachment #8735363 - Attachment is obsolete: true
Attachment #8735364 - Attachment is obsolete: true
Attachment #8735365 - Attachment is obsolete: true
Attachment #8735381 - Attachment is obsolete: true

Per IRC discussion I'm on it already, let me steal it.

Assignee: boris.chiou → emilio
Attachment #8735366 - Attachment is obsolete: true
Attachment #8735730 - Attachment is obsolete: true

Instead of storing them as LengthPercentage | Number, always store as
LengthPercentage, and use the unitless length quirk to parse numbers instead.

Further cleanups to use the rust representation can happen as a followup, which
will also get rid of the boolean argument (since we can poke at the rust length
itself). That's why I didn't bother to convert it to an enum class yet.

As it turns out we need this to avoid losing precision both during painting and
during serialization.

This patch also changes to serialize context-value if it's the computed value.

I could keep the previous behavior, but it makes no sense to serialize the
initial value. We're the only ones to support this value anyway, and I couldn't
find a definition or spec for this.

Also update tests and expectations for:

  • New unexpected passes.
  • Always serializing the unit in getComputedStyle.
  • Calc and interpolation support.

Chrome also always serializes the unit in getComputedStyle, so I'm pretty sure
this is compatible with them. Chrome is inconsistent and keeps numbers in
specified style, but that's inconsistent with itself and with other quirky
lengths, so I updated the tests instead.

Pushed by emilio@crisal.io: https://hg.mozilla.org/integration/autoland/rev/0c76d78aca48 Cleanup and fix interpolation of SVG lengths. r=boris https://hg.mozilla.org/integration/autoland/rev/0456bc2c98e2 Use rust lengths for the SVG lengths. r=boris
Pushed by emilio@crisal.io: https://hg.mozilla.org/integration/autoland/rev/16ebe5e32775 Use a different property other than word-spacing for a test. r=me

https://hg.mozilla.org/mozilla-central/rev/0c76d78aca48
https://hg.mozilla.org/mozilla-central/rev/0456bc2c98e2
https://hg.mozilla.org/mozilla-central/rev/16ebe5e32775

Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15716 for changes under testing/web-platform/tests
Whiteboard: [docs: see comment 3] [webcompat] → [docs: see comment 3] [webcompat][wptsync upstream]

This is more of a bug than a feature, so I don't think this needs specific documentation.

Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: