Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Login] Improve error messaging displaying a new error screen when application password is disabled #15031
base: trunk
Are you sure you want to change the base?
[Login] Improve error messaging displaying a new error screen when application password is disabled #15031
Changes from 8 commits
cf69dbb
74f3268
b292c92
170d58e
2887491
43e803d
965c4d3
58faf5e
79d524a
3f40c59
9b480ba
98810f3
6467c96
71d68ee
ec50237
6bdb62d
fc9335d
3e5b2ad
daee4c0
dc68ebc
4ae12d2
9fafc53
9e4663e
2c7f417
001af38
b8b44e1
9a84317
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this change is not necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function
applicationPasswordDisabledUI
undernavigateToErrorUI
uses thesiteURL
, so it needs to be declared as internal now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is hacky and prone to error. How about looking for the index of
ApplicationPasswordTutorialViewController
in the navigation stack, and pop to the index before that?A better option would be to send the view controller before the application password flow to this view controller via
presentApplicationPasswordWebView
inAuthenticationManager
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you and with your second suggestion, so I applied the new changes in this commit: 9b480ba
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you want to pop to the previous view controller,
popToRoot
seems incorrect. Should it bepopViewController
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I implemented it, during my testing process (at least in every test I did), the previous view controller immediately before the error screen was the loading screen—the one with just a loader in the middle of the view. If we pop back to it (by calling
popViewController
), the user would remain stuck there, so we need to go back further to ensure a better user experience.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation. From my understanding, you already sent the
previousViewController
for the application password flow, so this should not happen. Also, you are sendingnil
for the site credential login flow, so tapping "Try Again" should navigate back to the login screen instead of the prologue screen IMO. Is there any other edge case we should be aware of?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, in theory, there are no other edge cases and I didn't find any other specific edge cases so I'm putting
popViewController
. 👍