Refactor FXIOS-15780 Reader mode Add custom readermode scheme by Alex-Bangu · Pull Request #33846 · mozilla-mobile/firefox-ios

Alex-Bangu · 2026-05-15T22:43:52Z

📜 Tickets

💡 Description

Adds the readermode://app/... scheme handler scaffolding. After this PR the new types are present in the codebase but not yet visible to users. Reader mode still uses the legacy GCDWebServer/localhost path. The actual switch to the new scheme is enabled by a feature flag in FXIOS-15783.

Below is a full list of changes by file:

ReaderModeSchemeHandler.swift (new): The custom scheme handler that owns readermode://app/...; routes incoming requests and picks the right cache based on whether the tab is private.

PageRoute.swift (new): Handles readermode://app/page?url=... requests. Looks up the article in the cache, extracts it if missing, or shows a Load Original error page if extraction fails.

ReadabilityService.swift (modified): Added an async extract method on top of the existing operation so PageRoute can await a result instead of just side-effecting the cache.

ReaderModeCache.swift (modified): Made the protocol Sendable so it can be used as a stored property in Sendable types like PageRoute.

TabConfigurationProvider.swift (modified): Registers the new scheme handler on every tab's WKWebView config and passes the profile through instead of just prefs.

TabManagerImplementation.swift (modified): Updated to pass profile instead of profile.prefs into TabConfigurationProvider's new init.

BrowserViewController+WebViewDelegates.swift (modified): Added a branch in decidePolicyFor that allows readermode:// URLs through to WebKit instead of treating them as external-app links.

URLExtension.swift in BrowserKit/Common (modified): Broadened isReaderModeURL to recognize both the legacy localhost form and the new readermode:// form during the migration window.

ReaderMode.js (modified): Broadened the in-page URL regex to match both forms so the reader script keeps working after the cutover.

MockReaderModeCache.swift (new test): Tiny in-memory cache for tests; throws on miss to match the real caches.

PageRouteTests.swift (new test): Covers URL parsing, cache hit, cache miss with stubbed extractor, and the failure-fallback page.

ReaderModeSchemeHandlerTests.swift (new test): Covers scheme/host validation, nil URL handling, and unknown-path fallback through the default route.

URLExtensionTests.swift (modified test): Added two cases verifying isReaderModeURL accepts the new scheme form and rejects wrong-host variants.

🎥 Demos

No user-end demos to show for this pull request.

📝 Checklist

I filled in the ticket numbers and a description of my work
I updated the PR name to follow our PR naming guidelines
I ensured unit tests pass and wrote tests for new code
If working on UI, I checked and implemented accessibility (Dynamic Text and VoiceOver)
If adding telemetry, I read the data stewardship requirements and will request a data review
If adding or modifying strings, I read the guidelines and will request a string review from l10n
If needed, I updated documentation and added comments to complex code

mobiletest-ci-bot · 2026-05-15T23:21:51Z

	Messages
📖	Project coverage: 42.27%

💪 Quality guardian

1 tests files modified. You're a champion of test coverage! 🚀

🧹 Tidy commit

Just 4 file(s) touched. Thanks for keeping it clean and review-friendly!

💬 Description craftsman

Great PR description! Reviewers salute you 🫡

🧑‍💻 New `Task {}` detected

New Task {} added. Please add a concurrency reviewer on your PR: @Cramsden @ih-codes @lmarceau

File	Line	Change
`firefox-ios/Client/Frontend/Reader/SchemeHandler/ReaderModeSchemeHandler.swift`	74	Added

✅ New file code coverage

All new files meet their thresholds**.

✅ Existing file code coverage

No modified file detected so code coverage gate wasn't ran.

Client.app: Coverage: 40.77

File	Coverage
PageRoute.swift	94.74%	✅
ReaderModeSchemeHandler.swift	76.92%	✅

Generated by 🚫 Danger Swift against 8541d49

ih-codes

Great test cases, thanks @Alex-Bangu ! Most importantly, I appreciate you shrinking the scope of this PR. I was just starting to review as you updated and this was much more manageable for me to get through in a timely fashion. 😄

Nice work overall, I only had some tiny nitpicks but approve of the overall solution. I didn't run the app since this isn't hooked up yet, but I did run the new unit tests and confirmed they were passing for me too.

ih-codes · 2026-05-19T16:59:20Z

+import Shared
+import WebEngine
+
+/// Serves the reader-mode page document at `readermode://app/page?url=<encoded-article-url>`.


nitpick: In the plain English code documentation (here and other files), let's be consistent in describing reader mode as "reader mode", "readermode", or "ReaderMode" (the dash was specific to the legacy reader mode url scheme it seems?). I don't have strong feelings on which you prefer, but let's not use the dash and just be consistent across new documentation we add. 😄

ih-codes · 2026-05-19T17:01:54Z

+///        - `/app/page-exists`  -> (future)
+///        - `/app/styles/...`   -> (future, static)
+///        - `/app/fonts/...`    -> (future, static)


Just curious, what are these other future reader mode routes for, exactly? 🤔

Oops! Sorry about that, no other paths will be defined besides /app/page.

ih-codes · 2026-05-19T18:19:31Z

+/// This initial version validates the incoming URL parameters. Content rendering
+/// (cache integration, readability extraction, error pages) will be added in
+/// FXIOS-15783.


Thanks for splitting up the work! 👌

ih-codes · 2026-05-19T18:20:20Z

+    init(profile: Profile) {
+        self.profile = profile
+    }
+    // Always erros our for now, will actually handle in next PR


ih-codes · 2026-05-19T18:33:47Z

+    /// Normalizes any thrown `Error` into a `TinyRouterError`.
+    private func mapError(_ error: Error) -> TinyRouterError {
+        if let tinyError = error as? TinyRouterError {
+            return tinyError
+        }
+        return .unknown(String(describing: error))
+    }


I wonder if something like this would be better written as a (perhaps static?) method on the TinyRouterError itself.

@issammani, looks like you wrote that file initially, so what do you think?

Yes that would neatly work in the router package itself as a helper.

ih-codes · 2026-05-19T18:35:38Z

+    /// Validates incoming requests and forwards them to the router.
+    func webView(_ webView: WKWebView, start urlSchemeTask: WKURLSchemeTask) {
+        let id = ObjectIdentifier(urlSchemeTask)
+        let requestTask = Task { @MainActor in


Just FYI, since ReaderModeSchemeHandler is, as you noted, already isolated to the main actor, this Task will inherit @MainActor so it is not necessary to write this explicitly.

ih-codes · 2026-05-19T18:41:21Z

+        subject.webView(webView, start: task)
+        wait(for: [failExpectation], timeout: 1.0)
+
+        XCTAssertEqual(task.failedErrors.count, 1)


Let's check the other task properties, like on lines 38-41 here as well for completion's sake and extra validation.

Likewise for the other tests in this file.

ih-codes · 2026-05-19T18:44:00Z

@@ -0,0 +1,107 @@
+// This Source Code Form is subject to the terms of the Mozilla Public


I noticed we don't have any tests covering correct usage, i.e. good scheme, good host, good URL. Can we add one?

+1. @Alex-Bangu would be nice if you can also add a test for the default router rejecting files not registered as allowed per this comment

ih-codes · 2026-05-19T18:57:07Z

+        // Path doesn't match the "page" prefix, so it falls through to the default route
+        // (`StaticFileRoute`), which attempts `Bundle.main.url(forResource:withExtension:)`
+        // for a resource that does not exist and throws `TinyRouterError.badURL`.


Outside the scope of this PR, I have a question for @issammani... Would it make sense for line 17 of StaticFileRoute.swift to return an error that implies the asset is missing from the bundle rather than a badURL? badURL seems a bit vague of an error to receive in this situation when the real issue is an unhandled path. 🤔 In an ideal world we would actually be getting some error for the task suggesting that the real problem is that the URL is an unhandled path, not "bad" as in possibly malformed.

Yeah that makes sense. As I mentioned earlier in my comment we shouldn't use StaticFileRoute here. @Alex-Bangu feel free to create a new error in TinyRouterError

Thanks @issammani, an even better suggestion then! 👌

Co-authored-by: Isabella <173110554+ih-codes@users.noreply.github.com>

issammani

Great work so far. I left a few comments we should address before merging 👍

issammani · 2026-05-20T14:15:19Z

+/// This initial version validates the incoming URL parameters. Content rendering
+/// (cache integration, readability extraction, error pages) will be added in
+/// FXIOS-15783.


issammani · 2026-05-20T14:21:29Z

+        self.router = TinyRouter()
+            .register("page", PageRoute(profile: profile))
+            .setDefault(StaticFileRoute())


From a security perspective, I don't think we should use StaticFileRoute() as is. For translations, the engine is not part of the content process. It's in the background and we have total control over the assets we inject. That's not the case here for reader mode. I would create a router that only allows loading explicit files from reader-mode/ like styles/Reader.css and so on othewise it fails even if the files exist in that path.

issammani · 2026-05-20T14:23:31Z

+    /// Normalizes any thrown `Error` into a `TinyRouterError`.
+    private func mapError(_ error: Error) -> TinyRouterError {
+        if let tinyError = error as? TinyRouterError {
+            return tinyError
+        }
+        return .unknown(String(describing: error))
+    }


Yes that would neatly work in the router package itself as a helper.

issammani · 2026-05-20T14:25:01Z

@@ -0,0 +1,107 @@
+// This Source Code Form is subject to the terms of the Mozilla Public


+1. @Alex-Bangu would be nice if you can also add a test for the default router rejecting files not registered as allowed per this comment

issammani · 2026-05-20T14:27:39Z

+        // Path doesn't match the "page" prefix, so it falls through to the default route
+        // (`StaticFileRoute`), which attempts `Bundle.main.url(forResource:withExtension:)`
+        // for a resource that does not exist and throws `TinyRouterError.badURL`.


Yeah that makes sense. As I mentioned earlier in my comment we shouldn't use StaticFileRoute here. @Alex-Bangu feel free to create a new error in TinyRouterError

Co-authored-by: Issam Mani <issamouu69@gmail.com>

Alex-Bangu requested a review from a team as a code owner May 15, 2026 22:43

Alex-Bangu requested a review from ih-codes May 15, 2026 22:43

Alex-Bangu changed the title ~~Refactor 15780 Reader mode Add custom readermode scheme~~ Refactor FXIOS-15780 Reader mode Add custom readermode scheme May 15, 2026

Alex-Bangu requested a review from issammani May 15, 2026 23:01

Alex Bangu added 2 commits May 19, 2026 13:46

Simplified the changes on this branch to make it easier to review

da14f94

trailing whitespace fix

fef22c6

Alex-Bangu force-pushed the abangu/FXIOS-15780-add-readermode-scheme branch from 0ddee16 to fef22c6 Compare May 19, 2026 18:04

ih-codes reviewed May 19, 2026

View reviewed changes

Alex-Bangu and others added 2 commits May 19, 2026 15:13

Update firefox-ios/Client/Frontend/Reader/SchemeHandler/PageRoute.swift

a1dea83

Co-authored-by: Isabella <173110554+ih-codes@users.noreply.github.com>

new tests, whitespace changes, comment fixes

a8f232c

issammani requested changes May 20, 2026

View reviewed changes

Update PageRoute.swift to not allow dataURIs

8541d49

Co-authored-by: Issam Mani <issamouu69@gmail.com>

		@@ -0,0 +1,107 @@
		// This Source Code Form is subject to the terms of the Mozilla Public

Conversation

Alex-Bangu commented May 15, 2026

📜 Tickets

💡 Description

🎥 Demos

📝 Checklist

Uh oh!

mobiletest-ci-bot commented May 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💪 Quality guardian

🧹 Tidy commit

💬 Description craftsman

🧑‍💻 New Task {} detected

✅ New file code coverage

✅ Existing file code coverage

Client.app: Coverage: 40.77

Uh oh!

ih-codes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

issammani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mobiletest-ci-bot commented May 15, 2026 •

edited

Loading

🧑‍💻 New `Task {}` detected