Conversation
XCFramework BuildThis PR's XCFramework is available for testing. Add the following to your .package(url: "https://github.com/wordpress-mobile/GutenbergKit", branch: "pr-build/642")Built from 90377e2 |
6ccfb7b to
44b6656
Compare
88be637 to
2e3812e
Compare
2e3812e to
06123d5
Compare
cfe7cab to
ac9e598
Compare
b85299d to
56e9743
Compare
A crashed editor left Gutenberg's own fallback on screen — a desktop notice offering to copy the post contents and a stack trace. Neither helps here: the copy button reads the post through `getEditedPostContent()` at click time, and the boundary sits above `EditorProvider`, whose unmount clears the post pointer, so it always copies an empty string. Cover the editor with a `ContentUnavailableView`, matching the existing `displayError` treatment for load failures, and offer `reloadEditor()`. The reloaded editor starts from whatever the host returns from `editorDidRequestLatestContent`, so work up to the host's last autosave survives, and readiness is restored only once `onEditorLoaded` arrives again. The copy button is dropped rather than kept alongside the native UI, since it never worked. The description deliberately makes no claim about saved work. Whether anything was persisted is the host's business — some mirror content continuously, others read it only when the user saves — so a host that does mirror should override that key to reassure the user. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VuxMbKtUsaUF8nUVgKdxwK
Mirrors the iOS treatment: cover the editor with a notice offering to reload, rather than leaving Gutenberg's desktop fallback on screen. `EditorErrorView` gains an optional action button so the crash state can offer `reloadEditor()`, which clears `didFireEditorLoaded` so the reloaded editor re-announces itself, and returns to the spinner until it does. The description deliberately makes no claim about saved work, since whether anything was persisted is the host's business. Robolectric needs the merged resources to construct the error view now that it reads a string resource, so unit tests opt into Android resources and `GutenbergViewTest` drops `manifest = Config.NONE`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VuxMbKtUsaUF8nUVgKdxwK
`reloadEditor()` left `isReady` untouched, so reloading a ready editor made `didLoadEditor()` return early: the loading indicator stayed up, `editorDidLoad(_:)` was not sent again, and bridge calls reached the page while it reloaded. A terminated web content process also reloaded without clearing the crash notice, leaving it over a working editor and setting up that same state when the user tapped Reload. Reset readiness and hide the web view in `reloadEditor()`, and reload through it when the web content process terminates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K2bBCVQXkZ1M1J8MUBDS9i
`reloadEditor()` cleared `didFireEditorLoaded` but left `isEditorLoaded` set, so reloading a ready editor let bridge calls reach the page while it reloaded. Reset it immediately; `onEditorLoaded` restores it once the editor is usable again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K2bBCVQXkZ1M1J8MUBDS9i
The crash notice covered the web view visually, but the web view stayed in the accessibility hierarchy and nothing announced the change, so VoiceOver could land on Gutenberg's hidden error fallback behind the notice. Hide the web view from VoiceOver while the notice is up and move focus to the notice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K2bBCVQXkZ1M1J8MUBDS9i
The crash notice hid the web view only with `alpha = 0f`, which still lets it receive touches and accessibility focus. The notice is content-height and does not consume touches, so taps around it reached Gutenberg's hidden error fallback, whose "Copy error" button replaces the clipboard. Make the web view invisible while the notice is up, and visible again when the editor reloads so it can render and signal readiness. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K2bBCVQXkZ1M1J8MUBDS9i
The unavailable callbacks said the editor could not recover on its own and told hosts to disable controls, but not that the editor can now be reloaded or which callback marks it usable again. Point hosts to the reload notice and `reloadEditor()`, and to re-enable their controls on the next `editorDidLoad(_:)` or `onEditorAvailable`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K2bBCVQXkZ1M1J8MUBDS9i
Testing the editor crash notice meant tripping the `ErrorBoundary` from the web inspector. Add a menu action to both demo apps that does the same from the editor's navigation bar. It throws from a selector the block list reads outside the per-block error boundaries, so the crash reaches the editor-level `ErrorBoundary` rather than a single block's. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K2bBCVQXkZ1M1J8MUBDS9i
The error view's title was a plain `TextView`, so TalkBack users navigating by heading could not jump to the crash notice or the load failure message. Mark the title as a heading. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K2bBCVQXkZ1M1J8MUBDS9i
`showEditorCrashPhase()` repeated `showErrorPhase()` almost line for line, and the copies had already drifted: the crash phase hid the loading indicators instantly while the load failure faded them out. Route both through `showErrorView`, which takes the content to show. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y7VSMTRz2D8MPhuziM7CG5
The crash notice and load failure hid the web view, which could hold TalkBack focus, and faded in the error view without announcing it, so a TalkBack user lost their place with nothing said. iOS moves VoiceOver focus to its notice. Give the error view its title as a pane title, which TalkBack announces when it appears, and move accessibility focus to the title. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y7VSMTRz2D8MPhuziM7CG5
The reload docs promised that work up to the host's last autosave survives, but that holds only when the host returns content. A host that returns nil reloads the content the editor was opened with, so saving afterward can push that stale content over the host's own save from earlier in the session. `getPost` also claimed that fallback happened only without a native bridge. State the contract where hosts implement it: return the newest content held, including anything saved this session, while nil restores the initial content. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y7VSMTRz2D8MPhuziM7CG5
The Reload Editor button copied `view.tintColor` once, when the notice appeared. UIKit dims the tint a view inherits while a presentation such as an alert or the block inserter covers the editor, so a crash during one left the button gray, looking disabled, after the presentation went away. Read the tint from the notice's own view with its tint adjustment mode set to normal, which returns the host's unmodified tint. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y7VSMTRz2D8MPhuziM7CG5
VoiceOver neither moved to nor announced the crash notice. The screen change was posted from UIKit before SwiftUI laid out the notice, and targeted its hosting view rather than an element. A shared error view now focuses its title for VoiceOver when it appears. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y7VSMTRz2D8MPhuziM7CG5
The load failure view appeared without moving VoiceOver focus, unlike Android, which focuses its title. It now uses the same error view as the crash notice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y7VSMTRz2D8MPhuziM7CG5
`onEditorLoaded` posts its work to the main thread, and the ready-phase transition posts again from inside it, so a crash arriving before the first post ran left the ready phase queued behind the crash notice. It then hid the notice, leaving the user the editor's own error text with no way to reload. Readiness is reset on the JavaScript thread as soon as the crash arrives, so return from the post once the editor is no longer loaded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zAgnCCwPXpmf25NUueoQN
Ending the web content process reloads through the same path as a crash, but only the crash path dismissed the inserter. Left open across the reload, a pick made in it ran `window.blockInserter?.insertBlock(...)` against a page that never set that global, so nothing was inserted and the optional chaining kept the failure silent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zAgnCCwPXpmf25NUueoQN
Readiness resets when a new page starts, so the reload after a crash ran the first-load autofocus again. It decides from the content the editor was opened with, so a post that started empty and was restored from the host popped the keyboard over the restored content and moved the caret back to the title. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zAgnCCwPXpmf25NUueoQN
Ending the web content process hid the editor behind a spinner and reset readiness for the seconds its reload takes, but never told the delegate. The host kept undo, redo, the code editor and Save enabled against an editor that refused every call, and `editorDidLoad` then arrived without the unavailable the delegate documents as preceding it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zAgnCCwPXpmf25NUueoQN
The error view set an accessibility pane title, which TalkBack announces when the view appears, and then moved accessibility focus to that same title, which TalkBack reads again. Moving focus is the part the view needs, since it replaces content that could have held focus, so drop the pane title. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zAgnCCwPXpmf25NUueoQN
`onEditorUnavailable` shows the crash phase, whose action reloads the editor, so the crash phase now precedes `reloadEditor` rather than following it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zAgnCCwPXpmf25NUueoQN
Readiness resets when the editor reloads, so `didLoadEditor` ran the first-load autofocus again. `focus()` decides from the content the editor was opened with, so a post that started empty and was restored from the host had the keyboard raised over the restored content. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zAgnCCwPXpmf25NUueoQN
The load failure title was a hard-coded English string that `init` copied into a field so `setError` could restore it after `setActionableState` had replaced the title. Both places now read the same string resource, as the crash strings already did, and the description's fallback is translatable too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zAgnCCwPXpmf25NUueoQN
`didLoseEditor` displays the crash notice, whose button reloads the editor, so `displayEditorCrash` now precedes `reloadEditor` rather than following it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zAgnCCwPXpmf25NUueoQN
be91f93 to
ce89716
Compare
adalpari
left a comment
There was a problem hiding this comment.
The PR looks good to me.
Here are two findings from Claude, but they seem to be related to iOS. So I leave them to you and @crazytonyli
Findings, most severe first
- iOS reload reverts host configuration updates — ios/Sources/GutenbergKit/Sources/EditorViewController.swift:1045 (PLAUSIBLE)
reloadEditor() calls webView.reload(), which re-runs the WKUserScript that was built once in loadEditor(dependencies:). Any updateConfiguration(_:) the host applied after the first load is silently reverted. Android does not have this problem — it rebuilds window.GBKit from the live configuration in onEditorPageStarted(), so the two platforms diverge.
Failure scenario: host saves a new draft and calls updateConfiguration to record the postID → editor crashes → user taps Reload → the page comes back with the original GBKit.post (stale id/status/restBase) while title/content come from editorDidRequestLatestContent. Rated PLAUSIBLE rather than CONFIRMED only because updateConfiguration has no in-repo caller, so it depends on host usage.
- iOS crash notice leaves the spinner running underneath — ios/Sources/GutenbergKit/Sources/EditorViewController.swift:970 (CONFIRMED, low severity)
didLoseEditor() shows the crash notice but never calls hideActivityView(). For a crash before the first onEditorLoaded — a post whose own content throws on first render, which is the exact case the PR description calls out — the waitingView spins forever under the opaque notice. Worse, every Reload tap calls displayActivityView() again on the same view, re-activating a duplicate constraint set. Android’s showErrorView fades the spinner out for this case; iOS has no equivalent.
A crash before the editor loads left the indicator animating under the crash notice, and each Reload re-added it with another set of constraints. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Thanks for sharing this, @adalpari. I investigated both. 1. Reload reverts host configuration updates. I'm leaving this as is. 2. Spinner running under the crash notice. Fixed in 90377e2. The notice now removes the loading indicator. That stops each Reload tap from adding another set of constraints, and leaves no indicator behind the notice for VoiceOver to find. |
|
@crazytonyli will review when you have time please? 🙇🏻♂️ |
Including Android resources in unit tests (#642) makes Robolectric default to the project's SDK 36, which requires Java 21 and fails on CI's older JVM. Pin the SDK as the other Robolectric test classes already do. Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
What?
A crashed editor leaves only a web error message on screen, with no way back other than closing the post.
Why?
Completes CMM-2008 by making the crashed state clear and recoverable.
The message replaces a React tree that just crashed, so recovery belongs in native code.
How?
EditorErrorViewgains an optional action button: aMaterialButtonunder Material themes, and a platform button otherwise. It makes the web view invisible, so the web view receives no touches or TalkBack focus. Both states use a bundled error icon tinted from the theme, and localized strings.editorDidRequestLatestContent/LatestContentProvider. Readiness returns on the nextonEditorLoaded, which reapplies code editor mode when the host enabled it, and leaves autofocus to the first load so a reload can't raise the keyboard over restored content. A crash arriving in the moment after a load keeps the notice up rather than losing it to the ready phase.editorDidLoad(_:)/onEditorAvailable.editorDidRequestLatestContent/LatestContentProviderask for the newest content the host holds, including anything saved this session;nilreloads the content the editor was opened with.Decisions worth reviewing
The notice promises nothing about saved work. Whether anything was persisted depends on the host, which GutenbergKit can't see. Hosts that mirror content can override
editorCrashedDescription/gbk_editor_crashed_description.Reload is user-initiated. Automatic retries would loop when the post's own content causes the crash, and would silently discard edits since the last autosave.
Reloading is not public API. Only the notice and iOS web content process termination reload the editor, both after a page has loaded. Retrying after a load failure would need its own API that fetches dependencies again.
Testing Instructions
Note
Observing autosave requires WordPress-iOS or WordPress-Android rather than GBK's demo app.
Trip the boundary from the web inspector (iOS) or
chrome://inspect(Android), on a post with at least one block, in either the visual or code editor. In GBK's demo app, Trigger editor crash in the editor's overflow menu does the same.Regression check, in a fresh session with no crash: loading, load failures and normal editing are unchanged.
Accessibility Testing Instructions
With VoiceOver (iOS) or TalkBack (Android) on, trigger a crash:
Screenshots or screencast
🤖 Generated with Claude Code
https://claude.ai/code/session_01Y7VSMTRz2D8MPhuziM7CG5