Bugfix FXIOS-16715 Keep a translation from landing on the wrong document - #35451
Open
razvanlitianu wants to merge 1 commit into
Open
Conversation
razvanlitianu
force-pushed
the
rlitianu/fxios-16715-cross-document-translation
branch
from
August 31, 2026 12:40
7c49c47 to
5b2d776
Compare
💪 Quality guardian1 tests files modified. You're a champion of test coverage! 🚀 🧹 Tidy commitJust 5 file(s) touched. Thanks for keeping it clean and review-friendly! 🙌 Friday high-fiveThanks for pushing us across the finish line this week! 🙌 💬 Description craftsmanGreat PR description! Reviewers salute you 🫡 ✅ Code coverage
Client.app: Coverage: 46.09
Generated by 🚫 Danger Swift against 215365e |
Collaborator
Author
|
And sorry, there's one more stacked on top of it: #35451 (#35451). i know that's a lot to ask in one go. it fixes the translation landing on the page you navigated to, the thing you'd flagged. needs a language model that's never been downloaded to reproduce, steps are in the description. whenever you have room 🙏 @ih-codes |
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request fixes a race where translation JS could be applied to a different document than the one the user requested (e.g., navigating during slow model prewarm), causing incorrect UI state on the destination page.
Changes:
- Add a per-document identifier in the translations page script and require an
expectedDocumentIdmatch before starting translations. - In native, capture the document id before prewarm, re-check cancellation after prewarm, and treat a mismatch as a dedicated
.documentChangederror. - Suppress user-facing error/toast for
.documentChanged, while still clearing the translation icon and closing out the telemetry flow; add a middleware test for the dropped-translation behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| firefox-ios/firefox-ios-tests/Tests/ClientTests/TranslationsTests/TranslationsMiddlewareTests.swift | Adds a test ensuring .documentChanged does not surface an error/toast and clears loading state. |
| firefox-ios/Client/Frontend/UserContent/UserScripts/MainFrame/AtDocumentStart/TranslationsEntrypoint.js | Exposes documentId() and makes startTranslations return false on document mismatch. |
| firefox-ios/Client/Frontend/Translations/TranslationsMiddleware.swift | Treats .documentChanged as a drop: clears icon and flow id without dispatching translation failure UI. |
| firefox-ios/Client/Frontend/Translations/Service/TranslationsServiceError.swift | Introduces .documentChanged and a stable telemetry token. |
| firefox-ios/Client/Frontend/Translations/Service/TranslationsService.swift | Captures document id pre-prewarm, checks cancellation post-prewarm, and maps JS false to .documentChanged. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+112
to
+116
| private func currentDocumentIdJS(on webView: WKWebView) async throws -> String { | ||
| let failure = TranslationsServiceError.jsEvaluationFailed( | ||
| reason: "JS evaluation failed: currentDocumentIdJS" | ||
| ) | ||
| let js = "return window.__firefox__.Translations.documentId()" |
translateCurrentPage resolved the tab's WKWebView, awaited the model prewarm, then called into the page. Nothing between the await and the JS call checked for cancellation or for the document having changed, and a tab's WKWebView is a single object that outlives its documents. When the prewarm was slow, which happens the first time a language pair is used, the translation was applied to whatever page the user had navigated to. The page now mints a document identifier that native captures before the prewarm and hands back when starting; startTranslations refuses a request addressed to a different document. A cancellation check after the prewarm covers the case where the task is cancelled while the model downloads. Navigating away is not a failure, so the request is dropped silently rather than showing a translation error on the new page.
razvanlitianu
force-pushed
the
rlitianu/fxios-16715-cross-document-translation
branch
from
September 4, 2026 08:49
5b2d776 to
215365e
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
📜 Tickets
Jira ticket
Github issue
💡 Description
Stacked on #35450, review that first.
Ask for a translation, navigate before it starts, and it lands on the page you moved to, with a
grey icon over visibly translated text.
translateCurrentPageawaited the model prewarm and then called into the page, with nocancellation or document check in between. A tab's
WKWebViewoutlives its documents, so a slowprewarm, which is what the first use of a language pair gives you, means the JS arrives in
whatever loaded meanwhile.
The page now exposes a per-document id. Native reads it before the prewarm and passes it back
when starting, and
startTranslationsrefuses a mismatch. Only an explicitfalsemaps todocumentChanged, so a broken bridge still reports as a failure instead of going quiet.Navigating away clears the icon and closes the telemetry flow rather than showing a translation
error on the new page.
🎥 Demos
Translate with a model that has never been downloaded, then navigate away within a second. The page you land on used to be rendered in the target language under a grey icon, despite never being translated. Now it stays in its original language and no error toast appears.
🧪 Testing
Needs a language model that has never been downloaded on the device. With a cached model the
gap is a few hundred ms and the race cannot be hit.
appears. The icon is not left spinning.
📝 Checklist