Fix Site Address UI Bugs When Displaying In-Page Errors

[shiki]

Part of fixing woocommerce/woocommerce-ios#4034.

There is a linked WP PR for this: wordpress-mobile/WordPress-iOS#16655.

Originally intended to fix just WooCommerce but apparently, WP has the same issue. So this fixes both apps. 🤸

Findings

Both apps have this problem when logging in with a site address. This is for WordPress specifically:

1. Log in with a valid self-hosted site address domain. Don't include the scheme. Add a typo.
2. After the error is displayed, fix the typo. Still exclude the scheme.
3. Continue logging in, providing the valid credentials. The app will fail to log the user in. The WordPressOrgXMLRPCValidator.guessXMLRPCURLForSite fails because we did not provide the scheme.

This problem only surfaces when the user made a mistake on the first try. Which is quite likely. It also happens if there was previously an XML-RPC error. Or any other error that is displayed below the site address text field.

Here is a video showing this bug:

wp-before.mov

If there was no error on the first try, the app is able to log the user in because we automatically fix the siteAddress.

You'll also notice in the video that the text disappears when the error is displayed. And the Continue button also does not show the activity indicator anymore if the user tries a second time. It looks like the app is doing nothing.

Solution

Logging In

For some reason, if the error is displayed at some point, the loginFields.siteAddress gets reset because this callback gets called:

WordPressAuthenticator-iOS/WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewController.swift

Lines 312 to 315 in 45d02a7

	cell.onChangeSelectionHandler = { [weak self] textfield in
	self?.loginFields.siteAddress = textfield.nonNilTrimmedText()
	self?.configureSubmitButton()
	}

I still have no idea why that text field change callback is called. But I fixed it by resetting loginFields.siteAddress before any view controller is displayed. See here for the fix. Let me know if there's a better way for this.

Disappearing Text

I fixed the disappearing text by resetting the value when the cell is recreated in configureTextField:

WordPressAuthenticator-iOS/WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewController.swift

Line 311 in aa020aa

cell.textField.text = loginFields.siteAddress

Activity Indicator

The reason the activity indicator on the button does not show up when the user retries because this block gets called right after the activity indicator is shown:

WordPressAuthenticator-iOS/WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewController.swift

Line 309 in 26e7de7

self?.configureSubmitButton(animating: false)

So the activity indicator gets dismissed immediately.

I fixed this by keeping the “view is loading” state in a variable:

WordPressAuthenticator-iOS/WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewController.swift

Lines 22 to 29 in aa020aa

	/// A state variable that is `true` if network calls are currently happening and so the
	/// view should be showing a loading indicator.
	///
	/// This should only be modified within `configureViewLoading(_ loading:)`.
	///
	/// This state is mainly used in `configureSubmitButton()` to determine whether the button
	/// should show an activity indicator.
	private var viewIsLoading: Bool = false

And making sure that the configureSubmitButton uses that state so the activity indicator is not unintentionally dismissed:

WordPressAuthenticator-iOS/WordPressAuthenticator/Unified Auth/View Related/Site Address/SiteAddressViewController.swift

Lines 94 to 98 in aa020aa

	/// Configures the appearance and state of the submit button.
	///
	/// Use this instead of the overridden `configureSubmitButton(animating:)` since this uses the
	/// _current_ `viewIsLoading` state.
	private func configureSubmitButton() {

Let me know if there's a better way to do this. I wasn't quite sure how to properly handle this. I wanted to use a didSet handler but it looks like configureViewLoading is an override from the parent class. So I couldn't just remove it.

Here is the final result:

wp-after.mov

Testing

1. You can use the branch in Fix Site Address UI Bugs When Displaying In-Page Errors WordPress-iOS#16655.
2. Log in with a valid self-hosted site address domain. Don't include the scheme. Add a typo.
3. After the error is displayed, fix the typo. Still exclude the scheme.
4. Press Continue.
5. Confirm that the activity indicator is visible and the text field is disabled.
6. Provide the credentials.
7. Confirm that you were able to log in.

[shiki]

I removed this because this was preventing the text field from being disabled when the user tapped Continue for the second time. There are no implications in removing this, right? 😅

[ScoutHarris]

There doesn't appear to be a side effect. It looks like this was there to clean up setting the siteURLField as first responder, but your change seems to have made no difference. Sooooo 👍 .

[ScoutHarris]

I still have no idea why that text field change callback is called

The idea is to enable/disable the Continue button on the fly as the user enters the URL. What's supposed to happen is Continue is disabled until something resembling a URL is entered. But that's not happening. It's enabled as soon as the first character is entered. It looks like the validateSiteForSignin method isn't doing what we're expecting it to do, specifically the call to idnEncodedURL.

    func validateSiteForSignin() -> Bool {
        guard let url = URL(string: NSURL.idnEncodedURL(siteAddress)) else {
            return false
        }

        return !url.absoluteString.isEmpty
    }

I checked internal, and it's doing the same thing, so this definitely pre-dates your PR. But that's why that callback is there.

[ScoutHarris]

I know this isn't your doing (it appears to be mine 🤦 ), but there is a print("🔴 SAVC > fetchSiteInfo") in SiteAddressViewController here. Would you be a dear and remove it please?

[ScoutHarris]

LGTM!

[shiki]

It looks like the validateSiteForSignin method isn't doing what we're expecting it to do, specifically the call to idnEncodedURL.

Ah, thanks for the explanation!

Would you be a dear and remove it please?

Since you asked nicely... removed in d6447e7. 🤣

---

Thank you very much for the review, @ScoutHarris!