264132 - Implement fallback for SVG paint servers

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Scooter Morris]

The SVG 1.1 specification indicates that authors should provide fallback colors
when specifying URI's for paint servers in case there is an error in processing
the target uri.  See http://www.w3.org/TR/SVG11/painting.html#SpecifyingPaint
for details.  The Mozilla SVG Gradient implementation does not parse the
fallback information.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

See the end of bug 244917 comment 107 and bug 244917 comment 108.

Comment 2

[Robert Longson [:longsonr]]

*** Bug 363560 has been marked as a duplicate of this bug. ***

Comment 3

[Robert Longson [:longsonr]]

Would you mind if I had a go at this Scooter?

Comment 4

[Scooter Morris]

Absolutely not!  I'd be happy to have you take this on.

Comment 5

[Robert Longson [:longsonr]]

Attachment: patch

Comment 6

[Robert Longson [:longsonr]]

Comment on attachment 248499 [details] [diff] [review]
patch

Note that stop-color is just a colour according to the SVG spec.

Comment 7

[Robert Longson [:longsonr]]

Comment on attachment 248499 [details] [diff] [review]
patch

oops. Doesn't work in all circumstances.

Comment 8

[Robert Longson [:longsonr]]

Attachment: new and improved

Previous patch would not reserialise properly. It put normal fill/stroke colours in twice.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I'm pretty swamped with existing reviews; you really want someone else if you want the review before February.

Comment 10

[Robert Longson [:longsonr]]

Attachment: updated patch

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 248635 [details] [diff] [review]
updated patch

>+PRBool CSSParserImpl::ParsePaint(nsresult& aErrorCode,
>+                                 nsCSSValuePair* aResult,
>+                                 nsCSSProperty aPropID)

This could be significantly simplified by using early returns for failure cases and coalescing the ending parts (see below).  You also don't need to use temporaries in case of failure since TransferTempData / ClearTempData take care of that for you.

