feat: name the region that would not settle, and document the readiness recipes (#271, #273)

docs/configuration.md

@@ -356,6 +356,63 @@ test 'stability_time_limit' do
 end
 ```
 
+### When the page will not settle
+
+The failure names the area that kept changing, and hands you the command that
+fixes it:
+
+```
+Could not get stable screenshot for 'index-with-ticker' within 1.2s (5 attempts).
+  The page kept changing in 1 area, over 4 attempt pairs:
+    [67,50,213,68] (left,top,right,bottom edges) -- 0.55% of the 800x600 image, changed in 4 of 4 pairs
+  Always the same area, in every pair: that is an animation, clock, carousel or live counter.
+  Exclude it and the page is stable without waiting:
+    assert_matches_screenshot "index-with-ticker", skip_area: [67,50,213,68]
+  <one annotated attempt image per line>
+```
+
+The coordinates are measured, not guessed: they come from the comparisons the
+gem just ran between consecutive attempts, so pasting the suggested `skip_area`
+in works.
+
+**One caveat, and it is a real one.** The suggested box is the union of what
+changed across the attempts that ran — a *sample* of the animation, not a proven
+bound on it. If the moving thing also changes SIZE between frames (proportional
+text of varying width in a centred box is the common case), a later frame can
+render a pixel or two outside the box that was measured, and the masked run fails
+again with a much smaller region. Paste the new suggestion, or widen the box by a
+few pixels; it converges. For the usual case — a clock, spinner or counter
+repainting inside a fixed element — the extent does not move and the first
+suggestion is the whole fix.
+
+Read the "changed in N of N pairs" line before acting on it:
+
+- **N of N — one area, every single pair.** Something is animating in one place:
+  a clock, a carousel, a spinner, a live counter. `skip_area` is the fix, and
+  since masking no longer waits it costs nothing.
+- **Fewer than N, or several areas each changing once.** The page is still
+  *rendering*, not animating. Masking those areas would hide real content. The
+  message says so and suggests nothing to mask — settle the page in a
+  [readiness block](#the-readiness-block) instead, or raise `wait:`.
+
+### Knowing what the waiting cost
+
+Every run that waited for stability ends with what it actually paid:
+
+```
+[snap_diff] 34 screenshots waited for the page to settle: 0.19s and 2 attempts at worst. Every screenshot settled on its first retry, so a lower stability_time_limit would cost less per screenshot.
+```
+
+Two attempts is the floor — one capture, plus the retry that matched it. Hitting
+the floor across the whole run means no page was ever still moving when the
+retry was taken, so every `stability_time_limit` sleep was spent on a page that
+had already stopped. That is the evidence for tuning it *down*; without it,
+lowering the setting is guesswork, and guesswork loses to `sleep`.
+
+Run-level and silent when nothing waited, for the same reason as the
+never-matched-selector line: a line printed on every screenshot is a line
+people learn to skip.
+
 ### Maximum wait limit
 
 When the `stability_time_limit` is set, but no stable screenshot can be taken, a timeout occurs.
@@ -489,7 +546,9 @@ screenshot. That wait is gone.)
 So content that arrives late — lazy-loaded images, JS-injected widgets, anything
 behind an unresolved fetch — has to be settled *before* the assertion, or its
 mask will be empty and the unstable region will be compared. Settle it in the
-readiness block described below.
+[readiness block](#the-readiness-block) described below; the two cases people
+hit most (webfonts and lazy images) are written out in
+[Recipes](#recipes).
 
 If a selector matched nothing in *every* screenshot of a run, the end-of-run
 summary names it:
@@ -538,6 +597,58 @@ binds the block by Ruby's `{}`/`do...end` precedence rather than by intent, so
 the matcher does not take one. In Cucumber the DSL is in the World, so step
 definitions pass a block the same way a Minitest test does.
 
+#### Recipes
+
+These are the workarounds people hand-roll anyway. The block is where they
+belong, because that is the only place they are skipped when screenshots are
+off.
+
+**Webfonts.** A font swapping in mid-capture reflows every line of text that
+uses it, so the same page renders two different ways depending on when the
+screenshot lands — the classic "it only fails on CI" flake. People usually
+paper over it with a `skip_area`, a loosened `tolerance` and a retry, all three
+of which weaken the comparison everywhere. Wait for the swap instead:
+
+```ruby
+assert_matches_screenshot 'home' do
+  page.evaluate_async_script(
+    'var done = arguments[0]; document.fonts.ready.then(function(){ done(true) })'
+  )
+end
+```
+
+`document.fonts.ready` resolves once every font used by the current layout has
+loaded (or failed) — and on a warm cache that has already happened, so the call
+returns on the first round trip. It is a Font Loading API promise, supported in
+every browser Capybara drives.
+
+There is deliberately no built-in font wait: it would be a browser round trip
+imposed on every screenshot in every suite, and a driver-compatibility surface
+the gem would own forever, in exchange for one line you can write yourself.
+
+**Lazy-loaded images.** Anything behind `loading="lazy"`, an IntersectionObserver
+or an unresolved fetch is simply not there when the screenshot is taken. Force
+it in, then come back:
+
+```ruby
+assert_matches_screenshot 'gallery', skip_area: ['article img'] do
+  scroll_to :bottom
+  assert_text 'End of gallery'
+  scroll_to :top
+end
+```
+
+Note the order: `skip_area: ['article img']` masks what exists **at assertion
+time**, so the images have to be in the DOM before the mask is resolved. A
+selector for content that has not loaded yet produces an empty mask and the
+unstable region is compared anyway — see [Skipping an area](#skipping-an-area).
+
+**What does not belong here.** Waits that every screenshot needs regardless
+(`disable_animations`, `hide_caret`) are configuration, not readiness. And the
+block runs once per assertion, not once per stability retry, so it cannot be
+used to nudge a page that keeps moving — for that, see
+[When the page will not settle](#when-the-page-will-not-settle).
+
 The arguments are `[left, top, right, bottom]` for the area you want to ignore.  You can also set this globally:
 
 ```ruby

lib/snap_diff/attempts_reporter.rb

@@ -1,23 +1,48 @@
 # frozen_string_literal: true
 
 require "fileutils"
+require "json"
 
 require "snap_diff/comparison"
+require "snap_diff/region"
 
 module SnapDiff
+  # The message a user reads when a page would not hold still.
+  #
+  # It used to be a bare list of attempt paths (#271). That made diagnosis a
+  # matter of opening N PNGs and eyeballing them, and faced with that versus
+  # `sleep 2`, sleep wins -- so the suite got SLOWER as a consequence of the
+  # diagnosis being hard. The information needed was already here and thrown
+  # away: every consecutive pair of attempts is compared, and that comparison
+  # knows the region that changed.
+  #
+  # So name it, and hand over the escape hatch that removes the need to sleep.
   class AttemptsReporter
+    # One place on the page that changed between attempts, and how many of
+    # the attempt pairs it showed up in. `pairs == total` means it changed
+    # EVERY time -- an animation, and `skip_area` is the fix. Fewer means the
+    # page was still rendering there, and masking would hide a real change.
+    Area = Struct.new(:region, :pairs)
+
     def initialize(snapshot, comparison_options, stability_options = {})
       @snapshot = snapshot
       @comparison_options = comparison_options
       @wait = stability_options[:wait]
     end
 
     def generate
-      attempts_screenshot_paths = @snapshot.find_attempts_paths
+      # Sorted: `attempt_%02i` sorts lexically in capture order, and the
+      # reader wants oldest-first regardless of what the glob hands back.
+      attempts_screenshot_paths = @snapshot.find_attempts_paths.sort
 
-      annotate_attempts(attempts_screenshot_paths)
+      areas, dimensions = annotate_attempts(attempts_screenshot_paths)
 
-      "Could not get stable screenshot within #{@wait}s:\n#{attempts_screenshot_paths.join("\n")}"
+      [
+        "Could not get stable screenshot for '#{@snapshot.full_name}' within #{@wait}s " \
+          "(#{attempts_screenshot_paths.size} attempts).",
+        *diagnosis_lines(areas, dimensions),
+        *attempts_screenshot_paths
+      ].join("\n")
     end
 
     def build_comparison_for(attempt_path, previous_attempt_path)
@@ -26,14 +51,25 @@ def build_comparison_for(attempt_path, previous_attempt_path)
 
     private
 
+    # Annotates each attempt with its diff against the next one -- and keeps
+    # the regions, which is the whole point of #271.
+    #
+    # @return [Array(Array<Area>, Array(Integer, Integer))] the clustered
+    #   changed areas and the [width, height] of the attempts.
     def annotate_attempts(attempts_screenshot_paths)
+      regions = []
+      dimensions = nil
       previous_file = nil
+
       attempts_screenshot_paths.reverse_each do |file_name|
         if previous_file && File.exist?(previous_file)
           attempts_comparison = build_comparison_for(file_name, previous_file)
 
           if attempts_comparison.different?
             FileUtils.mv(attempts_comparison.reporter.annotated_base_image_path, previous_file, force: true)
+            region = attempts_comparison.difference.region
+            regions << region if region
+            dimensions ||= dimensions_of(attempts_comparison)
           else
             warn "[capybara-screenshot-diff] Some attempts was stable, but mistakenly marked as not: " \
                    "#{previous_file} and #{file_name} are equal"
@@ -45,7 +81,123 @@ def annotate_attempts(attempts_screenshot_paths)
         previous_file = file_name
       end
 
-      previous_file
+      # Worst offender first: the area that changed in the most pairs is the
+      # one to act on, and a stable order keeps the message diffable.
+      areas = cluster(regions).sort_by { |area| [-area.pairs, -area.region.size] }
+
+      [areas, dimensions]
+    end
+
+    def dimensions_of(comparison)
+      images = comparison.difference.comparison
+      comparison.driver.dimension(images.base_image) if images&.base_image
+    end
+
+    # Groups the per-pair regions into the places on the page they occupy: a
+    # region that overlaps one we have already seen is the same place, moved
+    # or resized, so the place grows to cover both.
+    #
+    # Merge every area the incoming region touches, not just the first.
+    # A grew to overlap C only after absorbing B, so first-overlap-wins would
+    # leave A and C as separate areas -- each then present in fewer pairs than
+    # the whole, so a single animation reads as churn and no mask is offered.
+    # Safe in the sense that it never invents a mask, but it withholds the one
+    # suggestion this message exists to make.
+    def cluster(regions)
+      regions.each_with_object([]) do |region, areas|
+        touching = areas.select { |area| area.region.intersect?(region) }
+        if touching.empty?
+          areas << Area.new(region, 1)
+        else
+          merged = touching.reduce(region) { |acc, area| union(acc, area.region) }
+          pairs = touching.sum(&:pairs) + 1
+          areas.reject! { |area| touching.include?(area) }
+          areas << Area.new(merged, pairs)
+        end
+      end
+    end
+
+    def union(one, other)
+      Region.from_edge_coordinates(
+        [one.left, other.left].min,
+        [one.top, other.top].min,
+        [one.right, other.right].max,
+        [one.bottom, other.bottom].max
+      )
+    end
+
+    def diagnosis_lines(areas, dimensions)
+      return [] if areas.empty? || dimensions.nil?
+
+      total_pairs = areas.sum(&:pairs)
+      # An area that changed in EVERY pair is animating. One that did not is
+      # the page still rendering -- masking it would hide a real change.
+      animating, settling = areas.partition { |area| area.pairs == total_pairs }
+
+      [
+        "  The page kept changing in #{count(areas.size, "area")}, over #{count(total_pairs, "attempt pair")}:",
+        *areas.map { |area| "    #{area_line(area, total_pairs, dimensions)}" },
+        *animating_lines(animating),
+        *settling_lines(settling, animating)
+      ]
+    end
+
+    def area_line(area, total_pairs, dimensions)
+      width, height = dimensions
+      share = area.region.size.to_f / (width * height)
+
+      "#{area.region.to_edge_coordinates.to_json} (left,top,right,bottom edges) " \
+        "-- #{Reporters::Default.percent(share)} of the #{width}x#{height} image, " \
+        "changed in #{area.pairs} of #{total_pairs} pairs"
+    end
+
+    # The escape hatch. Every coordinate here came off the comparison that
+    # just ran -- never a placeholder, and never a knob nothing reads.
+    def animating_lines(animating)
+      return [] if animating.empty?
+
+      skip_area = animating.map { |area| mask_coordinates(area.region) }
+      skip_area = skip_area.first if skip_area.size == 1
+
+      [
+        "  Always the same area, in every pair: that is an animation, clock, carousel or live counter.",
+        "  Exclude it and the page is stable without waiting:",
+        "    assert_matches_screenshot #{@snapshot.full_name.to_s.inspect}, skip_area: #{skip_area.to_json}"
+      ]
+    end
+
+    # A mask must COVER the region that moved, and it has to be pasteable.
+    # Edge coordinates arrive as floats on some drivers/platforms, so round
+    # OUTWARD -- floor the near edges, ceil the far ones. Truncating instead
+    # would shave the right/bottom edge and leave the moving pixels exposed,
+    # which is worse than suggesting nothing.
+    # Round OUTWARD so the mask covers the region rather than shaving it, and
+    # never emit an edge pair that collapses: Region carries WIDTH, and
+    # `from_edge_coordinates` derives it as `right - left`, so a one-pixel-wide
+    # change (a ticker digit, a caret) arrives here as left == right and would
+    # be suggested as a mask of width 0 -- one that masks nothing, leaves the
+    # page unstable, and tells the user to paste a fix that cannot work.
+    def mask_coordinates(region)
+      left, top, right, bottom = region.to_edge_coordinates
+      left, top = left.floor, top.floor
+
+      [left, top, [right.ceil, left + 1].max, [bottom.ceil, top + 1].max]
+    end
+
+    def settling_lines(settling, animating)
+      return [] if settling.empty?
+
+      subject = animating.empty? ? "D" : "The other #{count(settling.size, "area")}: d"
+
+      [
+        "  #{subject}ifferent areas at different times -- the page is still rendering, not animating in one place.",
+        "  skip_area masks a fixed area and will not help here: settle the page first (a readiness",
+        "  block on the assertion -- see docs/configuration.md) or raise wait:."
+      ]
+    end
+
+    def count(number, noun)
+      "#{number} #{noun}#{"s" unless number == 1}"
     end
   end
 end

lib/snap_diff/reporters/default.rb

@@ -61,6 +61,14 @@ def build_error_for_different_dimensions
 
       NEW_LINE = "\n"
 
+      # The one place the gem turns a fraction of the image into prose.
+      # Public because AttemptsReporter reports the same kind of number and
+      # must say it the same way (#264 vocabulary).
+      def self.percent(fraction)
+        value = fraction * 100
+        (value.positive? && value < 0.01) ? "<0.01%" : format("%.2f%%", value)
+      end
+
       # The thresholds a comparison is judged against, in the order they read
       # best. Only the ones actually set are printed -- see #thresholds.
       THRESHOLDS = [
@@ -146,8 +154,7 @@ def display_path(path)
       end
 
       def percent(fraction)
-        value = fraction * 100
-        (value.positive? && value < 0.01) ? "<0.01%" : format("%.2f%%", value)
+        self.class.percent(fraction)
       end
 
       def base_image_path