Closed Bug 1374161 Opened 3 years ago Closed 3 years ago

stylo: Handling of invalid values in SMIL animations does not match Gecko in some cases

Categories

(Core :: CSS Parsing and Computation, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: mantaroh, Assigned: mantaroh)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

Attached image test-case.svg
+++ This bug was initially created as a clone of Bug #1371199 +++

layout/reftests/svg/smil/style/anim-css-fill-1-by-ident-hex.svg fails on Stylo.

This test code is as follows:

 var animAttrHash = { "attributeName" : "fill",
                      "by"            : "#AAF573" };
 testAnimatedRectGrid("animate", [animAttrHash]);

Adding some loggingto the code I see:

  Parsed fill property value '#AAF573' => rgb(170, 245, 115)
  Failed to get zero_value

That is, the call to get_zero_value in Servo_AnimationValues_GetZeroValue is producing none.

I guess we need to implement get_zero_value on IntermediateSVGPaint.
Sorry, I submitted uncompleted bug...

The attached test case is a reduced test case from a failing test in layout/reftests/svg/smil/anim-paintserver-1.svg.

----------------------------------------
<svg xmlns="http://www.w3.org/2000/svg">
  <defs>
    <linearGradient id="lime">
      <stop offset="0.0" stop-color="lime"/>
    </linearGradient>
  </defs>
  <rect x="0" y="0" width="100" height="100" fill="url(#lime)">
    <animate attributeName="fill" to="pikapikaglittergold"/>
  </rect>
</svg>
----------------------------------------

We expected that it will display the lime color rectangle, but it is not display anything actually.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #1)
> Sorry, I submitted uncompleted bug...
> 
> The attached test case is a reduced test case from a failing test in
> layout/reftests/svg/smil/anim-paintserver-1.svg.
> 
> ----------------------------------------
> <svg xmlns="http://www.w3.org/2000/svg">
>   <defs>
>     <linearGradient id="lime">
>       <stop offset="0.0" stop-color="lime"/>
>     </linearGradient>
>   </defs>
>   <rect x="0" y="0" width="100" height="100" fill="url(#lime)">
>     <animate attributeName="fill" to="pikapikaglittergold"/>
>   </rect>
> </svg>
> ----------------------------------------
> 
> We expected that it will display the lime color rectangle, but it is not
> display anything actually.

In this test-case, getComputedStyle(rect).fill is 'None' on Stylo, But gecko is "url(\"#lime\")".
Summary: stylo: No zero value returned for SVG paint servers → stylo: The animate property of SVG with not exist 'to' value will change the underlying value.
We can narrow the description when we know if this is limited to paint servers or if it is more general.
Summary: stylo: The animate property of SVG with not exist 'to' value will change the underlying value. → stylo: Handling of invalid values in SMIL animations does not match Gecko in some cases
(In reply to Brian Birtles (:birtles) from comment #3)
> We can narrow the description when we know if this is limited to paint
> servers or if it is more general.

I tried other test case:
------------------------------
<svg xmlns="http://www.w3.org/2000/svg">
  <defs>
    <linearGradient id="lime">
      <stop offset="0.0" stop-color="lime"/>
    </linearGradient>
  </defs>
  <rect x="0" y="0" width="100" height="100" fill="url(#lime)">
    <animate attributeName="fill" to="red" dur="10s"/>
  </rect>
</svg>
-------------------------------

We expect rect color is : lime -> red -> lime. However stylo is: transparent -> red -> lime. If we remove the animate element in this test case, we can render correctly(i.e. displayed lime rectangle).

I'm going to continue investigating this.
Ah, this cause is here [1]:
-------------------------------------------------------------------------------------------------
    pub fn clone_${ident}(&self) -> longhands::${ident}::computed_value::T {
...
        let ref paint = ${get_gecko_property(gecko_ffi_name)};
...
        let kind = match paint.mType {
...
            nsStyleSVGPaintType::eStyleSVGPaintType_Server => {
                // FIXME (bug 1353966) this should animate
                SVGPaintKind::None
            }
...
    }
-------------------------------------------------------------------------------------------------
[1] http://searchfox.org/mozilla-central/rev/a3a739de04ee6134c11546568a33dbb6a6a29907/servo/components/style/properties/gecko.mako.rs#335


Now we can convert url raw data to SpecifiedUrl by using from_url_value_data[2]. I tried using this function into this clone function, and then I confirmted that passing this test case.

[2] http://searchfox.org/mozilla-central/rev/a3a739de04ee6134c11546568a33dbb6a6a29907/servo/components/style/gecko/url.rs#56
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #6)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=d195637443793e353ac6bc1e89b0bacccb633eec

Other test case failed:
------------------------------------------------------------------
  <!-- 12. Fallback color - none -->
  <rect x="200" y="300" width="100" height="100" fill="lime"/>
  <rect x="200" y="300" width="100" height="100" fill="red">
    <set attributeName="fill" to="url(#nonexistant) none"/>
  </rect>
------------------------------------------------------------------

If we specified fallback value to 'none', servo can't parse it since IntermediateSVGPaintServer use IntermediateRGBA[1]. In parsing SVGPaint, I think we need to check ident is 'none' like cssparser[1].

[1] http://searchfox.org/mozilla-central/rev/e8f4f51cd543f203e9cb861cecb7545ac43c836c/third_party/rust/cssparser/src/lib.rs#49
Hiro,

I modified the code related with svg paint server.
First patch will clone the url paint server data. Previously, it was not supported this behavior.
Second patch will support 'none' fallback. Previously, even if we specify the fallback as 'none', servo can't parse it.
Last is removing reftest fail annotation related with paint server.

Could you please review it?
Comment on attachment 8883456 [details]
Bug 1374161 - Clone the url data of paint server.

https://reviewboard.mozilla.org/r/154352/#review159488
Attachment #8883456 - Flags: review?(hikezoe) → review+
Comment on attachment 8883457 [details]
Bug 1374161 - Support none fallback for Paint server with URL.

https://reviewboard.mozilla.org/r/154354/#review159490

This patch should be reviewed by Manish or nox.
Attachment #8883457 - Flags: review?(hikezoe)
Comment on attachment 8883458 [details]
Bug 1374161 - Update stylo reftest exception for svg-paint-server with invalid value.

https://reviewboard.mozilla.org/r/154356/#review159492

::: commit-message-7745b:1
(Diff revision 1)
> +Bug 1374161 - Remove stylo reftest fail annotation of svg-paint-server. r?hiro

Nit: Update stylo reftest expectation for svg-paint-server with invalid value.
Attachment #8883458 - Flags: review?(hikezoe) → review+
Comment on attachment 8883458 [details]
Bug 1374161 - Update stylo reftest exception for svg-paint-server with invalid value.

https://reviewboard.mozilla.org/r/154356/#review159492

Thanks hiro.

I addressed the commit message.
Comment on attachment 8883457 [details]
Bug 1374161 - Support none fallback for Paint server with URL.

https://reviewboard.mozilla.org/r/154354/#review159490

OK.

Manish,
Could you please confirm this patch?

Thanks.
Comment on attachment 8883457 [details]
Bug 1374161 - Support none fallback for Paint server with URL.

https://reviewboard.mozilla.org/r/154354/#review159506
Attachment #8883457 - Flags: review?(manishearth) → review+
Attached file Servo PR, #17617
Attachment #8883456 - Attachment is obsolete: true
Attachment #8883457 - Attachment is obsolete: true
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b16e500c4f88
Update stylo reftest exception for svg-paint-server with invalid value. r=hiro
https://hg.mozilla.org/mozilla-central/rev/b16e500c4f88
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.