Skip to content
This repository has been archived by the owner on Dec 14, 2021. It is now read-only.

798: routing back to itemlist on login/search/feedback forms #890

Merged
merged 5 commits into from
Sep 9, 2019

Conversation

eliserichards
Copy link
Contributor

@eliserichards eliserichards commented Aug 28, 2019

Fixes #798

There are a number of things that are being cleaned up as part of this PR. The original issue was tracking the keyboard being minimized when clicking in text fields, and in some cases, being routed back to the item list.

Below you will find my notes and analysis which lead me to find the source of the original issue: the AutofillService was being triggered when a potentially fillable text field was encountered (e.g. the search text field or editable username/hostname/password fields). The mitigation for this was to mark those fields as not autofillable.

In addition to that, this PR fixes the following:

  • dispatch the AutofillStart action at the end of onConnect() instead of the beginning
  • add missing routes for locking that caused crashes
  • use TextInputEditText in place of EditText

Testing and Review Notes

This is easily reproducible on Pixel 2 with search and edit.

Search

Probably related to locking or routing... we're seeing the DataStore.state being updated, which causes the Unlocked workflow to send the user to the ItemList:

private fun dataStoreToRouteActions(storageState: DataStore.State): Optional<RouteAction> {
        return when (storageState) {
            is DataStore.State.Unlocked -> RouteAction.ItemList
            is DataStore.State.Locked -> RouteAction.LockScreen
            is DataStore.State.Unprepared -> RouteAction.Welcome
            else -> null
        }.asOptional()
    }

The state could be set to Unlocked in three places in the DataStore:

  • updateCredentials()
  • unlock() - coming from FingerprintAuthAction in the LockedPresenter
  • handleLock()

Todo:

  • add route from fragment_filter -> fragment_locked
  • add route from fragment_filter -> fragment_item_list (does this solve the issue?)

Edit

Caused by: java.lang.IllegalStateException: Cannot route from mozilla.lockbox:id/fragment_item_edit to mozilla.lockbox:id/fragment_locked. This is a developer bug, fixable by adding an action to mozilla.lockbox:id/graph_main.xml and/or AppRoutePresenter
        at mozilla.lockbox.presenter.RoutePresenter.navigateToFragment(RoutePresenter.kt:145)
        at mozilla.lockbox.presenter.RoutePresenter.navigateToFragment$default(RoutePresenter.kt:125)
        at mozilla.lockbox.presenter.AppRoutePresenter.route(AppRoutePresenter.kt:90)
        at mozilla.lockbox.presenter.AppRoutePresenter$onResume$1.invoke(AppRoutePresenter.kt:52)
        at mozilla.lockbox.presenter.AppRoutePresenter$onResume$1.invoke(AppRoutePresenter.kt:32)

Todo:

  • add route from fragment_item_edit -> fragment_locked

image

@eliserichards eliserichards changed the title [WIP] keyboard minimizing on login/search/feedback forms [WIP] 798: keyboard minimizing on login/search/feedback forms Aug 28, 2019
@eliserichards
Copy link
Contributor Author

eliserichards commented Sep 5, 2019

Updated investigation:

  • when a user clicks on the search icon and the fragment_filter is displayed, the autofill workflow is being triggered from LockboxAutofillService (onConnected())
  • this dispatches a LifecycleAction.AutofillStart action, causing us to evaluate locking in the DataStore (where this.foreground contains the AutofillStart action):
lifecycleStore.lifecycleEvents
            .filter { this.foreground.contains(it) && stateSubject.value != State.Unprepared }
            .subscribe { this.handleLock() }
            .addTo(compositeDisposable)
  • because of this, the app is routing the user back to the item list via dataStoreToRouteActions() (see notes above under "Search")

Questions:

  • should autofill be connecting when we search? why is it firing then and not on app startup?
  • why doesn't the logic recognize that the app is already foregrounded AND unlocked? we shouldn't have to unlock again.

