Fix "Please adopt containerBackgroundAPI" for iOS 17 Widgets by staskus · Pull Request #21705 · wordpress-mobile/WordPress-iOS

staskus · 2023-10-09T15:15:15Z

Partially addresses #21672

The goal of this PR is to conform to the new iOS 17 Widget APIs to avoid "Please adopt containerBackgroundAPI" message:

Implement an empty containerBackground for conformance. This API does not support the usage of AccessoryWidgetBackground, therefore I decided to remove the background altogether to fit the current design guidelines
Update UI of HomeScreenWidget to support removable background
Temporarily disabled iOS 17 margins to support StandBy mode

I want to make even better iOS 17 support in the short-term future. Plus, we probably should make naming changes, given HomeScreen widgets work on iPadOS Lock Screen as well.

To test:

Lock Screen and Home screen widgets should work as before
Home Screen widgets and Lock Screen Widgets should now be available on iPadOS Lock Screen vertical and horizontal versions

Regression Notes

Potential unintended areas of impact

Should be none, Widgets should work and appear as before

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)

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.

UI Changes testing checklist:

Portrait and landscape orientations.
Light and dark modes.
Fonts: Larger, smaller and bold text.
High contrast.
VoiceOver.
Languages with large words or with letters/accents not frequently used in English.
Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
iPhone and iPad.
Multi-tasking: Split view and Slide over. (iPad)

staskus · 2023-10-09T15:21:27Z

@guarani

This is not a high priority but I wanted to at least conform to these APIs so we wouldn't need to make quick beta fixes when CI is updated to Xcode 15.

wpmobilebot · 2023-10-09T15:39:46Z

📲 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	pr21705-1e80d49
	Version	23.9
	Bundle ID	org.wordpress.alpha
	Commit	`1e80d49`
	App Center Build	WPiOS - One-Offs #8354

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

staskus · 2023-10-10T08:21:27Z

@guarani review is not necessary right now.

CI throws an issue:

value of type 'some WidgetConfiguration' has no member 'contentMarginsDisabled'

It means that we cannot make these changes until Xcode is updated on CI, so we cannot merge it right now.

guarani

To summarize what we discussed in Slack, p1697208501951699/1697187055.060939-slack-C441E3YTS, oncecontainerBackground was implemented, Lock Screen widgets no longer showed the Please adopt containerBackground API, but Home Screen widgets still do, despite implementing containerBackgroundRemovable and passing in false.

staskus · 2023-10-18T08:35:05Z

Home Widgets (System Small)

I added to support background removal APIs to Small Home Screen Widgets. For them to appear nicely, I changed the order of value title and value and made the value font larger. They can be added on the iPad Lock Screen with a Horizontal Orientation.

These are the 2 variations that system small widgets can be presented in:

The inspiration was a weather widget example:

Lock Screen Widgets (Accessory Rectangular)

To make the selection consistent with iOS 17 design guidelines and newest widgets, I think the best option is to remove the background altogether:

Now all these widgets can be displayed together on the iPad Lock Screen:

staskus · 2023-10-19T14:55:42Z

@guarani

My plan is as follows:

Get feedback from designers about widgets without background pc8eDl-14z-p2#comment-935
Split this PR into two: 1) Widget Lock Screen Widget UI updates without a background and 2) iOS 17 APIs conformance
Review, test & merge PR 1) since it would not require Xcode changes, mostly it would remove background from lock screen widgets
Review, test & merge Lock Screen Widgets: This Week Chart Widget #21514, since it would be counter-productive to test it while this PR is still ongoing
Ideally, we would ship lock screen widget background updates and this week widget with 23.6
Once we enable Xcode 15, merge 2) iOS 17 APIs conformance PR.

Conforming by implementing containerBackground API. For now, Lock Screen Widgets keep using AccessoryWidgetBackground(), since this background is not supported by removable background.

Home Screen widgets require layout adjustment to support background removal

…acros

