Fix: App crashes when tapping more and share button in the Reader Details screen by salimbraksa · Pull Request #20490 · wordpress-mobile/WordPress-iOS

salimbraksa · 2023-04-06T23:51:11Z

Fixes #20487

Description

This PR fixed a bug that crashed the app when the user taps the more or share button in the Reader Details screen. This bug is only affecting iPad users.

Root Cause

This is the exact change in #20363 that caused the crash.

    func barButtonItem(with image: UIImage, action: Selector) -> UIBarButtonItem {
        let image = image.withRenderingMode(.alwaysTemplate)
-      let button = UIButton(frame: CGRect(x: 0, y: 0, width: 44.0, height: image.size.height))
-      button.setImage(image, for: UIControl.State())
-      button.addTarget(self, action: action, for: .touchUpInside)
- 
-       return UIBarButtonItem(customView: button)
+      return UIBarButtonItem(image: image, style: .plain, target: self, action: action)
    }

The code above removes usage of UIButton when creating a UIBarButtonItem, so the action param is now passed to a UIBarButtonItem instead of a UIButton. But the sender param of the action handlers was not updated accordingly.

// This method can be found in "ReaderDetailViewController.swift".
@objc private func didTapMenuButton(_ sender: UIButton) {
   // ...
}

The type of sender param type should be UIBarButtonItem instead of UIButton. The compiler didn't catch this, and I didn't either.

At runtime, however, the sender param type was UIBarButtonItem, which passed through several methods until it reached the following code.

// Can be found in `ReaderShowMenuAction.swift` and `PostSharingController.swift`.
if let presentationController = alertController.popoverPresentationController {
    presentationController.permittedArrowDirections = .any
    presentationController.sourceView = anchor
    presentationController.sourceRect = anchor.bounds
}

The presentationController.sourceView's type is UIView and anchor's type at runtime is UIBarButtonItem, thus the crash.

Screenshots

More Button	Share Button

Test Instructions

Please perform the following tests on iPhone and iPad.

Install the Jetpack app and log in.
Head to Reader > Following or Reader > Discover
Tap the More Button on any post.
Expect a sheet to show up on iPhone and a popover to show up on iPad.
Tap the post itself to reach the Reader Detail screen.
Tap the More button.
Expect a sheet/popover to appear.
Tap the Share button.
Expect a sheet/popover to appear.

Regression Notes

Potential unintended areas of impact
Double check the More and Share buttons work as expected in Reader Details and Reader Following / Discover screens.

	Reader Details	Reader Following & Discover Feeds
iPad	✅	✅
iPhone	✅	✅

What I did to test those areas of impact (or what existing automated tests I relied on)
Manual testing.
What automated tests I added (or what prevented me from doing so)
None.

PR submission checklist:

I have completed the Regression Notes.
I have considered adding unit tests for my changes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

wpmobilebot · 2023-04-07T00:13:40Z

📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.

	App Name	WordPress Alpha
	Configuration	Release-Alpha
	Build Number	pr20490-b99189a
	Version	22.1
	Bundle ID	org.wordpress.alpha
	Commit	`b99189a`
	App Center Build	WPiOS - One-Offs #5452

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

wpmobilebot · 2023-04-07T00:13:53Z

📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.

	App Name	Jetpack Alpha
	Configuration	Release-Alpha
	Build Number	pr20490-b99189a
	Version	22.1
	Bundle ID	com.jetpack.alpha
	Commit	`b99189a`
	App Center Build	jetpack-installable-builds #4481

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

… UIView or a UIBarButtonItem

peril-wordpress-mobile · 2023-04-07T00:59:36Z

	Warnings
⚠️	This PR contains changes to `RELEASE_NOTES.txt`. Note that these changes won't affect the final version of the release notes as this version is in code freeze. Please, get in touch with a release manager if you want to update the final release notes.

Generated by 🚫 dangerJS

guarani

Tested and works well! Thanks for the fix, @salimbraksa!

…iOS into fix/20487-reader-crash-when-wtapping-more-share-button

mokagio · 2023-04-12T05:08:11Z

I find it spooky that this PR keeps failing in AccoutSettingsServiceTests however I don't see how the changes here could have affected those tests.

Moreover, there might be something dodgy down in the reporting layer, because looking at the logs we can see that the test eventually succeeds after retrying automatically

