-
-
Notifications
You must be signed in to change notification settings - Fork 964
Refactor WebViewActivity path handling #6447
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
base: main
Are you sure you want to change the base?
Changes from 18 commits
65cbdce
065eab5
6bbaf17
46a54a3
f8b1965
ce033b4
0bcc194
eecbd93
edb1c27
6df7cde
e6f229a
4749190
f5a7621
da0d124
b764c84
9b1b064
dee94e5
7529564
369bb28
e30d005
1f2e7ed
3e47e4c
d596e23
e8baa89
672db56
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| package io.homeassistant.companion.android.util.compose.webview | ||
|
|
||
| import android.net.Uri | ||
| import android.webkit.WebView | ||
| import androidx.annotation.VisibleForTesting | ||
| import androidx.core.net.toUri | ||
| import io.homeassistant.companion.android.util.hasNonRootPath | ||
| import io.homeassistant.companion.android.util.hasSameOrigin | ||
|
|
||
| /** | ||
| * Convenience overload that extracts the previous URL from the [WebView]'s | ||
| * back/forward list and delegates to the pure [resolveBackAction] function. | ||
| */ | ||
| fun resolveBackAction(webView: WebView, loadedUrl: Uri?): BackAction { | ||
| val previousUrl = if (webView.canGoBack()) { | ||
| val backForwardList = webView.copyBackForwardList() | ||
| backForwardList.currentIndex | ||
| .takeIf { it > 0 } | ||
| ?.let { backForwardList.getItemAtIndex(it - 1).url } | ||
| ?.toUri() | ||
| } else { | ||
| null | ||
| } | ||
| return resolveBackAction(previousUrl, loadedUrl) | ||
| } | ||
|
|
||
| /** | ||
| * Determines the appropriate back action based on the previous and current URL. | ||
| * | ||
| * The resolution logic: | ||
| * 1. If [previousUrl] is a same-origin HTTP entry relative to [loadedUrl], | ||
| * returns [BackAction.GoBack] so the user can navigate back normally. | ||
| * 2. If there is no valid previous URL and the current URL has a non-root path, | ||
| * returns [BackAction.NavigateToRoot] so the user is taken to the home page first. | ||
| * 3. Otherwise returns [BackAction.None] — the caller decides what to do | ||
| * (e.g. exit the activity or pop the navigation stack). | ||
| * | ||
| * @param previousUrl the URL of the previous entry in the WebView's back stack, | ||
| * or `null` if the history is empty or the WebView cannot go back | ||
| * @param loadedUrl the current URL shown in the WebView (as tracked by the caller, | ||
| * not necessarily [WebView.getUrl] which can be `about:blank` during loads) | ||
| * @return the [BackAction] that the caller should execute | ||
| */ | ||
| @VisibleForTesting | ||
| internal fun resolveBackAction(previousUrl: Uri?, loadedUrl: Uri?): BackAction { | ||
|
Gifford47 marked this conversation as resolved.
Outdated
|
||
| if (previousUrl != null && | ||
| loadedUrl != null && | ||
| previousUrl.scheme?.startsWith("http") == true && | ||
| previousUrl.hasSameOrigin(loadedUrl) | ||
| ) { | ||
| return BackAction.GoBack | ||
| } | ||
|
|
||
| if (loadedUrl != null && loadedUrl.hasNonRootPath()) { | ||
| val rootUrl = loadedUrl.buildUpon() | ||
| .path("/") | ||
| .clearQuery() | ||
| .appendQueryParameter("external_auth", "1") | ||
| .fragment(null) | ||
| .build() | ||
| return BackAction.NavigateToRoot(rootUrl) | ||
| } | ||
|
|
||
| return BackAction.None | ||
| } | ||
|
|
||
| /** | ||
| * Represents the action to take when the user presses back in a WebView. | ||
| */ | ||
| sealed interface BackAction { | ||
| /** Navigate back in the WebView history. */ | ||
| data object GoBack : BackAction | ||
|
|
||
| /** Clear history and navigate to the root URL of the current server. */ | ||
| data class NavigateToRoot(val rootUrl: Uri) : BackAction | ||
|
|
||
| /** No more back navigation possible — the caller decides what to do. */ | ||
| data object None : BackAction | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -137,11 +137,14 @@ import io.homeassistant.companion.android.util.LifecycleHandler | |
| import io.homeassistant.companion.android.util.OnSwipeListener | ||
| import io.homeassistant.companion.android.util.TLSWebViewClient | ||
| import io.homeassistant.companion.android.util.applyInsets | ||
| import io.homeassistant.companion.android.util.compose.webview.BackAction | ||
| import io.homeassistant.companion.android.util.compose.webview.BLANK_URL | ||
| import io.homeassistant.companion.android.util.compose.webview.resolveBackAction | ||
| import io.homeassistant.companion.android.util.hasNonRootPath | ||
| import io.homeassistant.companion.android.util.hasSameOrigin | ||
| import io.homeassistant.companion.android.util.isStarted | ||
| import io.homeassistant.companion.android.util.sensitive | ||
| import io.homeassistant.companion.android.util.toRelativeUrl | ||
| import io.homeassistant.companion.android.websocket.WebsocketManager | ||
| import io.homeassistant.companion.android.webview.WebView.ErrorType | ||
| import io.homeassistant.companion.android.webview.addto.EntityAddToHandler | ||
|
|
@@ -455,7 +458,25 @@ class WebViewActivity : | |
|
|
||
| val onBackPressed = object : OnBackPressedCallback(webView.canGoBack()) { | ||
| override fun handleOnBackPressed() { | ||
| if (webView.canGoBack()) webView.goBack() | ||
| when (val action = resolveBackAction(webView, loadedUrl)) { | ||
| BackAction.GoBack -> webView.goBack() | ||
| is BackAction.NavigateToRoot -> { | ||
| clearHistory = true | ||
| loadedUrl = action.rootUrl | ||
| webView.loadUrl(action.rootUrl.toString()) | ||
|
Comment on lines
+465
to
+467
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason to not call
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. presenter.load() does a full reconnect cycle (URL resolution, auth check, security level dialog). For NavigateToRoot we stay on the same server and only change the path to / -going through the presenter would add overhead and could trigger side effects like the security level prompt.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually I think we want this side effect, the overhead is very small here and safer. @jpelgrom wdyt?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I looked into replacing the manual loadUrl call with presenter.load(lifecycle), but that wouldn't give us the behavior we need: collectUrlStateChanges only sets keepHistory = false when baseUrlChanged || isNewServer. For a NavigateToRoot action the base URL stays the same -so presenter.load() would pass keepHistory = true and leave the stale entries in the back stack. That's exactly what we want to avoid here (the user pressed back to get out of a sub-page). Going through the presenter would require adding a new loadRoot() method (or a forceClearHistory parameter), which felt like a bigger API change than this PR should do. the current code sets clearHistory/loadedUrl and calls webView.loadUrl directly, which mirrors what WebViewPresenterImpl.loadUrl does internally for this edge case. |
||
| } | ||
| BackAction.None -> { | ||
| // Already on root — let the system handle back (exit app). | ||
| // We must temporarily disable this callback so that the | ||
| // dispatcher invokes the next handler in the chain (the | ||
| // default Activity handler which finishes the activity). | ||
| // Re-enabling afterwards keeps the callback functional in | ||
| // case the activity is not destroyed (e.g. multi-window). | ||
| isEnabled = false | ||
| onBackPressedDispatcher.onBackPressed() | ||
| isEnabled = true | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -646,7 +667,12 @@ class WebViewActivity : | |
|
|
||
| override fun doUpdateVisitedHistory(view: WebView?, url: String?, isReload: Boolean) { | ||
| super.doUpdateVisitedHistory(view, url, isReload) | ||
| onBackPressed.isEnabled = canGoBack() | ||
| // Enable the callback when there's browser history OR when the | ||
| // current URL has a non-root path, so pressing back navigates to | ||
| // root before exiting. This keeps predictive back animations working | ||
| // correctly on Android 14+. | ||
| onBackPressed.isEnabled = canGoBack() || | ||
| url?.toUri()?.hasNonRootPath() == true | ||
|
Gifford47 marked this conversation as resolved.
Outdated
|
||
| presenter.stopScanningForImprov(false) | ||
| } | ||
| } | ||
|
|
@@ -1559,17 +1585,21 @@ class WebViewActivity : | |
| showSystemUI() | ||
| } | ||
|
|
||
| var path = intent.getStringExtra(EXTRA_PATH) | ||
| if (path?.startsWith("entityId:") == true) { | ||
| val intentPath = intent.getStringExtra(EXTRA_PATH) | ||
| // Let the presenter handle falling back to the current WebView path | ||
| // when no explicit navigation path is set. See https://github.com/home-assistant/android/issues/4983 | ||
| var path: String? = intentPath | ||
| if (intentPath?.startsWith("entityId:") == true) { | ||
| // Get the entity ID from a string formatted "entityId:domain.entity" | ||
| // https://github.com/home-assistant/core/blob/dev/homeassistant/core.py#L159 | ||
| val pattern = "(?<=^entityId:)((?!.+__)(?!_)[\\da-z_]+(?<!_)\\.(?!_)[\\da-z_]+(?<!_)$)".toRegex() | ||
| val entity = pattern.find(path)?.value ?: "" | ||
| val entity = pattern.find(intentPath)?.value ?: "" | ||
| if ( | ||
| entity.isNotBlank() && | ||
| serverManager.getServer(presenter.getActiveServer())?.version?.isAtLeast(2025, 6, 0) == true | ||
| ) { | ||
| path = "/?more-info-entity-id=$entity" | ||
| moreInfoEntity = "" | ||
|
Gifford47 marked this conversation as resolved.
Outdated
|
||
| } else { | ||
| moreInfoEntity = entity | ||
| } | ||
|
|
@@ -1580,6 +1610,10 @@ class WebViewActivity : | |
| } | ||
| } | ||
|
|
||
| override fun getCurrentWebViewRelativeUrl(): String? { | ||
| return webView.url?.toUri()?.toRelativeUrl(excludeParams = setOf("external_auth")) | ||
|
TimoPtr marked this conversation as resolved.
|
||
| } | ||
|
Gifford47 marked this conversation as resolved.
|
||
|
|
||
| override suspend fun unlockAppIfNeeded() { | ||
| appLocked.value = presenter.isAppLocked() | ||
| if (appLocked.value) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| package io.homeassistant.companion.android.util.compose.webview | ||
|
|
||
| import android.net.Uri | ||
| import org.junit.Assert.assertEquals | ||
| import org.junit.Assert.assertTrue | ||
| import org.junit.Test | ||
| import org.junit.runner.RunWith | ||
| import org.robolectric.RobolectricTestRunner | ||
|
|
||
| @RunWith(RobolectricTestRunner::class) | ||
| class WebViewBackNavigationTest { | ||
|
Gifford47 marked this conversation as resolved.
Outdated
|
||
|
|
||
| @Test | ||
| fun `given no previous url and root loaded url, when resolving back action, then returns None`() { | ||
|
Gifford47 marked this conversation as resolved.
Outdated
|
||
| val loadedUrl = Uri.parse("https://ha.local:8123/?external_auth=1") | ||
|
|
||
| val action = resolveBackAction(previousUrl = null, loadedUrl = loadedUrl) | ||
|
|
||
| assertEquals(BackAction.None, action) | ||
| } | ||
|
|
||
| @Test | ||
| fun `given no previous url and null loaded url, when resolving back action, then returns None`() { | ||
| val action = resolveBackAction(previousUrl = null, loadedUrl = null) | ||
|
|
||
| assertEquals(BackAction.None, action) | ||
| } | ||
|
|
||
| @Test | ||
| fun `given no previous url and sub-path loaded url, when resolving back action, then returns NavigateToRoot`() { | ||
| val loadedUrl = Uri.parse("https://ha.local:8123/history?external_auth=1") | ||
|
|
||
| val action = resolveBackAction(previousUrl = null, loadedUrl = loadedUrl) | ||
|
|
||
| assertTrue(action is BackAction.NavigateToRoot) | ||
| val rootUrl = (action as BackAction.NavigateToRoot).rootUrl | ||
| assertEquals("/", rootUrl.path) | ||
| assertEquals("1", rootUrl.getQueryParameter("external_auth")) | ||
| assertEquals("ha.local", rootUrl.host) | ||
| } | ||
|
|
||
| @Test | ||
| fun `given sub-path loaded url with extra params, when resolving back action, then NavigateToRoot strips query and fragment`() { | ||
| val loadedUrl = Uri.parse("https://ha.local:8123/history?start_date=2026-01-01&external_auth=1#tab") | ||
|
|
||
| val action = resolveBackAction(previousUrl = null, loadedUrl = loadedUrl) | ||
|
|
||
| assertTrue(action is BackAction.NavigateToRoot) | ||
| val rootUrl = (action as BackAction.NavigateToRoot).rootUrl | ||
| assertEquals("/", rootUrl.path) | ||
| assertEquals("1", rootUrl.getQueryParameter("external_auth")) | ||
| assertEquals(null, rootUrl.getQueryParameter("start_date")) | ||
| assertEquals(null, rootUrl.fragment) | ||
| } | ||
|
|
||
| @Test | ||
| fun `given same-origin previous url, when resolving back action, then returns GoBack`() { | ||
| val previousUrl = Uri.parse("https://ha.local:8123/lovelace/0") | ||
| val loadedUrl = Uri.parse("https://ha.local:8123/history?external_auth=1") | ||
|
|
||
| val action = resolveBackAction(previousUrl = previousUrl, loadedUrl = loadedUrl) | ||
|
|
||
| assertEquals(BackAction.GoBack, action) | ||
| } | ||
|
|
||
| @Test | ||
| fun `given cross-origin previous url and sub-path loaded url, when resolving back action, then returns NavigateToRoot`() { | ||
| val previousUrl = Uri.parse("https://other.server:8123/lovelace/0") | ||
| val loadedUrl = Uri.parse("https://ha.local:8123/history?external_auth=1") | ||
|
|
||
| val action = resolveBackAction(previousUrl = previousUrl, loadedUrl = loadedUrl) | ||
|
|
||
| assertTrue(action is BackAction.NavigateToRoot) | ||
| } | ||
|
|
||
| @Test | ||
| fun `given non-http previous url, when resolving back action, then does not go back`() { | ||
|
Gifford47 marked this conversation as resolved.
Outdated
|
||
| val previousUrl = Uri.parse("about:blank") | ||
| val loadedUrl = Uri.parse("https://ha.local:8123/history?external_auth=1") | ||
|
|
||
| val action = resolveBackAction(previousUrl = previousUrl, loadedUrl = loadedUrl) | ||
|
|
||
| assertTrue(action is BackAction.NavigateToRoot) | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.