Fix image alt text and word css contracts

TODO/02-reduce-spec-doubles.md

@@ -1,45 +1,163 @@
-# 18 — Reduce spec doubles (69 sites)
+# 18 — Reduce spec doubles
 
-**Priority:** Medium (spec quality)
-**Files:** Multiple spec files, worst offenders:
-- `spec/uniword/accessibility/rules/image_alt_text_rule_spec.rb` (~20)
-- `spec/uniword/accessibility/rules_integration_spec.rb` (~10)
-- `spec/uniword/accessibility/accessibility_checker_spec.rb` (~7)
-- `spec/uniword/math_equation_spec.rb` (~6)
+**Status:** Items 1-3 done. Item 4 outstanding. Count went 59 → 30 (29
+addressed). Both contract defects are fixed: `ImageAltTextRule` now reads
+`wp:docPr/@descr`, and `WordCss` now speaks `Wordprocessingml::Style`'s
+real API (`id`, `font_family`, `alignment.value`, plus a new `Style#italic`).
 
-## Problem
+What remains, by file:
+
+| file | sites | why |
+| --- | --- | --- |
+| `spec/uniword/math_equation_spec.rb` | 10 (7 `double`, 3 `class_double`) | Plurimath — out of scope |
+| `spec/uniword/math/plurimath_adapter_spec.rb` | 7 | Plurimath — out of scope |
+| `spec/uniword/accessibility/rules_integration_spec.rb` | 5 | table/heading rules — item 4 |
+| `spec/uniword/accessibility/accessibility_checker_spec.rb` | 2 (1 `double`, 1 `instance_double`) | item 4 |
+| 6 single-site files | 6 | item 4, fold in opportunistically |
+
+**Priority:** Medium for the spec cleanup; the two contract repairs below are
+correctness fixes.
+**Files:** see the work items below.
+
+## Status of the original note
 
-Project rule: never use `double()` in specs. Use real model instances or
-`Struct.new(...).new(...)` for plain data.
+Stale. It claimed 69 sites. Actual today, counting all RSpec double
+constructs: **59** — 55 plain `double(...)`, 3 `class_double`, 1
+`instance_double`. Its named worst offenders are no longer the worst.
+Distribution of plain doubles:
 
-69 `double()` callsites across spec/. Worst offenders use doubles to
-mock out complex model behavior, which means the tests verify that
-methods are called rather than that the actual behavior is correct.
+| file | sites |
+| --- | --- |
+| `spec/uniword/mhtml/word_css_spec.rb` | 14 |
+| `spec/uniword/accessibility/rules/image_alt_text_rule_spec.rb` | 12 |
+| `spec/uniword/math_equation_spec.rb` | 7 |
+| `spec/uniword/math/plurimath_adapter_spec.rb` | 7 |
+| `spec/uniword/accessibility/rules_integration_spec.rb` | 7 |
+| `spec/uniword/accessibility/accessibility_checker_spec.rb` | 2 (+1 `instance_double`) |
+| 6 other files | 1 each |
 
-## Fix
+The original fix section also said to use `Struct` for data-only
+doubles. `CLAUDE.md` says never use `Struct` or `OpenStruct`. Follow
+CLAUDE.md; this note predates the rule. Two `Struct.new` sites already
+exist in `spec/` — either violations to clean up or a rule needing a
+written exception. Do not add a third.
 
-For each spec:
-1. Identify what the double is mocking
-2. Replace with a real model instance constructed with the required
-   attributes
-3. If the model is hard to set up, build a small test factory
-4. Verify the test still asserts the same observable behavior
+## Problem
+
+The doubles are not lazy tests. Two of them are hiding broken production
+code, and that has to be fixed before the cleanup can even run.
 
-For data-only doubles (no behavior), use `Struct`:
-```ruby
-# Before
-let(:doc) { double("Doc", paragraphs: [...]) }
+### `ImageAltTextRule` crashes on any real document with an image
 
-# After
-DocumentStub = Struct.new(:paragraphs)
-let(:doc) { DocumentStub.new([...]) }
 ```
+document.images               => [Uniword::Wordprocessingml::Drawing]
+Drawing responds to alt_text? => false
+rule.check(doc)               => NoMethodError:
+                                 undefined method 'alt_text' for
+                                 an instance of Wordprocessingml::Drawing
+```
+
+`DocumentRoot#images` (`document_root.rb:299`) is documented
+`@return [Array<Drawing>]`. `ImageAltTextRule#check` calls
+`image.alt_text` (`image_alt_text_rule.rb:21`). `alt_text` exists only on
+`Uniword::Image` (`image.rb:28`), a different flat model that `#images`
+never returns. The 12 doubles inject `alt_text` and paper over the crash.
+
+### `WordCss` expects an API the real objects do not have
+
+The doubles invent `style_id`, `font` and `italic`. Real
+`Wordprocessingml::Style` exposes `id` (`style.rb:130`) and
+`font_family` (`style.rb:200`), and has no `italic` reader.
+`Mhtml::StylesConfiguration#styles` is declared
+`attribute :styles, :hash` (`styles_configuration.rb:15`), not an
+enumerable of style objects.
+
+So "replace the double with a real instance" cannot be done for these
+two without fixing production first.
+
+## 1. Repair the accessibility contract (prerequisite)
+
+Decide what `ImageAltTextRule` operates on. Most likely real OOXML
+drawings, reading the description off `wp:docPr/@descr`, not a phantom
+`alt_text` on `Drawing`. Then replace the 12 doubles in
+`image_alt_text_rule_spec.rb` and the dependent doubles in
+`rules_integration_spec.rb` and `accessibility_checker_spec.rb`.
+
+Regression test must be a parsed document containing a real drawing, and
+must fail before the fix.
+
+## 2. Repair the WordCss contract (prerequisite)
 
-For doubles that mock method calls, refactor the test to assert on
-observable output (return values, side effects) rather than on
-method-call counts.
+Decide whether `WordCss` consumes WordprocessingML styles or MHTML style
+hashes, then make the code and the type agree. Replace the 7 style-side
+doubles (5 `Style`, 2 `StylesConfiguration`).
+
+The namespace is **not** ambiguous, despite two classes sharing the name:
+`word_css.rb:36` does `styles_config.styles.map` and then calls style-object
+methods, while `Mhtml::StylesConfiguration#styles` is a plain `:hash` of CSS
+properties. So `WordCss` consumes the **WordprocessingML** configuration. State
+that as the contract.
+
+The real work is adapting `WordCss` to `Wordprocessingml::Style`'s actual API:
+it exposes `id` (`style.rb:130`) and `font_family` (`style.rb:200`), has a
+wrapper-backed `alignment`, and has no `italic` reader at all — whereas the
+doubles invent `style_id`, `font` and `italic`.
+
+## 3. WordCss numbering doubles
+
+The other 7 doubles in that file are 5 `NumberingInstance` and 2
+`NumberingConfiguration`.
+
+**This is independent of item 2 and can be done on its own.** `word_css.rb:50` calls
+`numbering_config.instances`, and only `Wordprocessingml::NumberingConfiguration`
+declares `instances` (`numbering_configuration.rb:16`); the MHTML one does not.
+So the namespace is settled by the call itself, and a real
+`NumberingConfiguration` plus `NumberingInstance` drops straight in.
+
+Two earlier revisions got this wrong in both directions — first calling it
+unblocked without checking, then calling it blocked on a namespace ambiguity
+that does not exist. The call site resolves it.
+
+## 4. The remaining doubles
+
+Whatever is left after items 1-3, largest file first, one file per PR.
+For each double: name the real class, construct it the way production
+does, keep the assertion on observable behavior rather than call counts.
+
+If a model turns out to be awkward to construct, that awkwardness is a
+finding about the model's API. Record it. Do not paper over it with
+another double.
 
 ## Verification
 
-`grep -rn "double(" spec/uniword/ | wc -l` should trend toward 0.
-Tests pass.
+- Per file: `bundle exec rspec <file>` green **and proven able to fail**.
+  Break the behavior under test once and watch it go red. Given what items
+  1 and 2 uncovered, a green double-free spec that cannot fail is the
+  main risk here.
+- The count drops from 59 by the number actually addressed. Count all three
+  constructs (`double`, `class_double`, `instance_double`), not just plain
+  `double(` — an earlier revision of this note undercounted by missing
+  `class_double`. **Do not state a target of zero.** List what remains
+  explicitly.
+- `bundle exec rubocop <touched files>`
+
+## Out of scope
+
+- `plurimath_adapter_spec.rb` and the Plurimath parts of
+  `math_equation_spec.rb` (14 sites). These stand in for an external
+  gem, so replacing them is a dependency-contract question, not spec
+  hygiene. Needs `/dependency-contract-check` against Plurimath first.
+  Should become its own TODO.
+- The 6 single-site files. Fold in only if the file is already open for
+  another reason.
+
+## Expected outcome, stated honestly
+
+This will not reduce the count much on its own. Its real output is the two
+contract defects in items 1 and 2.
+
+Severity differs between them. `ImageAltTextRule` is reachable and crashes on a
+real document. `WordCss.generate_style_css` / `generate_list_css` have **no
+caller under `lib/`** — only their specs call them — so they are broken public
+helpers rather than a demonstrated downstream failure. Fix both, but do not
+describe WordCss as a live production bug.

lib/uniword/accessibility/rules/image_alt_text_rule.rb

@@ -18,51 +18,74 @@ def check(document)
           violations = []
 
           document.images.each_with_index do |image, index|
-            # Check for alt text existence
-            if image.alt_text.nil? || image.alt_text.strip.empty?
+            alt_text = image.alt_text
+
+            # Drawing#alt_text already treats a blank descr as absent.
+            if alt_text.nil?
               violations << create_violation(
                 message: "Image #{index + 1} missing alternative text",
                 element: image,
                 severity: @config[:severity] || :error,
-                suggestion: @config[:suggestion] ||
-                  "Add descriptive alternative text using image.alt_text = '...'",
+                suggestion: missing_alt_text_suggestion(image),
               )
               next # Skip quality checks if no alt text
             end
 
             # Check alt text quality if enabled
             next unless @config[:check_quality]
 
-            violations.concat(check_alt_text_quality(image, index))
+            violations.concat(check_alt_text_quality(image, alt_text, index))
           end
 
           violations
         end
 
         private
 
+        # Word's older Alt Text dialog had both a Title and a Description
+        # field, and templates from that era put the description in Title.
+        # Title is a caption, not a text alternative, so the image still
+        # counts as undescribed — but say where the text already is.
+        #
+        # @param drawing [Wordprocessingml::Drawing] Drawing being reported
+        # @return [String] Suggestion text
+        def missing_alt_text_suggestion(drawing)
+          title = drawing.alt_title
+          if title
+            return "Move the drawing's docPr title (#{title.inspect}) into " \
+                   "its descr attribute; title is a caption, not alt text"
+          end
+
+          @config[:suggestion] ||
+            "Add descriptive alternative text via the drawing's " \
+            "docPr descr attribute"
+        end
+
         # Check quality of alt text
         #
-        # @param image [Image] Image to check
+        # @param image [Wordprocessingml::Drawing] Drawing to check
+        # @param alt_text [String] Alternative text read from the drawing
         # @param index [Integer] Image index
         # @return [Array<AccessibilityViolation>] Quality violations
-        def check_alt_text_quality(image, index)
+        def check_alt_text_quality(image, alt_text, index)
           violations = []
 
           # Check minimum length
-          if @config[:min_length] && image.alt_text.length < @config[:min_length]
+          if @config[:min_length] && alt_text.length < @config[:min_length]
             violations << create_violation(
-              message: "Image #{index + 1} has insufficient alt text (too short: #{image.alt_text.length} chars)",
+              message: "Image #{index + 1} has insufficient alt text " \
+                       "(too short: #{alt_text.length} chars)",
               element: image,
               severity: :warning,
               suggestion: "Alternative text should describe the image content meaningfully (min #{@config[:min_length]} chars)",
             )
           end
 
           # Check maximum length
