From 8e0b3010027af35265dba1ef4ca18b6f6a0c4295 Mon Sep 17 00:00:00 2001 From: ekager Date: Tue, 25 Aug 2020 17:29:51 -0400 Subject: [PATCH] For #7134 - Adds login selection to prompt feature --- .../autofill/GeckoLoginDelegateWrapper.kt | 2 +- .../feature/prompts/PromptFeature.kt | 48 +++---- .../feature/prompts/login/LoginPicker.kt | 28 ++-- .../res/layout/login_selection_list_item.xml | 17 ++- .../mozac_feature_login_multiselect_view.xml | 130 ++++++++++-------- .../feature/prompts/PromptFeatureTest.kt | 28 ++-- 6 files changed, 127 insertions(+), 126 deletions(-) diff --git a/components/browser/engine-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/autofill/GeckoLoginDelegateWrapper.kt b/components/browser/engine-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/autofill/GeckoLoginDelegateWrapper.kt index bd929ee61f6..ae946e49549 100644 --- a/components/browser/engine-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/autofill/GeckoLoginDelegateWrapper.kt +++ b/components/browser/engine-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/autofill/GeckoLoginDelegateWrapper.kt @@ -9,8 +9,8 @@ import kotlinx.coroutines.GlobalScope import kotlinx.coroutines.launch import mozilla.components.concept.storage.Login import mozilla.components.concept.storage.LoginStorageDelegate -import org.mozilla.geckoview.GeckoResult import org.mozilla.geckoview.Autocomplete +import org.mozilla.geckoview.GeckoResult /** * This class exists only to convert incoming [LoginEntry] arguments into [Login]s, then forward diff --git a/components/feature/prompts/src/main/java/mozilla/components/feature/prompts/PromptFeature.kt b/components/feature/prompts/src/main/java/mozilla/components/feature/prompts/PromptFeature.kt index b3b55b303ff..dbb4126ee9f 100644 --- a/components/feature/prompts/src/main/java/mozilla/components/feature/prompts/PromptFeature.kt +++ b/components/feature/prompts/src/main/java/mozilla/components/feature/prompts/PromptFeature.kt @@ -13,9 +13,7 @@ import androidx.fragment.app.FragmentManager import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.cancel import kotlinx.coroutines.flow.collect -import kotlinx.coroutines.flow.filter import kotlinx.coroutines.flow.map -import kotlinx.coroutines.flow.mapNotNull import mozilla.components.browser.state.action.ContentAction import mozilla.components.browser.state.selector.findTabOrCustomTab import mozilla.components.browser.state.selector.findTabOrCustomTabOrSelectedTab @@ -69,7 +67,6 @@ import mozilla.components.support.base.feature.OnNeedToRequestPermissions import mozilla.components.support.base.feature.PermissionsFeature import mozilla.components.support.base.log.logger.Logger import mozilla.components.support.ktx.kotlinx.coroutines.flow.ifAnyChanged -import mozilla.components.support.ktx.kotlinx.coroutines.flow.ifChanged import java.lang.ref.WeakReference import java.security.InvalidParameterException import java.util.Date @@ -77,8 +74,6 @@ import java.util.Date @VisibleForTesting(otherwise = PRIVATE) internal const val FRAGMENT_TAG = "mozac_feature_prompt_dialog" -private const val PROGRESS_ALMOST_COMPLETE = 90 - /** * Feature for displaying native dialogs for html elements like: input type * date, file, time, color, option, menu, authentication, confirmation and alerts. @@ -132,7 +127,6 @@ class PromptFeature private constructor( // These three scopes have identical lifetimes. We do not yet have a way of combining scopes private var handlePromptScope: CoroutineScope? = null private var dismissPromptScope: CoroutineScope? = null - private var sessionPromptScope: CoroutineScope? = null private var activePromptRequest: PromptRequest? = null internal val promptAbuserDetector = PromptAbuserDetector() @@ -243,6 +237,9 @@ class PromptFeature private constructor( .collect { state -> state?.content?.let { if (it.promptRequest != activePromptRequest) { + if (activePromptRequest is SelectLoginPrompt) { + loginPicker?.dismissCurrentLoginSelect(activePromptRequest as SelectLoginPrompt) + } onPromptRequested(state) } else if (!it.loading) { promptAbuserDetector.resetJSAlertAbuseState() @@ -252,33 +249,23 @@ class PromptFeature private constructor( } } - // Dismiss all prompts when page loads are nearly finished. This prevents prompts from the - // previous page from covering content. See Fenix#5326 + // Dismiss all prompts when page URL or session id changes. See Fenix#5326 dismissPromptScope = store.flowScoped { flow -> - flow.mapNotNull { state -> state.findTabOrCustomTabOrSelectedTab(customTabId) } - .ifChanged { it.content.progress } - .filter { it.content.progress >= PROGRESS_ALMOST_COMPLETE } - .collect { - val prompt = activePrompt?.get() - if (prompt?.shouldDismissOnLoad() == true) { - prompt.dismiss() - } - activePrompt?.clear() - loginPicker?.dismissCurrentLoginSelect() + flow.ifAnyChanged { state -> + arrayOf( + state.selectedTabId, + state.findTabOrCustomTabOrSelectedTab(customTabId)?.content?.url + ) + }.collect { + if (activePromptRequest is SelectLoginPrompt) { + loginPicker?.dismissCurrentLoginSelect(activePromptRequest as SelectLoginPrompt) } - } - - // Dismiss prompts when a new tab is selected. - sessionPromptScope = store.flowScoped { flow -> - flow.ifChanged { browserState -> browserState.selectedTabId } - .collect { - val prompt = activePrompt?.get() - if (prompt?.shouldDismissOnLoad() == true) { - prompt.dismiss() - } - activePrompt?.clear() - loginPicker?.dismissCurrentLoginSelect() + val prompt = activePrompt?.get() + if (prompt?.shouldDismissOnLoad() == true) { + prompt.dismiss() } + activePrompt?.clear() + } } fragmentManager.findFragmentByTag(FRAGMENT_TAG)?.let { fragment -> @@ -295,7 +282,6 @@ class PromptFeature private constructor( override fun stop() { handlePromptScope?.cancel() dismissPromptScope?.cancel() - sessionPromptScope?.cancel() } /** diff --git a/components/feature/prompts/src/main/java/mozilla/components/feature/prompts/login/LoginPicker.kt b/components/feature/prompts/src/main/java/mozilla/components/feature/prompts/login/LoginPicker.kt index 2e3773754e0..11604e854e2 100644 --- a/components/feature/prompts/src/main/java/mozilla/components/feature/prompts/login/LoginPicker.kt +++ b/components/feature/prompts/src/main/java/mozilla/components/feature/prompts/login/LoginPicker.kt @@ -4,11 +4,11 @@ package mozilla.components.feature.prompts.login -import androidx.annotation.VisibleForTesting import mozilla.components.browser.state.store.BrowserStore import mozilla.components.concept.engine.prompt.PromptRequest import mozilla.components.concept.storage.Login import mozilla.components.feature.prompts.consumePromptFrom +import mozilla.components.support.base.log.logger.Logger /** * The [LoginPicker] displays a list of possible logins in a [LoginPickerView] for a site after @@ -34,21 +34,13 @@ internal class LoginPicker( } internal fun handleSelectLoginRequest(request: PromptRequest.SelectLoginPrompt) { - if (currentRequest != null) { - store.consumePromptFrom(sessionId) { - (it as PromptRequest.SelectLoginPrompt).onDismiss() - } - } - currentRequest = request loginSelectBar.showPicker(request.logins) } - @VisibleForTesting - internal var currentRequest: PromptRequest.SelectLoginPrompt? = null - override fun onLoginSelected(login: Login) { - (currentRequest as PromptRequest.SelectLoginPrompt).onConfirm(login) - currentRequest = null + store.consumePromptFrom(sessionId) { + if (it is PromptRequest.SelectLoginPrompt) it.onConfirm(login) + } loginSelectBar.hidePicker() } @@ -57,13 +49,15 @@ internal class LoginPicker( dismissCurrentLoginSelect() } - fun dismissCurrentLoginSelect() { - if (currentRequest != null) { - store.consumePromptFrom(sessionId) { - (it as PromptRequest.SelectLoginPrompt).onDismiss() + @Suppress("TooGenericExceptionCaught") + fun dismissCurrentLoginSelect(promptRequest: PromptRequest.SelectLoginPrompt? = null) { + try { + promptRequest?.let { it.onDismiss() } ?: store.consumePromptFrom(sessionId) { + if (it is PromptRequest.SelectLoginPrompt) it.onDismiss() } + } catch (e: RuntimeException) { + Logger.error("Can't dismiss this login select prompt", e) } - currentRequest = null loginSelectBar.hidePicker() } } diff --git a/components/feature/prompts/src/main/res/layout/login_selection_list_item.xml b/components/feature/prompts/src/main/res/layout/login_selection_list_item.xml index afc845ad94a..e754c7b50b2 100644 --- a/components/feature/prompts/src/main/res/layout/login_selection_list_item.xml +++ b/components/feature/prompts/src/main/res/layout/login_selection_list_item.xml @@ -5,9 +5,12 @@ - \ No newline at end of file + tools:ignore="TextViewEdits" + tools:text="password" /> + diff --git a/components/feature/prompts/src/main/res/layout/mozac_feature_login_multiselect_view.xml b/components/feature/prompts/src/main/res/layout/mozac_feature_login_multiselect_view.xml index 8d67347e83e..34d99f70409 100644 --- a/components/feature/prompts/src/main/res/layout/mozac_feature_login_multiselect_view.xml +++ b/components/feature/prompts/src/main/res/layout/mozac_feature_login_multiselect_view.xml @@ -9,66 +9,78 @@ android:layout_height="wrap_content" tools:parentTag="androidx.constraintlayout.widget.ConstraintLayout"> - + - + - + + + + + - + + + diff --git a/components/feature/prompts/src/test/java/mozilla/components/feature/prompts/PromptFeatureTest.kt b/components/feature/prompts/src/test/java/mozilla/components/feature/prompts/PromptFeatureTest.kt index 16e9ee48886..5f15011b516 100644 --- a/components/feature/prompts/src/test/java/mozilla/components/feature/prompts/PromptFeatureTest.kt +++ b/components/feature/prompts/src/test/java/mozilla/components/feature/prompts/PromptFeatureTest.kt @@ -23,6 +23,7 @@ import kotlinx.coroutines.test.setMain import mozilla.components.browser.state.action.ContentAction import mozilla.components.browser.state.action.TabListAction import mozilla.components.browser.state.state.BrowserState +import mozilla.components.browser.state.state.ContentState import mozilla.components.browser.state.state.TabSessionState import mozilla.components.browser.state.state.createCustomTab import mozilla.components.browser.state.state.createTab @@ -1025,7 +1026,7 @@ class PromptFeatureTest { } @Test - fun `dialog will be dismissed if progress reaches 90%`() { + fun `dialog will be dismissed if tab ID changes`() { val feature = spy( PromptFeature( activity = mock(), @@ -1047,15 +1048,15 @@ class PromptFeatureTest { whenever(fragment.shouldDismissOnLoad()).thenReturn(true) feature.activePrompt = WeakReference(fragment) - store.dispatch(ContentAction.UpdateProgressAction(tabId, 0)).joinBlocking() - store.dispatch(ContentAction.UpdateProgressAction(tabId, 10)).joinBlocking() - store.dispatch(ContentAction.UpdateProgressAction(tabId, 11)).joinBlocking() - store.dispatch(ContentAction.UpdateProgressAction(tabId, 28)).joinBlocking() - store.dispatch(ContentAction.UpdateProgressAction(tabId, 32)).joinBlocking() - store.dispatch(ContentAction.UpdateProgressAction(tabId, 49)).joinBlocking() - store.dispatch(ContentAction.UpdateProgressAction(tabId, 60)).joinBlocking() - store.dispatch(ContentAction.UpdateProgressAction(tabId, 89)).joinBlocking() - store.dispatch(ContentAction.UpdateProgressAction(tabId, 90)).joinBlocking() + val secondTabId = "second-test-tab" + store.dispatch( + TabListAction.AddTabAction( + TabSessionState( + id = secondTabId, + content = ContentState(url = "mozilla.org") + ), select = true + ) + ).joinBlocking() verify(fragment, times(1)).dismiss() } @@ -1091,7 +1092,7 @@ class PromptFeatureTest { } @Test - fun `dialog will be dismissed if new page load progress skips past 90%`() { + fun `dialog will be dismissed if tab URL changes`() { val feature = spy( PromptFeature( activity = mock(), @@ -1113,10 +1114,7 @@ class PromptFeatureTest { whenever(fragment.shouldDismissOnLoad()).thenReturn(true) feature.activePrompt = WeakReference(fragment) - store.dispatch(ContentAction.UpdateProgressAction(tabId, 0)).joinBlocking() - store.dispatch(ContentAction.UpdateProgressAction(tabId, 10)).joinBlocking() - store.dispatch(ContentAction.UpdateProgressAction(tabId, 100)).joinBlocking() - + store.dispatch(ContentAction.UpdateUrlAction(tabId, "mozilla.org")).joinBlocking() verify(fragment, times(1)).dismiss() }