Improve syntax checking of property values

Previously, invalid values of properties would cause the property to
return to its default value in some cases, or take on the invalid value
in some other cases.  This is not what CSS is meant to do.

Now an invalid value in a property will be ignored, as if the property
was never provided.  This allows previously specified properties to
continue to be used, or for properties that inherit they will inherit
their parent's value.

This commit is part one of a larger change.  Part two will store parsed
values into the properties variable rather than the raw strings received
from the CSS.  For example, numbers will be stored as integers or
floats, and colours will be stored as a struct.

lib/prawn/svg/color.rb

@@ -211,6 +211,43 @@ def parse(color_string, gradients = nil, color_mode = :rgb)
       result.compact
     end
 
+    def parse_color(value)
+      case value
+      in ['rgb', args]
+        return unless args.length == 3
+
+        rgb =
+          args.map do |arg|
+            number = to_float(arg, 2.55) or break
+            format('%02x', number.round.clamp(0, 255))
+          end
+
+        rgb && RGB.new(rgb.join)
+
+      in ['device-cmyk', args]
+        return unless args.length == 4
+
+        cymk =
+          args.map do |arg|
+            number = to_float(arg, 0.01) or break
+            (number * 100).clamp(0, 100)
+          end
+
+        cymk && CMYK.new(cymk)
+
+      in /\A#([0-9a-f])([0-9a-f])([0-9a-f])\z/i
+        RGB.new("#{$1 * 2}#{$2 * 2}#{$3 * 2}")
+
+      in /\A#[0-9a-f]{6}\z/i
+        RGB.new(value[1..])
+
+      in String => color
+        if (hex = HTML_COLORS[color.downcase])
+          hex_color(hex, nil) # TODO : color mode
+        end
+      end
+    end
+
     def css_color_to_prawn_color(color)
       parse(color).detect { |value| value.is_a?(RGB) || value.is_a?(CMYK) }&.value
     end
@@ -221,6 +258,15 @@ def default_color(color_mode)
 
     private
 
+    def to_float(string, percentage_multiplier)
+      if string[-1] == '%'
+        number = Float(string[0..-2], exception: false)
+        number && (number * percentage_multiplier)
+      else
+        Float(string, exception: false)
+      end
+    end
+
     def hex_color(hex, color_mode)
       if color_mode == :cmyk
         r, g, b = [hex[0..1], hex[2..3], hex[4..5]].map { |h| h.to_i(16) / 255.0 }

lib/prawn/svg/elements/base.rb

@@ -182,7 +182,7 @@ def apply_colors
 
       next if [nil, 'inherit', 'none'].include?(color)
 
-      color = computed_properties.color if color == 'currentColor'
+      color = computed_properties.color if color == 'currentcolor'
 
       results = Prawn::SVG::Color.parse(color, document.gradients, document.color_mode)
 

lib/prawn/svg/elements/root.rb

@@ -8,7 +8,7 @@ def parse
   end
 
   def apply
-    if [nil, 'inherit', 'none', 'currentColor'].include?(properties.fill)
+    if [nil, 'inherit', 'none', 'currentcolor'].include?(properties.fill)
       add_call 'fill_color', Prawn::SVG::Color.default_color(@document.color_mode).value
     end
 

lib/prawn/svg/properties.rb

@@ -1,5 +1,5 @@
 class Prawn::SVG::Properties
