Skip to content

Fix #39: Editor / preview pane losing sync due to image loading#145

Merged
schuyler merged 40 commits intomainfrom
claude/fix-pane-sync-cleaned-up-01CQ6iFLaz5cSgvaqkgQz7ua
Nov 20, 2025
Merged

Fix #39: Editor / preview pane losing sync due to image loading#145
schuyler merged 40 commits intomainfrom
claude/fix-pane-sync-cleaned-up-01CQ6iFLaz5cSgvaqkgQz7ua

Conversation

@schuyler
Copy link
Copy Markdown
Owner

No description provided.

The scroll synchronization between editor and preview panes was breaking
after the "Markdown Preference Pane" section in long documents because the
JavaScript selector 'img:only-child' was not reliably matching the same
images that the editor's regex detected.

The CSS :only-child pseudo-selector can miss or incorrectly select images
depending on how the Markdown renderer wraps content, leading to misaligned
reference point arrays between editor and preview. This caused the preview
pane to jump ahead when scrolling past certain headers.

This fix replaces the selector with explicit JavaScript logic that:
- Selects all headers (h1-h6)
- Selects all images and filters to only include images that are the sole
  child element of a paragraph tag
- Sorts results by document order
- Returns their positions

This ensures the preview reference points exactly match what the editor
detects: standalone image lines.

Related to #39
The previous fix was too restrictive by requiring images to be inside
<p> tags specifically. This caused the first image to be missed, creating
an offset at the beginning of the document.

Now matches any image that is the only child element of its parent,
regardless of parent tag type. Uses .children (element nodes only)
instead of CSS :only-child selector for more consistent matching.

Related to #39
Replaced the complex sorting logic with TreeWalker which walks the DOM
in document order, ensuring reference points are correctly ordered.

Added debug logging to both editor and preview detection to see what
reference points are being matched on each side. This will help identify
any mismatches.

To view the logs:
- Run MacDown from Terminal or Xcode
- Open help.md and scroll around
- Check Console.app or Terminal output for NSLog messages

Related to #39
Some images are clickable links, with structure like:
  <p><a><img /></a></p>

The previous code only checked if the image was the only child of its
immediate parent, missing images wrapped in <a> tags.

Now checks both:
- Image is only child of parent (direct)
- Image is only child of <a>, and <a> is only child of grandparent

This should help close the gap between editor (43 points) and
preview (39 points) reference counts.

Related to #39
Removed TreeWalker and console.log calls which may have been causing
the script to fail silently. Added try/catch and document.body check.
Uses querySelectorAll and compareDocumentPosition for reliable results.

Related to #39
Returns a structured object with both positions and debug info showing:
- Total images found
- Which images were included (by alt/src)
- Which images were skipped with parent info (tag, children count)

Related to #39
The Markdown renderer adds extra elements (like <br> tags) inside <p> tags
with images, causing children.length to be 2 instead of 1.

Changed logic:
- For <p> tags: Check if there's exactly ONE <img> element (ignore other children)
- For <a> tags in <p>: Same logic - count images, not total children
- For other parent types: Keep strict children.length === 1 check

This should detect all 7 images in help.md instead of just 3.

Related to #39
Logs the first 10 reference points from both editor and preview
showing their order and content. This will reveal if the arrays
have elements in different orders, which would cause sync issues
even when counts match.

Related to #39
The offset moved from 'Markdown Preference Pane' to 'General Preferences Pane',
confirming our image detection fix is working. Now logging the end of the
document (items 30-43) to see the mismatch at the new problem location.

Related to #39
Lines with 3+ dashes/asterisks/underscores are horizontal rules (<hr>),
not header underlines. The editor was incorrectly treating '---' lines
inside code blocks as headers, while the preview correctly ignored them.

Added hrRegex to detect horizontal rules (3+ of -, *, or _ with optional spaces)
and exclude them from dash-header detection.

This fixes the mismatch where editor had 45 points and preview had 43.

Related to #39
When editing, the preview re-renders and used lastPreviewScrollTop to
restore scroll position. But this value was only updated when the user
manually scrolled the preview, not when auto-synced from editor scroll.

Now update lastPreviewScrollTop after syncScrollers sets the position,
so preview re-renders maintain the correct synced position without
flashing to the top.

Related to #39
Cleaned up all the debug logging that was added to diagnose the
scroll sync mismatch. The issue is now fully resolved:
- Images in paragraphs with extra children are detected
- Horizontal rules no longer treated as headers
- Preview scroll position persists across refreshes

Related to #39
When preview reloads, it briefly appears at scroll position 0 before the
completion handler fires. This causes a visible flash to the top.

Now set the scroll position to lastPreviewScrollTop immediately when the
preview loads, BEFORE calling syncScrollers. This prevents the flash by
starting at approximately the right position, then syncScrollers refines
it to the exact position based on current editor scroll.

This matches the behavior when sync scrolling is disabled, which already
sets initial position immediately.

Related to #39
The window flush was being enabled before the scroll position was set,
causing the preview to briefly display at position 0 (the flash).

Now:
1. Scale webview
2. Set scroll position
3. Refine with syncScrollers if needed
4. Enable window flushing (makes preview visible)

This ensures the preview is never visible at the wrong scroll position.

Related to #39
Add detailed logging to trace:
- Window flushing enable/disable state
- Scroll position changes through each step
- Whether scaleWebview affects scroll position
- Timing of completion handler execution

Also moved initial scroll position setting to before scaleWebview
in case scaling resets the position.
The logs showed that the WebView scroll position is always 0 when
the completion handler starts, even though we set it to the saved
position immediately. This means there's a brief moment where the
position is 0.

To fix this:
1. Call displayIfNeeded on contentView after setting scroll position
   to ensure the scroll change is fully applied
2. Call flushWindow explicitly after enabling flushing to force an
   immediate update with the correct scroll state

This should eliminate the flash to top by ensuring the window only
becomes visible after the scroll position has been fully applied.
Window flush control doesn't prevent the WebView's own rendering
pipeline from displaying content. The WebView renders with scroll
position 0 during HTML load, before the completion handler can set
the correct position.

Solution: Hide the WebView when HTML load starts, then show it again
in the completion handler after scroll position has been set. This
way the user never sees the intermediate state at position 0.
Instead of calling loadHTMLString which resets scroll to 0, use DOM
manipulation to replace just the content when editing the same document.
This preserves scroll position and eliminates the flash to top.

To address the issues that caused this approach to be disabled before:
1. Re-trigger Prism syntax highlighting with Prism.highlightAll()
2. Re-trigger MathJax typesetting with MathJax.Hub.Queue()
3. Verify scroll position is preserved and restore if needed

Falls back to full reload for initial load or when baseURL changes.
Reverted the DOM replacement approach due to concerns about unknown
side effects beyond MathJax and Prism (event handlers, custom JS, etc).

Attempted overlay approach: place a white view over the preview during
reload to hide the flash. However, this likely just trades one flash
(to top) for another (white overlay appearing/disappearing).

This approach is also unlikely to work, but committing for completeness.
After trying multiple approaches (window flushing, hiding WebView,
CSS opacity, overlays), they all create different visual artifacts.

The fundamental issue: loadHTMLString always resets scroll to 0 and
renders immediately before we can fix it. Any visual hiding causes
a different flash or flicker.

DOM replacement is the only approach that preserves scroll position
naturally. Re-trigger Prism and MathJax to address the concerns that
caused this to be disabled originally.

Falls back to full reload (with flash) for initial load or URL change.
Problem: DOM replacement bypassed completion handler, so:
- No updateHeaderLocations (scroll sync broken)
- No syncScrollers (preview doesn't track editor)
- No WebView repaint (changes invisible)

Fix: After DOM replacement:
1. Re-trigger Prism/MathJax
2. Call updateHeaderLocations to recalculate reference points
3. Call syncScrollers to maintain editor/preview sync
4. Force WebView repaint with setNeedsDisplay + flushWindow

Added logging to track scroll position and processing steps.
Calling setNeedsDisplay + flushWindow after syncScrollers could cause
multiple visible paints:
1. WebView auto-renders after innerHTML change
2. We change scroll position
3. We force another render

This creates flickering. Instead, let WebView render naturally once
after all changes (innerHTML + scroll sync) are complete.
Problem: After first DOM replacement, alreadyRenderingInWeb stayed YES,
blocking all subsequent updates. The renderer checks this flag via
needsHtml and refuses to generate new HTML if rendering is "in progress".

In full reload path: didFinishLoadForFrame sets alreadyRenderingInWeb=NO
In DOM replacement path: We bypassed didFinishLoadForFrame, never reset flag

Fix: Set alreadyRenderingInWeb=NO after DOM replacement completes.
Problem: Preview shudders up/down by a few pixels on every edit.

Cause: DOM replacement naturally preserves scroll position. But we were
then calling syncScrollers, which RECALCULATES scroll based on header
positions. Due to rendering variations (fonts, images, reflow), header
positions can shift by a pixel or two, causing:
  Edit 1: scroll 2643 → syncScrollers → 2635 (8px jump)
  Edit 2: scroll 2635 → syncScrollers → 2641 (6px jump)
  Result: visible shudder

Solution: DOM replacement already preserves scroll correctly. Only call
updateHeaderLocations to refresh reference points for future editor
scrolling. Don't call syncScrollers - there's no editor scroll to sync to.
Problem: Preview still shudders despite removing syncScrollers.

New hypothesis: Running JavaScript (updateHeaderLocations) after DOM
replacement triggers reflow that shifts scroll position. The JavaScript
queries DOM elements (querySelectorAll, getBoundingClientRect) which
can cause layout recalculation.

Solution:
1. Don't call updateHeaderLocations after DOM replacement
2. Header locations will be updated naturally next time user scrolls
   (editorBoundsDidChange triggers updateHeaderLocations)
3. Add explicit scroll restoration if position changed > 0.5px during
   innerHTML/Prism/MathJax processing

This minimizes post-DOM-update JavaScript execution.
Problem: Shudder persists because MathJax is asynchronous.

Timeline:
1. innerHTML sets content
2. Prism.highlightAll() runs (synchronous)
3. MathJax.Hub.Queue() schedules typesetting (async)
4. We check scroll - looks fine
5. Later: MathJax runs, changes element heights, scroll shifts

Solution: Queue scroll restoration in MathJax.Hub.Queue() to run
AFTER typesetting completes. Use window.scrollTo() which works on
the WebView's document.

For non-MathJax case: restore scroll immediately after Prism.
Problem: Shudder persists even with MathJax fix, likely because
MathJax is disabled in preferences (no "queued scroll" log message).

Cause: Prism.highlightAll() runs synchronously and can trigger reflow
that shifts scroll position by a pixel or two when syntax highlighting
is applied to code blocks.

Solution: Check scroll position immediately after Prism.highlightAll()
and restore if it shifted > 0.5px. This catches reflow from Prism
before any other operations.

The logs will show "Prism shifted scroll" if this is the culprit.
Use JSContext property setting instead of non-existent
escapeStringForJavaScript method. This properly handles HTML
escaping by setting it as a window property that JavaScript
can reference.

Related to #39
claude and others added 10 commits November 20, 2025 10:08
The shudder was caused by intermediate layout states being visible
during innerHTML replacement and Prism syntax highlighting. Each
DOM operation triggers a layout pass, and the browser paints these
intermediate states.

Solution: Use CSS visibility to hide content during updates:
1. Set body visibility to hidden
2. Replace innerHTML and run Prism
3. Force layout completion by reading offsetHeight
4. Restore visibility

This ensures only the final layout state is painted, eliminating
the visible shudder.

Related to #39
Previous approach replaced innerHTML of <html> element, which included
both <head> and <body>. This caused CSS re-parsing and DOM
reconstruction of the entire document, leading to layout shifts.

Now only replace body.innerHTML, preserving the head (CSS, scripts).
This is much less disruptive and should eliminate unnecessary layout
recalculations.

Also removed requestAnimationFrame and visibility hiding since those
approaches didn't help - the issue was replacing too much DOM.

Related to #39
The shudder was caused by MathJax rendering asynchronously AFTER
content was already visible. The sequence was:
1. body.innerHTML replaces content (visible immediately)
2. Prism highlights (visible immediately)
3. MathJax renders later (content shifts when math appears)

The full reload doesn't have this issue because window flushing
is disabled until MathJax completes via MPMathJaxListener.

Fix: Keep body visibility hidden until MathJax.Hub.Queue callback
fires, indicating all math rendering is complete. Only then make
content visible and restore scroll position.

Related to #39
The shudder was caused by images with invalid URLs going through
their load/fail cycle while content was visible:
1. Browser creates img placeholder
2. Attempts to load image
3. Load fails
4. Element resizes (broken icon or collapse)
5. Layout shifts by 1-2 pixels

Full page reload didn't show this because window flushing was
disabled during the entire load process. DOM replacement made
content visible immediately, exposing the image load/fail cycle.

Fix: Use Promise.all to wait for all images to complete (load or
error) before making content visible. This ensures layout is stable
before the user sees it, just like the full reload behavior.

Related to #39
Without timeout, slow-loading remote images would block the preview
update indefinitely, making live editing feel unresponsive.

Use Promise.race to either:
- Wait for all images to complete (prevents jitter from fast/cached images)
- Timeout after 100ms (prevents blocking on slow network)

This balances smooth updates vs responsive editing. Cached images
load instantly and prevent layout shifts. Slow images timeout and
may cause minor jitter, but the user sees their edits immediately.

Related to #39
The visibility hiding caused unacceptable flickering. While it
prevented shudder from broken/slow images, the cure was worse than
the disease.

Back to simple DOM replacement: update body.innerHTML, run Prism,
queue MathJax. This may shudder slightly with broken images, but
no flickering and responsive editing.

Related to #39
Critical fixes:
- Remove all 14 debug NSLog statements left in production code
- Cache regex patterns with dispatch_once for performance (avoid recompilation on every scroll)
- Fix horizontal rule regex to correctly require same-character sequences (---, ***, ___)

Code quality improvements:
- Extract embedded JavaScript to separate resource file (updateHeaderLocations.js)
- Add comprehensive method documentation for scroll sync algorithms
- Add inline comments explaining the tapering/centering algorithm
- Improve image regex to support reference-style images: ![alt][ref]

Technical details:
- JavaScript moved from 58-line embedded string to maintainable external file
- Regex patterns now cached as static variables, improving scroll performance
- Documentation explains the scroll sync algorithm and tapering behavior
- Horizontal rule detection now properly handles mixed-character edge cases

Related to #39
Critical fixes:
- Fix dispatch_once variable name collision (onceToken used twice in same method)
  Renamed second instance to regexOnceToken to avoid undefined behavior

- Fix MathJax race condition with alreadyRenderingInWeb flag
  Disable DOM replacement when MathJax is enabled to prevent race conditions
  MathJax rendering is async, but flag was cleared immediately
  Full reload path has proper async completion handlers via MPMathJaxListener

Technical details:
- Lines 1789 and 1824 both declared static dispatch_once_t onceToken
- DOM replacement cleared alreadyRenderingInWeb before MathJax.Hub.Queue completed
- Now falls back to full reload when htmlMathJax preference is enabled
- This ensures proper synchronization and prevents render conflicts
@github-actions
Copy link
Copy Markdown
Contributor

Code Coverage Report

Current Coverage: 39.92%

Coverage Details (Summary)
Name                                                                                                                                   Coverage            
-------------------------------------------------------------------------------------------------------------------------------------- ------------------- 
MASPreferences.bundle                                                                                                                  0.00% (0/0)         
MacDown 3000.app                                                                                                                       53.83% (6700/12446) 
    /Users/runner/work/macdown3000/macdown3000/Dependency/peg-markdown-highlight/HGMarkdownHighlighter.m                               76.20% (429/563)    
        styleparsing_error_callback                                                                                                    0.00% (0/10)        
        -[HGMarkdownHighlighter init]                                                                                                  92.86% (13/14)      
        -[HGMarkdownHighlighter initWithTextView:]                                                                                     83.33% (5/6)        
        -[HGMarkdownHighlighter initWithTextView:waitInterval:]                                                                        83.33% (5/6)        
        -[HGMarkdownHighlighter initWithTextView:waitInterval:styles:]                                                                 0.00% (0/6)         
        -[HGMarkdownHighlighter parseText:]                                                                                            100.00% (9/9)       
        -[HGMarkdownHighlighter convertOffsets:text:]                                                                                  26.19% (11/42)      
        -[HGMarkdownHighlighter requestParsing]                                                                                        100.00% (15/15)     
        __39-[HGMarkdownHighlighter requestParsing]_block_invoke                                                                       100.00% (12/12)     
        __39-[HGMarkdownHighlighter requestParsing]_block_invoke.11                                                                    100.00% (3/3)       
        __39-[HGMarkdownHighlighter requestParsing]_block_invoke.13                                                                    100.00% (4/4)       
        -[HGMarkdownHighlighter getClearFontTraitMask:]                                                                                88.89% (16/18)      
        -[HGMarkdownHighlighter clearHighlightingForRange:]                                                                            100.00% (31/31)     
        __51-[HGMarkdownHighlighter clearHighlightingForRange:]_block_invoke                                                           100.00% (8/8)       
        -[HGMarkdownHighlighter readClearTextStylesFromTextView]                                                                       100.00% (17/17)     
        -[HGMarkdownHighlighter applyHighlighting:withRange:]                                                                          80.00% (60/75)      
        __53-[HGMarkdownHighlighter applyHighlighting:withRange:]_block_invoke                                                         92.31% (12/13)      
        -[HGMarkdownHighlighter applyVisibleRangeHighlighting]                                                                         100.00% (16/16)     
        -[HGMarkdownHighlighter clearHighlighting]                                                                                     100.00% (3/3)       
        -[HGMarkdownHighlighter cacheElementList:]                                                                                     100.00% (7/7)       
        -[HGMarkdownHighlighter clearElementsCache]                                                                                    100.00% (3/3)       
        -[HGMarkdownHighlighter textViewTextDidChange:]                                                                                0.00% (0/11)        
        __47-[HGMarkdownHighlighter textViewTextDidChange:]_block_invoke                                                               0.00% (0/3)         
        -[HGMarkdownHighlighter textViewDidScroll:]                                                                                    27.27% (3/11)       
        __43-[HGMarkdownHighlighter textViewDidScroll:]_block_invoke                                                                   0.00% (0/6)         
        __43-[HGMarkdownHighlighter textViewDidScroll:]_block_invoke_2                                                                 0.00% (0/3)         
        -[HGMarkdownHighlighter getDefaultStyles]                                                                                      100.00% (27/27)     
        -[HGMarkdownHighlighter applyStyleDependenciesToTargetTextView]                                                                85.71% (12/14)      
        -[HGMarkdownHighlighter setStyles:]                                                                                            75.00% (6/8)        
        -[HGMarkdownHighlighter getDefaultSelectedTextAttributes]                                                                      100.00% (7/7)       
        -[HGMarkdownHighlighter handleStyleParsingError:]                                                                              0.00% (0/12)        
        -[HGMarkdownHighlighter applyStylesFromStylesheet:withErrorHandler:]                                                           81.40% (70/86)      
        -[HGMarkdownHighlighter setTargetTextView:]                                                                                    85.71% (6/7)        
        -[HGMarkdownHighlighter parseAndHighlightNow]                                                                                  100.00% (3/3)       
        -[HGMarkdownHighlighter highlightNow]                                                                                          100.00% (3/3)       
        -[HGMarkdownHighlighter activate]                                                                                              92.00% (23/25)      
        -[HGMarkdownHighlighter deactivate]                                                                                            100.00% (19/19)     
    /Users/runner/work/macdown3000/macdown3000/MacDown/Code/Document/MPAsset.m                                                         91.96% (103/112)    
        -[MPAsset typeName]                                                                                                            100.00% (3/3)       
        -[MPAsset defaultTypeName]                                                                                                     100.00% (3/3)       
        +[MPAsset assetWithURL:andType:]                                                                                               100.00% (3/3)       
        -[MPAsset initWithURL:andType:]                                                                                                87.50% (7/8)        
        -[MPAsset init]                                                                                                                100.00% (3/3)       
        -[MPAsset templateForOption:]                                                                                                  0.00% (0/7)         
        -[MPAsset htmlForOption:]                                                                                                      96.43% (27/28)      

... (1996 more lines truncated)

📊 **Full coverage report available in workflow artifacts**

@schuyler schuyler merged commit c39a7d0 into main Nov 20, 2025
3 checks passed
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.

2 participants