Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set the API context when using Comlink.wrap() #1085

Merged
merged 1 commit into from
Mar 5, 2024
Merged

Conversation

adamziel
Copy link
Collaborator

@adamziel adamziel commented Mar 5, 2024

Fixes WordPress Playground Block not loading when the Site Editor is wrapped in an iframe.

Playground uses Comlink to talk to the remote API via window.postMessage(). By default, Comlink binds the message event listener to the current global window object. However, when the Site Editor is iframed, the startPlaygroundWeb function runs in the global window context, but the Playground iframe lives inside of the Editor Canvas iframe context. Playground's remote.html would then send all its messages (via postMessage) to the parent window, which is Editor Canvas and not the global window. As a result, Comlink would never notice any communication and the Playground wouldn't load.

This PR ensires the context argument is always set to Playground's iframe parent window.

Testing instructions

This is convoluted:

  1. Clone https://github.com/WordPress/playground-tools/tree/trunk/packages/wordpress-playground-block
  2. Update the remote URL to http://localhost:5400/remote.html
  3. Build the @wp-playground/client package in this repo and substitute the block's dependency with it (or use npm link)
  4. Run the Playground block in the dev mode (as described in its README)
  5. Install the Gutenberg plugin to enable iframe-based Canvas
  6. Insert the Playground block
  7. Confirm Playground loads correctly

Fixes WordPress Playground Block not loading when the Site Editor is
wrapped in an iframe.

Playground uses Comlink to talk to the remote API via
`window.postMessage()`. By default, Comlink binds the `message` event
listener to the current global window object. However, when the Site
Editor is iframed, the `startPlaygroundWeb` function runs in the global
`window` context, but the Playground iframe lives inside of the Editor
Canvas iframe context. Playground's `remote.html` would then send a much
of messages (via `postMessage`) to its parent window (Editor Canvas),
and Comlink would never notice.

This PR ensires the `context` argument is always set to Playground's
iframe parent window.

 ## Testing instructions

1. Clone https://github.com/WordPress/playground-tools/tree/trunk/packages/wordpress-playground-block
1. Update the remote URL to `http://localhost:5400/remote.html`
1. Build the `@wp-playground/client` package in this repo and substitute
   the block's dependency with it (or use npm link)
1. Run the Playground block in the dev mode (as described in its README)
1. Install the Gutenberg plugin to enable iframe-based Canvas
1. Insert the Playground block
1. Confirm Playground loads correctly
@adamziel adamziel added [Type] Bug An existing feature does not function as intended [Aspect] Browser JS API labels Mar 5, 2024
@adamziel adamziel merged commit 6cf353f into trunk Mar 5, 2024
5 checks passed
@adamziel adamziel deleted the set-comlink-context branch March 5, 2024 16:24
adamziel added a commit that referenced this pull request Mar 6, 2024
Follows up on #1085 that correctly identified the issue with
`postMessage` event handlers, but passed the context argument to the
wrong function call. `wrap()` uesd in that PR is not responsible for binding the message
event handler. `windowEndpoint()` is. This PR fixes the mistake.
adamziel added a commit that referenced this pull request Mar 6, 2024
…1087)

Follows up on #1085 that correctly identified the issue with
`postMessage` event handlers, but passed the context argument to the
wrong function call. `wrap()` uesd in that PR is not responsible for
binding the message event handler. `windowEndpoint()` is. This PR fixes
the mistake.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Aspect] Browser [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant