-
Notifications
You must be signed in to change notification settings - Fork 121
Product Preview: Add WebKitViewController to handle authenticated sessions #7974
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
Conversation
You can test the changes from this Pull Request by:
|
a714aa2 to
bfa55f8
Compare
Generated by 🚫 dangerJS |
| let button = UIBarButtonItem(image: .gridicon(.cross), style: .plain, target: self, action: #selector(WebKitViewController.close)) | ||
| button.title = NSLocalizedString("webKit.button.dismiss", value: "Dismiss", comment: "Verb. Dismiss the web view screen.") | ||
| return button | ||
| }() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we don't need toolbar on the WebView yet (it's disabled by secureInteraction option), I decided to keep the (almost) full functionality. We can improve the preview feature by adding "device" button, already supported on WP-iOS with PreviewWebKitViewController subclass:
| mobile | desktop |
|---|---|
![]() |
![]() |
|
I tested this on my WP.com test store but unfortunately the preview isn't working for me there. I'm getting redirected to the WP.com login screen: Should this be working for all WP.com-hosted sites, including sites with custom domains? (The domain is the big difference I see with my test store.) |
|
@rachelmcr can you try the flow on the device? I have the same issue with simulator, but never on the device. Sorry I forgot to mention it in the description. For the reference, I've traced the issue to these lines: woocommerce-ios/WooCommerce/Classes/Authentication/WebAuth/AuthenticationService.swift Lines 188 to 189 in 5e67a0c
On the device this code correctly combines response cookies from 2 requests.
From the proxy log there is no difference in networking requests/responses. |
|
Aha, thanks! I ran into an odd issue on my device at first, where the (I lost track of the specific console error when that happened, but it was a problem with the web view trying to load the request But when I relaunched the app that seemed to correct itself, and now the previews are working consistently. Not sure if that's something worth digging into further, but thought I'd mention it. I'll finish reviewing the code now! |
rachelmcr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After I resolved the issues mentioned above, everything tested well here! 👍
Given the amount of code here that's the same between WPiOS and WCiOS, do you think it's worth refactoring it into a shared repo (maybe the WordPressUI pod)? (Not necessarily right now, but I'm thinking about future maintenance.)
It happens when the old
Do you think hiding "preview" button for empty
Agree! I considered moving this to WPAuthenticator library (also WP app comment), but it will require refactoring of both client apps and migration of additional dependencies. We can do it after we finish and validate this feature. Maybe we'll reuse it in other parts of the app and will have additional requirements by that time. |
Agreed, I think just checking for an empty
Sounds good! I missed that WP comment before but it sums up the effort pretty well. :) |




Closes #7911, #7946.
Description
This PR adds
WebKitViewControllerthat uses access token for current user to authenticate them in the context ofWKWebView.Most of the code copied from WordPress-iOS.
Testing
Test on the device, as iOS Simulator has a bug around cookies management.
Video
RPReplay_Final1667395250.mp4
RELEASE-NOTES.txtif necessary.