From 46a8f9fe1540fc55d9a50455f74fee791568844b Mon Sep 17 00:00:00 2001 From: Mog Nesbitt Date: Sat, 4 Jan 2025 12:47:00 +1300 Subject: [PATCH] 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 | 46 +++++++++ lib/prawn/svg/elements/base.rb | 2 +- lib/prawn/svg/elements/root.rb | 2 +- lib/prawn/svg/properties.rb | 147 +++++++++++++++++++-------- spec/prawn/svg/elements/base_spec.rb | 2 +- spec/prawn/svg/properties_spec.rb | 10 +- 6 files changed, 163 insertions(+), 46 deletions(-) diff --git a/lib/prawn/svg/color.rb b/lib/prawn/svg/color.rb index 0a744f5..8c2fe70 100644 --- a/lib/prawn/svg/color.rb +++ b/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 } diff --git a/lib/prawn/svg/elements/base.rb b/lib/prawn/svg/elements/base.rb index 7f4c7eb..6c29c25 100644 --- a/lib/prawn/svg/elements/base.rb +++ b/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) diff --git a/lib/prawn/svg/elements/root.rb b/lib/prawn/svg/elements/root.rb index 1cec90a..db5fd4c 100644 --- a/lib/prawn/svg/elements/root.rb +++ b/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 diff --git a/lib/prawn/svg/properties.rb b/lib/prawn/svg/properties.rb index f83ab60..7976813 100644 --- a/lib/prawn/svg/properties.rb +++ b/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 diff --git a/spec/prawn/svg/elements/base_spec.rb b/spec/prawn/svg/elements/base_spec.rb index 98452a8..b0ab34e 100644 --- a/spec/prawn/svg/elements/base_spec.rb +++ b/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 diff --git a/spec/prawn/svg/properties_spec.rb b/spec/prawn/svg/properties_spec.rb index cc2f168..122f1a2 100644 --- a/spec/prawn/svg/properties_spec.rb +++ b/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