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

fix!: Deprecate(/Remove) scrollView category extension on UIView #1400

Merged
merged 1 commit into from
Aug 24, 2024

Conversation

dpogue
Copy link
Member

@dpogue dpogue commented Feb 21, 2024

Platforms affected

iOS

Motivation and Context

As mentioned in #1399, this API was added as a compatibility shim at the time we supported both UIWebView and WKWebView, when WKWebView did not expose a public scrollView property. That property was made available in iOS 8, and this extension now risks causing problems due to being applied globally to all UIView subclasses.

Closes #1399.

Description

Deprecate the category extension to expose a scrollView property on all UIView object instances, and remove it entirely from visibility to Swift.

Testing

Checklist

  • I've run the tests to see all new and existing tests pass
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)

@dpogue dpogue added this to the 8.0.0 milestone Feb 21, 2024
@codecov-commenter
Copy link

codecov-commenter commented Feb 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.31%. Comparing base (3a426fd) to head (609170a).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1400   +/-   ##
=======================================
  Coverage   78.31%   78.31%           
=======================================
  Files          16       16           
  Lines        1826     1826           
=======================================
  Hits         1430     1430           
  Misses        396      396           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dpogue dpogue marked this pull request as ready for review February 27, 2024 16:27
@dpogue
Copy link
Member Author

dpogue commented Aug 14, 2024

This causes issues for the statusbar plugin which relies on this to access the scroll view without knowing the specific type of the web view implementation.

So putting this on hold for the moment.

@dpogue dpogue marked this pull request as draft August 14, 2024 07:53
dpogue added a commit to dpogue/cordova-plugin-statusbar that referenced this pull request Aug 16, 2024
Currently this works because of a category extension that adds a
`scrollView` method to every UIView instance, but that causes issues for
SwiftUI so we want to remove that extension from cordova-ios.

Since we do need to be able to look up the scrollView in this plugin, we
can just define a private local method that does the same thing in a way
that doesn't pollute global UIKit classes.

Ref: apache/cordova-ios#1400
dpogue added a commit to dpogue/cordova-plugin-statusbar that referenced this pull request Aug 16, 2024
Currently this works because of a category extension that adds a
`scrollView` method to every UIView instance, but that causes issues for
SwiftUI so we want to remove that extension from cordova-ios.

Since we do need to be able to look up the scrollView in this plugin, we
can just define a private local method that does the same thing in a way
that doesn't pollute global UIKit classes.

Ref: apache/cordova-ios#1400
dpogue added a commit to dpogue/cordova-plugin-statusbar that referenced this pull request Aug 16, 2024
Currently this works because of a category extension that adds a
`scrollView` method to every UIView instance, but that causes issues for
SwiftUI so we want to remove that extension from cordova-ios.

Since we do need to be able to look up the scrollView in this plugin, we
can just define a private local method that does the same thing in a way
that doesn't pollute global UIKit classes.

Ref: apache/cordova-ios#1400
dpogue added a commit to apache/cordova-plugin-statusbar that referenced this pull request Aug 20, 2024
Currently this works because of a category extension that adds a
`scrollView` method to every UIView instance, but that causes issues for
SwiftUI so we want to remove that extension from cordova-ios.

Since we do need to be able to look up the scrollView in this plugin, we
can just define a private local method that does the same thing in a way
that doesn't pollute global UIKit classes.

Ref: apache/cordova-ios#1400
@dpogue
Copy link
Member Author

dpogue commented Aug 20, 2024

Status Bar plugin is fixed, but this API is used in a number of 3rd party plugins as well... I feel like our only option is going to be deprecation instead of removal (which doesn't actually address the issue that was raised about this) 😞

@jcesarmobile
Copy link
Member

I don’t see what’s the problem with introducing a breaking change.
Changing the template to swift is also breaking to some plugins and it’s merged.

@dpogue dpogue changed the title refactor!: Remove scrollView category extension on UIView fix!: Deprecate(/Remove) scrollView category extension on UIView Aug 21, 2024
@dpogue
Copy link
Member Author

dpogue commented Aug 21, 2024

The best short term solution seems to be the following:

  • Remove the extension from being visible to Swift code. This solves the problem raised in Please remove org_apache_cordova_UIView_Extension because it isn't necessary since iOS 8.  #1399
    • This breaks any Swift plugins that use this API, but there seem to only be a few and they are not maintained and don't have high download counts
  • Mark the Objective C method as deprecated so that existing 3rd party plugins that call it will see warnings
  • Prepare to remove this entirely in Cordova iOS 9 as per the deprecation

Re: breaking plugins due to moving the template to Swift, we tested and added compatible headers so that existing plugins should continue working even with the new template. It's possible we missed some edge cases, but we're definitely trying to keep existing plugins working (with lots of noisy deprecation warnings)

@dpogue dpogue marked this pull request as ready for review August 21, 2024 02:25
This **removes** the API from Swift to solve the immediate problem in apache#1399
but leaves it available and deprecated in Objective-C due to use in 3rd party
plugins. (There are only 2 Swift plugins that use this API as far as I can
tell, and neither of them have very high usage or ongoing maintenance.)

Closes apacheGH-1399.
Copy link
Member

@erisu erisu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I tested and confirmed that Obj-C based plugins with self.webView.scrollView will continue to work as expected and displays the deprecation warning.

Swift based plugins will need to update to continue working, which is expected.

var webViewScrollView: UIScrollView? {
    let scrollViewSelector = NSSelectorFromString("scrollView")
    
    if webView.responds(to: scrollViewSelector) {
        return webView.perform(scrollViewSelector)?.takeUnretainedValue() as? UIScrollView
    }
    
    return nil
}

@dpogue
Copy link
Member Author

dpogue commented Aug 24, 2024

The equivalent Objective C code would be (copied from the statusbar plugin):

- (UIScrollView *)webViewScrollView
{
    SEL scrollViewSelector = NSSelectorFromString(@"scrollView");

    if ([self.webView respondsToSelector:scrollViewSelector]) {
        return ((id (*)(id, SEL))objc_msgSend)(self.webView, scrollViewSelector);
    }

    return nil;
}

@dpogue dpogue merged commit 39fbf29 into apache:master Aug 24, 2024
10 checks passed
@dpogue dpogue deleted the scrollview-cleanup branch August 24, 2024 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Please remove org_apache_cordova_UIView_Extension because it isn't necessary since iOS 8.
4 participants