Closed Bug 1338764 Opened 7 years ago Closed 7 years ago

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

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: manishearth, Assigned: xidorn)

References

Details

Attachments

(4 files, 4 obsolete files)

Bug 1338764 - Implement context-{fill|stroke}-opacity property values.
59 bytes, text/x-review-board-request
Details
Bug 1338764 part 1 - Use const rather than enum for context flags of nsStyleSVG.
59 bytes, text/x-review-board-request
manishearth
: review+
Details
Bug 1338764 part 2 - Add context-{fill,stroke}-opacity support to {fill,stroke}-opacity.
59 bytes, text/x-review-board-request
manishearth
: review+
Details
Bug 1338764 part 3 - Update test expectations.
59 bytes, text/x-review-board-request
manishearth
: review+
Details 
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
Assignee: nobody → cku
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)
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)
Flags: needinfo?(jwatt)
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)
(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)
Assignee: cku → kuoe0
Blocks: 1356087
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.
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.
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)
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
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
Status: NEW → ASSIGNED
Priority: P2 → P1
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
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
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.
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.
Blocks: 1369614
No longer depends on: 1369614
Comment on attachment 8892315 [details]
Make stroke-dasharray accept context-value.

https://reviewboard.mozilla.org/r/163274/#review168664

::: servo/components/style/properties/helpers/animated_properties.mako.rs:3077
(Diff revision 1)
> +impl<LengthType> Animatable for SVGStrokeDashArray<LengthType> {
> +    #[inline]
> +    fn add_weighted(&self, _other: &Self, _self_portion: f64, _other_portion: f64) -> Result<Self, ()> {
> +        Err(())
> +    }
> +
> +    #[inline]
> +    fn compute_distance(&self, _other: &Self) -> Result<f64, ()> {
> +        Err(())
> +    }
> +}
> +
> +impl<LengthType> ToAnimatedZero for SVGStrokeDashArray<LengthType> {
> +    #[inline]
> +    fn to_animated_zero(&self) -> Result<Self, ()> {
> +        Err(())
> +    }
> +}

Looks like we do need to support animation to some extent.
Comment on attachment 8892311 [details]
Bug 1338764 part 1 - Use const rather than enum for context flags of nsStyleSVG.

https://reviewboard.mozilla.org/r/163266/#review168666
Attachment #8892311 - Flags: review+
Comment on attachment 8892312 [details]
Add svg mods and move SVG-related types into them.

https://reviewboard.mozilla.org/r/163268/#review168668
Attachment #8892312 - Flags: review+
Comment on attachment 8892313 [details]
Have CoordDataValue derive Debug and PartialEq.

https://reviewboard.mozilla.org/r/163270/#review168670
Attachment #8892313 - Flags: review+
Comment on attachment 8892314 [details]
Add SVGLength which accepts context-value, and use it for stroke-{width,dashoffset}.

https://reviewboard.mozilla.org/r/163272/#review168672

::: servo/components/style/properties/gecko.mako.rs:564
(Diff revision 1)
>              convert_nscolor_to_rgba(${get_gecko_property(gecko_ffi_name)})
>          }
>      % endif
>  </%def>
>  
> +<%def name="impl_svg_length(ident, gecko_ffi_name, need_clone=False)">

nit: probably leave a comment somewhere explaining how the context flag stuff works

