1359343 - stylo: enable stylo for SVG documents

Status: RESOLVED FIXED

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

Description

[Cameron McCormack (:heycam)]

Splitting this out from bug 1355762, which is for enabling stylo for all non-HTML documents.

Here's a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2ce098bb692c315eb9b3c096754ea06fc596c7e

I plan to enable stylo for SVG documents and annotate the current failures.  See bug 1355762 comment 5 for a summary of the reasons for the failures as of a week ago.

Comment 1

[Cameron McCormack (:heycam)]

Jeremy's going to take over this.  Here's the latest try run I did:

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

which is these two patches:

https://hg.mozilla.org/try/rev/d6e9a1c48810
https://hg.mozilla.org/try/rev/0572de184b03c3d4f4b726568835c7faef418b82

Comment 2

[Jeremy Chen ([e-mail])]

Cameron has done some investigation works and summarized the work in Bug 1355762 comment 5.

Let's move the comment to here:

(In reply to Cameron McCormack (:heycam) from comment #5)
> Here's a summary of the test failures with an updated try run, after bug
> 1342559 landed:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=f716e94dd0f9017093d65a045c940a19ec2fcf2f
> 
> dom/smil/crashtests/690994-1.svg
> layout/reftests/svg/smil/** (150 tests)
> layout/reftests/svg/text/textLength-6.svg
> layout/reftests/svg/mask-type-04.svg
>   lack of SMIL support (bug 1302948)
> 
> layout/reftests/bugs/220165-1.svg
> layout/reftests/bugs/490177-1.svg
> layout/reftests/svg/foreignObject-form-theme.svg
>   button not using native theming (bug 1349651)
> 
> layout/reftests/scoped-style/scoped-style-019.svg
> layout/reftests/scoped-style/scoped-style-dynamic-012.svg
> layout/reftests/scoped-style/scoped-style-dynamic-013.svg
> layout/reftests/scoped-style/scoped-style-dynamic-014.svg
> layout/reftests/scoped-style/scoped-style-dynamic-015.svg
> layout/reftests/scoped-style/scoped-style-dynamic-015.svg
>   lack of <style scoped> support (bug 1345702)
> 
> layout/reftests/svg/sizing/scrollbars-01.svg
> layout/reftests/svg/sizing/scrollbars-02.svg
> layout/reftests/svg/outer-svg-border-and-padding-01.svg
>   scroll bars not working (bug 1321769)
> 
> layout/reftests/svg/text/pseudo-first-line.svg
> layout/reftests/svg/text/pseudo-first-line-2.svg
>   lack of ::first-line support (bug 1324619)
> 
> layout/reftests/svg/moz-only/xbl-grad-ref--grad-in-resources-01.svg
> layout/reftests/svg/moz-only/xbl-grad-ref--grad-in-resources-02.svg
>   something to do with XBL and resolving local url() references (no bug)
> 
> layout/reftests/svg/conditions-07.svg
> layout/reftests/svg/dynamic-conditions-01.svg
> layout/reftests/svg/dynamic-conditions-02.svg
> layout/reftests/svg/dynamic-conditions-03.svg
> layout/reftests/svg/dynamic-conditions-04.svg
> layout/reftests/svg/dynamic-conditions-06.svg
> layout/reftests/svg/dynamic-conditions-09.svg
> layout/reftests/svg/dynamic-conditions-10.svg
> layout/reftests/svg/dynamic-conditions-13.svg
>   dynamic changes to conditional processing attributes aren't handled --
>   maybe we're not correctly generating/processing the ReconstructFrame
>   hint in nsSVGElement::GetAttributeChangeHint?  (no bug)
> 
> layout/reftests/svg/currentColor-01.svg
> layout/reftests/svg/currentColor-02.svg
> layout/reftests/svg/currentColor-03.svg
> layout/reftests/svg/use-02-extref.svg
>   fill="currentColor" isn't working?  (no bug)
> 
> layout/reftests/svg/dynamic-mask-01.svg
>   timed out waiting for MozReftestInvalidate, not sure what the
>   issues is (no bug)
> 
> layout/reftests/svg/dynamic-text-01.svg
> layout/reftests/svg/dynamic-text-02.svg
> layout/reftests/svg/dynamic-text-03.svg
> layout/reftests/svg/dynamic-text-04.svg
> layout/reftests/svg/dynamic-text-07.svg
> layout/reftests/svg/dynamic-text-08.svg
> layout/reftests/svg/dynamic-textPath-01.svg
> layout/reftests/svg/gradient-live-01a.svg
> layout/reftests/svg/gradient-live-01b.svg
> layout/reftests/svg/gradient-live-01c.svg
> layout/reftests/svg/paint-order-02.svg
> layout/reftests/svg/stroke-dasharray-and-text-01.svg
> layout/reftests/svg/text-gradient-02.svg
> layout/reftests/svg/text-in-link-03.svg
> layout/reftests/svg/text-layout-01.svg
> layout/reftests/svg/text-layout-02.svg
> layout/reftests/svg/text-layout-03.svg
> layout/reftests/svg/text-layout-04.svg
> layout/reftests/svg/text-layout-06.svg
> layout/reftests/svg/text-layout-07.svg
> layout/reftests/svg/text-scale-01.svg
> layout/reftests/svg/textPath-01.svg
>   text size wrong, might be related to mFontSizeScaleFactor?  (no bug)
> 
> layout/reftests/svg/fallback-color-02a.svg
> layout/reftests/svg/fallback-color-02b.svg
> layout/reftests/svg/fallback-color-04.svg
>   something with a failing paint server reference and fallback color
>   (no bug)
> 
> layout/reftests/svg/foreignObject-change-transform-01.svg
> layout/reftests/svg/g-transform-01.svg
>   maybe an issue with transform="" attribute updating not causing
>   a restyle?  (no bug)
> 
> layout/reftests/svg/stroke-dasharray-02.svg
>   maybe an issue with stroke-width glue, or accuracy?  (no bug)

Comment 3

[Brian Birtles (:birtles)]

Thanks Jeremy for working on this!

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #2)
> > dom/smil/crashtests/690994-1.svg
> > layout/reftests/svg/smil/** (150 tests)
> > layout/reftests/svg/text/textLength-6.svg
> > layout/reftests/svg/mask-type-04.svg
> >   lack of SMIL support (bug 1302948)

I can take care of these

> > layout/reftests/svg/currentColor-01.svg
> > layout/reftests/svg/currentColor-02.svg
> > layout/reftests/svg/currentColor-03.svg
> > layout/reftests/svg/use-02-extref.svg
> >   fill="currentColor" isn't working?  (no bug)

This might overlap with bug 1345709 somewhat (although that is more about interpolation).

> > layout/reftests/svg/dynamic-text-01.svg
> > layout/reftests/svg/dynamic-text-02.svg
> > layout/reftests/svg/dynamic-text-03.svg
> > layout/reftests/svg/dynamic-text-04.svg
> > layout/reftests/svg/dynamic-text-07.svg
> > layout/reftests/svg/dynamic-text-08.svg
> > layout/reftests/svg/dynamic-textPath-01.svg
> > layout/reftests/svg/gradient-live-01a.svg
> > layout/reftests/svg/gradient-live-01b.svg
> > layout/reftests/svg/gradient-live-01c.svg
> > layout/reftests/svg/paint-order-02.svg
> > layout/reftests/svg/stroke-dasharray-and-text-01.svg
> > layout/reftests/svg/text-gradient-02.svg
> > layout/reftests/svg/text-in-link-03.svg
> > layout/reftests/svg/text-layout-01.svg
> > layout/reftests/svg/text-layout-02.svg
> > layout/reftests/svg/text-layout-03.svg
> > layout/reftests/svg/text-layout-04.svg
> > layout/reftests/svg/text-layout-06.svg
> > layout/reftests/svg/text-layout-07.svg
> > layout/reftests/svg/text-scale-01.svg
> > layout/reftests/svg/textPath-01.svg
> >   text size wrong, might be related to mFontSizeScaleFactor?  (no bug)

Bug 1357296 is probably related (i.e. we need to factor out text-zoom for animation values, but I guess in general too).

Comment 4

[Jeremy Chen ([e-mail])]

Attachment: Bug 1359343 - enable stylo for SVG documents.

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

Comment 5

[Jeremy Chen ([e-mail])]

Attachment: Bug 1359343 - annotate svg related crashtests to enable stylo for SVG documents.

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

Comment 6

[Jeremy Chen ([e-mail])]

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

Looks like the only failure (dynamic-mask-01.svg) is caused by loading timeout, maybe we should annotate it with skip-if(stylo) instead of fails-if(stylo).

The other issue would be the leakcheck failure from the crashtest, no idea why it passes on e10s but fails on non-e10s.... :(

Comment 7

[Jeremy Chen ([e-mail])]

Comment on attachment 8864805 [details]
Bug 1359343 - enable stylo for SVG documents.

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

Comment 8

[Jeremy Chen ([e-mail])]

Comment on attachment 8864806 [details]
Bug 1359343 - annotate svg related crashtests to enable stylo for SVG documents.

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

Comment 9

[Jeremy Chen ([e-mail])]

After skip all the loading timeout tests, reftest all green now \o/
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa135cc7a7f54cb091330516b964581a2ddc479c

Keep working on crashtest.

Comment 10

[Jeremy Chen ([e-mail])]

Attachment: Bug 1359343 - annotate svg related reftests to enable stylo for SVG documents.

Skip tests that use <select> elements in SVG.

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

Comment 11

[Jeremy Chen ([e-mail])]

Comment on attachment 8864805 [details]
Bug 1359343 - enable stylo for SVG documents.

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

Comment 12

[Jeremy Chen ([e-mail])]

Comment on attachment 8864806 [details]
Bug 1359343 - annotate svg related crashtests to enable stylo for SVG documents.

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

Comment 13

[Jeremy Chen ([e-mail])]

(In reply to Brian Birtles (:birtles) from comment #3)
> Thanks Jeremy for working on this!
> 
> (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #2)
> > > dom/smil/crashtests/690994-1.svg
> > > layout/reftests/svg/smil/** (150 tests)

Note that bunch of these tests use "reftest-wait" to help verify the test, which causes test timeout rather than test failures, so I have to mark them as skip-if instead of fails-if. Filed Bug 1363045.

> > > layout/reftests/svg/text/textLength-6.svg
> > > layout/reftests/svg/mask-type-04.svg
> > >   lack of SMIL support (bug 1302948)
> 
> I can take care of these

Comment 14

[Jeremy Chen ([e-mail])]

Comment on attachment 8864805 [details]
Bug 1359343 - enable stylo for SVG documents.

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

Comment 15

[Jeremy Chen ([e-mail])]

Comment on attachment 8864806 [details]
Bug 1359343 - annotate svg related crashtests to enable stylo for SVG documents.

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

Comment 16

[Jeremy Chen ([e-mail])]

Comment on attachment 8865380 [details]
Bug 1359343 - annotate svg related reftests to enable stylo for SVG documents.

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

Comment 17

[u459114]

Comment on attachment 8865380 [details]
Bug 1359343 - annotate svg related reftests to enable stylo for SVG documents.

https://reviewboard.mozilla.org/r/137050/#review140146

::: layout/reftests/scoped-style/reftest.list:44
(Diff revision 2)
>  fails-if(stylo) == scoped-style-dynamic-008.html scoped-style-dynamic-008-ref.html
>  skip-if(stylo) == scoped-style-dynamic-009.html scoped-style-dynamic-009-ref.html
>  asserts-if(stylo,1) == scoped-style-dynamic-010.html scoped-style-dynamic-010-ref.html
>  fails-if(stylo) == scoped-style-dynamic-011.html scoped-style-dynamic-011-ref.html
> -== scoped-style-dynamic-012.svg scoped-style-dynamic-012-ref.svg
> -== scoped-style-dynamic-013.svg scoped-style-dynamic-013-ref.svg
> +asserts-if(stylo,1) == scoped-style-dynamic-012.svg scoped-style-dynamic-012-ref.svg
> +fails-if(stylo) asserts-if(stylo,1) == scoped-style-dynamic-013.svg scoped-style-dynamic-013-ref.svg

do you have a log for this assert failure?

Comment 18

[Jeremy Chen ([e-mail])]

(In reply to C.J. Ku[:cjku](UTC+8) from comment #17)
> Comment on attachment 8865380 [details]
> Bug 1359343 - annotate svg related reftests to enable stylo for SVG
> documents.
> 
> https://reviewboard.mozilla.org/r/137050/#review140146
> 
> ::: layout/reftests/scoped-style/reftest.list:44
> (Diff revision 2)
> >  fails-if(stylo) == scoped-style-dynamic-008.html scoped-style-dynamic-008-ref.html
> >  skip-if(stylo) == scoped-style-dynamic-009.html scoped-style-dynamic-009-ref.html
> >  asserts-if(stylo,1) == scoped-style-dynamic-010.html scoped-style-dynamic-010-ref.html
> >  fails-if(stylo) == scoped-style-dynamic-011.html scoped-style-dynamic-011-ref.html
> > -== scoped-style-dynamic-012.svg scoped-style-dynamic-012-ref.svg
> > -== scoped-style-dynamic-013.svg scoped-style-dynamic-013-ref.svg
> > +asserts-if(stylo,1) == scoped-style-dynamic-012.svg scoped-style-dynamic-012-ref.svg
> > +fails-if(stylo) asserts-if(stylo,1) == scoped-style-dynamic-013.svg scoped-style-dynamic-013-ref.svg
> 
> do you have a log for this assert failure?

No, this is initially added in Cameron's patch (see comment 1). I can't find the log from the try link in comment 0 either.
This test is mentioned at Bug 1355762 comment 5 by Cameron, maybe he still keeps some info/log?

Comment 19

[Jeremy Chen ([e-mail])]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=40111fc012a93a3b5e90f2c6b7b89aa17e16a47c

Ask for review then.

Comment 20

[Cameron McCormack (:heycam)]

No, I didn't get a copy of the log.

Comment 21

[Cameron McCormack (:heycam)]

Comment on attachment 8864805 [details]
Bug 1359343 - enable stylo for SVG documents.

https://reviewboard.mozilla.org/r/136486/#review140398

Comment 22

[Cameron McCormack (:heycam)]

Comment on attachment 8864806 [details]
Bug 1359343 - annotate svg related crashtests to enable stylo for SVG documents.

https://reviewboard.mozilla.org/r/136488/#review140400

Comment 23

[Cameron McCormack (:heycam)]

Comment on attachment 8865380 [details]
Bug 1359343 - annotate svg related reftests to enable stylo for SVG documents.

https://reviewboard.mozilla.org/r/137050/#review140402

::: commit-message-7b07a:3
(Diff revision 2)
> +Bug 1359343 - annotate svg related reftests to enable stylo for SVG documents.
> +
> +Note that bunch of tests in layout/reftests/svg/smil/style/reftest.list are

s/bunch/a bunch/

::: commit-message-7b07a:5
(Diff revision 2)
> +Bug 1359343 - annotate svg related reftests to enable stylo for SVG documents.
> +
> +Note that bunch of tests in layout/reftests/svg/smil/style/reftest.list are
> +annotated as skip-if instead of fails-if. The reason is that the lack of SMIL
> +support seems prevent the tests from removing the "reftest-wait". I've filed

s/seems/seem to/

Comment 24

[Jeremy Chen ([e-mail])]

Comment on attachment 8864805 [details]
Bug 1359343 - enable stylo for SVG documents.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136486/diff/4-5/

Comment 25

[Jeremy Chen ([e-mail])]

Comment on attachment 8864806 [details]
Bug 1359343 - annotate svg related crashtests to enable stylo for SVG documents.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136488/diff/4-5/

Comment 26

[Jeremy Chen ([e-mail])]

Comment on attachment 8865380 [details]
Bug 1359343 - annotate svg related reftests to enable stylo for SVG documents.

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

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/69157d41d49f
enable stylo for SVG documents. r=heycam
https://hg.mozilla.org/integration/autoland/rev/b75f0048831d
annotate svg related crashtests to enable stylo for SVG documents. r=heycam
https://hg.mozilla.org/integration/autoland/rev/5c7b1cecbe78
annotate svg related reftests to enable stylo for SVG documents. r=heycam

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/69157d41d49f
https://hg.mozilla.org/mozilla-central/rev/b75f0048831d
https://hg.mozilla.org/mozilla-central/rev/5c7b1cecbe78