Closed Bug 1350015 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: SVG, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 759252
Blocks: 1347543
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.
Attached patch patch (obsolete) — Splinter Review
Attachment #8850819 - Flags: review?(dholbert)
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 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 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/
Attachment #8850819 - Flags: review?(dholbert) → review-
Attached patch patchSplinter Review
Good catch on the hashing issue!
Attachment #8850819 - Attachment is obsolete: true
Attachment #8851184 - Flags: review?(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
Attachment #8851184 - Flags: review?(dholbert) → review+
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7f3f26cc004
Fix the 'fill-opacity' property to work with fill="context-fill". r=dholbert
https://hg.mozilla.org/mozilla-central/rev/c7f3f26cc004
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.