Skip to content

Fix table relayout racing the reading-width transition - #25

Open
jedrzejsieracki wants to merge 1 commit into
ashtree74:mainfrom
jedrzejsieracki:fix/table-relayout-transition-race
Open

Fix table relayout racing the reading-width transition#25
jedrzejsieracki wants to merge 1 commit into
ashtree74:mainfrom
jedrzejsieracki:fix/table-relayout-transition-race

Conversation

@jedrzejsieracki

Copy link
Copy Markdown

Fix table relayout racing the reading-width transition

Problem

Tables settle into their "wide surface" layout (escaped from the text column,
horizontal scroll) based on the previous reading width rather than the
current one. The result is sticky, nondeterministic table layout after using
the reading-width control, the width presets, or after resizing the window:
the same width can leave a table wide or normal depending on which width it
came from, and the table expand/collapse button then starts from that stale
state.

Root cause

#preview-document animates column-width changes:

#preview-document {
  transition: width 180ms ease, …;
}

but previewmdSetLayout relaid out tables on the next animation frame:

window.requestAnimationFrame(enhanceTables);

The rAF callback runs in the first frame of the 180 ms transition, so
articleContentWidth() reads the pre-animation width and layoutTable
bases the wide/normal decision on it. The window.resize listener called
enhanceTables() synchronously, which has the same problem (the column width
transitions when the window is resized).

Evidence

Measured in the embedded web view while sweeping the reading width
480 → 720 → 1100 → 480 → 1100, probing the table state 50 ms and 550 ms after
each change (before this fix):

target width measured column width during the transition final column width table state
480 469 px 323 px normal (wrong: 432 px min table > 323 px column → should be wide)
720 435 px 563 px wide (wrong: 432 < 563 → should be normal)
1100 619 px 725 px normal (lucky)
1100 422 px 725 px wide (unlucky)

The wide/normal decision tracked the previous width every time.

Fix

Debounce the table relayout past the transition (240 ms), re-arming the timer
on every layout change, so the measurement always happens at the final column
width. During continuous changes (slider drags, window resizes) no intermediate
relayout happens at all; the table is laid out once, correctly, when the column
settles. Reduced-motion configurations are unaffected — with no transition the
timer simply relayouts after 240 ms.

previewmdSetLayout and the resize listener both route through the new
scheduleTableRelayout().

After the fix, all sweeps settle deterministically: 480 → wide, 720 → normal,
1100 → normal, and expand → collapse returns the table to the width-appropriate
state at every width (14/14 probes correct).

Testing on macOS

  1. Open the showcase (the "Project pulse" table has a minimum width of
    4 × 144 px = 576 px, close to the default column width — a good probe).
  2. Drag the reading-width control back and forth across the ~576 px boundary;
    switch between the narrow/comfortable/wide/table presets. The table must
    end up wide exactly when the column is too narrow for it, every time.
  3. Resize the window with a fixed reading width; same expectation.
  4. Expand a table (↗) then collapse it (↙) at several widths; collapsing must
    return the table to the width-appropriate layout.

Tests

  • The existing renderer tests were reviewed for compatibility:
    testWideTableKeepsReadableColumnsAndExpandsIndependently
    (EditorWebViewTests) exercises the initial layout, which stays synchronous
    (enhanceTables() still runs inline during previewmdRender); no test
    asserts the timing of previewmdSetLayout or window resizes.
  • No new XCTest added: a deterministic test of a transition race is timing
    machinery that could not be validated here (this patch was authored and
    verified on Linux against the bundled renderer in a WebKitGTK shell — the
    measurement table above is from that harness). swift test should run on
    macOS CI; happy to add a regression test as a follow-up if the maintainer
    wants one.

Scope

Renderer-internal change (Resources/Renderer/renderer.js only). No API or
payload changes; hosts drive the renderer exactly as before. Found while
building the experimental .NET port slice (docs/PORTING.md); the bug is in
the shared renderer and affects the macOS app as well — large jumps (width
presets) reproduce it there.

Signed-off-by: jedrzejsieracki <jedrzejsieracki@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant