Skip to content

Add markdown rendering for read-only document viewer#235

Merged
zarvox32 merged 6 commits intoXGDevGroup:mainfrom
mohammad-aloufi:feature/markdown-docs
Apr 16, 2026
Merged

Add markdown rendering for read-only document viewer#235
zarvox32 merged 6 commits intoXGDevGroup:mainfrom
mohammad-aloufi:feature/markdown-docs

Conversation

@mohammad-aloufi
Copy link
Copy Markdown
Contributor

Summary

Replaces the plain-text read-only document viewer with a rendered Markdown viewer. Documents now display with proper headings, bold/italic, lists, tables, and code blocks instead of raw markdown syntax.

Changes

  • New MarkdownViewerDialog (clients/desktop/ui/markdown_viewer_dialog.py)
    • Converts markdown → HTML using python-markdown with extensions: tables, fenced_code, sane_lists, nl2br
    • Renders in wx.html2.WebView (Edge/WebView2 backend on Windows)
    • Uses system colors (wx.SystemSettings) — respects dark mode and high-contrast themes
    • Sets focus on load via wxEVT_WEBVIEW_LOADED for screen reader access
    • Escape key and Close button dismiss the dialog
  • Modified on_server_request_input() (clients/desktop/ui/main_window.py)
    • When read_only=True AND multiline=True → opens MarkdownViewerDialog instead of a plain wx.TextCtrl
    • On close → sends the same editbox(text="") response as before
    • No server-side changes needed
  • New dependency: markdown>=3.5 added to pyproject.toml

Testing

  1. cd clients/desktop && uv sync to install the new markdown dependency
  2. Start the server and connect with the desktop client
  3. Navigate to any document and open it to view it
  4. Verify rendering: headings, bold/italic, lists, tables, and code blocks should display formatted — not as raw markdown syntax
  5. Verify close: press Escape or click Close — should return to the previous menu
  6. Verify editing: open a document for editing — should still show the plain-text editor (not the rendered viewer)
  7. Verify with screen reader: with the screen reader active, confirm elements navigation works in the rendered view

@zarvox32
Copy link
Copy Markdown
Contributor

Review by Claude (Opus 4.6)

Requested by the PR reviewer; posted verbatim.

Overview

  • Adds MarkdownViewerDialog (200 lines, new file) that renders markdown to HTML inside wx.html2.WebView (Edge/WebView2).
  • Intercepts request_input when read_only and multiline and default_value and routes to the new dialog instead of the plain multiline TextCtrl. Sends on_submit("") on close so the server navigates back — this matches the server contract; _handle_document_editbox at browsing.py:140 just calls _show_document_actions or _show_documents_list on any response text.
  • Adds markdown>=3.5 to clients/desktop/pyproject.toml.

Smoke test (ran locally in an ephemeral env)

  • markdown.markdown(...) with the four chosen extensions (tables, fenced_code, sane_lists, nl2br) produces expected h1/h2, strong, em, code, table with thead/th, pre, blockquote, br. Good.
  • _HTML_TEMPLATE.format(...) interpolates cleanly — the CSS {{ }} escaping is correct.
  • What cannot be verified non-interactively: WebView2 backend availability, focus behavior after load, actual screen-reader navigation.

Issues (ranked)

1. Raw HTML / script injection — real concern
python-markdown passes raw HTML through by default. A smoke test confirmed <script>alert(1)</script> in document source renders as a live <script> tag in the WebView. Any user who can author a document can execute JS inside every other user's viewer. WebView2 is relatively sandboxed, but this is still a defense-in-depth hole. Fix: sanitize post-conversion (e.g. bleach.clean) or use a custom Markdown tree processor that strips <script>, <iframe>, event handlers, and javascript: URLs. Given documents are user-editable and transmitted over the network, don't trust them.

2. Gating on multiline and read_only is too broad
The PR comment concedes this ("read-only multiline content is likely markdown"). Right now only browsing.py:625 sends this combination, so it works — but any future user.show_editbox(..., multiline=True, read_only=True) silently becomes a markdown view, and a document containing raw text with # or * would be mangled. Better: add an explicit signal to the request_input packet, e.g. content_format: "markdown" | "text", and key the dispatch on that. The server side (show_editbox signature, RequestInputPacket model in server/network/packet_models.py) needs one new optional field. ~20 lines of server change plus one client check.

3. Empty document falls through to plain-text editor
read_only and multiline and default_value — the and default_value means an empty document goes to the read-only plain TextCtrl. Probably rare, but inconsistent with the rest of the viewer UX. Drop the default_value check and pass "" to the dialog — it will render nothing, which is correct.

4. No tests added
200 lines of new UI code, zero tests. At minimum: a unit test that constructs MarkdownViewerDialog headless and asserts the generated HTML contains expected tags for sample input. wx dialogs can be tested with wx.App() headless on Windows. The rendering helpers (_sys_color_hex, template formatting) are trivially testable in isolation and should be extracted if needed.

5. Accessibility — must verify interactively before merge
Per CLAUDE.md: "Silence is a bug." A wx.html2.WebView inside a dialog is an embedded browser; NVDA/JAWS behavior when focus lands there is not guaranteed without testing. The SetFocus() + document.body.focus() on load is a reasonable attempt but isn't a substitute for actual screen-reader testing. Also: there's no announcement of the viewer opening, no speak of the document title before focus moves to the HTML, and Escape closes but is not announced. Please test with NVDA before merging — this is the change's main risk area given the target audience.

6. No WebView2 fallback
wx.html2.WebView.New() can return None or fail on Windows installs without the WebView2 runtime (older Win10 images, some kiosk installs) and on Linux/Mac without a webkit backend. Nothing handles that here. At minimum, wrap construction in try/except and fall back to the existing plain-text view with a note like "markdown viewer unavailable; showing raw text".

7. Style nits

  • _sys_color_hex is used for four colors but the two derived tones (code_bg, th_bg) bypass it with hand-rolled blending — fine, but this section could be extracted into a _blend(bg_c, fg_c, amount) helper; the two copies differ only in divisor.
  • RunScript("document.body.focus();") — no guarantee this fires before SR has scraped the page; not a bug, just a caveat.
  • _HTML_TEMPLATE is inline as a module-level string; fine, but consider moving to a sibling .html template file if this grows.

Recommendation

Request changes on items 1 and 5 (must-fix), 2 and 6 (should-fix), 3, 4, 7 (nice-to-fix). Item 1 is the blocker — landing this without HTML sanitization introduces a real XSS surface through the documents system.

@mohammad-aloufi
Copy link
Copy Markdown
Contributor Author

Done. I fixed 1 and 2. The problem of js code injection is one didn't cross my mind at first but now it's fixed.
Same as with number 2. I fixed it from client and server side.

Couldn't add proper tests for number 4 in time though sorry, though from my testing it's working.

Regarding 5, the new view is accessible with NVDA.

@zarvox32 zarvox32 merged commit 7ede9b8 into XGDevGroup:main Apr 16, 2026
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