…ground Small Home Screen Widget can be shown in iPad lock screen on horizontal orientation: - Change the order of title and value - Increase font size of value

guarani · 2024-01-02T13:57:49Z

FYI: The PR that updates CI to Xcode 15 is targeting Monday's 24.0 release: #21921

We could spend some time this week to get necessary widget changes done for Monday's beta cut. Wdyt? I haven't been focusing on these PRs but I think it makes sense to do so now.

staskus · 2024-01-02T16:59:01Z

FYI: The PR that updates CI to Xcode 15 is targeting Monday's 24.0 release: #21921

@guarani, thanks for making a note! Yes, I followed it but I wasn't sure if it would make it on time for 24.0.

Wdyt? I haven't been focusing on these PRs but I think it makes sense to do so now.

Yes, once they merge Xcode 15 updates we'll need to get this PR merged for 24.0 as well. It would be good to build it with Xcode 15 locally and review it so it would be ready. 👍

mokagio

I'll start by saying that I haven't looked at the whole code, so apologies if the question has an easy answer. But I wonder if any of this could go into JetpackStatsWidgetsCore.

staskus · 2024-01-03T08:04:42Z

I'll start by saying that I haven't looked at the whole code, so apologies if the question has an easy answer. But I wonder if any of this could go into JetpackStatsWidgetsCore.

Great question, @mokagio. I'll check! I made this PR 3 months ago to prepare for Xcode 15 updates when this module didn't exist.

…or-widgets-xcode-15-ios-17

staskus · 2024-01-05T12:05:27Z

I'll start by saying that I haven't looked at the whole code, so apologies if the question has an easy answer. But I wonder if any of this could go into JetpackStatsWidgetsCore.

I checked the affected files and I don't think any of them needs to go to JetpackStatsWidgetCore. Changes are only to Stats Widget UI code which can continue belonging to JetpackStatsWidgets. JetpackStatsWidgetCore should contain data types and functionality that needs to be reused between targets.

mokagio · 2024-01-08T05:07:12Z

+    init(viewData: GroupedViewData) {
+        self.title = viewData.widgetTitle
+        self.description = viewData.siteName
+        self.valueTitle = viewData.upperLeftTitle
+        self.value = viewData.upperLeftValue
+    }


