Fix 'contentEdgeInsets' was deprecated in iOS 15.0 #8094

iamgabrielma · 2022-11-11T04:14:15Z

Description

By updating the minimum release version to iOS 15 recently, we've seen some deprecation warnings. This PR fixes the case for 'contentEdgeInsets' was deprecated in iOS 15.0: This property is ignored when using UIButtonConfiguration, for some cases.

Changes

Updated the following classes to use UIButton.Configuration contentInsets

Added titleLabel to subview for ButtonActivityIndicator due to an edge case encountered while testing: Using the new configuration approach made the app crash (and test fail) when we went through the card present payment flow if the titleLabel required to use multiline (pe: small screens, different languages, bigger system accessibility fonts, ... ). This is because we are calling pinSubViewsToAllEdgeMargins(_:) while label and button do not share the same ancestor for that specific case, getting this error:

'Unable to activate constraint with anchors <NSLayoutXAxisAnchor:0x600003f55700 "UILayoutGuide:0x600001334460'UIViewLayoutMarginsGuide'.leading"> and <NSLayoutXAxisAnchor:0x600003f55740 "UILabel:0x155d4f9a0.leading"> because they have no common ancestor.  
Does the constraint or its anchors reference items in different view hierarchies?  That's illegal.'

Testing instructions

Go to the following views and confirm that buttons appear correctly:

Go to Orders > Filter
Orders > For different Orders confirm that primary ("Mark Order as Completed") and secondary buttons ( "Issue Refund") appear correctly, as well as the "Add Customer Note" button
Menu > Reviews > Confirm that spam, trash, approve, and reply buttons
Menu > Settings > Send Feedback > Confirm the "Contact us here" button after feedback is sent

Screenshots

UIButton+Helpers	CustomerNoteTableViewCell	FilteredOrdersHeaderBar

SurveySubmittedViewController	VerticalButton

wpmobilebot · 2022-11-11T04:27:48Z

You can test the changes from this Pull Request by:

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

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

By using UIButton.Configuration, calls to pinSubviewToAllEdgeMargins() make the app crash, as is illegal to activate constraints with anchors in different view hierarchies. By commenting this out temporarily, while buttons doesn’t look great if they have multiple lines, we avoid the app crash.

WooCommerce/Classes/Extensions/UIButton+Helpers.swift

By adding the UILabel as subview to the UIButton explicitely we avoid a crash on pinSubviewToAllEdgeMargins(_:) due not sharing a common ancestor.

ealeksandrov

Thanks for working on this @iamgabrielma!
I've checked all impacted button types and it looks good!

The only misplaced button is "add customer" - it's missing padding between the image and the text label, but I see it's already fixed in #8129.

Added titleLabel to subview for ButtonActivityIndicator

There is a new subviews.contains check in the enableMultipleLines() helper. Shouldn't it already prevent the crash in ButtonActivityIndicator? Can you help me reproduce the issue that requires adding custom initializer?

ealeksandrov · 2022-11-17T08:19:21Z

WooCommerce/Classes/Extensions/UIButton+Helpers.swift

        static let defaultBorderWidth = CGFloat(1.0)
-        static let defaultEdgeInsets = UIEdgeInsets(top: 12, left: 22, bottom: 12, right: 22)
+        static let defaultVerticalInsets = CGFloat(12)
+        static let defaultHorizontalInsets = CGFloat(22)


nit: since now it's a single value instead of UIEdgeInsets type, should we rename both constants to something like Inset or Padding?

Definitely! That reads much better. Changed on a6df43f along with the other nits 🙇

ealeksandrov · 2022-11-17T09:17:49Z

WooCommerce/Classes/ViewRelated/Survey/SurveySubmittedViewController.swift

        contactUsButton.applyLinkButtonStyle()
        contactUsButton.titleLabel?.applyCalloutStyle()
-        contactUsButton.contentEdgeInsets = .zero
+        var contactUsconfiguration = UIButton.Configuration.plain()


nit:

Suggested change

var contactUsconfiguration = UIButton.Configuration.plain()

var contactUsConfiguration = UIButton.Configuration.plain()

ealeksandrov · 2022-11-17T09:19:53Z

WooCommerce/Classes/ViewRelated/ReusableViews/VerticalButton.swift

    enum Settings {
        static let cornerRadius     = CGFloat(10)
-        static let edgeInsets       = UIEdgeInsets(top: 5, left: 0, bottom: 5, right: 0)
+        static let horizontalEdgeInsets = CGFloat(5)


nit: same as above, renaming single value to Inset or Padding can improve readability

iamgabrielma · 2022-11-18T01:33:51Z

There is a new subviews.contains check in the enableMultipleLines() helper. Shouldn't it already prevent the crash in ButtonActivityIndicator? Can you help me reproduce the issue that requires adding custom initializer?

Thanks for the review! You're right, it does prevent the crash at this point 🤔 However, views that use ButtonActivityIndicator as button (like ButtonTableViewCell) will still not have their label added to the parent view, which I think could cause problems in the future and these don't have to be necessarily catch on compile time but when is already out there.

Would you say is best to remove the initializer from ButtonActivityIndicator? I have no strong opinion about it.

ealeksandrov

However, views that use ButtonActivityIndicator as button (like ButtonTableViewCell) will still not have their label added to the parent view, which I think could cause problems in the future and these don't have to be necessarily catch on compile time but when is already out there.

The question is: what problems can arise from that? The code we're discussing is about pinning the button's frame to the text. So breaking it can lead to the text overflow out of the button's bounds. I don't see any cases of that and wasn't able to reproduce it in IssueRefundViewController with ButtonActivityIndicator.

On other hand, manipulating with UIButton view hierarchy can bite us back. It's not defined in the documentation, so Apple is free to change it in any updates. And the titleLabel is read-only:

Although this property is read-only, its own properties are read/write. Use these properties primarily to configure the text of the button.

So I'm leaning towards removing custom init and keeping an eye on custom buttons to make sure our helpers don't break. When we find broken buttons, then we can work on case-by-case basis, fixing those helpers/subclasses/views.

What do you think?

peril-woocommerce · 2022-11-23T10:31:09Z

	Warnings
⚠️	This PR is assigned to a milestone which is closing in less than 2 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

iamgabrielma · 2022-11-23T10:31:17Z

So I'm leaning towards removing custom init and keeping an eye on custom buttons to make sure our helpers don't break. When we find broken buttons, then we can work on case-by-case basis, fixing those helpers/subclasses/views.

Thanks for the explanation, that makes sense 🙇 Init removed on db0bd1d

ealeksandrov

Thank you for the significant refactoring! 🚀

Use UIButton.Configuration on CustomerNote cell

4f2f048

iamgabrielma added type: enhancement A request for an enhancement. status: draft This is a draft, still need more work but can be reviewed and commented if asked. iOS 15 Specific to iOS 15 labels Nov 11, 2022

iamgabrielma added this to the 11.3 milestone Nov 11, 2022

Update applyLinkButtonStyle() with UIButton.Configuration

2e3a9e8

iamgabrielma added 8 commits November 11, 2022 13:28

Update edgeInsets SurveySubmittedViewController

13ebacd

Update edgeInsets applySecondaryButtonStyle

f5f359b

Update paymentsModalCancelButtonStyle edgeInsets

4a66349

Use Button Configuration on applying button style

3fb5e8f

Revert testing button backgrounds

84eba80

Add Configuration to applyLinkButtonStyle

5450244

revert testing button configuration

028beb1

iamgabrielma commented Nov 15, 2022

View reviewed changes

WooCommerce/Classes/Extensions/UIButton+Helpers.swift Outdated Show resolved Hide resolved

iamgabrielma added 8 commits November 15, 2022 15:08

Add Button Configuration to VerticalButton

fa02ba8

Add ButtonConfiguration to EmptyStateViewController

ad6678a

Add ButtonConfiguration to CustomerNoteTableViewCell

bc3c27e

Add ButtonConfiguration to FilteredOrdersHeaderBar

7490c9f

Add Configuration to SurveySubmittedViewController

1a40698

Add label as subview to UIButton

dba89ac

By adding the UILabel as subview to the UIButton explicitely we avoid a crash on pinSubviewToAllEdgeMargins(_:) due not sharing a common ancestor.

Remove unnecessary var

cf68aa2

Add label as subview on UIActivityIndicatorView

87d67d8

iamgabrielma removed the status: draft This is a draft, still need more work but can be reviewed and commented if asked. label Nov 16, 2022

iamgabrielma requested a review from ealeksandrov November 16, 2022 01:40

Change CustomerNote button configuration to plain

3fb678b

iamgabrielma mentioned this pull request Nov 16, 2022

Fix 'imageEdgeInsets' was deprecated in iOS 15.0 #8129

Merged

ealeksandrov reviewed Nov 17, 2022

View reviewed changes

iamgabrielma modified the milestones: 11.3, 11.4 Nov 18, 2022

Rename constants for readability

a6df43f

iamgabrielma requested a review from ealeksandrov November 18, 2022 02:48

ealeksandrov reviewed Nov 22, 2022

View reviewed changes

Removed custom init from ButtonActivityIndicator

db0bd1d

ealeksandrov approved these changes Nov 23, 2022

View reviewed changes

iamgabrielma merged commit 742ca9c into trunk Nov 24, 2022

iamgabrielma deleted the task/contentedgeinsets-deprecated-ios-15 branch November 24, 2022 01:07

This was referenced Dec 5, 2022

iOS 15 deprecation warnings: UIButtonConfiguration #7881

Open

"Enter site address" login text overflows button for long strings #8360

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix 'contentEdgeInsets' was deprecated in iOS 15.0 #8094

Fix 'contentEdgeInsets' was deprecated in iOS 15.0 #8094

Uh oh!

iamgabrielma commented Nov 11, 2022 •

edited

Loading

Uh oh!

wpmobilebot commented Nov 11, 2022 •

edited

Loading

Uh oh!

Uh oh!

ealeksandrov left a comment

Uh oh!

ealeksandrov Nov 17, 2022

Uh oh!

iamgabrielma Nov 18, 2022

Uh oh!

ealeksandrov Nov 17, 2022

Uh oh!

ealeksandrov Nov 17, 2022

Uh oh!

iamgabrielma commented Nov 18, 2022

Uh oh!

ealeksandrov left a comment

Uh oh!

peril-woocommerce bot commented Nov 23, 2022

Uh oh!

iamgabrielma commented Nov 23, 2022

Uh oh!

ealeksandrov left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	var contactUsconfiguration = UIButton.Configuration.plain()
	var contactUsConfiguration = UIButton.Configuration.plain()

Fix 'contentEdgeInsets' was deprecated in iOS 15.0 #8094

Fix 'contentEdgeInsets' was deprecated in iOS 15.0 #8094

Uh oh!

Conversation

iamgabrielma commented Nov 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Testing instructions

Screenshots

Uh oh!

wpmobilebot commented Nov 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ealeksandrov left a comment

Choose a reason for hiding this comment

Uh oh!

ealeksandrov Nov 17, 2022

Choose a reason for hiding this comment

Uh oh!

iamgabrielma Nov 18, 2022

Choose a reason for hiding this comment

Uh oh!

ealeksandrov Nov 17, 2022

Choose a reason for hiding this comment

Uh oh!

ealeksandrov Nov 17, 2022

Choose a reason for hiding this comment

Uh oh!

iamgabrielma commented Nov 18, 2022

Uh oh!

ealeksandrov left a comment

Choose a reason for hiding this comment

Uh oh!

peril-woocommerce bot commented Nov 23, 2022

Uh oh!

iamgabrielma commented Nov 23, 2022

Uh oh!

ealeksandrov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iamgabrielma commented Nov 11, 2022 •

edited

Loading

wpmobilebot commented Nov 11, 2022 •

edited

Loading