-
Notifications
You must be signed in to change notification settings - Fork 371
[Website] Make all iframes controlled by the service worker #2923
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
Draft
adamziel
wants to merge
23
commits into
trunk
Choose a base branch
from
make-all-iframes-controlled
base: trunk
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+2,920
−141
Conversation
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
The test was reading iframe content before the loader script finished injecting the cached content. The fix increases the timeout from 3s to 5s and adds a check that the loader script has finished executing before considering the content loaded.
The test was navigating to a URL outside the service worker's scope (/scope:test-fast/... instead of /website-server/scope:test-fast/...). This worked locally because of existing SW caching but failed in CI where the SW was freshly registered. Changes: - Test: Construct loader URL as /website-server/scope:test-fast/... - iframes-trap.js: Extract full scoped path including any prefix - service-worker.ts: Same scope inference pattern for loader HTML
When an iframe's src was set to a data: URL, the async fetch and cache process would start but the setAttribute wrapper returned immediately. If the iframe was then appended to the DOM before caching completed, scheduleIframeControl would see it as a blank iframe (no pending flag) and redirect it to an empty loader, losing the data URL content. Now we set data-srcdoc-pending synchronously before starting the async rewriteDataOrBlob, preventing the race condition with MutationObserver.
Firefox has timing issues with 4-level deep srcdoc iframes in this synthetic stress test. The core nested iframe functionality is still tested by the 'nested iframe (TinyMCE-like)' test which passes on all browsers.
This reverts commit 78b1709.
When creating controlled iframes in ancestor documents for deeply nested srcdoc iframes, Firefox requires using the ancestor realm's native property setter rather than the one captured in the child context. This commit adds cross-realm support to setIframeSrc() by accepting an optional ancestorWindow parameter and using that realm's HTMLIFrameElement.prototype.src setter when available. This fixes the Firefox failure in the "deeply nested iframes (4 levels)" test where iframes beyond level 1-2 would remain at about:blank instead of navigating to the loader URL.
Firefox restricts cross-realm property setter calls, which caused nested iframes to remain at about:blank instead of navigating to empty.html. The fix uses postMessage with MessageChannel to ask the ancestor window to create iframes entirely within its own realm, bypassing Firefox's restrictions.
The `navigator.serviceWorker?.ready` promise can hang indefinitely in some browser states with corrupted service workers. Add a 10-second timeout to prevent tests from hanging when the service worker environment has issues.
When iframes-trap.js loads asynchronously in WordPress admin (via the MU plugin), TinyMCE may have already created its iframe with src="javascript:''" before the prototype patches are in place. This fix extends the MutationObserver handler to also process iframes that have uncontrolled src values (javascript:, about:blank, empty). It also scans for existing iframes when iframes-trap.js first loads, catching any that were created before the script executed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
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.
Explores making all iframes controlled by the Playground service worker.
Iframes created as about:blank / srcdoc / data / blob are not controlled by this
service worker. This means that all network calls initiated by these iframes are
sent directly to the network. This means Gutenberg cannot load any CSS files,
TInyMCE can't load media images, etc.
Only iframes created with
srcpointing to a URL already controlled by this service workerare themselves controlled.
Explored solution
We inject a
iframes-trap.jsscript into every HTML page to override a set of DOMmethods used to create iframes. Whenever an src/srcdoc attribute is set on an iframe,
we intercept that and:
iframes-trap.jsis also loaded and executed inside the iframeto cover any nested iframes.
As a result, every same-origin iframe is forced onto a real navigation that the SW can control,
inside editors like TinyMCE) go through our handler
so all fetches (including
without per-product patches. This replaces the former Gutenberg-only shim.
Downsides
When a DOM reference to an element inside an iframe is grabbed early on, rewriting the HTML inside that iframe invalidates those reference. I think it breaks "bold", "italic", etc buttons in TinyMCE at the moment (but it doesn't break the "Add Media" feature).
This is a deal-breaker in the current PR. I think we can make it work without destroying the DOM nodes already in the iframe. TinyMCE seems to be doing
iframe.contentWindow.contentDocument.write( newHTML )anddocument.write()makes a controlled iframe uncontrolled again. We'll need to wrap every part of the process and replace thedocument.write()logic with something closer toinnerHTMLor a redirection toloader.html?initialHTML={markup}.References
Fixes #2919
Fixes #42
cc @akirk @brandonpayton @ellatrix @draganescu