1350015 - In SVG-as-an-image, using the 'context-fill' value causes 'fill-opacity' to be ignored

Status: RESOLVED FIXED

(Core :: SVG, enhancement)

Description

[Jonathan Watt [:jwatt]]

Dao tells me that when he uses the 'context-fill' value on an element it causes the 'fill-opacity' property on the element to be ignored.

Comment 1

[Jonathan Watt [:jwatt]]

So the aOpacity that gets passed to SVGContextPaint's (or in our case SVGEmbeddingContextPaint's) GetFillPattern/GetStrokePattern methods is the fill/stroke opacity:

https://hg.mozilla.org/mozilla-central/annotate/e03e0c60462c775c7558a1dc9d5cf2076c3cd1f9/layout/svg/nsSVGUtils.cpp#l1496

The current code mistakenly ignores that argument.

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 3

[Jonathan Watt [:jwatt]]

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

Review of attachment 8850819 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/svg/SVGContextPaint.h
@@ +238,4 @@
>    }
>  
> +  /**
> +   * Always of type PatternType::COLOR.

I changed these comments to:

   * Returns a pattern of type PatternType::COLOR, or else nullptr.

Comment 4

[Daniel Holbert [:dholbert]]

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

Review of attachment 8850819 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/svg/SVGContextPaint.cpp
@@ +321,1 @@
>    }

It looks to me like this implementation will have unwanted/unnecessary collisions, between e.g. an object that represents "context-fill: red" and an object that represents "context-stroke: red".

Both would produce HashGeneric(0, "red".ToABGR()).  So they'd collide in any hash-map they end up in, resulting in suboptimal hash table usage.

This problem is pretty trivial to avoid (and it'll provide improved hash lookup performance) so we might as well.  I *think* we can work around this by changing the first clause to e.g.:
  if (mFillIsSet) {
    hash = HashGeneric(hash, mFill.ToABGR());
  } else {
    // Arbitrary number, just to avoid trivial hash collisions between pairs of
    // instances with a.mFill == b.mStroke & with the other fields unset.
    hash = 1;
  }

::: layout/svg/SVGContextPaint.h
@@ +266,5 @@
>  private:
> +  Color mFill;
> +  Color mStroke;
> +  bool mFillIsSet;
> +  bool mStrokeIsSet;

Could you make these Maybe<Color>, for better encapsulation & for built-in assertion checking that we only use the color when the boolean actually says it's been set?

Minor downside of Maybe<> is that it won't pack as well, but that doesn't really matter here because 
 - this struct is temporary anyway, and we won't ever have tons of them at once I think
 - both strategies work out to 64 bytes in practice (36 bytes the way you've got it, 40 with Maybe<>, according to local sizeof() measurements -- and both of those round up to 64 with word alignment).

Comment 5

[Daniel Holbert [:dholbert]]

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

Review of attachment 8850819 [details] [diff] [review]:
-----------------------------------------------------------------

Two more final nits. And then, this is basically r+-with-nits-addressed, but it's probably worth a final look after you've converted to Maybe<> -- assuming you're cool with that -- since that will tweak a decent amount of the patch.  Hence, r- for the moment, but I'll do my best to grant swift r+ when you post an updated version.

First nit: the commit message:
> Bug 1350015 - Fix the 'fill-opacity' property work with fill="context-fill". r=dholbert

This doesn't quite make grammatical sense. I think maybe you're missing a word -- s/work/to work/, maybe?  (Anyway, please reword to make sense.)

::: layout/svg/SVGContextPaint.cpp
@@ +288,5 @@
> +    return nullptr;
> +  }
> +  // The gfxPattern that we create below depends on aFillOpacity, and since
> +  // different elements in the SVG image may pass in different values for
> +  // fill opacities we don't try to cache the gfxContexts that we create.

Second nit:  s/gfxContexts/gfxPatterns/

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: patch

Good catch on the hashing issue!

Comment 7

[Daniel Holbert [:dholbert]]

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

Review of attachment 8851184 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Is it possible to add a reftest for this?  If so, it'd be great to land one with this patch (or soon afterwards if folks like Dao need the fix to be in ASAP).

r=me

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7f3f26cc004
Fix the 'fill-opacity' property to work with fill="context-fill". r=dholbert

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c7f3f26cc004