stylo: support context values for SVG stroke-* and fill-*

ASSIGNED
Assigned to

Status

()

Core
CSS Parsing and Computation
P1
normal
ASSIGNED
6 months ago
9 days ago

People

(Reporter: manishearth, Assigned: xidorn)

Tracking

(Depends on: 2 bugs, Blocks: 3 bugs)

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

6 months ago
Many of the stroke-* and fill-* properties affect mContextFlags in nsStyleSVG when they are set to a context value. We should make these properties deal with that.

http://searchfox.org/mozilla-central/rev/d3307f19d5dac31d7d36fc206b00b686de82eee4/layout/style/nsStyleStruct.h#3714 contains a list of properties affected by this.
Priority: -- → P2
Duplicate of this bug: 1339349
Blocks: 1352284

Updated

4 months ago
Assignee: nobody → cku

Comment 2

3 months ago
Hi Manish
I verify this property by
1. Turn on svg.context-properties.content.enabled in about:config
2. Visit context-fill-03.html on stylo-build.

I can see fill-opacity="context-fill" be represented correctly on stylo build. Could you share me an example that context-xxx not work?
Flags: needinfo?(manishearth)
(Reporter)

Comment 3

3 months ago
We parse the context values for fill and stroke, but not for fill-opacity (and the other similar properties).

See the callers of https://dxr.mozilla.org/mozilla-central/rev/c0ea5ed7f91a6be996a4a3c5ab25e2cdf6b4377e/layout/style/nsRuleNode.cpp#9393

We may not have tests for this; I'm not exactly sure what the expected behavior is here. A quick grep doesn't turn up anything relevant.
Flags: needinfo?(manishearth)
Comment hidden (spam)
Comment hidden (spam)

Updated

3 months ago
Flags: needinfo?(jwatt)
Comment hidden (mozreview-request)

Comment 7

3 months ago
Hi jwatt
We allow user to set fill-opacity as "context-fill-opcity', but we actually does not enable it while rendering
http://searchfox.org/mozilla-central/source/layout/svg/SVGContextPaint.h#282

Support context-{fill|stroke}-opacity value in stylo is not that difficult(you may refert to the path in this bug), but I am wondering do we really need to do it? Or, it's a dead code already?
Flags: needinfo?(jwatt)
Comment hidden (mozreview-request)
(In reply to Manish Goregaokar [:manishearth] from comment #3)
> We may not have tests for this; I'm not exactly sure what the expected
> behavior is here. A quick grep doesn't turn up anything relevant.

We have tests in:

https://dxr.mozilla.org/mozilla-central/source/layout/reftests/text-svgglyphs/resources/glyphs-objectopacity.svg
Flags: needinfo?(jwatt)

Updated

2 months ago
Duplicate of this bug: 1368380

Updated

2 months ago
Assignee: cku → kuoe0
(Assignee)

Updated

2 months ago
Blocks: 1356087
Issue link on servo: https://github.com/servo/servo/issues/15211
(Assignee)

Comment 12

2 months ago
Note that we probably need to support switching those values on and off via pref gfx.font_rendering.opentype_svg.enabled, and we have test checking that this pref actually works.
(Assignee)

Updated

2 months ago
Duplicate of this bug: 1355412
Hi Xidorn, after some investigation, I don't know how to test this bug.

I tried to open the following test cases:

- layout/reftests/text-svgglyphs/svg-glyph-objectopacity.svg
- layout/reftests/text-svgglyphs/svg-glyph-objectopacity2.svg

And got the same result for gecko and stylo.

I also tried the another test case "layout/reftests/svg/as-image/context-fill-04.html" with turning on "svg.cont the ext-properties.content.enabled".

The opacity for gecko and stylo are correct. But, the color between them is different. It should be green, and we got red on stylo. I think it should be something wrong on "context-fill" not "context-fill-opacity."
Flags: needinfo?(xidorn+moz)
I filed another bug (Bug 1370797) to focus on the wrong color issue for context-fill/stroke.
(Assignee)

Comment 16

2 months ago
I'm not completely sure, but I would suspect that we haven't been using stylo for SVG images inside fonts, and thus the first two tests pass in Stylo while the later doesn't.

birtles may know more about our status of SVG support in stylo.
Flags: needinfo?(xidorn+moz) → needinfo?(bbirtles)
That sounds plausible to me--I'm afraid I don't know any more than you do.
Flags: needinfo?(bbirtles)
(Assignee)

Comment 18

2 months ago
Investigating the code a bit, it seems to me that we indeed haven't enabled stylo for SVG-in-OpenType. The reason is that the SVG document for SVG-in-OpenType is in [1], and it doesn't seem to call into nsIDocument::SetContainer at all. And the SVG document is no longer mutated after being created, so the SVG document seems to never have a document container.

A document without container means it wouldn't pass the check for enabling stylo anyway [2], because it seems stylo always requires document container somehow.


[1] https://dxr.mozilla.org/mozilla-central/rev/a49112c7a5765802096b3fc298069b9495436107/gfx/thebes/gfxSVGGlyphs.cpp#345-426
[2] https://dxr.mozilla.org/mozilla-central/rev/a49112c7a5765802096b3fc298069b9495436107/dom/base/nsDocument.cpp#13162-13169
(Assignee)

Comment 19

2 months ago
Filed bug 1371187.

So I think the context-* values do not work at all as expected. The svg-glyph reftests don't fail simply because SVG there doesn't use stylo for styling.

To verify that this bug is fixed properly, we need to ensure that the svg/as-image/context-* reftests pass, and corresponding checks in mochitest get fixed [1].


[1] https://dxr.mozilla.org/mozilla-central/rev/a49112c7a5765802096b3fc298069b9495436107/layout/style/test/stylo-failures.md#81-83

Updated

27 days ago
Status: NEW → ASSIGNED
Priority: P2 → P1
(Assignee)

Comment 20

11 days ago
Stealing this. This is the last failure in test_value_storage.html (after bug 1380918 lands), and I have some thought about how this should be fixed.
Assignee: kuoe0 → xidorn+moz
(Assignee)

Comment 21

10 days ago
It seems there is some work happening in bug 1369614 which touches some properties we need to touch here as well. Will hold on a bit and see what things would look like after that lands.
Depends on: 1369614
(Assignee)

Comment 22

9 days ago
Oh, and the context values are behind the pref gfx.font_rendering.opentype_svg.enabled, which means we need bug 1366956 to land first as well.
Depends on: 1366956
Hi Xidorn, I'm working on Bug 1377158. Because lack of the support of context-*-opacity, I'll turn some test cases off in Bug 1377158. Maybe you should turn them on in this bug.
(Assignee)

Comment 24

9 days ago
I suppose most of the failure would just be fails-if? In that case, we should be able to catch the result change in try run. If you have to skip some tests because of this, please mark them with this bug number so that I can re-enable them.
Yes, they would be `fails-if`. And I'll mark this bug number on them.
You need to log in before you can comment on or make changes to this bug.