Skip to content

Conversation

@dcalhoun
Copy link
Member

@dcalhoun dcalhoun commented Sep 24, 2024

Description

Gracefully handle failed attempts to load a site's remote editor by falling back
to the local, default editor and displaying a notice to the user. The notice
includes both "retry" and "dismiss" actions to empower the user. In the future,
we could further expand error handling to better surface was exactly occurred --
e.g., no network connection, problematic plugin.

Related:

Screen recording
ScreenRecording_09-24-2024.13-03-56_1.MP4

Testing Instructions

Tip

Use the WordPress-iOS prototype build for testing.

Important

You must test the following flows with a self-hosted site that is connected via an application password and has the block-editor-assets-endpoint plugin activated.

Important

It appears a change to the Jetpack plugin may have broken support for Jetpack blocks. Testing third-party block support with Coblocks is known to work at this time.

1 - Fallback to local editor

  1. Navigate to MeApp SettingsExperimental Features and enable the following features:
    • Experimental Block Editor
    • Experimental Block Editor Styles
  2. Navigate to MeApp SettingsDebugFeature Flags and enable the following feature flags:
    • Experimental Block Editor Plugins
  3. Open a post containing custom blocks with the editor for a self-hosted site connected via an application password.
  4. Verify the editor loads successfully.
  5. Close the editor.
  6. Disable your network connection.
  7. Re-open the editor.
  8. Verify the default, local editor loads (w/o plugins).

2 - Dismiss the notice

  1. Repeat the steps for test case 1.
  2. Tap "Dismiss."
  3. Verify the notice is removed.

3 - Auto-dismiss the notice

  1. Repeat the steps for test case 1.
  2. Wait 20+ seconds.
  3. Verify the notice is removed.

4 - Retry loading the remote editor

  1. Repeat the steps for test case 1.
  2. Re-enable your network connection.
  3. Tap "Retry."
  4. Verify the remote editor is loaded (w/ plugins).

@dcalhoun dcalhoun added the [Type] Enhancement A suggestion for improvement. label Sep 24, 2024
@dcalhoun dcalhoun marked this pull request as ready for review September 25, 2024 13:28
@dcalhoun dcalhoun requested a review from geriux September 25, 2024 13:28
@dcalhoun dcalhoun removed the request for review from geriux October 17, 2024 18:54
Empower the user to continue editing with the default, local editor
capable of editing core blocks and notify them of the load failure.
Empower users to resolve issues themselves.
Increase the utility of the message.
@dcalhoun dcalhoun force-pushed the feat/handle-remote-editor-load-failure branch from 6e827cb to cfc1273 Compare October 17, 2024 19:00
@dcalhoun
Copy link
Member Author

👋🏻 @kean. When you have time, I would appreciate your review and testing of this pull request and its sibling, wordpress-mobile/WordPress-iOS#23624. I recommend using the Coblocks plugin as a test for third-party block support, as it appears the Jetpack plugin may have changed and broken support for it.

Thanks!

@kean
Copy link
Contributor

kean commented Oct 18, 2024

I tested it and got the following result:

Screenshot 2024-10-18 at 4 04 49 PM

Does the editor attempt to fetch custom blocks for every site, even if they don't include any? Does it block you from interacting with the editor until the request finishes? It's not ideal if this error is shown for sites with no custom blocks and/or increases time-to-interactive.

Some additional feedback:

  • It's a bit overwhelming to see two CTAs on the screen at the same time ("Keep HTML" and "Retry").
  • There is no loading indicator when preparing the editor

(likely unrelated) I used the "coblocks/map" block and every time I open the post (with third-party blocks loaded), I see the following message:

Screenshot 2024-10-18 at 4 05 36 PM

@dcalhoun
Copy link
Member Author

@kean thank you for taking time to review this. I apologize for not providing additional context sooner that might've better set the expectations for this PR. These changes slightly improve the "remote" editor experience to avoid a blank white page if the editor fails to load. The remote editor is very much a prototype with a lot of rough edges—performance, stability, usability, etc.

The remote editor is less of a priority while I figure out how we support it from the API (pbArwn-6Fz-p2), but I wanted to avoid letting these proposed changes become stale in the meantime. I hoped to merge this work in the name of iterative progress. I agree with your comments, there is a lot of room for improvement.

Let me know what you think about merging this rudimentary error handling as-is for the time being. Thanks! 🙇🏻


Does the editor attempt to fetch custom blocks for every site, even if they don't include any?

I'm uncertain if by "they" you mean the post content or the site. Regardless, yes, this prototype remote editor loads all the editor assets regardless if custom blocks are present.

Does it block you from interacting with the editor until the request finishes? It's not ideal if this error is shown for sites with no custom blocks and/or increases time-to-interactive.

Yes, the request for the remote editor must resolve before one can interact with the editor. I agree there is room to improve the performance, my initial thoughts go to strategic caching.

There could be a fundamentally different loading approach that might avoid blocking editor interactivity, but it may prove very complicated to achieve near instant interactivity while also avoiding/resolving version conflicts with editor/block assets fetched from a remote site.

It's a bit overwhelming to see two CTAs on the screen at the same time ("Keep HTML" and "Retry").

Understandable. The former is a core editor message regarding an unsupported block, the latter is the error handling proposed in these changes.

I'll spend time pondering how we might avoid such a state, but my initial thoughts are that it is unavoidable due to the nature of the problem at hand—the remote editor, and thus the support for the custom block, failed to load. Hence, the two notices. I suppose we could display an editor load failure message that requires acknowledgement to progress to the fallback editor with unsupported custom blocks.

There is no loading indicator when preparing the editor

Yes, a very blatant omission for this prototype. I agree we need to communicate the activity to the user.

(likely unrelated) I used the "coblocks/map" block and every time I open the post (with third-party blocks loaded), I see the following message:

Interesting, thank you for sharing. For reasons unknown to me at this time, it appears the Google Maps URL found run the DOM block markup includes hl=en-US, as opposed to the saved block markup containing hl=en. This discrepancy results in the editor notice.

Full block validation error

[Error] Block validation: Block validation failed for `coblocks/map` ({name: "coblocks/map", icon: Object, keywords: ["coblocks", "address", "maps", "google", "directions"], attributes: Object, providesContext: {}, …}).

Content generated by `save` function:

<div style="min-height:400px" data-map-attr="/qaddress/q:/q1 E 161st St, Bronx, NY 10451/q||/qlat/q:/qundefined/q||/qlng/q:/qundefined/q||/qskin/q:/qstandard/q||/qzoom/q:/q12/q||/qiconSize/q:/q36/q||/qmapTypeControl/q:/qtrue/q||/qzoomControl/q:/qtrue/q||/qstreetViewControl/q:/qtrue/q||/qfullscreenControl/q:/qtrue/q" class="wp-block-coblocks-map"><iframe title="Google Map" frameborder="0" style="width:100%;min-height:400px" src="https://www.google.com/maps?q=1%20E%20161st%20St%2C%20Bronx%2C%20NY%2010451&amp;output=embed&amp;hl=en&amp;z=12"></iframe></div>

Content retrieved from post body:

<div style="min-height:400px" data-map-attr="/qaddress/q:/q1 E 161st St, Bronx, NY 10451/q||/qlat/q:/qundefined/q||/qlng/q:/qundefined/q||/qskin/q:/qstandard/q||/qzoom/q:/q12/q||/qiconSize/q:/q36/q||/qmapTypeControl/q:/qtrue/q||/qzoomControl/q:/qtrue/q||/qstreetViewControl/q:/qtrue/q||/qfullscreenControl/q:/qtrue/q" class="wp-block-coblocks-map"><iframe title="Google Map" frameborder="0" style="width:100%;min-height:400px" src="https://www.google.com/maps?q=1%20E%20161st%20St%2C%20Bronx%2C%20NY%2010451&amp;output=embed&amp;hl=en-US&amp;z=12"></iframe></div>
	forEach
	io (index.min.js:7:45559)
	(anonymous function) (index.min.js:7:45926)
	reduce
	(anonymous function) (index.min.js:1:250981)
	useMemo (react-dom.min.js:1:114228)
	ml (index.min.js:1:250815)
	(anonymous function) (index.min.js:145:11386)
	(anonymous function) (index.min.js:145:11780)
	gt (react-dom.min.js:1:45669)
	js (react-dom.min.js:1:120126)
	Sl (react-dom.min.js:1:88334)
	kl (react-dom.min.js:1:88262)
	bl (react-dom.min.js:1:88125)
	dl (react-dom.min.js:1:85303)
	Nn (react-dom.min.js:1:32434)
	(anonymous function) (react-dom.min.js:1:82945)

@kean kean self-requested a review October 21, 2024 15:26
@kean
Copy link
Contributor

kean commented Oct 21, 2024

I suppose we could display an editor load failure message that requires acknowledgement to progress

I was also thinking something along these lines, yes. It doesn't help that the root cause of the first error (site does not include support for block) is explained in the second error message.

@dcalhoun
Copy link
Member Author

It doesn't help that the root cause of the first error (site does not include support for block) is explained in the second error message.

Agreed. For better or worse, the second error message is provided by core and more universal. We could simply rely solely on the second error message, but I questioned if it provides enough pointed information in this context (editor load failed) to be helpful (allow the user to attempt recovery themselves).

@dcalhoun
Copy link
Member Author

@kean thank you for the approval. Are you comfortable approving the sibling integration PR? wordpress-mobile/WordPress-iOS#23624

@dcalhoun dcalhoun merged commit 849118a into trunk Oct 21, 2024
@dcalhoun dcalhoun deleted the feat/handle-remote-editor-load-failure branch October 21, 2024 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Type] Enhancement A suggestion for improvement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants