Fix/10930 email error dissapears on rotation - continued

[jd-alexander]

Fixes #11003

Findings

This PR is a continuation of #11003 where the email error wasn't being preserved when a configuration change took place.

Testing

1. Press Log In
2. Enter an invalid email address and press Next. An error message will be shown
3. Rotate the screen.
4. Notice the error is still shown.

Reviewing

1. Only 1 reviewer is needed but anyone can review.
2. Once approved, close Issue/10930 email address error is preserved on rotation #11003

Submitter Checklist

• I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.
• I have considered adding accessibility improvements for my changes.
• If it's feasible, I have added unit tests.

[peril-wordpress-mobile]

Messages

📖

This PR contains changes in the subtree libs/login/. It is your responsibility to ensure these changes are merged back into wordpress-mobile/WordPress-Login-Flow-Android. Follow these handy steps! WARNING: Make sure your git version is 2.19.x or lower - there is currently a bug in later versions that will corrupt the subtree history! cd WordPress-Android git checkout fix/10930-email-error-dissapears-on-rotation git subtree push --prefix=libs/login/ https://github.com/wordpress-mobile/WordPress-Login-Flow-Android.git merge/WordPress-Android/11537 Browse to https://github.com/wordpress-mobile/WordPress-Login-Flow-Android/pull/new/merge/WordPress-Android/11537 and open a new PR.

Generated by 🚫 dangerJS

[peril-wordpress-mobile]

You can test the changes on this Pull Request by downloading the APK here.

[ashiagr]

👋@jd-alexander 😊! I tested your changes, code-wise it looks good and it should have worked but it seems Android behaves differently 😖and TextWatcher's onTextChanged() is called again when phone orientation is changed which alters the behaviour.

If it might help, tested on
Pixel 3a, API level 29
Pixel 3, API level 27

[jd-alexander]

Thanks @ashiagr will check it out! Yeah, The onTextChanged threw it off. I am thinking if adding that is really necessary. Being that when you rotate and there was an error, it reminds you that you need to enter a correct email. I will think some more about it.

[jd-alexander]

I removed the clearing of the mIsValidEmail 3eb51e8 Let me know what you think. Do you think it has to be preserved for all states?

[ashiagr]

I removed the clearing of the mIsValidEmail

Thanks @jd-alexander !

I was wondering if we can delay adding text changed listener till the screen renders after rotation as described here while retaining mIsValidEmail inside onTextChanged. How about something like this?

mEmailInput.getViewTreeObserver().addOnGlobalLayoutListener(new ViewTreeObserver.OnGlobalLayoutListener() {
    @Override public void onGlobalLayout() {
        mEmailInput.getViewTreeObserver().removeOnGlobalLayoutListener(this);
        mEmailInput.addTextChangedListener(LoginEmailFragment.this);
    }
});

This way we can avoid redisplay of error text on rotation once it is cleared after text change.

Also since we're trying to preserve email address error, I was thinking to also preserve another email error email_not_registered_wpcom. We can create a new issue if you feel so.

[jd-alexander]

I was wondering if we can delay adding text changed listener till the screen renders after rotation as described here while retaining mIsValidEmail inside onTextChanged. How about something like this?

Nice @ashiagr Knew you would have something cool I didn't remember 😉

I used .post() instead and it works the same. Let me know if that's fine 26cef97

Also since we're trying to preserve email address error, I was thinking to also preserve another email error email_not_registered_wpcom. We can create a new issue if you feel so.

Makes sense! Good eye 😄 Since we are here right now, I just decided to support all the email errors in this class.
Done in 709c35e

[ashiagr]

Perfect @jd-alexander 🎉! LGTM 🚢.

P.S. As discussed on slack, we should also push the changes to the login lib repo.

[hypest]

👋 @jd-alexander , does this PR also address this other ticket? #10930. Let's close it if it does, thanks!

[jd-alexander]

Yes, it does @hypest Closing!