Currently, if you do > <p style="fill: url(#a)"></p> the computed value of fill would be "url(#a) rgba(0, 0, 0, 0)", however, in this case, it should actually be "url(#a) none" or just "url(#a)". This is because nsStyleSVGPaint always stores a color for the fallback, and SetSVGPaint (in nsRuleNode.cpp) does the computation. I think a reasonable solution would be to split eStyleSVGPaintType_Server value into eStyleSVGPaintType_Server and eStyleSVGPaintType_ServerWithoutFallback, and use the latter for the cases where fallback is none. test_bug_1293164 in layout/style/test/test_computed_style.html can be simplified after fixing this. (Servo side would also need similiar fix.)

That's not really going to work as context-fill and context-stroke can have optional fallback. On top off that we don't really distinguish between no fallback and the none keyword. I think we need a new enum (nothing, none, color) to track what kind of fallback we have.

Allows gecko to distinguish between url() url() none url() blue We serialise what we are given now. Previously there was no way to distinguish the first two cases, in fact they both became url() rgba(0, 0, 0, 0) Currently url() and url() none are treated identically but bug 1351243 suggests that context-fill without a fallback colour should be treated as if it were context-fill black. This patch would allow that to be supported fairly simply.

Not really sure if this is a) how to support url() none (is that supported currently in stylo?) b) sufficient except for the known limitation of a) c) if it compiles given that the gecko patch with the enum in has to land. Can I tie my gecko and server trees together locally somehow? d) how to land it after the gecko patch lands. Without what I've done so far stylo's url() color support would disappear though.

Comment on attachment 8859028 [details] [diff] [review] 1347409.txt Review of attachment 8859028 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the following addressed (and assuming you use that comment as the commit message). ::: layout/style/nsComputedDOMStyle.cpp @@ +5740,5 @@ > + } > + valueList->AppendCSSValue(val.forget()); > + valueList->AppendCSSValue(fallback.forget()); > + return valueList.forget(); > + } Nit: trailing space. @@ +5759,5 @@ > + } > + valueList->AppendCSSValue(val.forget()); > + valueList->AppendCSSValue(fallback.forget()); > + return valueList.forget(); > + } Nit: trailing space. It might be nice to factor out the serialization of the fallback into a separate function, since the code is the same here as for URLs above. ::: layout/style/nsStyleStruct.h @@ +3370,4 @@ > eStyleSVGPaintType_ContextStroke > }; > > +enum nsStyleSVGFallbackType { Maybe we should make this (and nsStyleSVGPaint) |: uint8_t|, just to avoid wasting a bit of space in nsStyleSVGPaint. ::: layout/svg/nsSVGUtils.cpp @@ +1458,5 @@ > const nsStyleSVGPaint &paint = aStyleContext->StyleSVG()->*aFillOrStroke; > nsStyleContext *styleIfVisited = aStyleContext->GetStyleIfVisited(); > + nscolor color; > + switch (paint.Type()) { > + case eStyleSVGPaintType_Server: Nit: cases should be indented one level. @@ +1555,5 @@ > } > > + if (style->mFill.GetFallbackType() == eStyleSVGFallbackType_None) { > + return DrawResult::SUCCESS; > + } Nit: I'd put a blank line below this. @@ +1634,5 @@ > } > > + if (style->mStroke.GetFallbackType() == eStyleSVGFallbackType_None) { > + return DrawResult::SUCCESS; > + } Nit: and here.

Comment on attachment 8859030 [details] [diff] [review] attempt at servo patch Review of attachment 8859030 [details] [diff] [review]: ----------------------------------------------------------------- ::: components/style/properties/gecko.mako.rs @@ +448,4 @@ > } > > if let Some(fallback) = fallback { > + paint.mFallbackType = nsStyleSVGFallbackType::eStyleSVGFallbackType_Color; I think instead of this, we should extend the specified and computed SVGPaint types in Servo (in components/style/values/{specified,computed}/mod.rs) so that the fallback is an enum type that allows distinguishing none from the presence of a colour. Then we can set mFallbackType to eStyleSVGFallbackType_None when appropriate. (We could extend Gecko_nsStyleSVGPaint_SetURLValue with arguments to indicate the fallback type/value, but given we're poking in the nsStyleSVGPaint object directly here anyway, for the non-URL types, we may as well do all that poking here.)

(In reply to Robert Longson from comment #4) > a) how to support url() none (is that supported currently in stylo?) "No", per my comment just now, but we should add support for that. > b) sufficient except for the known limitation of a) I think so. > c) if it compiles given that the gecko patch with the enum in has to land. > Can I tie my gecko and server trees together locally somehow? If you make your local changes to the files under servo/ in your mozilla-central checkout, and you have |ac_add_options --enable-stylo| in your mozconfig, it should all compile together. (I usually do that, and use my separate Servo checkout when it's time to put those patches up in a PR.) > d) how to land it after the gecko patch lands. The current procedure for changes that involve Gecko and Servo are to do all the review here in Bugzilla, then when you've got your r+es, open a Servo PR with just the servo/ changes (re-jigged to not apply to files under servo/!), mention in the PR that it's already been reviewed by whoever and link to the Bugzilla bug. Someone will say "@bors-servo delegate+" in the PR, which means you then have control over when the patch will be tested and merged (you type "@bors-servo r=whoever" in the PR). After the hour or so of CI testing, it should merge if there are no Servo test failures, and then you can land the Gecko change ASAP. Unfortunately we don't have anything better at the moment, so if you don't think you'll be around to click the button to land the Gecko patch in an hour after r+ing the Servo PR, better to wait until you will have time. Or you could ask someone on #servo to help you do that. > Without what I've done so far stylo's url() color support would disappear > though. Yeah, I think I noticed some test failures just earlier today due to not supporting the fallback properly. (It turns out we don't enable stylo for SVG documents at the moment -- see bug 1355762.)

Also, when doing a try run for changes that affect stylo, please include "-p linux64-stylo" in your try syntax. That might reveal some test passes (maybe unlikely until bug 1355762 is fixed), but if there are, you should update the annotations for those in the Gecko patch.

Pushed by longsonr@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3ff1f94e5767 part 1 - introduce nsStyleSVGFallbackType so that we can begin to distinguish the type of paintserver fallback r=cam

Comment on attachment 8859030 [details] [diff] [review] attempt at servo patch I think I'd like to land this as-is and then implement comment 7 in a follow-up bug so as not to block progress on bug 1352258. I've landed just enough of this bug so that the servo patch should be landable now.

Comment on attachment 8859030 [details] [diff] [review] attempt at servo patch Review of attachment 8859030 [details] [diff] [review]: ----------------------------------------------------------------- Doing that in the followup sounds fine. Please do file it though.

fix servo patch so that it compiles. Some kind of include was missing.

Pushed by longsonr@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/08735c334ad1 part 2 - serialise fill and stroke fallback properly r=cam