-          if @config[:max_length] && image.alt_text.length > @config[:max_length]
+          if @config[:max_length] && alt_text.length > @config[:max_length]
             violations << create_violation(
-              message: "Image #{index + 1} has excessive alt text (too long: #{image.alt_text.length} chars)",
+              message: "Image #{index + 1} has excessive alt text " \
+                       "(too long: #{alt_text.length} chars)",
               element: image,
               severity: :warning,
               suggestion: "Keep alternative text concise (max #{@config[:max_length]} chars)",
@@ -71,12 +94,12 @@ def check_alt_text_quality(image, index)
 
           # Check for unhelpful generic text
           unhelpful = %w[image picture photo img graphic icon]
-          alt_lower = image.alt_text.downcase
+          alt_lower = alt_text.downcase
           if unhelpful.any? do |word|
             alt_lower == word || alt_lower.start_with?("#{word} of")
           end
             violations << create_violation(
-              message: "Image #{index + 1} has generic alt text: '#{image.alt_text}'",
+              message: "Image #{index + 1} has generic alt text: '#{alt_text}'",
               element: image,
               severity: :warning,
               suggestion: "Describe what the image shows, not that it's an image",

lib/uniword/builder/image_builder.rb

@@ -147,6 +147,7 @@ def self.create_drawing(document, path, width: nil, height: nil,
         inline.doc_properties = WpDrawing::DocProperties.new(
           id: deterministic_id("inline", path),
           name: File.basename(path, ".*"),
+          descr: alt_text,
         )
         inline.graphic = build_graphic(r_id, w, h)
 
@@ -227,6 +228,7 @@ def self.create_floating(document, path, width: nil, height: nil, alt_text: nil,
         anchor.doc_properties = WpDrawing::DocProperties.new(
           id: deterministic_id("anchor", path),
           name: File.basename(path, ".*"),
+          descr: alt_text,
         )
         anchor.graphic = build_graphic(r_id, w, h)
 

lib/uniword/builder/sdt_builder.rb

@@ -63,19 +63,27 @@ def alias(value)
 
       # Set the lock / content cannot be edited
       #
+      # Passing false writes an explicit <w:temporary w:val="false"/> rather
+      # than leaving the flag out, so the intent survives a round trip.
+      #
       # @param value [Boolean] Lock content (default true)
       # @return [self]
-      def lock(_value = true)
-        properties.temporary = Wordprocessingml::StructuredDocumentTag::Temporary.new
+      def lock(value = true)
+        properties.temporary =
+          Wordprocessingml::StructuredDocumentTag::Temporary.new(value: value)
         self
       end
 
       # Set placeholder text showing the placeholder header
       #
+      # Passing false writes an explicit <w:showingPlcHdr w:val="false"/>,
+      # the same way #lock does.
+      #
       # @param value [Boolean] Show placeholder (default true)
       # @return [self]
-      def showing_placeholder(_value = true)
-        properties.showing_placeholder_header = Wordprocessingml::StructuredDocumentTag::ShowingPlaceholderHeader.new
+      def showing_placeholder(value = true)
+        klass = Wordprocessingml::StructuredDocumentTag::ShowingPlaceholderHeader
+        properties.showing_placeholder_header = klass.new(value: value)
         self
       end
 

lib/uniword/mhtml/word_css.rb

@@ -119,29 +119,37 @@ def self.build_section_div_rule(section_name)
 
       # Build a CSS rule for a style.
       #
-      # @param style [Style] The style
+      # @param style [Wordprocessingml::Style] The style
       # @return [String, nil] The CSS rule or nil
       def self.build_style_rule(style)
         return nil unless style
 
+        # w:styleId is optional in the schema, and without it there is no
+        # class selector to hang the rule on.
+        style_id = style.id
+        return nil if style_id.nil? || style_id.empty?
+
         properties = []
 
         # Font properties
-        properties << "font-family: '#{style.font}'" if style.font
+        font_family = style.font_family
+        properties << "font-family: '#{font_family}'" if font_family
         if style.font_size
-          properties << "font-size: #{CssNumberFormatter.format(style.font_size, 'pt',
-                                                                precision: 1)}"
+          # w:sz is in half-points, so it needs the font-size formatter.
+          formatted_size = CssNumberFormatter
+            .format_font_size(style.font_size, precision: 1)
+          properties << "font-size: #{formatted_size}"
         end
         properties << "font-weight: bold" if style.bold
         properties << "font-style: italic" if style.italic
 
-        # Paragraph properties
-        properties << "text-align: #{style.alignment}" if style.alignment
+        # Paragraph properties, whose alignment is a w:jc wrapper element
+        alignment = style.alignment&.value
+        properties << "text-align: #{alignment}" if alignment
 
         return nil if properties.empty?
 
-        selector = ".#{style.style_id}"
-        "#{selector} {\n  #{properties.join(";\n  ")};\n}"
+        ".#{style_id} {\n  #{properties.join(";\n  ")};\n}"
       end
 
       # Build a CSS rule for list numbering.