I see no reason to hold merging this PR because of that.

tiagomar · 2023-04-12T12:24:41Z

Hey @mokagio @salimbraksa , testUpdateFailure() is the least reliable unit test we have but in this case we have a 100% reliable test failing which is testShowShareSheet() and, for some unknown reason, it doesn't appear in the console logs with the red cross. However, we can see it listed among the Failing tests. So we'll need to either fix something in the code or fix the test.

Failing tests:
  AccountSettingsServiceTests.testUpdateFailure()
  AccountSettingsServiceTests.testUpdateFailure()
  ReaderDetailCoordinatorTests.testShowShareSheet()

There were conflicts on `Podfile` and `Podfile.lock` because `release/22.1` and `trunk` both changed the Gutenberg version. As usual, I resolved them with `git checkout --theirs`, keeping the value from `trunk` under the assumption that the latest version of Gutenberg, 1.93.0-alpha2, is what `trunk` needs to work and that if the 1.92.1 hotfix hasn't already been integrated in the latest release, it will be soon and the Gutenberg team will followup with a new version update on `trunk`. There was also a conflict on the `RELEASE-NOTES.txt` that GitHub picked up with its more refined conflicts engine and that auto-merge didn't address properly but that I manually fixed. That was due to `release/22.1` adding new release note entries – which will not make it into the App Store release notes – in the 22.1 section and `trunk` having #20128 editing that section, too. #20128 was meant to land on `release/22.1` but I forgot to update the base branch after starting the code freeze 🤦‍♂️😭 ``` <<<<<<< release/22.1 -- Incoming Change * [**] Add a "Personalize Home Tab" button to the bottom of the Home tab that allows changing cards visibility. [#20369] * [*] [Reader] Fix an issue that was causing the app to crash when tapping the More or Share buttons in Reader Detail screen. [#20490] * [*] Block editor: Avoid empty Gallery block error [WordPress/gutenberg#49557] ======= * [**] [internal] Refactored Google SignIn implementation to not use the Google SDK [#20128] >>>>>>> trunk -- Current Change ```