>+{
>+  nsCSSValue color;
>+  if (ParseVariant(aErrorCode, color, VARIANT_HC | VARIANT_NONE | VARIANT_URL,
>+                   nsnull)) {
>+    nsCSSValue fallbackColor;
>+    if (color.GetUnit() == eCSSUnit_URL) {
>+      if (ParseVariant(aErrorCode, fallbackColor, VARIANT_HC | VARIANT_NONE,
>+                       nsnull)) {

This should be just "VARIANT_COLOR | VARIANT_NONE"; inherit is only valid on its own.

>+        if (ExpectEndProperty(aErrorCode, PR_TRUE)) {
>+          aResult->mXValue = color;
>+          aResult->mYValue = fallbackColor;
>+          mTempData.SetPropertyBit(aPropID);
>+          return PR_TRUE;
>+        }
>+        return PR_FALSE;
>+      }
>+      fallbackColor.SetColorValue(NS_RGB(0, 0, 0));

Why black rather than SetNoneValue() ?

>+    } else {
>+      fallbackColor = color;
>+    }
>+
>+    if (ExpectEndProperty(aErrorCode, PR_TRUE)) {
>+      aResult->mXValue = color;
>+      aResult->mYValue = fallbackColor;
>+      mTempData.SetPropertyBit(aPropID);
>+      return PR_TRUE;
>+    }
>+  }
>+  return PR_FALSE;

So I think this should be simplified to something like:

>+  if (!ParseVariant(aErrorCode, aResult->mXValue,
>+                    VARIANT_HC | VARIANT_NONE | VARIANT_URL,
>+                    nsnull))
>+    return PR_FALSE;
>+  if (color.GetUnit() == eCSSUnit_URL) {
>+    if (!ParseVariant(aErrorCode, aResult->mYValue,
>+                      VARIANT_COLOR | VARIANT_NONE,
>+                      nsnull))
>+      aResult->mYValue.SetColorValue(NS_RGB(0, 0, 0));    // XXX or SetNoneValue?
>+  } else {
>+    aResult->mYValue = aResult->mXValue;
>+  }
>+
>+  if (!ExpectEndProperty(aErrorCode, PR_TRUE))
>+    return PR_FALSE;
>+
>+  mTempData.SetPropertyBit(aPropID);
>+  return PR_TRUE;


>Index: layout/style/nsRuleNode.cpp
>+SetSVGPaint(const nsCSSValuePair& aValue, const nsStyleSVGPaint& parentPaint,

>-  } else if (aValue.GetUnit() == eCSSUnit_URL) {
>+  } else if (aValue.mXValue.GetUnit() == eCSSUnit_URL) {
>     aResult.mType = eStyleSVGPaintType_Server;
>-    aResult.mPaint.mPaintServer = aValue.GetURLValue();
>+    aResult.mPaint.mPaintServer = aValue.mXValue.GetURLValue();
>     NS_IF_ADDREF(aResult.mPaint.mPaintServer);
>+    SetColor(aValue.mYValue, parentPaint.mFallbackColor, aPresContext, aContext, aResult.mFallbackColor, aInherited);

You don't want to pass parentPaint.mFallbackColor through to this -- I'd pass NS_RGB(0, 0, 0) instead, an add an assertion that aValue.mYValue.GetUnit() != eCSSUnit_Inherit.  You do need to pass aInherited through for the currentColor case, though.

Also, you need to handle None yourself; SetColor doesn't handle that.  You might be able to get away with handling it using NS_RGBA(0, 0, 0, 0).

>-  } else if (SetColor(aValue, parentPaint.mPaint.mColor, aPresContext, aContext, aResult.mPaint.mColor, aInherited)) {
>+  } else if (SetColor(aValue.mXValue, parentPaint.mPaint.mColor, aPresContext, aContext, aResult.mPaint.mColor, aInherited)) {
>     aResult.mType = eStyleSVGPaintType_Color;
>+    aResult.mFallbackColor = aResult.mPaint.mColor;

Why set mFallbackColor here but not all the other cases?

> nsSVGGeometryFrame::SetupCairoStroke(gfxContext *aContext,
>+  } else if (GetStyleSVG()->mStroke.mType == eStyleSVGPaintType_Server) {
>+    SetupCairoColor(ctx,
>+                    GetStyleSVG()->mStroke.mFallbackColor,
>+                    GetStyleSVG()->mFillOpacity);

You'll need to figure out how to handle however you handled 'none' as a fallback color here.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #11)
> Why set mFallbackColor here but not all the other cases?

To clarify, I don't think you need to set it there.

Comment 13

[Robert Longson [:longsonr]]

Attachment: address review comments

(In reply to comment #11)
> (From update of attachment 248635 [details] [diff] [review])

> Why black rather than SetNoneValue() ?

As per bug 354295 Opera/Safari default to black if the URL is invalid to make it easier to spot. Note that as far as I can tell Opera does not seem to support fallback colours for stroke. I've therefore left the code defaulting to black if no fallback is specified.

> So I think this should be simplified to something like:
> 
> >+  if (!ParseVariant(aErrorCode, aResult->mXValue,
> >+                    VARIANT_HC | VARIANT_NONE | VARIANT_URL,
> >+                    nsnull))
> >+    return PR_FALSE;
...

Done.

> 
> You don't want to pass parentPaint.mFallbackColor through to this -- I'd pass
> NS_RGB(0, 0, 0) instead, an add an assertion that aValue.mYValue.GetUnit() !=
> eCSSUnit_Inherit.  You do need to pass aInherited through for the currentColor
> case, though.

Done.

> 
> Also, you need to handle None yourself; SetColor doesn't handle that.  You
> might be able to get away with handling it using NS_RGBA(0, 0, 0, 0).

Done. Changed SVG code to support RGBA colours too.

> >+    aResult.mFallbackColor = aResult.mPaint.mColor;
> 
> Why set mFallbackColor here but not all the other cases?

You're right this is not necessary. Removed.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 250343 [details] [diff] [review]
address review comments

>+      aResult->mYValue.SetColorValue(NS_RGB(0, 0, 0));

I'm still a little skeptical about using black, but I suppose it's ok.

>   // stop-color: 
>-  SetSVGPaint(SVGData.mStopColor, parentSVGReset->mStopColor,
>-              mPresContext, aContext, svgReset->mStopColor, inherited);
>+  if (eCSSUnit_None == SVGData.mStopColor.GetUnit()) {
>+    svgReset->mStopColor = NS_RGBA(0, 0, 0, 0);
>+  } else {
>+    SetColor(SVGData.mStopColor, parentSVGReset->mStopColor, mPresContext, aContext, svgReset->mStopColor, aInherited);
>+  }

stop-color doesn't take 'none' as a value, so this change seems unnecessary.  Instead you should change CSSParserImpl::ParseSingleValueProperty so VARIANT_NONE is not allowed.

With that, r=dbaron.

Comment 15

[Robert Longson [:longsonr]]

Comment on attachment 250343 [details] [diff] [review]
address review comments


Fixed stop-color processing to exclude invalid 'none' possibility.

Comment 16

[Robert Longson [:longsonr]]

Attachment: version checked in

Additional changes required:

integrated with bug 326143 (flood-color changed in the same way as stop-color)
integrated with bug 360316 (merge opacity changes)

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 250940 [details] [diff] [review]
version checked in

>Index: layout/style/nsRuleNode.cpp
>-  SetSVGPaint(SVGData.mStopColor, parentSVGReset->mStopColor,
>-              mPresContext, aContext, svgReset->mStopColor, inherited);
>+  SetColor(SVGData.mStopColor, parentSVGReset->mStopColor, mPresContext, aContext, svgReset->mStopColor, aInherited);

Oops, this should have continued to use inherited, not aInherited, but I asked tor to fix that in bug 326143.

Comment 18

[Robert Longson [:longsonr]]

(In reply to comment #17)
> (From update of attachment 250940 [details] [diff] [review])
> >Index: layout/style/nsRuleNode.cpp
> >-  SetSVGPaint(SVGData.mStopColor, parentSVGReset->mStopColor,
> >-              mPresContext, aContext, svgReset->mStopColor, inherited);
> >+  SetColor(SVGData.mStopColor, parentSVGReset->mStopColor, mPresContext, aContext, svgReset->mStopColor, aInherited);
> 
> Oops, this should have continued to use inherited, not aInherited, but I asked
> tor to fix that in bug 326143.
> 

I think tor fixed flood-color. This fault is stop-color. bug 366535 raised to fix it.

Comment 19

[Robert Longson [:longsonr]]

(In reply to comment #18)
> (In reply to comment #17)
> > (From update of attachment 250940 [details] [diff] [review] [details])
> > >Index: layout/style/nsRuleNode.cpp
> > >-  SetSVGPaint(SVGData.mStopColor, parentSVGReset->mStopColor,
> > >-              mPresContext, aContext, svgReset->mStopColor, inherited);
> > >+  SetColor(SVGData.mStopColor, parentSVGReset->mStopColor, mPresContext, aContext, svgReset->mStopColor, aInherited);
> > 
> > Oops, this should have continued to use inherited, not aInherited, but I asked
> > tor to fix that in bug 326143.
> > 
> 
> I think tor fixed flood-color. This fault is stop-color. bug 366535 raised to
> fix it.
> 

On checking further I was wrong. Tor fixed both of them. 'Tis all my fault.

Comment 20

[Robert Longson [:longsonr]]

Reftests checked in as part of bug 368840