Fix back navigation and omnibar text #5656
Conversation
a85d014 to
54106e3
Compare
There was a problem hiding this comment.
Good job here @CrisBarreiro Most of the issues are gone and I like you moved into the viewmodel some checks we were doing in the UI.
Your test cases worked well, however found 2 issues we should look into. I will post a video in the Asana task.
| @@ -835,7 +838,7 @@ class BrowserTabViewModel @Inject constructor( | |||
| setAdClickActiveTabData(url) | |||
|
|
|||
| // we expect refreshCta to be called when a site is fully loaded if browsingShowing -trackers data available-. | |||
| if (!currentBrowserViewState().browserShowing && !currentBrowserViewState().maliciousSiteDetected) { | |||
| if (!currentBrowserViewState().browserShowing && !currentBrowserViewState().maliciousSiteBlocked) { | |||
There was a problem hiding this comment.
am I right to think in the future we might check any error instead of just malicious Site blocked? If so, let's just leave a comment indicating that. Example: "currently only checking maliciousSiteBlocked, but we could expand logic to other errors"
There was a problem hiding this comment.
I don't know. I did it this way cause I didn't want the keyboard to show on this specific error, but didn't want to change the behavior for other error pages. That being said, I plan to refactor BrowserViewState in the future and have a single sealed class for status, rather than individual booleans that shouldn't all be true at the same time, but that we don't enforce in any way
| @@ -1312,7 +1317,7 @@ class BrowserTabViewModel @Inject constructor( | |||
| return true | |||
| } | |||
|
|
|||
| if (currentBrowserViewState().maliciousSiteDetected) { | |||
| if (currentBrowserViewState().maliciousSiteBlocked) { | |||
| command.postValue(HideWarningMaliciousSite) | |||
There was a problem hiding this comment.
Haven't tested yet, but If we hide waning, we could also update the privacy shield status to avoid showing the red warning until next page loads.
There was a problem hiding this comment.
Added to recoverFromWarningPage()
| @@ -1375,7 +1380,14 @@ class BrowserTabViewModel @Inject constructor( | |||
| browserViewState.value = currentState.copy(isFullScreen = false) | |||
| } | |||
|
|
|||
| override fun navigationStateChanged(newWebNavigationState: WebNavigationState) { | |||
| /* | |||
| * Calling this method when an error page is shown might cause unexpected issues | |||
There was a problem hiding this comment.
Apart from warning about something observed while implementing malicious sites, it's not fully clear to me what's the expectation here, or if this will be clear to everyone.
"Calling this method when an error page is shown might cause unexpected issues" is it about calling this when error is shown or calling this if we have blocked the request?
"Specifically, malicious site protection might prevent a page load from starting altogether, which means by stopping the WebView, we're effectively stopping the load for the previously loaded page. " This is our scenario, but does the same apply to other errors? For example, I assume SSL this is not a problem. If this is correct, being more specific about the issue could be more useful. For example does it apply to all errors or only those which stop page loading? is it same blocking vs stopping once it has started? Do we have a suggestion on what's the right way to handle this?
There was a problem hiding this comment.
Apart from warning about something observed while implementing malicious sites, it's not fully clear to me what's the expectation here, or if this will be clear to everyone.
The expectation here is to not make this method public in the API anymore, so we can't call it directly from the Fragment or WebView client
"Calling this method when an error page is shown might cause unexpected issues" is it about calling this when error is shown or calling this if we have blocked the request?
This is about calling this method when we block a request locally, before onPageStarted has been executed. One of the steps we take when blocking a request (locally or not) is to stop the WebView client, which triggers onStop and onProgressChanged callbacks for the previous page (the one that's currently loaded in the WebView, because the malicious one never went that far). This creates an inconsistent state, which is what I'm trying to prevent.
"Specifically, malicious site protection might prevent a page load from starting altogether, which means by stopping the WebView, we're effectively stopping the load for the previously loaded page. " This is our scenario, but does the same apply to other errors? For example, I assume SSL this is not a problem.
This isn't an issue for other errors, because we never block the request before leaving the device in such cases, and the same happens if we block a site after it's already been loaded
I'll revisit the comment to be more specific
| @@ -1593,7 +1606,7 @@ class BrowserTabViewModel @Inject constructor( | |||
| } | |||
|
|
|||
| private suspend fun updateLoadingStatePrivacy(domain: String) { | |||
| val privacyProtectionDisabled = isPrivacyProtectionDisabled(domain) | |||
| val privacyProtectionDisabled = isPrivacyProtectionDisabled(domain) || currentBrowserViewState().maliciousSiteBlocked | |||
There was a problem hiding this comment.
I'm trying to understand why the check is here.
In other similar methods like updatePrivacyProtectionState we don't have it.
Also privacyProtectionDisabled will be true if site is blocked, which for me that represents privacy protections enabled. can you elaborate why this check?
There was a problem hiding this comment.
val privacyProtectionDisabled, which I'm assigning here, is what's used to then assign privacyOn in LoadingViewState. privacyOn is then used to determine whether or not the trackers blocked animation needs to be shown. Therefore, if maliciousSiteBlocked (which could be renamed to maliciousSiteWarningShowing), we need to set privacyOn to false. For clarity, I've renamed privacyOn to trackersAnimationEnabled
In other similar methods like updatePrivacyProtectionState we don't have it.
updatePrivacyProtectionState is meant to update isPrivacyProtectionDisabled, which is only used to display the entry to enable/disable protections in the context menu, and we're hiding that entry
Also privacyProtectionDisabled will be true if site is blocked, which for me that represents privacy protections enabled. can you elaborate why this check?
The why is explained above, but I can update the naming
| } | ||
|
|
||
| override fun pageStarted(webViewNavigationState: WebViewNavigationState) { | ||
| navigationStateChanged(webViewNavigationState) |
There was a problem hiding this comment.
This methods seems to be the exception, as it's the only one not checking if (!currentBrowserViewState().maliciousSiteBlocked). why?
There was a problem hiding this comment.
Because this means the WebView is actually starting to load the page, as opposed to the page being loaded before it starts to load
| ) | ||
| browserViewState.postValue( | ||
| currentBrowserViewState().copy( | ||
| browserShowing = false, | ||
| showPrivacyShield = HighlightableButton.Gone, |
There was a problem hiding this comment.
You have BrowserStateModifier.copyForHomeShowing, you can rely on that for most of the state changes, add yours there, etc.
| @@ -177,8 +177,8 @@ class OmnibarLayoutViewModel @Inject constructor( | |||
| } | |||
| } else { | |||
| _viewState.update { | |||
| val shouldUpdateOmnibarText = it.viewMode is Browser | |||
| Timber.d("Omnibar: lost focus in Browser mode $shouldUpdateOmnibarText") | |||
| val shouldUpdateOmnibarText = it.viewMode is Browser || it.viewMode is MaliciousSiteWarning | |||
There was a problem hiding this comment.
NIT: this can be extracted into ViewState as
data class ViewState(
// ... properties ...
) {
fun shouldUpdateOmnibarText(): Boolean {
return this.viewMode is ViewMode.Browser || this.viewMode is ViewMode.MaliciousSiteWarning
}
}
54106e3 to
217ae07
Compare
Task/Issue URL: https://app.asana.com/0/72649045549333/1209457528786121
Description
Fixes the following issues:
Steps to test this PR
Run all test cases from new tab page
1
2
3
4
5
6
7
8
9
10
11
12
13