mokagio · 2023-04-13T02:12:08Z

 * [***] [internal] Refactor uploading photos (from the device photo, the Free Photo library, and other sources) to the WordPress Media Library. Affected areas are where you can choose a photo and upload, including the "Media" screen, adding images to a post, updating site icon, etc. [#20322]
 * [**] [WordPress-only] Warns user about sites with only individual plugins not supporting core app features and offers the option to switch to the Jetpack app. [#20408]
 * [**] Add a "Personalize Home Tab" button to the bottom of the Home tab that allows changing cards visibility. [#20369]
+* [*] [Reader] Fix an issue that was causing the app to crash when tapping the More or Share buttons in Reader Detail screen. [#20490]


Notice that since this landed in the release branch after code freeze, the App Store release notes won't include it.

There were conflicts on `Podfile` and `Podfile.lock` because `release/22.1` and `trunk` both changed the Gutenberg version. As usual, I resolved them with `git checkout --theirs`, keeping the value from `trunk` under the assumption that the latest version of Gutenberg, 1.93.0-alpha2, is what `trunk` needs to work and that if the 1.92.1 hotfix hasn't already been integrated in the latest release, it will be soon and the Gutenberg team will followup with a new version update on `trunk`. There was also a conflict on the `RELEASE-NOTES.txt` that GitHub picked up with its more refined conflicts engine and that auto-merge didn't address properly but that I manually fixed. That was due to `release/22.1` adding new release note entries – which will not make it into the App Store release notes – in the 22.1 section and `trunk` having #20128 editing that section, too. #20128 was meant to land on `release/22.1` but I forgot to update the base branch after starting the code freeze 🤦‍♂️😭 Also, the entry about the personalize home tab had been removed on `trunk` via ec8a377. The release notes editor was notified about it, #20483 (review) ``` <<<<<<< release/22.1 -- Incoming Change * [**] Add a "Personalize Home Tab" button to the bottom of the Home tab that allows changing cards visibility. [#20369] * [*] [Reader] Fix an issue that was causing the app to crash when tapping the More or Share buttons in Reader Detail screen. [#20490] * [*] Block editor: Avoid empty Gallery block error [WordPress/gutenberg#49557] ======= * [**] [internal] Refactored Google SignIn implementation to not use the Google SDK [#20128] >>>>>>> trunk -- Current Change ```

salimbraksa self-assigned this Apr 6, 2023

salimbraksa added Reader [Type] Crash Jetpack App labels Apr 6, 2023

salimbraksa modified the milestone: 22.1 ❄️ Apr 6, 2023

salimbraksa requested a review from staskus April 7, 2023 00:36

salimbraksa marked this pull request as ready for review April 7, 2023 00:36

salimbraksa added 4 commits April 7, 2023 00:53

Add a PopoverAnchor enum indicating that the anchor could either be a…

fdbe668

… UIView or a UIBarButtonItem

Add UIPopoverPresentationController+PopoverAnchor.swift file

c4176a2

Pass a UIBarButtonItem anchor to ReaderShowMenuAction

023f66b

Pass a UIBarButtonItem anchor to PostSharingController

5571dc9

salimbraksa force-pushed the fix/20487-reader-crash-when-wtapping-more-share-button branch from 015a81e to 5571dc9 Compare April 7, 2023 00:54

salimbraksa changed the base branch from trunk to release/22.1 April 7, 2023 00:54

Update RELEASE-NOTES

a520d13

salimbraksa requested a review from alpavanoglu April 7, 2023 10:39

staskus mentioned this pull request Apr 9, 2023

Fix Reader Crash on iPad after tapping more/share button in the details view #20489

Closed

4 tasks

guarani self-requested a review April 10, 2023 16:29

guarani approved these changes Apr 10, 2023

View reviewed changes

salimbraksa added this to the 22.1 ❄️ milestone Apr 11, 2023

Merge branch 'release/22.1' of github.com:wordpress-mobile/WordPress-…

b99189a

…iOS into fix/20487-reader-crash-when-wtapping-more-share-button

salimbraksa requested a review from mokagio April 11, 2023 15:04

mokagio merged commit 35ac8a9 into release/22.1 Apr 12, 2023

mokagio deleted the fix/20487-reader-crash-when-wtapping-more-share-button branch April 12, 2023 05:08

guarani mentioned this pull request Apr 12, 2023

Reader: Crash on iPad after tapping more/share button in the details view #20487

Closed

mokagio mentioned this pull request Apr 13, 2023

Merge beta 22.1.0.2 #20525

Closed

3 tasks

mokagio mentioned this pull request Apr 13, 2023

Merge beta 22.1.0.2 #20527

Merged

3 tasks

mokagio reviewed Apr 13, 2023

View reviewed changes

salimbraksa mentioned this pull request May 19, 2023

Reader: Block and Report actions are missing from Following and Discover feeds #20706

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix: App crashes when tapping more and share button in the Reader Details screen#20490

Fix: App crashes when tapping more and share button in the Reader Details screen#20490
mokagio merged 6 commits into
release/22.1from
fix/20487-reader-crash-when-wtapping-more-share-button

salimbraksa commented Apr 6, 2023 •

edited

Loading

Uh oh!

wpmobilebot commented Apr 7, 2023 •

edited

Loading

Uh oh!

wpmobilebot commented Apr 7, 2023 •

edited

Loading

Uh oh!

peril-wordpress-mobile Bot commented Apr 7, 2023 •

edited

Loading

Uh oh!

guarani left a comment

Uh oh!

mokagio commented Apr 12, 2023

Uh oh!

tiagomar commented Apr 12, 2023

Uh oh!

mokagio Apr 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

salimbraksa commented Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Root Cause

Screenshots

Test Instructions

Regression Notes

PR submission checklist:

Uh oh!

wpmobilebot commented Apr 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Apr 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peril-wordpress-mobile Bot commented Apr 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guarani left a comment

Choose a reason for hiding this comment

Uh oh!

mokagio commented Apr 12, 2023

Uh oh!

tiagomar commented Apr 12, 2023

Uh oh!

mokagio Apr 13, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

salimbraksa commented Apr 6, 2023 •

edited

Loading

wpmobilebot commented Apr 7, 2023 •

edited

Loading

wpmobilebot commented Apr 7, 2023 •

edited

Loading

peril-wordpress-mobile Bot commented Apr 7, 2023 •

edited

Loading