resolves asciidoctor#2134 use base-border-color as default border col…

…or; control appearance of border on border-width value alone * if border-color for element is not set or nil, fall back to base-border-color * don't set default value for base_border_color key on theme object * change fallback value for table border color to base-border-color * change fallback value for thematic break stroke color to base-border-color * don't use base-border-width as fallback width for admonition column rule * interpret nil value in table border color array as transparent * interpret nil value in table border width array as 0
mojavelinux · May 4, 2022 · 1163c52 · 1163c52
1 parent b56c8a5
commit 1163c52
Show file tree

Hide file tree

Showing 4 changed files with 37 additions and 44 deletions.
diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc
@@ -15,6 +15,7 @@ Enhancements::
 * allow theme to configure which end the caption is placed for a block image (#2115)
 * add `Page#imported` method to mark page as imported (which suppresses running contennt)
 * add support for `smallcaps` text transform by replacing lowercase letters with small capital variants (#1192)
+* use `base-border-color` as default border color; control appearance of border on `border-width` value alone (#2134)
 
 Bug Fixes::
 

diff --git a/lib/asciidoctor/pdf/converter.rb b/lib/asciidoctor/pdf/converter.rb
@@ -463,7 +463,6 @@ def load_theme doc
       end
 
       def prepare_theme theme
-        theme.base_border_color ||= '000000'
         theme.base_border_width ||= 0
         theme.base_font_color ||= '000000'
         theme.base_font_size ||= 12
@@ -481,8 +480,9 @@ def prepare_theme theme
         theme.list_item_spacing ||= 0
         theme.description_list_term_spacing ||= 0
         theme.description_list_description_indent ||= 0
+        theme.table_border_color ||= (theme.base_border_color || '000000')
         theme.table_border_width ||= 0.5
-        theme.thematic_break_border_color ||= '000000'
+        theme.thematic_break_border_color ||= (theme.base_border_color || '000000')
         theme.image_border_width ||= 0
         theme.code_linenum_font_color ||= '999999'
         theme.callout_list_margin_top_after_code ||= 0
@@ -905,8 +905,8 @@ def convert_admonition node
           pad_box [0, cpad[1], 0, lpad[3]] do
             if extent
               label_height = extent.single_page_height || cursor
-              if (rule_color = @theme.admonition_column_rule_color) &&
-                  (rule_width = @theme.admonition_column_rule_width || @theme.base_border_width) > 0
+              if (rule_color = @theme.admonition_column_rule_color || @theme.base_border_color) &&
+                  (rule_width = @theme.admonition_column_rule_width || 0) > 0
                 rule_style = @theme.admonition_column_rule_style&.to_sym || :solid
                 float do
                   extent.each_page do |first_page, last_page|
@@ -1060,7 +1060,7 @@ def convert_quote_or_verse node
         category = node.context == :quote ? :quote : :verse
         # NOTE: b_width and b_left_width are mutually exclusive
         if (b_left_width = @theme[%(#{category}_border_left_width)]) && b_left_width > 0
-          b_color = @theme[%(#{category}_border_color)]
+          b_color = @theme[%(#{category}_border_color)] || @theme.base_border_color
         else
           b_left_width = nil
           b_width = nil if (b_width = @theme[%(#{category}_border_width)]) == 0
@@ -1658,7 +1658,7 @@ def convert_image node, opts = {}
       end
 
       def draw_image_border top, w, h, alignment
-        if (Array @theme.image_border_width).any? {|it| it&.> 0 } && @theme.image_border_color
+        if (Array @theme.image_border_width).any? {|it| it&.> 0 } && (@theme.image_border_color || @theme.base_border_color)
           if (@theme.image_border_fit || 'content') == 'auto'
             bb_width = bounds.width
           elsif alignment == :center
@@ -2194,12 +2194,12 @@ def convert_table node
 
         rect_side_names = [:top, :right, :bottom, :left]
         grid_axis_names = [:rows, :cols]
-        border_color = (rect_side_names.zip expand_rect_values theme.table_border_color, theme.base_border_color).to_h
+        border_color = (rect_side_names.zip expand_rect_values theme.table_border_color, 'transparent').to_h
         border_style = (rect_side_names.zip (expand_rect_values theme.table_border_style, :solid).map(&:to_sym)).to_h
-        border_width = (rect_side_names.zip expand_rect_values theme.table_border_width, theme.base_border_width).to_h
-        grid_color = (grid_axis_names.zip expand_grid_values (theme.table_grid_color || [border_color[:top], border_color[:left]]), theme.base_border_color).to_h
+        border_width = (rect_side_names.zip expand_rect_values theme.table_border_width, 0).to_h
+        grid_color = (grid_axis_names.zip expand_grid_values (theme.table_grid_color || [border_color[:top], border_color[:left]]), 'transparent').to_h
         grid_style = (grid_axis_names.zip (expand_grid_values (theme.table_grid_style || [border_style[:top], border_style[:left]]), :solid).map(&:to_sym)).to_h
-        grid_width = (grid_axis_names.zip expand_grid_values (theme.table_grid_width || [border_width[:top], border_width[:left]]), theme.base_border_width).to_h
+        grid_width = (grid_axis_names.zip expand_grid_values (theme.table_grid_width || [border_width[:top], border_width[:left]]), 0).to_h
 
         if table_header_size
           head_border_bottom_color = theme.table_head_border_bottom_color || grid_color[:rows]
@@ -2355,7 +2355,9 @@ def convert_table node
 
       def convert_thematic_break node
         theme_margin :thematic_break, :top
-        stroke_horizontal_rule @theme.thematic_break_border_color, line_width: @theme.thematic_break_border_width, line_style: (@theme.thematic_break_border_style&.to_sym || :solid)
+        stroke_horizontal_rule @theme.thematic_break_border_color,
+          line_width: @theme.thematic_break_border_width,
+          line_style: (@theme.thematic_break_border_style&.to_sym || :solid)
         theme_margin :thematic_break, ((block_next = next_enclosed_block node) ? :bottom : :top), block_next || true
       end
 
@@ -2997,7 +2999,7 @@ def ink_heading string, opts = {}
             }.merge(opts)
           end
           if h_category && @theme[%(#{h_category}_border_width)] &&
-              @theme[%(#{h_category}_border_color)] && page_number == start_page_number
+              (@theme[%(#{h_category}_border_color)] || @theme.base_border_color) && page_number == start_page_number
             float do
               bounding_box [bounds.left, start_cursor], width: bounds.width, height: start_cursor - cursor do
                 theme_fill_and_stroke_bounds h_category
@@ -3457,7 +3459,7 @@ def ink_running_content periphery, doc, skip = [1, 1], body_start_page_number =
           theme_font periphery do
             canvas do
               bounding_box [trim_styles[:content_left][side], trim_styles[:top][side]], width: trim_styles[:content_width][side], height: trim_styles[:height] do
-                if (trim_column_rule_width = trim_styles[:column_rule_width]) > 0
+                if trim_styles[:column_rule_color] && (trim_column_rule_width = trim_styles[:column_rule_width]) > 0
                   trim_column_rule_spacing = trim_styles[:column_rule_spacing]
                 else
                   trim_column_rule_width = nil
@@ -3565,12 +3567,12 @@ def allocate_running_content_layout doc, page, periphery, cache
             # NOTE: we've already verified this property is set
             height: (trim_height = @theme[%(#{periphery}_height)]),
             bg_color: (resolve_theme_color %(#{periphery}_background_color).to_sym),
-            border_color: (trim_border_color = resolve_theme_color %(#{periphery}_border_color).to_sym),
+            border_width: (trim_border_width = @theme[%(#{periphery}_border_width)] || 0),
+            border_color: trim_border_width > 0 ? (resolve_theme_color %(#{periphery}_border_color).to_sym, @theme.base_border_color) : nil,
             border_style: (@theme[%(#{periphery}_border_style)]&.to_sym || :solid),
-            border_width: (trim_border_width = trim_border_color ? @theme[%(#{periphery}_border_width)] || @theme.base_border_width : 0),
-            column_rule_color: (trim_column_rule_color = resolve_theme_color %(#{periphery}_column_rule_color).to_sym),
+            column_rule_width: (trim_column_rule_width = @theme[%(#{periphery}_column_rule_width)] || 0),
+            column_rule_color: trim_column_rule_width > 0 ? (resolve_theme_color %(#{periphery}_column_rule_color).to_sym) : nil,
             column_rule_style: (@theme[%(#{periphery}_column_rule_style)]&.to_sym || :solid),
-            column_rule_width: (trim_column_rule_color ? @theme[%(#{periphery}_column_rule_width)] || 0 : 0),
             column_rule_spacing: (@theme[%(#{periphery}_column_rule_spacing)] || 0),
             valign: valign_offset ? [valign, valign_offset] : valign,
             img_valign: @theme[%(#{periphery}_image_vertical_align)],
@@ -3863,9 +3865,9 @@ def resolve_text_transform key, use_fallback = true
 
       # QUESTION: should we pass a category as an argument?
       # QUESTION: should we make this a method on the theme ostruct? (e.g., @theme.resolve_color key, fallback)
-      def resolve_theme_color key, fallback_color = nil
-        if (color = @theme[key.to_s]) && color != 'transparent'
-          color
+      def resolve_theme_color key, fallback_color = nil, transparent_color = fallback_color
+        if (color = @theme[key.to_s])
+          color == 'transparent' ? transparent_color : color
         else
           fallback_color
         end
@@ -3876,7 +3878,7 @@ def resolve_font_kerning keyword
       end
 
       def theme_fill_and_stroke_bounds category, opts = {}
-        fill_and_stroke_bounds opts[:background_color], @theme[%(#{category}_border_color)],
+        fill_and_stroke_bounds opts[:background_color], @theme[%(#{category}_border_color)] || @theme.base_border_color,
           line_width: @theme[%(#{category}_border_width)],
           line_style: (@theme[%(#{category}_border_style)]&.to_sym || :solid),
           radius: @theme[%(#{category}_border_radius)]
@@ -3902,11 +3904,9 @@ def theme_fill_and_stroke_block category, extent, opts = {}
           ink_caption node_with_caption, category: category if node_with_caption
           return
         end
-        if (b_color = @theme[%(#{category}_border_color)]) == 'transparent'
-          b_color = @page_bg_color
-        end
+        b_color = resolve_theme_color %(#{category}_border_color).to_sym, @theme.base_border_color, @page_bg_color
         b_radius ||= (@theme[%(#{category}_border_radius)] || 0) + (b_width || 0)
-        if b_width && b_color
+        if b_width
           if b_color == @page_bg_color # let page background cut into block background
             b_gap_color, b_shift = @page_bg_color, (b_width * 0.5)
           elsif (b_gap_color = bg_color) && b_gap_color != b_color

diff --git a/spec/admonition_spec.rb b/spec/admonition_spec.rb
@@ -1077,22 +1077,18 @@
       end
     end
 
-    it 'should assign default width to column rule if key is not specified' do
+    it 'should not assign default width to column rule if key is not specified' do
       pdf_theme = {
         admonition_column_rule_color: '222222',
         admonition_column_rule_width: nil,
+        base_border_width: nil,
       }
       pdf = to_pdf <<~'EOS', pdf_theme: pdf_theme, analyze: :line
       TIP: You can use the theme to customize the color and width of the column rule.
       EOS
 
       lines = pdf.lines
-      (expect lines).to have_size 1
-      column_rule = lines[0]
-      (expect column_rule[:from][:x]).to eql column_rule[:to][:x]
-      (expect column_rule[:color]).to eql '222222'
-      (expect column_rule[:width]).to eql 0.5
-      (expect column_rule[:style]).to eql :solid
+      (expect lines).to be_empty
     end
 
     it 'should not fail if base border width is not set when using original theme' do
@@ -1134,7 +1130,7 @@
       (expect column_rule2[:from][:x] - column_rule1[:from][:x]).to eql 2.0
     end
 
-    it 'should use base border width for column rule if column rule width is nil' do
+    it 'should not use base border width for column rule if column rule width is nil' do
       pdf_theme = {
         base_border_width: 2,
         admonition_column_rule_color: '222222',
@@ -1145,11 +1141,7 @@
       EOS
 
       lines = pdf.lines
-      (expect lines).to have_size 1
-      column_rule = lines[0]
-      (expect column_rule[:from][:x]).to eql column_rule[:to][:x]
-      (expect column_rule[:color]).to eql '222222'
-      (expect column_rule[:width]).to eql 2
+      (expect lines).to be_empty
     end
 
     it 'should allow theme to add border', visual: true do
@@ -1233,8 +1225,8 @@
       (expect (text_left - left).to_f).to eql 72.0
     end
 
-    it 'should allow theme to disable column rule by setting color to nil' do
-      pdf_theme = { admonition_column_rule_color: nil }
+    it 'should allow theme to disable column rule by setting width to 0' do
+      pdf_theme = { admonition_column_rule_width: 0 }
       pdf = to_pdf <<~'EOS', pdf_theme: pdf_theme, analyze: :line
       TIP: You can use the theme to add a background color.
       EOS

diff --git a/spec/running_content_spec.rb b/spec/running_content_spec.rb
@@ -1494,12 +1494,12 @@
       (expect to_file).to visually_match 'running-content-border-style.pdf'
     end
 
-    it 'should use base border width and solid style if border width and style are not specified', visual: true do
+    it 'should use base border color and solid style if border width and style are not specified', visual: true do
       pdf_theme = {
-        base_border_width: 1,
-        footer_border_width: nil,
+        base_border_color: '000000',
+        footer_border_color: nil,
+        footer_border_width: 1,
         footer_border_style: nil,
-        footer_border_color: '000000',
       }
 
       to_file = to_pdf_file 'content', 'running-content-border-defaults.pdf', enable_footer: true, pdf_theme: pdf_theme