Fix share extension navigation bar being transparent #19700

mokagio · 2022-12-01T12:04:09Z

I came to this PR via #19513 (comment).

Originally, it was just a matter of removing the #available(iOS 13, *) check, but looking at the code and how it behaves at runtime, it became clear there's some more work to do in regards to the Share Extension. In particular whether I removed the entire else branch or I left it as it was, the presentation of the view controller for the share extension always resulted with a close to fully transparent navigation bar (see image below).

The fix I implemented came after a bunch of trial and error and feels a bit like duck-tape. I left a comment with a suggestion for a better fix, but that would need to be a bigger structural-level kind of work.

What my fix consists of is merely changing the color of the navigation bar to match the standard one. 🙃

It's totally possible I'm misusing UIKit here—UI development is not my strong suit. If that's the case, I'm happy to hand this over to someone more capable than me in this area.

Before

After

On iPad

I don't know why they don't have the same size, but I think fixing that is out of scope for this PR.

Save to Draft

Share

Testing

To test these changes, I launched the app in the Simulator, went out of it (Shift Cmd H), opened Safari, and loaded both "Share" and "Save to Draft" extensions to verify the nav bar background color, modal presentation, and scrolling behavior behaved correctly

Regression Notes

Potential unintended areas of impact – N.A.
What I did to test those areas of impact (or what existing automated tests I relied on) – N.A.
What automated tests I added (or what prevented me from doing so) – N.A.

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

It always return `true` since the deployment version is 13.0, and will soon become 14.0.

wpmobilebot · 2022-12-01T13:06:54Z

You can test the changes in WordPress from this Pull Request by:

Clicking here or scanning the QR code below to access App Center
Then installing the build number pr19700-237e5db on your iPhone

If you need access to App Center, please ask a maintainer to add you.

wpmobilebot · 2022-12-01T13:07:58Z

You can test the changes in Jetpack from this Pull Request by:

Clicking here or scanning the QR code below to access App Center
Then installing the build number pr19700-237e5db on your iPhone

If you need access to App Center, please ask a maintainer to add you.

mokagio · 2022-12-01T16:03:45Z

WordPress/WordPressShareExtension/MainShareViewController.swift

            // We  need to make sure we don't end up with stacked modal view controllers by using this:
            shareNavController.modalPresentationStyle = .overFullScreen
-        } else {
-            shareNavController.transitioningDelegate = extensionTransitioningManager


ExtensionTransitioningManager is now unused. I didn't remove it in the context of this PR because removing it triggers a cascade of removals which felt more appropriate for a dedicated PR.

Using `.overFullScreen` or leaving `.automatic` resulted in Share being dismissable via swipe but that didn't restore Safari to the share menu, leaving it instead blocked as if the presented VC was still there. `.overCurrentContext` has no effect on the Save to Draft behavior, leaving it the same as when using `.overFullScreen`. On iPad, this setting result in both extensions not being full screen. Otherwise, Share would have been full screen while Save to Draft wouldn't.

mokagio · 2022-12-01T16:41:34Z

WordPress/WordPressShareExtension/MainShareViewController.swift

-            shareNavController.transitioningDelegate = extensionTransitioningManager
-            shareNavController.modalPresentationStyle = .custom
-        }
+        // We need to make sure we don't end up with stacked modal view controllers by using this:


🔍 notice the removal of the double space between "We" and "need" 🤓

mokagio · 2022-12-01T16:48:35Z

@bjtitus @twstokes I ping you following GitHub's recommendation, but feel free to forward to who this may concern.

twstokes · 2022-12-01T17:13:01Z

@bjtitus @twstokes I ping you following GitHub's recommendation, but feel free to forward to who this may concern.

👋 I'll be glad to look @mokagio, though I thought I slipped GitHub a $20 not to recommend me.

Not quite related to this PR but something to flag, it looks like I see a prompt to open the WordPress app although it was triggered from the Jetpack sharing button. In this particular case my device had both apps installed and only WordPress was logged in.

RPReplay_Final1669914567.MP4

mokagio · 2022-12-01T17:44:24Z

@twstokes thank you Tanner! I guess 20 bucks is not enough for them? 🙃

Looks like "WordPress app" is hardcoded in the string, even though the copy is used in both Jetpack and WordPress.

More ammo for my argument this ought to be a framework, not code added to multiple targets.

twstokes

This LGTM @Gio2018! 🚀

I tested:

iOS 15 and 16
Light and dark modes
iPhone and iPad (iOS 16 only)
Different orientations

it would be great to extract all this logic in a standalone framework or package, and then make the individual extensions import it, and instantiate and configure the view controller to their liking

Sounds like a good idea!

twstokes · 2022-12-01T18:16:51Z

FYI - I'll put in an issue for the above comment.

Done: #19704

Remove one redundant #available(iOS 13, *) check

6c4567f

It always return `true` since the deployment version is 13.0, and will soon become 14.0.

mokagio added 2 commits December 1, 2022 16:48

Add note about UINavigationBar.appearance() usage

4dcd17a

Set the navigation bar background color for the share extension

d00af63

mokagio commented Dec 1, 2022

View reviewed changes

mokagio changed the title ~~Fix share extension presentation~~ Dec 1, 2022

Add entry for #19700 to RELEASE-NOTES.txt

237e5db

mokagio requested review from twstokes and bjtitus December 1, 2022 16:47

mokagio added this to the 21.4 milestone Dec 1, 2022

mokagio self-assigned this Dec 1, 2022

mokagio added the [Type] Bug label Dec 1, 2022

mokagio marked this pull request as ready for review December 1, 2022 16:48

mokagio mentioned this pull request Dec 1, 2022

Remove redundant #available(iOS 13, *) checks #19513

Closed

4 tasks

twstokes approved these changes Dec 1, 2022

View reviewed changes

mokagio merged commit ef665f9 into trunk Dec 2, 2022

mokagio deleted the mokagio/remove-ios-13-available-share-extension branch December 2, 2022 04:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix share extension navigation bar being transparent #19700

Fix share extension navigation bar being transparent #19700

mokagio commented Dec 1, 2022 •

edited

Loading

wpmobilebot commented Dec 1, 2022 •

edited

Loading

wpmobilebot commented Dec 1, 2022 •

edited

Loading

mokagio Dec 1, 2022

mokagio Dec 1, 2022

mokagio commented Dec 1, 2022

twstokes commented Dec 1, 2022

mokagio commented Dec 1, 2022

twstokes left a comment

twstokes commented Dec 1, 2022 •

edited

Loading

Fix share extension navigation bar being transparent #19700

Fix share extension navigation bar being transparent #19700

Conversation

mokagio commented Dec 1, 2022 • edited Loading

Before

After

On iPad

Testing

Regression Notes

wpmobilebot commented Dec 1, 2022 • edited Loading

wpmobilebot commented Dec 1, 2022 • edited Loading

mokagio Dec 1, 2022

Choose a reason for hiding this comment

mokagio Dec 1, 2022

Choose a reason for hiding this comment

mokagio commented Dec 1, 2022

twstokes commented Dec 1, 2022

mokagio commented Dec 1, 2022

twstokes left a comment

Choose a reason for hiding this comment

twstokes commented Dec 1, 2022 • edited Loading

mokagio commented Dec 1, 2022 •

edited

Loading

wpmobilebot commented Dec 1, 2022 •

edited

Loading

wpmobilebot commented Dec 1, 2022 •

edited

Loading

twstokes commented Dec 1, 2022 •

edited

Loading