-  Config = Struct.new(:default, :inheritable?, :keywords, :keyword_restricted?, :attr, :ivar)
+  Config = Struct.new(:default, :inheritable?, :valid_values, :attr, :ivar)
 
   EM = 16
   FONT_SIZES = {
@@ -13,35 +13,35 @@ class Prawn::SVG::Properties
   }.freeze
 
   PROPERTIES = {
-    'clip-path'         => Config.new('none', false, %w[inherit none]),
-    'color'             => Config.new('', true),
-    'display'           => Config.new('inline', false, %w[inherit inline none], true),
-    'fill'              => Config.new('black', true, %w[inherit none currentColor]),
-    'fill-opacity'      => Config.new('1', true),
-    'fill-rule'         => Config.new('nonzero', true, %w[inherit nonzero evenodd]),
-    'font-family'       => Config.new('sans-serif', true),
+    'clip-path'         => Config.new('none', false, ['none', :funciri]),
+    'color'             => Config.new('', true, [:color]),
+    'display'           => Config.new('inline', false, %w[inline none]),
+    'fill'              => Config.new('black', true, ['none', 'currentcolor', :paint]),
+    'fill-opacity'      => Config.new('1', true, [:number]),
+    'fill-rule'         => Config.new('nonzero', true, %w[nonzero evenodd]),
+    'font-family'       => Config.new('sans-serif', true, [:any]),
     'font-size'         => Config.new('medium', true,
-      %w[inherit xx-small x-small small medium large x-large xx-large larger smaller]),
-    'font-style'        => Config.new('normal', true, %w[inherit normal italic oblique], true),
-    'font-variant'      => Config.new('normal', true, %w[inherit normal small-caps], true),
-    'font-weight'       => Config.new('normal', true, %w[inherit normal bold 100 200 300 400 500 600 700 800 900], true), # bolder/lighter not supported
-    'letter-spacing'    => Config.new('normal', true, %w[inherit normal]),
-    'marker-end'        => Config.new('none', true, %w[inherit none]),
-    'marker-mid'        => Config.new('none', true, %w[inherit none]),
-    'marker-start'      => Config.new('none', true, %w[inherit none]),
-    'opacity'           => Config.new('1', false),
-    'overflow'          => Config.new('visible', false, %w[inherit visible hidden scroll auto], true),
-    'stop-color'        => Config.new('black', false, %w[inherit none currentColor]),
-    'stop-opacity'      => Config.new('1', false),
-    'stroke'            => Config.new('none', true, %w[inherit none currentColor]),
-    'stroke-dasharray'  => Config.new('none', true, %w[inherit none]),
-    'stroke-linecap'    => Config.new('butt', true, %w[inherit butt round square], true),
-    'stroke-linejoin'   => Config.new('miter', true, %w[inherit miter round bevel], true),
-    'stroke-opacity'    => Config.new('1', true),
-    'stroke-width'      => Config.new('1', true),
-    'text-anchor'       => Config.new('start', true, %w[inherit start middle end], true),
-    'text-decoration'   => Config.new('none', true, %w[inherit none underline], true),
-    'dominant-baseline' => Config.new('auto', true, %w[inherit auto middle], true)
+      [:positive_length, :positive_percentage, 'xx-small', 'x-small', 'small', 'medium', 'large', 'x-large', 'xx-large', 'larger', 'smaller']),
+    'font-style'        => Config.new('normal', true, %w[normal italic oblique]),
+    'font-variant'      => Config.new('normal', true, %w[normal small-caps]),
+    'font-weight'       => Config.new('normal', true, %w[normal bold 100 200 300 400 500 600 700 800 900]), # bolder/lighter not supported
+    'letter-spacing'    => Config.new('normal', true, [:length, 'normal']),
+    'marker-end'        => Config.new('none', true, [:funciri, 'none']),
+    'marker-mid'        => Config.new('none', true, [:funciri, 'none']),
+    'marker-start'      => Config.new('none', true, [:funciri, 'none']),
+    'opacity'           => Config.new('1', false, [:number]),
+    'overflow'          => Config.new('visible', false, %w[visible hidden scroll auto]),
+    'stop-color'        => Config.new('black', false, [:color_with_lcc, 'currentcolor']),
+    'stop-opacity'      => Config.new('1', false, [:number]),
+    'stroke'            => Config.new('none', true, ['none', 'currentcolor', :paint]),
+    'stroke-dasharray'  => Config.new('none', true, [:dasharray, 'none']),
+    'stroke-linecap'    => Config.new('butt', true, %w[butt round square]),
+    'stroke-linejoin'   => Config.new('miter', true, %w[miter round bevel]),
+    'stroke-opacity'    => Config.new('1', true, [:number]),
+    'stroke-width'      => Config.new('1', true, [:positive_length, :positive_percentage]),
+    'text-anchor'       => Config.new('start', true, %w[start middle end]),
+    'text-decoration'   => Config.new('none', true, %w[none underline]),
+    'dominant-baseline' => Config.new('auto', true, %w[auto middle])
   }.freeze
 
   PROPERTIES.each do |name, value|
@@ -64,17 +64,7 @@ def load_default_stylesheet
   end
 
   def set(name, value)
-    if (config = PROPERTIES[name.to_s.downcase])
-      value = value.strip
-      keyword = value.downcase
-      keywords = config.keywords || ['inherit']
-
-      if keywords.include?(keyword)
-        value = keyword
-      elsif config.keyword_restricted?
-        value = config.default
-      end
-
+    if (config = PROPERTIES[name.to_s.downcase]) && (value = parse_value(config, value.strip))
       instance_variable_set(config.ivar, value)
     end
   end
@@ -125,4 +115,81 @@ def compute_font_size_property(value)
       FONT_SIZES[value] || value.to_f
     end
   end
