1352258 - When loaded directly in a browser window, the chrome file connecting.svg does not display

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[magicp]

[Steps to reproduce]
1. Start latest Nightly
2. Enter "chrome://browser/skin/tabbrowser/connecting.svg" in urlbar
3. Does not display connecting.svg in content
4. Save connecting.svg in local
5. Open connecting.svg using Chrome and IE

[Actual Results]
Does not display connecting.svg in content on Firefox.
Firefox: NG
Chrome: OK
IE: OK

[Expected Results]
Firefox displays connecting.svg in content.

Comment 1

[Robert Longson [:longsonr]]

It will be displayed differently as it has fill="context-fill" which the other UAs probably don't recognise yet.

Comment 2

[Jonathan Watt [:jwatt]]

See bug 1058040 comment 35, or the whiteboard of that bug - for context paint to work in content you need to flip the pref svg.context-properties.content.enabled to true. However, that won't help you in this case since there is no context element on which to specify context fill/stroke.

One option is for the author of the SVG to specify a fallback color after they specify 'context-paint'. For example, as is done in this test where 'blue' is specified as a fallback color:

https://dxr.mozilla.org/mozilla-central/rev/8df9fabf2587b7020889755acb9e75b664fe13cf/layout/reftests/svg/as-image/context-fill-01.html#16

But normally 'context-fill' would fallback to 'black'. I guess that doesn't happen when the SVG is loaded as a top-level document, but I think it should for consistency. I'll investigate.

Comment 3

[Jonathan Watt [:jwatt]]

This behavior seems to be unspecified in SVG 1.1, but it looks like it is required by the SVG 2 Editor's Draft as it currently stands:

  https://svgwg.org/svg2-draft/painting.html#SpecifyingPaint

  If a paint server reference in a <paint> is invalid, and no
  fall-back value is given, no paint is rendered for that layer.

Nowadays I don't agree with that, even though it has been our behavior for a long time. I would hope we could change it to be the initial value of fill if a paint server fails given this is an error condition, but we may break a small amount of content if we do that.

Comment 4

[Jonathan Watt [:jwatt]]

From an implementation point of view what happens is we parse the 'fill' property using CSSParserImpl::ParsePaint which hits the CSSParseResult::NotFound case looking for a fallback, and so sets the fallback explicitly to be 'none' (i.e. 'transparent') on the style rule:

https://dxr.mozilla.org/mozilla-central/rev/8df9fabf2587b7020889755acb9e75b664fe13cf/layout/style/nsCSSParser.cpp#17257

When computing style we then call nsCSSValue::SetContextValue passing that 'transparent' value:

https://dxr.mozilla.org/mozilla-central/rev/8df9fabf2587b7020889755acb9e75b664fe13cf/layout/style/nsRuleNode.cpp#9383

So when we call nsSVGUtils::GetFallbackOrPaintColor under nsSVGUtils::MakeFillPatternFor while painting the element we return the 'transparent' value and use that.

Comment 5

[Jonathan Watt [:jwatt]]

Any thoughts on this, Cameron?

Comment 6

[Cameron McCormack (:heycam)]

I would worry that changing the choice of fallback paint when one isn't specified and a paint server reference fails would break more content than we expect.

However, context-fill isn't a paint server reference (only url() values, and the nobody-implements child value, are).  So that sentence doesn't apply.  The spec also does say:

  If there is no context element and these keywords are used, then no paint is applied.

so the effect is the same. :-)

Perhaps we should just make context-fill fall back to black and context-stroke fall back to none, without changing the general <paint> fallback rules?

Comment 7

[Dão Gottwald [:dao]]

Doesn't really block bug 759252 in a meaningful way.

Comment 8

[Robert Longson [:longsonr]]

Should be pretty simple to do once I land the patch in bug 1347409

Comment 9

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #8)
> Should be pretty simple to do once I land the patch in bug 1347409

How's that bug coming along. And do you fancy taking this one? :)

Comment 10

[Robert Longson [:longsonr]]

Attachment: patch with reftest

Comment 11

[Jonathan Watt [:jwatt]]

Comment on attachment 8863938 [details] [diff] [review]
patch with reftest

Thanks!

Comment 12

[Robert Longson [:longsonr]]

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

Comment 13

[Robert Longson [:longsonr]]

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

Comment 14

[Robert Longson [:longsonr]]

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/71386606fa25
default fallback for context-fill to black to match default fill colour r=jwatt

Comment 16

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/71386606fa25

Comment 17

[magicp]

This bug was fixed in latest Nightly build (20170504030320). Thanks!