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.

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

Intro note submit button doesn't leave screen but posts multiple of the same note #1726

Closed
johnshenio opened this issue Nov 17, 2023 · 19 comments
Assignees
Labels
bug Something is not working, or not working as intended

Comments

@johnshenio
Copy link

The new update on my iPhone 12 pro, iOS 17 with the introduction notes posts multiple of the same button after clicking the submit button. The submit button doesn't leave the post creation screen

I created a new npub but I couldn't get back to the intro note screen

@alltheseas
Copy link
Collaborator

Thanks for reporting. I believe this is a duplicate of #1502

@jb55
Copy link
Collaborator

jb55 commented Nov 17, 2023

I believe this is different

@alltheseas
Copy link
Collaborator

@jb55
Copy link
Collaborator

jb55 commented Nov 17, 2023

this is really bad

@alltheseas
Copy link
Collaborator

@alltheseas
Copy link
Collaborator

Is it possible to get a quick fix in, or remove this code for now?

cc @danieldaquino

@danieldaquino
Copy link
Contributor

@alltheseas, @jb55, I believe this is precondition dependent.

Two examples:

Device: iPhone 15 Pro simulator
iOS: 17.0
Damus: v1.6 (29) (f976f23854dfddc390bf00095c69d27460c6776d) (Local build)
Behavior: Post view dismissed after posting (works)

Device: iPhone 13 Mini
iOS: 17.1
Damus: v1.6 (29) (f976f23854dfddc390bf00095c69d27460c6776d) (Official build)
Behavior: Post view NOT dismissed after posting

@danieldaquino
Copy link
Contributor

I suspect this might be due to our use of .presentationMode, which upon checking, seems to be deprecated.

https://developer.apple.com/documentation/swiftui/environmentvalues/presentationmode?changes=latest_major

I will check if that's indeed the root cause

@danieldaquino
Copy link
Contributor

Device: iPhone 13 Mini
iOS: 17.1
Damus: v1.6 (29) (f976f23) (Local build)
Behavior: Post view NOT dismissed after posting

@danieldaquino
Copy link
Contributor

danieldaquino commented Nov 17, 2023

Original passing test preconditions (from bf43842)

Device: iPhone 14 Pro simulator
iOS: 17.0
Damus: bf43842

It seems to be failing from iOS 17.1 (based on the data so far)

@danieldaquino
Copy link
Contributor

I suspect this might be due to our use of .presentationMode, which upon checking, seems to be deprecated.

https://developer.apple.com/documentation/swiftui/environmentvalues/presentationmode?changes=latest_major

I will check if that's indeed the root cause

It does not seem to be the only cause. Changing it to the new dismiss() also did not work

@danieldaquino
Copy link
Contributor

@jb55, this is starting to look like one of those obscure SwiftUI bugs.

The relevant factors that determine whether or not the bug occurs seem to be one or many of these:

  • iOS version: Tests on 17.0 work as far as I have tested, Tests on 17.1 fail as far as I have tested
  • Real device vs. simulator: Simulator works as far as I have tested, real devices do not seem to work as far as I have tested
  • View hierarchy: Normal posts seem to be unaffected. However, the exact same post view under the onboarding seems to fail.

In the interest of time, I will send a patch to temporarily disable this onboarding feature

@alltheseas
Copy link
Collaborator

this is starting to look like one of those obscure SwiftUI bugs.

Does this experience make the case for UIkit?

@danieldaquino
Copy link
Contributor

danieldaquino commented Nov 17, 2023

this is starting to look like one of those obscure SwiftUI bugs.

Does this experience make the case for UIkit?

Nah, UIKit also has some weird quirks (e.g. the text editor wrapping bugs) LOL

@danieldaquino
Copy link
Contributor

@jb55, I sent an emergency patch for temporarily disabling the feature: https://groups.google.com/a/damus.io/g/patches/c/T6pqnIhJzHc

I will work on a full clean fix tomorrow. I believe the root cause is a combination of:

  • Inconsistent behavior of the environment variable .presentationMode between iOS versions or whether the device is real or a simulator (The deprecation might explain the inconsistency)
  • The new environment variable .dismiss seems to have limited propagation. It works on OnboardingSuggestionsView, but not on any of its subviews (At least on iOS 17.1 + iPhone 13)

If you could double-check my testing under a different precondition I would appreciate 🙏 (we don't know yet the extent of the inconsistencies around this).

@alltheseas alltheseas added the bug Something is not working, or not working as intended label Nov 17, 2023
@jb55
Copy link
Collaborator

jb55 commented Nov 17, 2023 via email

@danieldaquino
Copy link
Contributor

@jb55, @alltheseas, sent a patch with the fix!

Root causing, code, fix explanation, and testing report are all in the patch email (Can be viewed here: https://groups.google.com/a/damus.io/g/patches/c/DJ8XymT9mHA)

Important notes:

  • I observed another issue (not as bad as this one) on iOS 16. I reported it under a separate ticket: "Who to follow" nav title shows on onboarding post view in iOS 16 #1739
  • I made this patch directly on top of the current release. We probably want to create a hotfix branch where we merge in only essential fixes, to avoid regression risk from other recent changes in master
  • I tried to test as much and as carefully as possible, but I appreciate any extra independent testing to make sure I did not miss anything 🙏

@jb55
Copy link
Collaborator

jb55 commented Nov 20, 2023 via email

@jb55 jb55 closed this as completed in 878b1ca Nov 20, 2023
@danieldaquino danieldaquino self-assigned this Nov 21, 2023
danieldaquino added a commit that referenced this issue Dec 15, 2023
This commit fixes the issue where clicking "post" on the onboarding sheet does not dismisses itself under certain device/iOS combos.

The root cause is that the behavior of `dismiss` calls under a deeply nested view (i.e. not a direct subview of the sheet) is inconsistent depending on the device or iOS.

This fix does two things:
1. It upgrades the usage of `presentationMode` (which is deprecated) to the new `dismiss` API
2. It makes the onboarding sheet view (A direct subview of the sheet) to listen to signals from the post view and use that to also call `dismiss()`, which is explicitly supported by Apple in their docs (https://developer.apple.com/documentation/swiftui/environmentvalues/dismiss)

Testing
-------

PASS

Device: iPhone 13 mini (physical device)
iOS: 17.1
Damus: This commit (Local build, no local mods)
Setting: "Always show onboarding" is set to ON
Coverage:
1. Clicking "post" on onboarding post view publishes the post and dismisses the view. PASS
2. Clicking "cancel" on onboarding post view dismisses the view without publishing. PASS
3. Dragging the onboarding post view down dismisses the view without publishing. PASS
4. Making a normal post (I replied to a thread) still publishes the post and dismisses the normal post view sheet. PASS

Testing on other Device/iOS combos
---------------------------------

PASS

Preconditions:
- iPhone 15 Pro (simulator) on iOS 17.0.1
- iPhone SE 3rd gen (simulator) on iOS 16.4
Damus: This commit (Local build, no local mods)
Setting: "Always show onboarding" is set to ON
Coverage:
1. Clicking "post" on onboarding post view publishes the post and dismisses the view. PASS

Closes: #1726
Changelog-Fixed: Fix onboarding post view not being dismissed under certain conditions
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
Reviewed-by: William Casarin <jb55@jb55.com>
Signed-off-by: William Casarin <jb55@jb55.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working, or not working as intended
Projects
Archived in project
Development

No branches or pull requests

4 participants