Skip to content
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.

Sign up for GitHub

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

Jump to bottom

FXIOS-759 ⁃ Fix #7123: Empty logins view after switching to a different app #7164

Merged
merged 3 commits into from
Aug 25, 2020
Merged

FXIOS-759 ⁃ Fix #7123: Empty logins view after switching to a different app #7164

merged 3 commits into from
Aug 25, 2020

Conversation

vphong
Copy link
Contributor

@vphong vphong commented Aug 19, 2020 •
edited by data-sync-user
Loading

Fixes #7123. Removes the "go to root view controller" behavior on app deactivation in SensitiveViewController which LoginList and LoginDetail view controllers inherit from. That seemed to have prevented detecting app reactivation from the Login Details view.

┆Issue is synchronized with this Jira Task

@vphong
Copy link
Contributor Author

vphong commented Aug 19, 2020

Not sure if this is a stable fix since I'm not sure why that behavior was written in in the first place. Any thoughts @garvankeeley?

@kaylagalway
Copy link
Contributor

kaylagalway commented Aug 20, 2020 •
edited
Loading

It is possible this may affect the behavior when you set face id and passcode as required to access logins? You can go to Firefox Settings and then scroll down to "Face ID & Passcode" and turn either Face ID or Passcode on and check to see if this breaks that security behavior @vphong

@vphong
Copy link
Contributor Author

vphong commented Aug 20, 2020

The self.dismiss() lines should take care of the Logins/Login Detail view dismissing on ID cancellation. Here's a video: https://imgur.com/a/NZ70Let

@data-sync-user data-sync-user changed the title Fix #7123: Empty logins view after switching to a different app FXIOS-759 ⁃ Fix #7123: Empty logins view after switching to a different app Aug 21, 2020
@garvankeeley
Copy link
Contributor

Does this patch still solve the STR in https://bugzilla.mozilla.org/show_bug.cgi?id=1562486 ?

@vphong
Copy link
Contributor Author

vphong commented Aug 21, 2020 •
edited
Loading

Yes, here's another recording using a passcode instead of Touch ID: https://imgur.com/a/YvibYr3
I didn't remove the dismissal of the VC, just the pop to root behavior. So it should still dismiss entirely on cancel

garvankeeley
garvankeeley approved these changes Aug 24, 2020
View reviewed changes
Copy link
Contributor

@garvankeeley garvankeeley left a comment

Choose a reason for hiding this comment

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

Thanks for the video of this working!

kaylagalway
kaylagalway approved these changes Aug 24, 2020
View reviewed changes
Copy link
Contributor

@kaylagalway kaylagalway left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for remedying this! Approving since this behavior is currently much better than crashing - but just want to note that @vphong chatted about trying to make the navigation bar dismissal a little smoother, if it's possible with the way the code is currently laid out.

@vphong
Copy link
Contributor Author

vphong commented Aug 25, 2020

Sorry, I was busy today but would just like to make sure this looks OK with my recent push: https://gfycat.com/negativegloomyharpseal @kaylagalway @garvankeeley

@kaylagalway
Copy link
Contributor

Sorry, I was busy today but would just like to make sure this looks OK with my recent push: https://gfycat.com/negativegloomyharpseal @kaylagalway @garvankeeley

Looks perfect. Thank you @vphong !

@mozillamobilebot
Copy link

SwiftLint found issues

Warnings

File Line Reason
SensitiveViewController.swift 30 An object should only remove itself as an observer in deinit. (notification_center_detachment)

Generated by 🚫 Danger

@kaylagalway kaylagalway merged commit 324cf49 into mozilla-mobile:main Aug 25, 2020
vphong added a commit to vphong/firefox-ios that referenced this pull request Aug 26, 2020
@vphong
Merge branch 'main' into issue#6312
aa5a90c 
* main: (25 commits)
  FXIOS-708 ⁃ [iOS14 Widgets] : iOS14 Quick Action Widgets (Medium/Small) Versions (mozilla-mobile#7051)
  FXIOS-805 ⁃ Default browser change, remove adjust, remove photopicker string (mozilla-mobile#7215)
  Revert "Bug 1608838: Include data sensitivity category (mozilla-mobile#7112)" (mozilla-mobile#7216)
  FXIOS-759 ⁃ Fix mozilla-mobile#7123: Empty logins view after switching to a different app (mozilla-mobile#7164)
  Bug 1608838: Include data sensitivity category (mozilla-mobile#7112)
  Fix mozilla-mobile#5937: Re-enable more XCUI tests (mozilla-mobile#7158)
  Update default browser strings (mozilla-mobile#7180)
  FXIOS-714 ⁃ [Today Widget] Adding Close Private tabs button to today widget (mozilla-mobile#6971)
  Refactored small size search in title to be more clear (mozilla-mobile#7168)
  For mozilla-mobile#7130 - XCUITest fix Downloads tests (mozilla-mobile#7153)
  Added comments to new line strings for translators (mozilla-mobile#7162)
  For mozilla-mobile#7154 - UITests fix compilation error (mozilla-mobile#7155)
  Refactored strings for Quick Action - Small Size widget (mozilla-mobile#7157)
  Fix mozilla-mobile#6847 new tab button (mozilla-mobile#7038)
  Refactored some strings for v29 (mozilla-mobile#7152)
  Breach Alerts Feature (mozilla-mobile#7136)
  Fix mozilla-mobile#7000: show about:blank for blank target popup windows (mozilla-mobile#7125)
  Fix mozilla-mobile#7053: Add prompt visible UI test for HTTP Basic Auth (mozilla-mobile#7124)
  String update: Added missing serial comma (mozilla-mobile#7132)
  Fix mozilla-mobile#7091 mozilla-mobile#7092 mozilla-mobile#7113 - String updates for Widgets, Default Browser and ETP changes (mozilla-mobile#7129)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garvankeeley garvankeeley garvankeeley approved these changes

@kaylagalway kaylagalway kaylagalway approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

FXIOS-779 ⁃ Empty logins view after switching to a different app
4 participants
@vphong @kaylagalway @garvankeeley @mozillamobilebot