Conversation
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
|
👋 Howdy! We are freezing |
I can confirm that this does appear to fix #15296 💯 |
|
Hi @aerych, thanks a lot for the PR! Here's the results of my testing. There are some issues that came up and while I haven't been able to dig into the errors individually, I hope this is still useful. A question first: Should we also be checking the Reader? As far as I know it uses previews which might be affected by this change.
Simple WP.com site mapped domain (Public)While this worked on iPhone, on an iPad I consistently got an "Invalid key" error: Atomic site (Public)Jetpack-connected self-hosted site (Public) |
diegoreymendez
left a comment
There was a problem hiding this comment.
I tested this following the instructions and found no issues.
I added a few comments to the code, which I'd suggest to at least consider. But since none of those changes really affect the functionality of this PR, I'm approving it.
WordPress/Classes/Utility/Networking/RequestAuthenticator.swift
Outdated
Show resolved
Hide resolved
WordPress/Classes/Utility/Networking/RequestAuthenticator.swift
Outdated
Show resolved
Hide resolved
WordPress/Classes/Utility/Networking/RequestAuthenticator.swift
Outdated
Show resolved
Hide resolved
Thanks! I check this and WordPress.com login was ON, but I tried again and now it works :D, so I think we can consider this solved.
TIL 👍
Shared details on a DM.
Good point, I tested this by adding associating a new Jurassic Ninja site to my WP.com in a Chrome tab, and then logging into my same WP.com account in a different browser (to ensure no cookies were brought across) and indeed I'm not "logged in" when viewing a published post from that Jurassic Ninja site.
I agree, I'm not familiar with r-login but this seems to match the Calypso experience.
Yeah, I agree that the Visit-in-web-view feature of Reader should have the same functionality as the View Site option. I'll give it a check too in my next round of tests for this PR. |
I'm speculating that the different results we're seeing might be due to being proxied when testing. I've been testing unproxied, but noticed when I proxied I could reproduce the issue you saw on the iPad. Assuming our proxy was the issue, and since it shouldn't be a factor for others, maybe this is fine? |
Great idea, that seems to be it. Unproxied and it worked. |
|
After chatting with @designsimply about it we'd like to target 16.2 for this branch. I'll re-target and confirm there are no issues. Currently, we're double checking some behavior with a like button that never loads. We'll confirm if it's another regression or existing behavior, then decide to merge or not. |
guarani
left a comment
There was a problem hiding this comment.
Testing build 37971 on an iPhone (unproxied), the key takeaways are:
- No web views worked in my tests for Jetpack-connected self-hosted sites (i.e. the web views showed the content but were not logged in)
- Visiting a private Simple WP.com site with a mapped domain, the Reader showed a login screen
- The Reader web view is often not logged in for comments and likes (it is often "fixed" by going to View Site first and then coming back to Reader)
- A "Loading..." message is often shown instead of the like button (and fails to resolve)
- In all tests of draft previews, the app (incorrectly) lets you submit a comment but then (correctly) rejects it with an error message
Questions:
- Are there any downsides to the
frame-noncequery param being included in URLs when sharing links from inside the web views?
| View Post | Draft Preview | View Site | Reader | |
|---|---|---|---|---|
| Simple WP.com site (Public) | ✅ | The UI incorrectly lets me write a comment (it's a draft, comments shouldn't be allowed) but attempting to submit the comment is correctly blocked | ✅ | ✅ |
| Simple WP.com site (Private) | ✅ | The UI incorrectly lets me write a comment (it's a draft, comments shouldn't be allowed) but attempting to submit the comment is correctly blocked | ✅ | ✅ |
| Simple WP.com site mapped domain (Public) | The Like button takes me to mobile Safari where I'm shown a login page | The UI incorrectly lets me write a comment (it's a draft, comments shouldn't be allowed) but attempting to submit the comment is correctly blocked | ✅ | - - The Like button is shown but tapping it presents and dismisses a modal popup and the Like action is not completed |
| Simple WP.com site mapped domain (Private) | A "Loading..." message is shown instead of the like button (it fails to load) | The UI incorrectly lets me write a comment (it's a draft, comments shouldn't be allowed) but attempting to submit the comment is correctly blocked | ✅ | ❌ The web view is not logged in (going to View Site and coming back doesn't fix the issue) |
| Atomic site (Public) | - The frame-nonce query param is included in URLs when sharing links from inside the web views - The Like button takes me to mobile Safari where I'm shown a login page |
The UI incorrectly lets me write a comment (it's a draft, comments shouldn't be allowed) but attempting to submit the comment is correctly blocked | ✅ | - - The Like button is shown but tapping it presents and dismisses a modal popup and the Like action is not completed |
| Atomic site (Private) | - The frame-nonce query param is included in URLs when sharing links from inside the web views - A "Loading..." message is shown instead of the like button (it fails to load) |
The UI incorrectly lets me write a comment (it's a draft, comments shouldn't be allowed) but attempting to submit the comment is correctly blocked | A "Loading..." message is shown instead of the like button (it fails to load) | - - A "Loading..." message is shown instead of the like button (it fails to load) |
| Jetpack-connected self-hosted site (Public) | - - The frame-nonce query param is included in URLs when sharing links from inside the web views |
|
You're an amazing tester @guarani :) Like button opens browser
Like button shows loading
Reader logged out
Draft Previews:
Jetpack-connected self-hosted
I've pushed changes for the reader auth (which won't be perfect but should help) and for the frame-nonce. One last peek? |
The Like button works for me (tested in View Post on a public Simple WP.com site with a mapped domain) using 15.9.0.2 (the latest 15.9 build on TestFlight), maybe there's something we're doing slightly different. It could still be considered a minor issue, let me know if you'd still prefer we tackle this is a separate issue (it looks similar to the existing issue #13431).
Again this works for me on 15.9 (15.9.0.2), so I'm not sure if we're taking the exact same steps. I used a private Simple WP.com site with a mapped domain and was able to see the Like button load.
I agree that this is not a regression, I created #15340 to track this.
Sounds good to me 👍
I created #15342 to track this.
I'll test that shortly and reply back here. |
Ah! I'd checked out the release/15.9 branch and was testing with it in the simulator. There's once difference. :) |
Circling back to this, I can confirm that the frame nonce is no longer showing up on share links from web views 👍. As we discussed offline, the issues mentioned in the Reader column in #15309 (review) are unchanged. |
…nto issues/15266-fix-preview-url
|
After realizing that some of the different behaviors we were seeing while testing was due to whether we tested with the existing 15.9 internal beta, or with a debug build (device or simulator) I shared what we were seeing internally. See pbArwn-1p6-p2. Props @koke for catching that the internal build of 15.9 was likely built against the iOS 13 sdk and that changes related to App Bound Domains and Intelligent Tracking Prevention starting in iOS 14 could explain what we were seeing as others had reported some similar difficulty with their apps. To summarize: Our consensus is that ITP is the likely explanation for the behavior differences and we're content to go ahead and merge the changes in this branch. Even without the changes merged in 16.0, the previous behavior in 15.9 would have been impacted by ITP in iOS 14/ Xcode 12. With this in mind we're comfortable saying there's likely not a regression introduced in this patch. That said, we'll merge this to develop targeting 16.3 for additional time to test and refine if needed. cc @guarani |
There was a problem hiding this comment.
Thanks @aerych! After testing the latest build 38302 on an iPhone 11 (iOS 14), I think we can say that:
- Simple sites without a mapped domain are working great 💯
- The biggest issue with Simple sites that use a mapped domain is that if the site is private, they fail to load in the Reader's web view
- Similarly, the biggest issue with Atomic sites are with private sites also, they too fail to load in the Reader's web view
- All other issues seem lower priority (and marked are with a
⚠️ below)
I'm approving these changes, although this feels has gotten way more complex than first thought. Do you still think we should merge and then address the other issues separately?
| View Post | Draft Preview | View Site | Reader | "Read the source post" from pingback notifications | |
|---|---|---|---|---|---|
| Simple WP.com site (Public) | ✅ | ✅ | ✅ | ✅ | ✅ |
| Simple WP.com site (Private) | ✅ | ✅ | ✅ | ✅ | not applicable (pingbacks don't apply to private sites AFAIK) |
| Simple WP.com site mapped domain (Public) | ✅ | ||||
| Simple WP.com site mapped domain (Private) | ✅ | ❌ Unable to access web view | not applicable (pingbacks don't apply to private sites AFAIK) | ||
| Atomic site (Public) | ✅ | Pingback doesn't work | |||
| Atomic site (Private) | ✅ | ❌ Unable to access web view | not applicable (pingbacks don't apply to private sites AFAIK) | ||
| Jetpack-connected self-hosted site (Public) | ✅ | Reader doesn't work with Jurassic Ninja sites: Automattic/wp-calypso#47289 | Pingback doesn't work |
Breakdown
View Post and View Site
The different errors I've come across are:
- Scenario "Unable to like posts": If I try to like a post, mobile Safari is opened to an unauthenticated web view.
- Scenario "The like button fails to load": On private simple sites the text "Loading..." is shown instead of the Like button. On private Atomic sites both the Like button and the star button show "Loading..."
- Scenario "Unable to like or star" (only affects public Atomic sites): Tapping the like or star buttons either opens and closes a modal web view OR opens mobile Safari
- Scenario "The web view is not logged in": The main problem with this is that every time the user views their own site "View Site" or post "View Post", their viewing stats for that page or site get incremented
Reader
- In the "Unable to like or comment on this type of site" scenario above, an unauthenticated web view is shown. There are no known workarounds for this.
- In the "❌ Unable to access web view" scenario above, a "Protected Blog" screen is shown for private Simple sites and a "This site is private" screen is shown for private Atomic sites.
- There appears to be an bug that prevents the Reader from following Jurassic Ninja sites (it might be a backend issue so I've linked to a Calypso issue there)
Pingback
-
After receiving a pingback notification, users might often wish to "like" or comment on the source post from within the web view — however, in the scenarios marked with a
⚠️ above, tapping on the "Read the source post" button shows an unauthenticated web view:Workaround: Go to the Reader, follow the source site (if not already following it), locate the source post and like or comment using the app's native like and comment functionality (i.e. not using the web view).
-
Pingbacks don't appear to work on Atomic and Jetpack sites, I'm not sure yet why (#15387).
I do. Even with the shortcomings remaining with likes, etc. identified above, this patch is still an improvement over the current situation. If we're correct in thinking the remaining issues are related to ITP changes in WebKit between iOS SDK versions, any additional improvements are likely to be larger in scope. It seems better to make incremental improvements now than delay until there is a comprehensive solution. Thanks @guarani !! |
|
Opting to forgo any thing added to release notes given there are still things we want to address elsewhere. |








Fixes #15266
Refs #14220
This PR changes the way the app handles previews to resolve the issue reported in #15266 and potentially mitigates the issue reported in #15296 but needs confirmation. The changes include:
To test:
Perform the following tests
Using the following site configurations:
Ping me if you need a site to test with. I have examples of them all :)
There's a lot to cover with these checks so maybe a couple of folks could help out. @guarani , @diegoreymendez would y'all be able to take a peek?
PR submission checklist:
RELEASE-NOTES.txtif necessary.