::: servo/components/style/properties/gecko.mako.rs:571
(Diff revision 1)
> +        use values::generics::svg::SVGLength;
> +        use gecko_bindings::structs::nsStyleSVG_${ident.upper()}_CONTEXT as CONTEXT_VALUE;
> +        self.gecko.mContextFlags &= !CONTEXT_VALUE;
> +        let length = match v {
> +            SVGLength::Length(length) => {
> +                self.gecko.mContextFlags &= !CONTEXT_VALUE;

we do this twice. Perhaps eliminate the time it gets set outside of the match?

::: servo/components/style/properties/gecko.mako.rs:609
(Diff revision 1)
> +        let length = match self.gecko.${gecko_ffi_name}.as_value() {
> +            CoordDataValue::Factor(number) => Either::First(number),
> +            CoordDataValue::Coord(coord) => Either::Second(LengthOrPercentage::Length(Au(coord))),
> +            CoordDataValue::Percent(p) => Either::Second(LengthOrPercentage::Percentage(Percentage(p))),
> +            CoordDataValue::Calc(calc) => Either::Second(LengthOrPercentage::Calc(calc.into())),
> +            _ => unreachable!(),

nit: Preferably keep a string within the unreachable!(), like `x => unreachable!("Found unexpected coordinate {:?} in ${ident}", x).

(So that we get better crash reports)

::: servo/components/style/values/specified/svg.rs:25
(Diff revision 1)
>  pub type SVGPaintKind = generic::SVGPaintKind<RGBAColor>;
> +
> +#[cfg(feature = "gecko")]
> +fn is_context_value_enabled() -> bool {
> +    use gecko_bindings::structs::mozilla;
> +    unsafe { mozilla::StylePrefs_sOpentypeSVGEnabled }

is this safe to access off main thread?


There should be a comment mentioning why this access is safe.
Attachment #8892314 - Flags: review+
Comment on attachment 8892314 [details]
Add SVGLength which accepts context-value, and use it for stroke-{width,dashoffset}.

https://reviewboard.mozilla.org/r/163272/#review168678

::: servo/components/style/properties/helpers/animated_properties.mako.rs:3062
(Diff revision 1)
> +    fn compute_distance(&self, other: &Self) -> Result<f64, ()> {
> +        match (self, other) {
> +            (&SVGLength::Length(ref this), &SVGLength::Length(ref other)) => {
> +                this.compute_distance(other)
> +            }
> +            _ => Err(())

Are you sure we don't need to be able to animate between context values?
Comment on attachment 8892314 [details]
Add SVGLength which accepts context-value, and use it for stroke-{width,dashoffset}.

https://reviewboard.mozilla.org/r/163272/#review168672

> nit: Preferably keep a string within the unreachable!(), like `x => unreachable!("Found unexpected coordinate {:?} in ${ident}", x).
> 
> (So that we get better crash reports)

This piece was copied from the code removed below... but ok, I'll add a string.

> is this safe to access off main thread?
> 
> 
> There should be a comment mentioning why this access is safe.

The pref bools can only be mutated on the main thread, so it is safe to access either when we are on the main thread, or the main thread is blocked. Actually, I wouldn't worry about this at all. It is just a bool affecting whether a value can be parsed. It doesn't really matter even if it is possible to change when we access it...
Comment on attachment 8892314 [details]
Add SVGLength which accepts context-value, and use it for stroke-{width,dashoffset}.

https://reviewboard.mozilla.org/r/163272/#review168678

> Are you sure we don't need to be able to animate between context values?

We don't... At least we are not able to do that in later phases of the rendering pipeline at all as far as I know.
Priority: P1 → --
Although I didn't file this bug, this is definitely P1 since it blocks many other work.
Priority: -- → P1
Comment on attachment 8892315 [details]
Make stroke-dasharray accept context-value.

https://reviewboard.mozilla.org/r/163274/#review169072
Attachment #8892315 - Flags: review?(manishearth) → review+
Comment on attachment 8892316 [details]
Bug 1338764 part 2 - Add context-{fill,stroke}-opacity support to {fill,stroke}-opacity.

https://reviewboard.mozilla.org/r/163276/#review169074
Attachment #8892316 - Flags: review?(manishearth) → review+
Comment on attachment 8892317 [details]
Bug 1338764 part 3 - Update test expectations.

https://reviewboard.mozilla.org/r/163278/#review169076
Attachment #8892317 - Flags: review?(manishearth) → review+
Attachment #8892312 - Attachment is obsolete: true
Attachment #8892313 - Attachment is obsolete: true
Attachment #8892314 - Attachment is obsolete: true
Attachment #8892315 - Attachment is obsolete: true
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0d9da5e23a91
part 1 - Use const rather than enum for context flags of nsStyleSVG. r=manishearth
https://hg.mozilla.org/integration/autoland/rev/9832e77f267e
part 2 - Add context-{fill,stroke}-opacity support to {fill,stroke}-opacity. r=manishearth
https://hg.mozilla.org/integration/autoland/rev/8ec6c744ce57
part 3 - Update test expectations. r=manishearth
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: