diff options
| author | Peter Stone <thepeterstone@gmail.com> | 2026-11-13 08:40:44 +0000 |
|---|---|---|
| committer | Peter Stone <thepeterstone@gmail.com> | 2026-11-13 08:40:44 +0000 |
| commit | fc25307e7f258b1043e1367a44a8800d03085c66 (patch) | |
| tree | 14eeabea19e6665e03008c95441c6242ae7b2e10 /app/src | |
| parent | 04408cceea17f3e32b6e88edf4da94b26385b026 (diff) | |
Make an image zoomable even when the feed didn't spell out where it lives
Tapping an image in a Ftrain article did nothing. The zoom affordance was
there, and the rendering path was the ordinary one -- the feed's own HTML, with
reader mode off -- but a tap went nowhere at all.
Tap detection works by wrapping every article image in a link to a scheme
nothing can navigate to, which the WebView then hands to
shouldOverrideUrlLoading. That href has to carry the complete image URL on its
own, so an image whose src was relative got no wrapper and no tap: better an
image that can't be zoomed, the reasoning went, than a link that goes nowhere.
Ftrain's <content:encoded> is entirely site-relative -- every image in it is
src="/art/uploads/..." -- so not one of them was zoomable.
That reasoning had an unexamined premise. The page itself resolves those srcs
perfectly well, against the base URL loadReaderHtml already hands
loadDataWithBaseURL: the article's own link. Resolving them the same way at
HTML-assembly time produces exactly the URL the WebView went and fetched, so
the wrapper can be built after all. Both reader surfaces now pass that base URL
down to the zoom pass. Where there genuinely is no base URL to resolve
against -- nothing else in the app loads reader HTML without one today -- the
old behavior stands and the image stays un-zoomable rather than acquiring a
dead link, and a data: or javascript: src is still refused however it was
reached, since the viewer could never open one.
This is why the report could only have come from a feed with reader mode off.
Reader mode never hits it: Readability rewrites every src to an absolute URL
before the extracted HTML reaches us, which is also why the same images zoom
fine the moment the feed's reader toggle is on.
TDD: site-relative and path-relative resolution, and the whole trip through
wrapReaderHtml, were written first against the unused parameter and went red on
assertion mismatches (3 of 38 in ImageZoomTest), green on the resolution
itself. The data:-src case was written at the same time and passed immediately
-- it pins existing behavior against the new code path rather than having
driven it.
Verified on the emulator against the reported article itself, subscribed live
from ftrain.com/feed.xml with reader mode off: on the previous build, a tap on
the header image left the screen byte-identical; on this one the same tap at
the same point opens the full-screen viewer.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018AwnALZL6bD4VBtATDVv3B
Diffstat (limited to 'app/src')
5 files changed, 93 insertions, 16 deletions
diff --git a/app/src/main/java/org/terst/feedreader/ui/ImageZoomLinks.kt b/app/src/main/java/org/terst/feedreader/ui/ImageZoomLinks.kt index b83b600..d924c02 100644 --- a/app/src/main/java/org/terst/feedreader/ui/ImageZoomLinks.kt +++ b/app/src/main/java/org/terst/feedreader/ui/ImageZoomLinks.kt @@ -1,5 +1,7 @@ package org.terst.feedreader.ui +import java.net.URI + // How a tap on an article image is noticed at all, and turned into a URL the viewer // (ImageZoom.kt) can open. Split from the viewer itself because this half runs at // HTML-assembly time, inside wrapReaderHtml -- it's reader-markup plumbing, not gestures. @@ -35,7 +37,7 @@ private val imageFileExtensions = listOf(".jpg", ".jpeg", ".png", ".gif", ".webp * and wrapping it would swallow the tap that plays the video). Those are still zoomable when * the link points at an image file, which [zoomableImageTarget] handles on the other side. */ -fun linkifyImagesForZoom(html: String): String = buildString { +fun linkifyImagesForZoom(html: String, baseUrl: String? = null): String = buildString { var copiedUpTo = 0 var anchorDepth = 0 for (tag in anchorOrImageTagRegex.findAll(html)) { @@ -45,7 +47,7 @@ fun linkifyImagesForZoom(html: String): String = buildString { text.startsWith("<a", ignoreCase = true) -> anchorDepth++ anchorDepth == 0 -> { val src = imgSrcRegex.find(text)?.let { it.groupValues[1].ifEmpty { it.groupValues[2] } } - val href = src?.let { zoomHrefForSrc(it) } + val href = src?.let { zoomHrefForSrc(it, baseUrl) } if (href != null) { append(html, copiedUpTo, tag.range.first) append("<a href=\"").append(href).append("\">").append(text).append("</a>") @@ -57,17 +59,14 @@ fun linkifyImagesForZoom(html: String): String = buildString { append(html, copiedUpTo, html.length) } -private fun zoomHrefForSrc(src: String): String? { +private fun zoomHrefForSrc(src: String, baseUrl: String?): String? { val trimmed = src.trim() val absolute = when { // Protocol-relative (//host/path) is common in feed markup and resolves fine in the // page itself, but our scheme-prefixed href has to carry a complete URL on its own. trimmed.startsWith("//") -> "https:$trimmed" trimmed.startsWith("http://", ignoreCase = true) || trimmed.startsWith("https://", ignoreCase = true) -> trimmed - // Anything else (site-relative, data:, javascript:) is skipped rather than guessed at: - // a relative src resolves against the WebView's base URL, which a custom-scheme href - // can't reproduce, so wrapping it would only build a dead link. - else -> null + else -> resolvedAgainstBase(trimmed, baseUrl) } ?: return null // The src came out of a quoted attribute and goes straight back into one, already escaped // as the source wrote it. A raw quote or angle bracket could only arrive via a @@ -76,6 +75,32 @@ private fun zoomHrefForSrc(src: String): String? { } /** + * What a relative src actually points at, or null if there's no way to tell or it isn't + * something the viewer could open. + * + * A feed's own HTML routinely keeps its images site-relative (Ftrain's <content:encoded> is + * entirely `<img src="/art/uploads/...">`), and that path is what the non-reader rendering + * shows verbatim -- reader mode never hits this because Readability rewrites every src to an + * absolute URL before we see it. The page resolves those against the base URL that + * loadReaderHtml hands loadDataWithBaseURL, which is the article's own link, so resolving them + * the same way here yields exactly the URL the WebView itself fetched. With no base URL there + * is still nothing to resolve against, and the image stays un-zoomable rather than getting a + * dead link. + */ +private fun resolvedAgainstBase(src: String, baseUrl: String?): String? { + if (src.isEmpty() || baseUrl.isNullOrBlank()) return null + // URI() rejects a src with a space or other illegal character in it rather than guessing, + // which is the right answer here too -- an un-zoomable image beats a dead link. + val resolved = runCatching { URI(baseUrl).resolve(src) }.getOrNull() + // data: and javascript: srcs are already absolute, so resolve() hands them straight back; + // only a real network URL is something Coil could ever open. + return when (resolved?.scheme?.lowercase()) { + "http", "https" -> resolved.toString() + else -> null + } +} + +/** * The image a tapped URL should open full-screen, or null if it isn't an image at all. * * Covers both halves of the problem: links [linkifyImagesForZoom] built, and links the article diff --git a/app/src/main/java/org/terst/feedreader/ui/LinkReaderScreen.kt b/app/src/main/java/org/terst/feedreader/ui/LinkReaderScreen.kt index 9f9f74a..69de173 100644 --- a/app/src/main/java/org/terst/feedreader/ui/LinkReaderScreen.kt +++ b/app/src/main/java/org/terst/feedreader/ui/LinkReaderScreen.kt @@ -265,14 +265,14 @@ fun LinkReaderScreen(url: String, onBack: () -> Unit, onOpenLink: (String) -> Un onFontSizeChanged = { newSizePx -> scope.launch { settings.setReaderFontSizePx(newSizePx) } } ) ).apply { - val html = wrapReaderHtml(currentExtractedContent, extractedTitle, fontSizePx = fontSizePx) + val html = wrapReaderHtml(currentExtractedContent, extractedTitle, fontSizePx = fontSizePx, baseUrl = url) loadReaderHtml(url, html, fontSizePx) loadedKey.value = currentExtractedContent to fontSizePx } }, update = { webView -> if (loadedKey.value != currentExtractedContent to fontSizePx) { - val html = wrapReaderHtml(currentExtractedContent, extractedTitle, fontSizePx = fontSizePx) + val html = wrapReaderHtml(currentExtractedContent, extractedTitle, fontSizePx = fontSizePx, baseUrl = url) webView.loadReaderHtml(url, html, fontSizePx) loadedKey.value = currentExtractedContent to fontSizePx } diff --git a/app/src/main/java/org/terst/feedreader/ui/ReaderScreen.kt b/app/src/main/java/org/terst/feedreader/ui/ReaderScreen.kt index 0d326ea..5a90c9e 100644 --- a/app/src/main/java/org/terst/feedreader/ui/ReaderScreen.kt +++ b/app/src/main/java/org/terst/feedreader/ui/ReaderScreen.kt @@ -611,14 +611,18 @@ private fun ReaderContent( onFontSizeChanged = { newSizePx -> scope.launch { settings.setReaderFontSizePx(newSizePx) } } ) ).apply { - val html = wrapReaderHtml(content, article?.title, article?.publishedAt, article?.imageUrl, fontSizePx) + val html = wrapReaderHtml( + content, article?.title, article?.publishedAt, article?.imageUrl, fontSizePx, baseUrl + ) loadReaderHtml(baseUrl, html, fontSizePx) loadedKey.value = content to fontSizePx } }, update = { webView -> if (loadedKey.value != content to fontSizePx) { - val html = wrapReaderHtml(content, article?.title, article?.publishedAt, article?.imageUrl, fontSizePx) + val html = wrapReaderHtml( + content, article?.title, article?.publishedAt, article?.imageUrl, fontSizePx, baseUrl + ) webView.loadReaderHtml(baseUrl, html, fontSizePx) loadedKey.value = content to fontSizePx } diff --git a/app/src/main/java/org/terst/feedreader/ui/ReaderWebView.kt b/app/src/main/java/org/terst/feedreader/ui/ReaderWebView.kt index 68562e8..fd0c6af 100644 --- a/app/src/main/java/org/terst/feedreader/ui/ReaderWebView.kt +++ b/app/src/main/java/org/terst/feedreader/ui/ReaderWebView.kt @@ -508,12 +508,17 @@ private fun escapeHtml(text: String): String = text // on whatever heading Readability happened to keep in bodyContent (increasingly nothing -- // see ReaderExtractor's <header> stripping) or on the top app bar's title, which truncates // to one line and was the only place a long title showed up before this. +// Six arguments, but five of them are defaulted document-assembly inputs that no caller has to +// spell out, and the only thing they have in common is "something the page needs" -- bundling +// them into a type purely to satisfy the count would make the call sites longer, not shorter. +@Suppress("LongParameterList") fun wrapReaderHtml( bodyContent: String, title: String? = null, publishedAt: Long? = null, imageUrl: String? = null, - fontSizePx: Int = DEFAULT_READER_FONT_SIZE_PX + fontSizePx: Int = DEFAULT_READER_FONT_SIZE_PX, + baseUrl: String? = null ): String { val processedBody = replaceGenericIframes(replaceYouTubeIframes(bodyContent)) // NB: linkifyImagesForZoom runs at the very end (below, over hero + body together), after @@ -576,6 +581,6 @@ fun wrapReaderHtml( </style></head> <body>${if (!title.isNullOrBlank()) "<h1 class=\"article-title\">${escapeHtml(title)}</h1>" else ""}${ if (publishedAt != null) "<p class=\"article-date\">${escapeHtml(absoluteDateText(publishedAt))}</p>" else "" - }${linkifyImagesForZoom(heroHtml + processedBody)}</body></html> + }${linkifyImagesForZoom(heroHtml + processedBody, baseUrl)}</body></html> """.trimIndent() } diff --git a/app/src/test/java/org/terst/feedreader/ui/ImageZoomTest.kt b/app/src/test/java/org/terst/feedreader/ui/ImageZoomTest.kt index 3f7078d..aead885 100644 --- a/app/src/test/java/org/terst/feedreader/ui/ImageZoomTest.kt +++ b/app/src/test/java/org/terst/feedreader/ui/ImageZoomTest.kt @@ -38,14 +38,46 @@ class ImageZoomTest { } @Test - fun `a relative image source is left alone`() { - // Relative srcs resolve against the WebView's base URL, which a custom-scheme href - // can't reproduce -- better to leave the image un-zoomable than to build a dead link. + fun `a relative image source is left alone when nothing says where it came from`() { + // Relative srcs resolve against the WebView's base URL -- with no base URL to resolve + // against, better to leave the image un-zoomable than to build a dead link. val original = """<img src="/local/a.jpg">""" assertEquals(original, linkifyImagesForZoom(original)) } @Test + fun `a site-relative image source resolves against the page it came from`() { + // Verbatim shape of the Ftrain feed's own markup, which is where this surfaced: + // every image in <content:encoded> is site-relative, so none of them were zoomable. + val html = linkifyImagesForZoom( + """<img src="/art/uploads/header.png" alt="x">""", + baseUrl = "https://ftrain.com/aboard-let-s-have-a-mouse-party" + ) + assertEquals( + """<a href="feedreader-image:https://ftrain.com/art/uploads/header.png">""" + + """<img src="/art/uploads/header.png" alt="x"></a>""", + html + ) + } + + @Test + fun `a path-relative image source resolves against the article's own directory`() { + val html = linkifyImagesForZoom( + """<img src="pic.png">""", + baseUrl = "https://ex.com/posts/2026/hello" + ) + assertTrue(html.contains("""<a href="feedreader-image:https://ex.com/posts/2026/pic.png">""")) + } + + @Test + fun `a data uri image is still left alone when there is a page url`() { + // Resolving a data: src against a base URL just gives the data: URI back -- it is + // not something the viewer could ever open, so it must not become a zoom target. + val original = """<img src="data:image/png;base64,AAAA">""" + assertEquals(original, linkifyImagesForZoom(original, baseUrl = "https://ex.com/post")) + } + + @Test fun `a data uri image is left alone`() { val original = """<img src="data:image/png;base64,AAAA">""" assertEquals(original, linkifyImagesForZoom(original)) @@ -87,6 +119,17 @@ class ImageZoomTest { } @Test + fun `reader html makes a feed article's relative images zoomable`() { + // The non-reader path renders the feed's own HTML, which routinely keeps relative + // srcs -- the reader path never hits this because Readability absolutizes them. + val html = wrapReaderHtml( + """<p>x</p><img src="/art/a.png">""", + baseUrl = "https://ftrain.com/aboard-let-s-have-a-mouse-party" + ) + assertTrue(html.contains("""<a href="feedreader-image:https://ftrain.com/art/a.png">""")) + } + + @Test fun `reader html makes the synthesized hero image zoomable too`() { val html = wrapReaderHtml("<p>no image of its own</p>", imageUrl = "https://ex.com/hero.jpg") assertTrue(html.contains("""<a href="feedreader-image:https://ex.com/hero.jpg">""")) |
