fix: correctly handle fill/stroke opacity properties

Previously, the fill-opacity and stroke-opacity properties were being
treated like the opacity property in that they were applied to all their
children.

It's correct that opacity on an element should apply to its children,
and another opacity property on a child should multiply the opacity of
its parent.  But fill-opacity and stroke-opacity are different in that
the value is inherited and any change to a child's values must
overwrite their parent's value.

This bug was spotted because `fill-opacity="0"` was being declared at
the top level of a document, with `fill-opacity="1"` on its children.
With the old code, 0 was multiplied with 1, giving an opacity of 0.
With the new code, the child's 1 value overrides the parent's 0 value.

Fixes #177

lib/prawn/svg/attributes/opacity.rb

@@ -1,15 +1,30 @@
 module Prawn::SVG::Attributes::Opacity
   def parse_opacity_attributes_and_call
-    # We can't do nested opacities quite like the SVG requires, but this is close enough.
-    opacity = properties.opacity.to_f.clamp(0, 1) if properties.opacity
-    fill_opacity = properties.fill_opacity.to_f.clamp(0, 1) if properties.fill_opacity
-    stroke_opacity = properties.stroke_opacity.to_f.clamp(0, 1) if properties.stroke_opacity
+    # The opacity property is not inherited, but the opacity applies to all children underneath the element.
+    #
+    # Having an opacity property on a parent, and again on a child, multiplies the parent and child's opacities
+    # when drawing the children.
+    #
+    # The fill-opacity and stroke-opacity properties are inherited, but children which have a different value
+    # are displayed at that opacity rather than multiplying the parent's fill/stroke opacity with the child's.
+    #
+    # opacity and fill/stroke opacity can both be applied to the same element, and they multiply together.
+
+    opacity = computed_properties.opacity&.to_f&.clamp(0, 1)
+    fill_opacity = computed_properties.fill_opacity.to_f.clamp(0, 1)
+    stroke_opacity = computed_properties.stroke_opacity.to_f.clamp(0, 1)
 
     if opacity || fill_opacity || stroke_opacity
-      state.fill_opacity *= [opacity || 1, fill_opacity || 1].min
-      state.stroke_opacity *= [opacity || 1, stroke_opacity || 1].min
+      state.opacity *= opacity if opacity
+
+      fill_opacity = (fill_opacity || 1) * state.opacity
+      stroke_opacity = (stroke_opacity || 1) * state.opacity
 
-      add_call_and_enter 'transparent', state.fill_opacity, state.stroke_opacity
+      if state.last_fill_opacity != fill_opacity || state.last_stroke_opacity != stroke_opacity
+        state.last_fill_opacity = fill_opacity
+        state.last_stroke_opacity = stroke_opacity
+        add_call_and_enter 'transparent', fill_opacity, stroke_opacity
+      end
     end
   end
 end

lib/prawn/svg/elements/marker.rb

@@ -56,6 +56,8 @@ def apply_marker(element, point: nil, angle: 0)
 
       new_state = state.dup
       new_state.computed_properties = computed_properties.dup
+      new_state.last_fill_opacity = element.state.last_fill_opacity
+      new_state.last_stroke_opacity = element.state.last_stroke_opacity
 
       container = Prawn::SVG::Elements::Container.new(document, nil, [], new_state)
       container.properties.compute_properties(new_state.computed_properties)

lib/prawn/svg/state.rb

@@ -1,14 +1,16 @@
 class Prawn::SVG::State
   attr_accessor :text, :preserve_space,
-    :fill_opacity, :stroke_opacity, :stroke_width,
+    :opacity, :last_fill_opacity, :last_stroke_opacity,
+    :stroke_width,
     :computed_properties,
     :viewport_sizing,
     :inside_use, :inside_clip_path
 
   def initialize
     @stroke_width = 1
-    @fill_opacity = 1
-    @stroke_opacity = 1
+    @opacity = 1
+    @last_fill_opacity = 1
+    @last_stroke_opacity = 1
     @computed_properties = Prawn::SVG::Properties.new.load_default_stylesheet
   end
 

spec/prawn/svg/attributes/opacity_spec.rb

@@ -10,6 +10,10 @@ def initialize
       @properties = ::Prawn::SVG::Properties.new
       @state = ::Prawn::SVG::State.new
     end
+
+    def computed_properties
+      @state.computed_properties
+    end
   end
 
   let(:element) { OpacityTestElement.new }
@@ -26,55 +30,58 @@ def initialize
 
     context 'with opacity' do
       it 'sets fill and stroke opacity' do
-        element.properties.opacity = '0.4'
+        element.computed_properties.opacity = '0.4'
 
         expect(element).to receive(:add_call_and_enter).with('transparent', 0.4, 0.4)
         subject
 
