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

Status: RESOLVED FIXED

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

Description

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: 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.

Comment 1

[Mantaroh Yoshinaga[:mantaroh]]

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.

Comment 2

[Mantaroh Yoshinaga[:mantaroh]]

(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\")".

Comment 3

[Brian Birtles (:birtles)]

We can narrow the description when we know if this is limited to paint servers or if it is more general.

Comment 4

[Mantaroh Yoshinaga[:mantaroh]]

(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.

Comment 5

[Mantaroh Yoshinaga[:mantaroh]]

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

Comment 6

[Mantaroh Yoshinaga[:mantaroh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d195637443793e353ac6bc1e89b0bacccb633eec

Comment 7

[Mantaroh Yoshinaga[:mantaroh]]

(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

Comment 8

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1374161 - Clone the url data of paint server.

Review commit: https://reviewboard.mozilla.org/r/154352/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154352/

Comment 9

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1374161 - Support none fallback for Paint server with URL.

Review commit: https://reviewboard.mozilla.org/r/154354/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154354/

Comment 10

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1374161 - Update stylo reftest exception for svg-paint-server with invalid value.

Review commit: https://reviewboard.mozilla.org/r/154356/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154356/

Comment 11

[Mantaroh Yoshinaga[:mantaroh]]

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 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8883456 [details]
Bug 1374161 - Clone the url data of paint server.

https://reviewboard.mozilla.org/r/154352/#review159488

Comment 13

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 15

[Mantaroh Yoshinaga[:mantaroh]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154354/diff/1-2/

Comment 16

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8883458 [details]
Bug 1374161 - Update stylo reftest exception for svg-paint-server with invalid value.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154356/diff/1-2/

Comment 17

[Mantaroh Yoshinaga[:mantaroh]]

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 18

[Mantaroh Yoshinaga[:mantaroh]]

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 19

[Manish Goregaokar [:manishearth]]

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

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

Comment 20

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Servo PR, #17617

Comment 21

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8883458 [details]
Bug 1374161 - Update stylo reftest exception for svg-paint-server with invalid value.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154356/diff/2-3/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b16e500c4f88
Update stylo reftest exception for svg-paint-server with invalid value. r=hiro

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b16e500c4f88