Did you consider delegating to self.init(title:...?

Good observation. I'll use self.init..

mokagio · 2024-01-08T05:22:27Z

        .configurationDisplayName(config.displayName)
        .description(config.description)
        .supportedFamilies(config.supportFamilies)
+        .iOS17ContentMarginsDisabled() /// Temporarily disable additional iOS17 margins for widgets


Super nitpick, /// is a documentation comment, as opposed to the standard //. With /// Xcode and other tools will render the comment as documentation reading tags and markdown. However, the comment needs to be on a type or method declaration. We don't need to "document" method calls. I just wanted to point it out but I don't expect this to be addressed. 👍 😄

…-widgets-xcode-15-ios-17 This is necessary to get up to date with a CI configuration change that requires running on Xcode 15.1. See #22270

mokagio · 2024-01-08T05:27:03Z

            .fontWeight(fontWeight)
            .foregroundColor(foregroundColor)
            .lineLimit(lineLimit)
+            .minimumScaleFactor(0.5)


0.5 is an arbitrary value, could be smaller or a bit larget as well.

Previously created rectangular widgets can now appear without a background on iPadOS Horizontal Lock Screen. Since they appear without a background, I increased font sizes to have less space and resemble the look of other widgets without a background.

However, when numbers become large, I want font to Value font to shrink, to be fully displayed.

Without minimumScaleFactor:

With minimumScaleFactor:

mokagio · 2024-01-08T05:29:09Z

@staskus thank you for looking into whether some files could go into the module.

JetpackStatsWidgetCore should contain data types and functionality that needs to be reused between targets.

Yes, and also any kind of standalone logic that we might want (ought to!) unit test. We can't run unit tests on app extension targets, so anything we want to test should go into that Swift Package.

As far as I can see, this PR is all about SwiftUI configurations so there's nothing that fits that criteria either 👍

wpmobilebot · 2024-01-08T05:32:34Z

📲 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	pr21705-1e80d49
	Version	23.9
	Bundle ID	com.jetpack.alpha
	Commit	`1e80d49`
	App Center Build	jetpack-installable-builds #7377

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

mokagio · 2024-01-08T05:46:36Z

            .accessibilityElement(children: .combine)
        } else {
            Text("Not implemented for widget family \(family.debugDescription)")
+                .removableWidgetBackground()


Just curious about the Swift UI architecture, which is not my strong suit. Would it have been possible to group the conditional logic, e.g. in an *Stack, to only write the removableWidgetBackground() transformation once?

Good question. I think it would be possible 👍

mokagio

Run it on the Simulator with Xcode 15.1 after merging trunk into this branch. Everything looks good as far as I can see, but I have little context around the expected widgets look and feel.

Let's merge this so it's part of 24.0, get feedback from @osullivanchris (Chris has already commented on the widgets, see pc8eDl-14z-p2), and address possible tweaks as beta fixes.

iPhone

iPad

staskus · 2024-01-08T06:31:38Z

Thanks for the review, @mokagio!

I can merge it to 24.0.

This PR was all about basic iOS 17 support. It meant supporting removableWidgetBackground and also making sure existing medium and large widgets could appear without a background on the iPad OS Horizontal Lock Screen.

I agree we can make other tweaks after we merge this PR.

mokagio · 2024-01-08T06:50:56Z

I think the fact the latest build on this branch was cancelled made all the CI checks go "Errored"

I started a new one so we can merge this ASAP.

@staskus what do you mean with "I can merge it to 24.0." and with the "[Status] Blocked" label? I read this as "there's a reason why we shouldn't merge it to trunk right now" but I don't see what the reason is 🤔

staskus · 2024-01-08T06:56:06Z

I started a new one so we can merge this ASAP.

@mokagio

Okay. Sorry, there was a bit of a misunderstanding. I missed the fact you already merged it with trunk. Yes, we can merge it whenever CI succeeds. 👍

guarani · 2024-01-08T13:00:15Z

I forgot to review on Friday 🙈 Thanks for handling this folks!

staskus added [Type] Task Widgets Anything related to Home Screen widgets labels Oct 9, 2023

staskus requested a review from guarani October 9, 2023 15:15

staskus mentioned this pull request Oct 9, 2023

Widgets: Provide mandatory iOS 17 SDK Support #21672

Closed

5 tasks

staskus added this to the 23.5 milestone Oct 9, 2023

staskus removed the request for review from guarani October 10, 2023 08:20

staskus added the [Status] Blocked label Oct 10, 2023

staskus modified the milestones: 23.5, Pending Oct 12, 2023

staskus requested a review from guarani October 13, 2023 13:39

staskus commented Oct 13, 2023

View reviewed changes

Comment thread WordPress/JetpackStatsWidgets/Widgets/HomeWidgetAllTime.swift Outdated

guarani reviewed Oct 13, 2023

View reviewed changes

staskus force-pushed the fix/please-adopt-container-background-api-for-widgets-xcode-15-ios-17 branch from 75c2d00 to dbb2b6b Compare October 20, 2023 08:49

staskus marked this pull request as draft October 20, 2023 13:28

staskus added 11 commits October 23, 2023 11:35

Disable additional iOS17 margins for widgets

aa6e017

Conform to iOS 17 removable background API on Lock Screen Widgets

dfaf4e8

Conforming by implementing containerBackground API. For now, Lock Screen Widgets keep using AccessoryWidgetBackground(), since this background is not supported by removable background.

Disable background removing for Home Screen widgets

478e075

Home Screen widgets require layout adjustment to support background removal

Silence Xcode 14 warnings by wrapping new API calls in preprocessor m…

5aab0b7

…acros

Update iOS17WidgetAPIs.swift

190578f

Remove iOS17ContainerBackgroundRemovable

3356e3d

Change Home Screen Small Widget design if it's displayed without back…

94fc839

…ground Small Home Screen Widget can be shown in iPad lock screen on horizontal orientation: - Change the order of title and value - Increase font size of value

Set removableWidgetBackground for unimplemented text

bb41a62

Update Home Screen widgets according to design requirements

a1d932f

Add icon to title view

b02d1a3

Update label color of unconfigured view

7130ef2

staskus force-pushed the fix/please-adopt-container-background-api-for-widgets-xcode-15-ios-17 branch from dbb2b6b to 7130ef2 Compare October 23, 2023 10:34

staskus mentioned this pull request Jan 3, 2024

Test Fixes to Run Xcode 15 in CI #22270

Merged

4 tasks

mokagio reviewed Jan 3, 2024

View reviewed changes

staskus added 2 commits January 5, 2024 11:49

Merge branch 'trunk' into fix/please-adopt-container-background-api-f…

42c03e8

…or-widgets-xcode-15-ios-17

Code structure

ca4673e

mokagio reviewed Jan 8, 2024

View reviewed changes

Merge remote trunk into fix/please-adopt-container-background-api-for…

1e80d49

…-widgets-xcode-15-ios-17 This is necessary to get up to date with a CI configuration change that requires running on Xcode 15.1. See #22270

mokagio reviewed Jan 8, 2024

View reviewed changes

mokagio approved these changes Jan 8, 2024

View reviewed changes

staskus marked this pull request as ready for review January 8, 2024 06:31

staskus removed the [Status] Blocked label Jan 8, 2024

staskus enabled auto-merge January 8, 2024 06:57

staskus merged commit 6326dfc into trunk Jan 8, 2024

staskus deleted the fix/please-adopt-container-background-api-for-widgets-xcode-15-ios-17 branch January 8, 2024 07:23

Uh oh!

Conversation

staskus commented Oct 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To test:

Regression Notes

Uh oh!

staskus commented Oct 9, 2023

Uh oh!

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

Uh oh!

Uh oh!

staskus commented Oct 10, 2023

Uh oh!

Uh oh!

guarani left a comment

Choose a reason for hiding this comment

Uh oh!

staskus commented Oct 18, 2023

Home Widgets (System Small)

Lock Screen Widgets (Accessory Rectangular)

Uh oh!

staskus commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guarani commented Jan 2, 2024

Uh oh!

staskus commented Jan 2, 2024

Uh oh!

mokagio left a comment

Choose a reason for hiding this comment

Uh oh!

staskus commented Jan 3, 2024

Uh oh!

staskus commented Jan 5, 2024

Uh oh!

mokagio Jan 8, 2024

Choose a reason for hiding this comment

Uh oh!

staskus Jan 8, 2024

Choose a reason for hiding this comment

Uh oh!

mokagio Jan 8, 2024

Choose a reason for hiding this comment

Uh oh!

mokagio Jan 8, 2024

Choose a reason for hiding this comment

Uh oh!

staskus Jan 8, 2024

Choose a reason for hiding this comment

Uh oh!

mokagio commented Jan 8, 2024

Uh oh!

wpmobilebot commented Jan 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mokagio Jan 8, 2024

Choose a reason for hiding this comment

Uh oh!

staskus Jan 8, 2024

Choose a reason for hiding this comment

Uh oh!

mokagio left a comment

Choose a reason for hiding this comment

iPhone

iPad

Uh oh!

staskus commented Jan 8, 2024

Uh oh!

mokagio commented Jan 8, 2024

Uh oh!

staskus commented Jan 8, 2024

Uh oh!

guarani commented Jan 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

staskus commented Oct 9, 2023 •

edited

Loading

wpmobilebot commented Oct 9, 2023 •

edited

Loading

staskus commented Oct 19, 2023 •

edited

Loading

wpmobilebot commented Jan 8, 2024 •

edited

Loading