-        expect(element.state.fill_opacity).to eq 0.4
-        expect(element.state.stroke_opacity).to eq 0.4
+        expect(element.state.opacity).to eq 0.4
+        expect(element.state.last_fill_opacity).to eq 0.4
+        expect(element.state.last_stroke_opacity).to eq 0.4
       end
     end
 
     context 'with just fill opacity' do
       it 'sets fill opacity and sets stroke opacity to 1' do
-        element.properties.fill_opacity = '0.4'
+        element.computed_properties.fill_opacity = '0.4'
 
         expect(element).to receive(:add_call_and_enter).with('transparent', 0.4, 1)
         subject
 
-        expect(element.state.fill_opacity).to eq 0.4
-        expect(element.state.stroke_opacity).to eq 1
+        expect(element.state.opacity).to eq 1
+        expect(element.state.last_fill_opacity).to eq 0.4
+        expect(element.state.last_stroke_opacity).to eq 1
       end
     end
 
-    context 'with an existing fill/stroke opacity' do
+    context 'with an existing stroke opacity' do
       it 'multiplies the new opacity by the old' do
-        element.state.fill_opacity = 0.5
-        element.state.stroke_opacity = 0.8
+        element.state.opacity = 0.5
 
-        element.properties.fill_opacity = '0.4'
-        element.properties.stroke_opacity = '0.5'
+        element.computed_properties.fill_opacity = '0.4'
+        element.computed_properties.stroke_opacity = '0.5'
 
-        expect(element).to receive(:add_call_and_enter).with('transparent', 0.2, 0.4)
+        expect(element).to receive(:add_call_and_enter).with('transparent', 0.2, 0.25)
         subject
 
-        expect(element.state.fill_opacity).to eq 0.2
-        expect(element.state.stroke_opacity).to eq 0.4
+        expect(element.state.opacity).to eq 0.5
+        expect(element.state.last_fill_opacity).to eq 0.2
+        expect(element.state.last_stroke_opacity).to eq 0.25
       end
     end
 
     context 'with stroke, fill, and opacity all specified' do
       it 'choses the lower of them' do
-        element.properties.fill_opacity = '0.4'
-        element.properties.stroke_opacity = '0.6'
-        element.properties.opacity = '0.5'
+        element.computed_properties.fill_opacity = '0.4'
+        element.computed_properties.stroke_opacity = '0.6'
+        element.computed_properties.opacity = '0.5'
 
-        expect(element).to receive(:add_call_and_enter).with('transparent', 0.4, 0.5)
+        expect(element).to receive(:add_call_and_enter).with('transparent', 0.2, 0.3)
         subject
 
-        expect(element.state.fill_opacity).to eq 0.4
-        expect(element.state.stroke_opacity).to eq 0.5
+        expect(element.state.opacity).to eq 0.5
+        expect(element.state.last_fill_opacity).to eq 0.2
+        expect(element.state.last_stroke_opacity).to eq 0.3
       end
     end
   end

spec/prawn/svg/elements/marker_spec.rb

@@ -69,21 +69,19 @@ def new_state
           ['rectangle', [[-0.5, 600.0], 4.0, 3.0], {}, []],
           ['clip', [], {}, []],
           ['transformation_matrix', [0.3, 0, 0, 0.3, 0, 0], {}, []],
-          ['transparent', [1.0, 1.0], {}, [
-            ['fill_color', ['000000'], {}, []],
-            ['line_width', [1.0], {}, []],
-            ['cap_style', [:butt], {}, []],
-            ['join_style', [:miter], {}, []],
-            ['undash', [], {}, []],
-            ['save', [], {}, []],
-            ['fill', [], {}, [
-              ['move_to', [[0.0, 600.0]], {}, []],
-              ['line_to', [[10.0, 595.0]], {}, []],
-              ['line_to', [[0.0, 590.0]], {}, []],
-              ['close_path', [], {}, []]
-            ]],
-            ['restore', [], {}, []]
-          ]]
+          ['fill_color', ['000000'], {}, []],
+          ['line_width', [1.0], {}, []],
+          ['cap_style', [:butt], {}, []],
+          ['join_style', [:miter], {}, []],
+          ['undash', [], {}, []],
+          ['save', [], {}, []],
+          ['fill', [], {}, [
+            ['move_to', [[0.0, 600.0]], {}, []],
+            ['line_to', [[10.0, 595.0]], {}, []],
+            ['line_to', [[0.0, 590.0]], {}, []],
+            ['close_path', [], {}, []]
+          ]],
+          ['restore', [], {}, []]
         ]],
         ['restore', [], {}, []]
       ]