@eliserichards eliserichards changed the title [WIP] 798: keyboard minimizing on login/search/feedback forms [WIP] 798: routing back to itemlist on login/search/feedback forms Sep 5, 2019
@eliserichards eliserichards marked this pull request as ready for review September 6, 2019 17:30
@eliserichards eliserichards requested a review from a team as a code owner September 6, 2019 17:30
@eliserichards eliserichards changed the title [WIP] 798: routing back to itemlist on login/search/feedback forms 798: routing back to itemlist on login/search/feedback forms Sep 6, 2019
Copy link
Contributor

@jhugman jhugman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Please land.

telemetryStore.injectContext(this)
accountStore.injectContext(this)
fxaSupport.injectContext(this)
dataStore = DataStore.shared
dispatcher.dispatch(LifecycleAction.AutofillStart)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rawk

@@ -103,7 +103,7 @@ sealed class DialogAction(
R.color.red
),
listOf(
ItemDetail(itemId)
ItemList
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this what is required? Will check the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to follow up with UX on whether we're routing to ItemList, ItemList with a toast notification ("You updated XXXX credential!"), or the item detail. Will confirm.

@eliserichards eliserichards merged commit fea2a77 into master Sep 9, 2019
eliserichards pushed a commit that referenced this pull request Sep 11, 2019
* Add release notes for 1.1.2.4405

* Bump version to 1.2.0

* Add ignore to the flaky test. This needs to be fixed later. (#793)

* 626, 627: Add kebab menu for delete  (#764)

* Add kebab menu icon and strings. Set up fragment for addition to toolbar.

* Create fragment and dialog.

* Add confirmation dialog, actions, and presenters

* Routing

* Ensure item is actually deleted and item list is refreshed before navigating back.

* Refactor and remove edit for now

* Keeping edit button, just not doing any functionality with it

* Test for item detail presenter

* Ellipsize long item titles in detail view

* Delete confirmation toast

* Delete toast and tests

* Consumable toast notification for delete

* Specify return type on Optional extension

* ItemListPresenterTest needed a consumable for the delete toast test

* Cleanup

* Fix toast notifications

* Import strings from android-l10n (#809)

State: mozilla-l10n/android-l10n@63a5240

* 806-uitest-add-delete (#808)

* Adjust spinner dropdown size to fit longer words (l10n) (#812)

* Ellipsize text in item list (#811)

* 810: Reset support when syncing (#826)

* Reset support when info is null

* Reset support every time we sync.

* Removing timeout to be fixed in #791 (#833)

* Removing timeout to be fixed in #791

* Remove strings

* 828-l10nScreenshotsTest-delete (#829)

* Add dot to sentence (#837)

* 814: Telemetry for sync (#835)

* Add telemetry actions for sync, list update, and error states.

* Sync timeout instead of sync end

* Add sync complete event

* Add errors and ids to telemetry calls

* Update metrics docs to reflect telemetry sync changes (#841)

* Revert delete PRs (#836)

* Revert "828-l10nScreenshotsTest-delete (#829)"

This reverts commit bab9fe8.

* Revert "806-uitest-add-delete (#808)"

This reverts commit ec56009.

* Revert "626, 627: Add kebab menu for delete  (#764)"

This reverts commit 17605c3.

* Remove unused imports and fix lint error.

* Rebase and lint

* Update to version 1.1.3 (#844)

* Update version to 1.1.3

* Add release notes for 1.1.3

* Update releases.md (#847)

* Add more context to release notes

* Add descriptions for proguard rules upload.

* Revert "Revert delete PRs (#836)" (#842)

This reverts commit 974a585.

* 816: string updates for localization (#852)

* Revert "Revert delete PRs (#836)"

This reverts commit 974a585.

* Lockwise placeholder in delete_description and descriptive comment in edit string

* Update app services, android components, and megazord configuration (#865)

* Update app services, android components, and megazord configuration

* Update a-c to v7.0.0

* Send hashed UID ping when sync completes successfully.

* Remove handling for synctelemetryping, to be completed in separate issue. Update AS to 0.37.1

* Update Readme with l10n process (#872)

This is something we've been adding to all Readmes of mobile repos to direct people to Pontoon for localization (vs GitHub repos, where work isn't supposed to happen).

* 867 - Fix obvious NPE sync crash (#884)

* 871: Create infrastructure for feature flags (#882)

* Delete feature flag

* Move feature flags to separate support file

* Create support to check build configs. Create flags for CUD

* Add telemetry probes to syncIfRequired trigger syncs (#887)

* Import strings from l10n (#893)

* Import strings from android-l10n

State: mozilla-l10n/android-l10n@349efdb

* Commenting unused string for Espanol

* 791: Remove fake timeout from sync (#889)

* Remove fake timeout from sync

* Remove all sync timeout strings

* Rebase and lint

* Fail fast if the edge between to routes is not in the nav graph. (#895)

* Auto-signin with mozilla AuthProvider (#879) — behind a feature flag.

* WIP Automatic FxA sign-in support

* Cleanup welcome fragment/account detection

* Upgrade to Android Components 9.0.0

* Ensure landable, even if not releasable

* gradlew lint

* 628: edit view and routing (#843)

* Edit xml layout

* Add route, presenter, and fragment

Button clicks and dialog

Update dependencies

Edit presenter tests

Routing stuff

Usable state with spinner

Buttons working

Password visibility in edit mode

Lint

View

Create popup item and insert menu into detail view's kebab buttton

* Popup menu and click listener

Dropdown menu and formatting

Update list - in progress

DataStoreTest - mocked up test, stuck on init

Datastore update item detail test

* Save edited changes

Lint

Update dependencies

Edit unit tests

Add sync to datastore editing. Update var name in robot and screenshot tests

Reformatting dropdown menu and edit view

Adding inclusive popup to edit->detail view nav definition

Address nullable server passwords for delete and edit. Remove unused string.

Remove learn more clicks from test

Refactoring pushError into a helper method.

* Add fxalogin to autofill onboarding route. (#901)

* 798: routing back to itemlist on login/search/feedback forms (#890)

* Investigation

* Add locking routes.

* Explicitly ignore autofill on search and edit fields

* Ignore autofill on webviews

* Remove comments and clean up lint

* Add null text value for empty usernames (#902)

* Add null text value for empty usernames

* gradlew lint

* 907 fix ui tests (#908)

* 907: fix testSettings

* fixing routing issues

* fixing more routes

* changing button name and adding route

* fixing conflicts after rebase

* Update release-notes.md with v1.1.4

* Fix lint errors

* Update release-notes.md to correct bitrise version
eliserichards pushed a commit that referenced this pull request Sep 12, 2019
* Add release notes for 1.1.2.4405

* Bump version to 1.2.0

* Add ignore to the flaky test. This needs to be fixed later. (#793)

* 626, 627: Add kebab menu for delete  (#764)

* Add kebab menu icon and strings. Set up fragment for addition to toolbar.

* Create fragment and dialog.

* Add confirmation dialog, actions, and presenters

* Routing

* Ensure item is actually deleted and item list is refreshed before navigating back.

* Refactor and remove edit for now

* Keeping edit button, just not doing any functionality with it

* Test for item detail presenter

* Ellipsize long item titles in detail view

* Delete confirmation toast

* Delete toast and tests

* Consumable toast notification for delete

* Specify return type on Optional extension

* ItemListPresenterTest needed a consumable for the delete toast test

* Cleanup

* Fix toast notifications

* Import strings from android-l10n (#809)

State: mozilla-l10n/android-l10n@63a5240

* 806-uitest-add-delete (#808)

* Adjust spinner dropdown size to fit longer words (l10n) (#812)

* Ellipsize text in item list (#811)

* 810: Reset support when syncing (#826)

* Reset support when info is null

* Reset support every time we sync.

* Removing timeout to be fixed in #791 (#833)

* Removing timeout to be fixed in #791

* Remove strings

* 828-l10nScreenshotsTest-delete (#829)

* Add dot to sentence (#837)

* 814: Telemetry for sync (#835)

* Add telemetry actions for sync, list update, and error states.

* Sync timeout instead of sync end

* Add sync complete event

* Add errors and ids to telemetry calls

* Update metrics docs to reflect telemetry sync changes (#841)

* Revert delete PRs (#836)

* Revert "828-l10nScreenshotsTest-delete (#829)"

This reverts commit bab9fe8.

* Revert "806-uitest-add-delete (#808)"

This reverts commit ec56009.

* Revert "626, 627: Add kebab menu for delete  (#764)"

This reverts commit 17605c3.

* Remove unused imports and fix lint error.

* Rebase and lint

* Update to version 1.1.3 (#844)

* Update version to 1.1.3

* Add release notes for 1.1.3

* Update releases.md (#847)

* Add more context to release notes

* Add descriptions for proguard rules upload.

* Revert "Revert delete PRs (#836)" (#842)

This reverts commit 974a585.

* 816: string updates for localization (#852)

* Revert "Revert delete PRs (#836)"

This reverts commit 974a585.

* Lockwise placeholder in delete_description and descriptive comment in edit string

* Update app services, android components, and megazord configuration (#865)

* Update app services, android components, and megazord configuration

* Update a-c to v7.0.0

* Send hashed UID ping when sync completes successfully.

* Remove handling for synctelemetryping, to be completed in separate issue. Update AS to 0.37.1

* Update Readme with l10n process (#872)

This is something we've been adding to all Readmes of mobile repos to direct people to Pontoon for localization (vs GitHub repos, where work isn't supposed to happen).

* 867 - Fix obvious NPE sync crash (#884)

* 871: Create infrastructure for feature flags (#882)

* Delete feature flag

* Move feature flags to separate support file

* Create support to check build configs. Create flags for CUD

* Add telemetry probes to syncIfRequired trigger syncs (#887)

* Import strings from l10n (#893)

* Import strings from android-l10n

State: mozilla-l10n/android-l10n@349efdb

* Commenting unused string for Espanol

* 791: Remove fake timeout from sync (#889)

* Remove fake timeout from sync

* Remove all sync timeout strings

* Rebase and lint

* Fail fast if the edge between to routes is not in the nav graph. (#895)

* Auto-signin with mozilla AuthProvider (#879) — behind a feature flag.

* WIP Automatic FxA sign-in support

* Cleanup welcome fragment/account detection

* Upgrade to Android Components 9.0.0

* Ensure landable, even if not releasable

* gradlew lint

* 628: edit view and routing (#843)

* Edit xml layout

* Add route, presenter, and fragment

Button clicks and dialog

Update dependencies

Edit presenter tests

Routing stuff

Usable state with spinner

Buttons working

Password visibility in edit mode

Lint

View

Create popup item and insert menu into detail view's kebab buttton

* Popup menu and click listener

Dropdown menu and formatting

Update list - in progress

DataStoreTest - mocked up test, stuck on init

Datastore update item detail test

* Save edited changes

Lint

Update dependencies

Edit unit tests

Add sync to datastore editing. Update var name in robot and screenshot tests

Reformatting dropdown menu and edit view

Adding inclusive popup to edit->detail view nav definition

Address nullable server passwords for delete and edit. Remove unused string.

Remove learn more clicks from test

Refactoring pushError into a helper method.

* Add fxalogin to autofill onboarding route. (#901)

* 798: routing back to itemlist on login/search/feedback forms (#890)

* Investigation

* Add locking routes.

* Explicitly ignore autofill on search and edit fields

* Ignore autofill on webviews

* Remove comments and clean up lint

* Add null text value for empty usernames (#902)

* Add null text value for empty usernames

* gradlew lint

* 907 fix ui tests (#908)

* 907: fix testSettings

* fixing routing issues

* fixing more routes

* changing button name and adding route

* fixing conflicts after rebase

* Update release-notes.md with v1.1.4

* Update release notes for v2.0.0

* Update version code and version name to v2.0.0
@eliserichards eliserichards deleted the 798-keyboard-issues branch October 23, 2019 17:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

On screen Keyboard minimizes immediately on fxa login, search, and feedback forms
2 participants