+
+  def parse_value(config, value)
+    keyword = value.downcase
+
+    return 'inherit' if keyword == 'inherit'
+
+    config.valid_values.detect do |type|
+      result = parse_value_with_type(type, value, keyword)
+      break result if result
+    end
+  end
+
+  NUMBER_REGEXP = /\A[+-]?\d*(\.\d+)?\z/.freeze
+  LENGTH_REGEXP = /\A[+-]?\d*(\.\d+)?(em|ex|px|in|cm|mm|pt|pc)?\z/i.freeze
+  PERCENTAGE_REGEXP = /\A[+-]?\d*(\.\d+)?%\z/.freeze
+  POSITIVE_LENGTH_REGEXP = /\A+?\d*(\.\d+)?(em|ex|px|in|cm|mm|pt|pc)?\z/i.freeze
+  POSITIVE_PERCENTAGE_REGEXP = /\A+?\d*(\.\d+)?%\z/.freeze
+
+  def parse_value_with_type(type, value, keyword)
+    case type
+    when String
+      type == keyword ? keyword : nil
+    when :color
+      values = Prawn::SVG::CSS::ValuesParser.parse(value)
+      if values.length == 1
+        Prawn::SVG::Color.parse_color(values[0]) ? value : nil
+      end
+    when :funciri
+      case Prawn::SVG::CSS::ValuesParser.parse(value)
+      in [['url', [_url]]]
+        value
+      else
+        nil
+      end
+    when :paint
+      case Prawn::SVG::CSS::ValuesParser.parse(value)
+      in [['url', [_url]]]
+        value
+      in [['url', [_url]], other]
+        ['none', 'currentcolor'].include?(other.downcase) || Prawn::SVG::Color.parse_color(other) ? value : nil
+      in [['url', [_url]], other, ['icc-color', _args]] # rubocop:disable Lint/DuplicateBranch
+        ['none', 'currentcolor'].include?(other.downcase) || Prawn::SVG::Color.parse_color(other) ? value : nil
+      in [other, ['icc-color', _args]]
+        Prawn::SVG::Color.parse_color(other) ? value : nil
+      in [other] # rubocop:disable Lint/DuplicateBranch
+        Prawn::SVG::Color.parse_color(other) ? value : nil
+      else
+        nil
+      end
+    when :color_with_lcc
+      case Prawn::SVG::CSS::ValuesParser.parse(value)
+      in [other, ['icc-color', _args]]
+        Prawn::SVG::Color.parse_color(other) ? value : nil
+      in [other] # rubocop:disable Lint/DuplicateBranch
+        Prawn::SVG::Color.parse_color(other) ? value : nil
+      else
+        nil
+      end
+    when :dasharray
+      values = value.split(Prawn::SVG::Elements::COMMA_WSP_REGEXP)
+      values.all? { |value| value.match(POSITIVE_LENGTH_REGEXP) || value.match(POSITIVE_PERCENTAGE_REGEXP) } ? value : nil
+    when :number
+      value.match(NUMBER_REGEXP) ? value : nil
+    when :length
+      value.match(LENGTH_REGEXP) ? value : nil
+    when :percentage
+      value.match(PERCENTAGE_REGEXP) ? value : nil
+    when :positive_length
+      value.match(POSITIVE_LENGTH_REGEXP) ? value : nil
+    when :positive_percentage
+      value.match(POSITIVE_PERCENTAGE_REGEXP) ? value : nil
+    when :any
+      value
+    else
+      raise "Unknown valid value type: #{type}"
+    end
+  end
 end

spec/prawn/svg/elements/base_spec.rb

@@ -139,7 +139,7 @@
 
     it "uses the color attribute if 'currentColor' fill attribute provided" do
       expect(element).to receive(:add_call).with('fill_color', 'ff0000')
-      element.properties.fill = 'currentColor'
+      element.properties.fill = 'currentcolor'
       element.state.computed_properties.color = 'red'
       subject
     end

spec/prawn/svg/properties_spec.rb

@@ -33,9 +33,13 @@
       expect(subject.color).to eq 'Red'
     end
 
-    it "sets a 'keyword restricted' property to its default if the value doesn't match a keyword" do
-      subject.set('stroke-linecap', 'invalid')
-      expect(subject.stroke_linecap).to eq 'butt'
+    it 'ignores invalid values, retaining any previously set value' do
+      subject.set('display', 'invalid')
+      expect(subject.display).to be nil
+      subject.set('display', 'none')
+      expect(subject.display).to eq 'none'
+      subject.set('display', 'invalid')
+      expect(subject.display).to eq 'none'
     end
   end