Closed Bug 1359343 Opened 7 years ago Closed 7 years ago

stylo: enable stylo for SVG documents

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: heycam, Assigned: chenpighead)

References

Details

Attachments

(3 files)

Bug 1359343 - enable stylo for SVG documents.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1359343 - annotate svg related crashtests to enable stylo for SVG documents.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1359343 - annotate svg related reftests to enable stylo for SVG documents.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
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.
No longer blocks: 1355762
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)
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).
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.... :(
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.
(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 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?
(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?
Flags: needinfo?(cam)
Attachment #8864805 - Flags: review?(cam)
Attachment #8864806 - Flags: review?(cam)
Attachment #8865380 - Flags: review?(cam)
No, I didn't get a copy of the log.
Flags: needinfo?(cam)
Comment on attachment 8864805 [details]
Bug 1359343 - enable stylo for SVG documents.

https://reviewboard.mozilla.org/r/136486/#review140398
Attachment #8864805 - Flags: review?(cam) → review+
Comment on attachment 8864806 [details]
Bug 1359343 - annotate svg related crashtests to enable stylo for SVG documents.

https://reviewboard.mozilla.org/r/136488/#review140400
Attachment #8864806 - Flags: review?(cam) → review+
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/
Attachment #8865380 - Flags: review?(cam) → review+
Pushed by jichen@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/69157d41d49f
https://hg.mozilla.org/mozilla-central/rev/b75f0048831d
https://hg.mozilla.org/mozilla-central/rev/5c7b1cecbe78
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Blocks: 1367118
Blocks: 1367327
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: