Conversation
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
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
Contributor
Code Coverage ReportCurrent Coverage: 39.92% Coverage Details (Summary) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.