Fix accessibility issues in Private DNS Settings

- Keep the Save button enabled at all times

- Show error in the Hostname view to remind the user
  - "The field is required" error
  - "The hostname you typed isn't valid" error

Bug: 386323822
Flag: EXEMPT bugfix
Test: Manual testing
atest -c PrivateDnsModeDialogPreferenceTest \
         PrivateDnsPreferenceControllerTest

Change-Id: I63973bd5001b838d7f27827e6a6d4ac96ac78ca9

res/layout/private_dns_mode_dialog.xml

@@ -16,8 +16,10 @@
 
 <ScrollView
     xmlns:android="http://schemas.android.com/apk/res/android"
+    xmlns:app="http://schemas.android.com/apk/res-auto"
     android:layout_width="match_parent"
-    android:layout_height="wrap_content">
+    android:layout_height="wrap_content"
+    android:theme="@style/Theme.AppCompat.DayNight">
 
     <LinearLayout
         android:layout_width="match_parent"
@@ -43,17 +45,24 @@
                 android:id="@+id/private_dns_mode_provider"
                 layout="@layout/preference_widget_dialog_radiobutton"/>
 
-            <EditText
+            <com.google.android.material.textfield.TextInputLayout
-                android:id="@+id/private_dns_mode_provider_hostname"
+                android:id="@+id/private_dns_mode_provider_hostname_layout"
-                android:hint="@string/private_dns_mode_provider_hostname_hint"
-                style="@android:style/Widget.CompoundButton.RadioButton"
-                android:imeOptions="actionDone"
-                android:inputType="textFilter|textUri|textMultiLine"
                 android:layout_width="match_parent"
                 android:layout_height="wrap_content"
+                android:hint="@string/private_dns_title"
+                android:theme="@style/Theme.Settings"
+                app:endIconMode="clear_text"
+                app:errorEnabled="true">
+
+                <com.google.android.material.textfield.TextInputEditText
+                    android:id="@+id/private_dns_mode_provider_hostname"
+                    android:layout_width="match_parent"
+                    android:layout_height="wrap_content"
+                    android:imeOptions="actionDone"
+                    android:inputType="textFilter|textUri|textMultiLine"
                     android:layout_marginStart="3dp"
-                android:layout_marginEnd="8dp"
+                    android:layout_marginEnd="8dp"/>
-                android:minHeight="@dimen/developer_option_dialog_min_height"/>
+            </com.google.android.material.textfield.TextInputLayout>
         </RadioGroup>
 
         <include

res/values/strings.xml

@@ -2937,6 +2937,12 @@
     <string name="emergency_address_title">Emergency address</string>
     <!-- Summary of Update Emergency Address preference, explaining usage of emergency address [CHAR LIMIT=NONE] -->
     <string name="emergency_address_summary">Used as your location when you make an emergency call over Wi\u2011Fi</string>
+    <!-- Title of a preference for private DNS provider hostname [CHAR LIMIT=40] -->
+    <string name="private_dns_title">Hostname</string>
+    <!-- Message of private dns hostname that the field is required. [CHAR LIMIT=NONE] -->
+    <string name="private_dns_field_require">The field is required</string>
+    <!-- The error if the private dns hostname is not valid -->
+    <string name="private_dns_hostname_invalid">The hostname you typed isn\u2019t valid</string>
     <!-- Message of private dns that provides a help link. [CHAR LIMIT=NONE] -->
     <string name="private_dns_help_message"><annotation id="url">Learn more</annotation> about Private DNS features</string>
     <!-- Message to display when private dns is on. [CHAR LIMIT=10] -->

src/com/android/settings/network/PrivateDnsModeDialogPreference.java

@@ -23,14 +23,12 @@ import static com.android.settingslib.RestrictedLockUtils.EnforcedAdmin;
 
 import android.app.settings.SettingsEnums;
 import android.content.ActivityNotFoundException;
-import android.content.ContentResolver;
 import android.content.Context;
 import android.content.DialogInterface;
 import android.content.Intent;
 import android.net.ConnectivitySettingsManager;
 import android.os.UserHandle;
 import android.os.UserManager;
-import android.provider.Settings;
 import android.text.Editable;
 import android.text.TextWatcher;
 import android.text.method.LinkMovementMethod;
@@ -55,6 +53,7 @@ import com.android.settingslib.HelpUtils;
 import com.android.settingslib.RestrictedLockUtils;
 import com.android.settingslib.RestrictedLockUtilsInternal;
 
+import com.google.android.material.textfield.TextInputLayout;
 import com.google.common.net.InternetDomainName;
 
 import java.util.HashMap;
@@ -64,7 +63,7 @@ import java.util.Map;
  * Dialog to set the Private DNS
  */
 public class PrivateDnsModeDialogPreference extends CustomDialogPreferenceCompat implements
-        DialogInterface.OnClickListener, RadioGroup.OnCheckedChangeListener, TextWatcher {
+        RadioGroup.OnCheckedChangeListener, TextWatcher {
 
     public static final String ANNOTATION_URL = "url";
 
@@ -80,16 +79,9 @@ public class PrivateDnsModeDialogPreference extends CustomDialogPreferenceCompat
     }
 
     @VisibleForTesting
-    static final String MODE_KEY = Settings.Global.PRIVATE_DNS_MODE;
+    TextInputLayout mHostnameLayout;
     @VisibleForTesting
-    static final String HOSTNAME_KEY = Settings.Global.PRIVATE_DNS_SPECIFIER;
+    EditText mHostnameText;
-
-    public static String getHostnameFromSettings(ContentResolver cr) {
-        return Settings.Global.getString(cr, HOSTNAME_KEY);
-    }
-
-    @VisibleForTesting
-    EditText mEditText;
     @VisibleForTesting
     RadioGroup mRadioGroup;
     @VisibleForTesting
@@ -136,22 +128,17 @@ public class PrivateDnsModeDialogPreference extends CustomDialogPreferenceCompat
             // by the controller.
             holder.itemView.setEnabled(true);
         }
+
+        setSaveButtonListener();
     }
 
     @Override
     protected void onBindDialogView(View view) {
         final Context context = getContext();
-        final ContentResolver contentResolver = context.getContentResolver();
-
         mMode = ConnectivitySettingsManager.getPrivateDnsMode(context);
-
-        mEditText = view.findViewById(R.id.private_dns_mode_provider_hostname);
-        mEditText.addTextChangedListener(this);
-        mEditText.setText(getHostnameFromSettings(contentResolver));
-
         mRadioGroup = view.findViewById(R.id.private_dns_radio_group);
-        mRadioGroup.setOnCheckedChangeListener(this);
         mRadioGroup.check(PRIVATE_DNS_MAP.getOrDefault(mMode, R.id.private_dns_mode_opportunistic));
+        mRadioGroup.setOnCheckedChangeListener(this);
 
         // Initial radio button text
         final RadioButton offRadioButton = view.findViewById(R.id.private_dns_mode_off);
@@ -163,6 +150,13 @@ public class PrivateDnsModeDialogPreference extends CustomDialogPreferenceCompat
         final RadioButton providerRadioButton = view.findViewById(R.id.private_dns_mode_provider);
         providerRadioButton.setText(com.android.settingslib.R.string.private_dns_mode_provider);
 
+        mHostnameLayout = view.findViewById(R.id.private_dns_mode_provider_hostname_layout);
+        mHostnameText = view.findViewById(R.id.private_dns_mode_provider_hostname);
+        if (mHostnameText != null) {
+            mHostnameText.setText(ConnectivitySettingsManager.getPrivateDnsHostname(context));
+            mHostnameText.addTextChangedListener(this);
+        }
+
         final TextView helpTextView = view.findViewById(R.id.private_dns_help_info);
         helpTextView.setMovementMethod(LinkMovementMethod.getInstance());
         final Intent helpIntent = HelpUtils.getHelpIntent(context,
@@ -176,22 +170,8 @@ public class PrivateDnsModeDialogPreference extends CustomDialogPreferenceCompat
         } else {
             helpTextView.setText("");
         }
-    }
 
-    @Override
+        updateDialogInfo();
-    public void onClick(DialogInterface dialog, int which) {
-        if (which == DialogInterface.BUTTON_POSITIVE) {
-            final Context context = getContext();
-            if (mMode == PRIVATE_DNS_MODE_PROVIDER_HOSTNAME) {
-                // Only clickable if hostname is valid, so we could save it safely
-                ConnectivitySettingsManager.setPrivateDnsHostname(context,
-                        mEditText.getText().toString());
-            }
-
-            FeatureFactory.getFeatureFactory().getMetricsFeatureProvider().action(context,
-                    SettingsEnums.ACTION_PRIVATE_DNS_MODE, mMode);
-            ConnectivitySettingsManager.setPrivateDnsMode(context, mMode);
-        }
     }
 
     @Override
@@ -241,24 +221,58 @@ public class PrivateDnsModeDialogPreference extends CustomDialogPreferenceCompat
         return getEnforcedAdmin() != null;
     }
 
-    private Button getSaveButton() {
-        final AlertDialog dialog = (AlertDialog) getDialog();
-        if (dialog == null) {
-            return null;
-        }
-        return dialog.getButton(DialogInterface.BUTTON_POSITIVE);
-    }
-
     private void updateDialogInfo() {
         final boolean modeProvider = PRIVATE_DNS_MODE_PROVIDER_HOSTNAME == mMode;
-        if (mEditText != null) {
+        if (mHostnameLayout != null) {
-            mEditText.setEnabled(modeProvider);
+            mHostnameLayout.setEnabled(modeProvider);
-        }
+            mHostnameLayout.setErrorEnabled(false);
-        final Button saveButton = getSaveButton();
-        if (saveButton != null) {
-            saveButton.setEnabled(modeProvider
-                    ? InternetDomainName.isValid(mEditText.getText().toString())
-                    : true);
         }
     }
+
+    private void setSaveButtonListener() {
+        View.OnClickListener onClickListener = v -> doSaveButton();
+        DialogInterface.OnShowListener onShowListener = dialog -> {
+            if (dialog == null) {
+                Log.e(TAG, "The DialogInterface is null!");
+                return;
+            }
+            Button saveButton = ((AlertDialog) dialog).getButton(DialogInterface.BUTTON_POSITIVE);
+            if (saveButton == null) {
+                Log.e(TAG, "Can't get the save button!");
+                return;
+            }
+            saveButton.setOnClickListener(onClickListener);
+        };
+        setOnShowListener(onShowListener);
+    }
+
+    @VisibleForTesting
+    void doSaveButton() {
+        Context context = getContext();
+        if (mMode == PRIVATE_DNS_MODE_PROVIDER_HOSTNAME) {
+            if (mHostnameLayout == null || mHostnameText == null) {
+                Log.e(TAG, "Can't find hostname resources!");
+                return;
+            }
+            if (mHostnameText.getText().isEmpty()) {
+                mHostnameLayout.setError(context.getString(R.string.private_dns_field_require));
+                Log.w(TAG, "The hostname is empty!");
+                return;
+            }
+            if (!InternetDomainName.isValid(mHostnameText.getText().toString())) {
+                mHostnameLayout.setError(context.getString(R.string.private_dns_hostname_invalid));
+                Log.w(TAG, "The hostname is invalid!");
+                return;
+            }
+
+            ConnectivitySettingsManager.setPrivateDnsHostname(context,
+                    mHostnameText.getText().toString());
+        }
+
+        ConnectivitySettingsManager.setPrivateDnsMode(context, mMode);
+
+        FeatureFactory.getFeatureFactory().getMetricsFeatureProvider()
+                .action(context, SettingsEnums.ACTION_PRIVATE_DNS_MODE, mMode);
+        getDialog().dismiss();
+    }
 }

src/com/android/settings/network/PrivateDnsPreferenceController.java

@@ -135,7 +135,7 @@ public class PrivateDnsPreferenceController extends BasePreferenceController
                                 com.android.settingslib.R.string.private_dns_mode_opportunistic);
             case PRIVATE_DNS_MODE_PROVIDER_HOSTNAME:
                 return dnsesResolved
-                        ? PrivateDnsModeDialogPreference.getHostnameFromSettings(cr)
+                        ? ConnectivitySettingsManager.getPrivateDnsHostname(mContext)
                         : res.getString(
                                 com.android.settingslib.R.string.private_dns_mode_provider_failure);
         }

tests/robotests/src/com/android/settings/network/PrivateDnsModeDialogPreferenceTest.java

@@ -21,14 +21,12 @@ import static android.net.ConnectivitySettingsManager.PRIVATE_DNS_MODE_OPPORTUNI
 import static android.net.ConnectivitySettingsManager.PRIVATE_DNS_MODE_PROVIDER_HOSTNAME;
 
 import static com.google.common.truth.Truth.assertThat;
-import static com.google.common.truth.Truth.assertWithMessage;
 
 import static org.mockito.Mockito.anyInt;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
 import android.content.Context;
-import android.content.DialogInterface;
 import android.net.ConnectivitySettingsManager;
 import android.view.LayoutInflater;
 import android.view.View;
@@ -88,31 +86,31 @@ public class PrivateDnsModeDialogPreferenceTest {
     }
 
     @Test
-    public void testOnCheckedChanged_dnsModeOff_disableEditText() {
+    public void onCheckedChanged_dnsModeOff_disableHostnameText() {
         mPreference.onCheckedChanged(null, R.id.private_dns_mode_off);
 
         assertThat(mPreference.mMode).isEqualTo(PRIVATE_DNS_MODE_OFF);
-        assertThat(mPreference.mEditText.isEnabled()).isFalse();
+        assertThat(mPreference.mHostnameText.isEnabled()).isFalse();
     }
 
     @Test
-    public void testOnCheckedChanged_dnsModeOpportunistic_disableEditText() {
+    public void onCheckedChanged_dnsModeOpportunistic_disableHostnameText() {
         mPreference.onCheckedChanged(null, R.id.private_dns_mode_opportunistic);
 
         assertThat(mPreference.mMode).isEqualTo(PRIVATE_DNS_MODE_OPPORTUNISTIC);
-        assertThat(mPreference.mEditText.isEnabled()).isFalse();
+        assertThat(mPreference.mHostnameText.isEnabled()).isFalse();
     }
 
     @Test
-    public void testOnCheckedChanged_dnsModeProvider_enableEditText() {
+    public void onCheckedChanged_dnsModeProvider_enableHostnameText() {
         mPreference.onCheckedChanged(null, R.id.private_dns_mode_provider);
 
         assertThat(mPreference.mMode).isEqualTo(PRIVATE_DNS_MODE_PROVIDER_HOSTNAME);
-        assertThat(mPreference.mEditText.isEnabled()).isTrue();
+        assertThat(mPreference.mHostnameText.isEnabled()).isTrue();
     }
 
     @Test
-    public void testOnBindDialogView_containsCorrectData() {
+    public void onBindDialogView_containsCorrectData() {
         // Don't set settings to the default value ("opportunistic") as that
         // risks masking failure to read the mode from settings.
         ConnectivitySettingsManager.setPrivateDnsMode(mContext, PRIVATE_DNS_MODE_OFF);
@@ -123,57 +121,74 @@ public class PrivateDnsModeDialogPreferenceTest {
                 new LinearLayout(mContext), false);
         mPreference.onBindDialogView(view);
 
-        assertThat(mPreference.mEditText.getText().toString()).isEqualTo(HOST_NAME);
+        assertThat(mPreference.mHostnameText.getText().toString()).isEqualTo(HOST_NAME);
         assertThat(mPreference.mRadioGroup.getCheckedRadioButtonId()).isEqualTo(
                 R.id.private_dns_mode_off);
     }
 
     @Test
-    public void testOnCheckedChanged_switchMode_saveButtonHasCorrectState() {
+    public void doSaveButton_changeToOffMode_saveData() {
-        final String[] INVALID_HOST_NAMES = new String[] {
+        // Set the default settings to OPPORTUNISTIC
-                INVALID_HOST_NAME,
+        ConnectivitySettingsManager.setPrivateDnsMode(mContext, PRIVATE_DNS_MODE_OPPORTUNISTIC);
-                "2001:db8::53",  // IPv6 string literal
-                "192.168.1.1",   // IPv4 string literal
-        };
 
-        for (String invalid : INVALID_HOST_NAMES) {
+        mPreference.mMode = PRIVATE_DNS_MODE_OFF;
-            // Set invalid hostname
+        mPreference.doSaveButton();
-            mPreference.mEditText.setText(invalid);
-
-            mPreference.onCheckedChanged(null, R.id.private_dns_mode_off);
-            assertWithMessage("off: " + invalid).that(mSaveButton.isEnabled()).isTrue();
-
-            mPreference.onCheckedChanged(null, R.id.private_dns_mode_opportunistic);
-            assertWithMessage("opportunistic: " + invalid).that(mSaveButton.isEnabled()).isTrue();
-
-            mPreference.onCheckedChanged(null, R.id.private_dns_mode_provider);
-            assertWithMessage("provider: " + invalid).that(mSaveButton.isEnabled()).isFalse();
-        }
-    }
-
-    @Test
-    public void testOnClick_positiveButtonClicked_saveData() {
-        // Set the default settings to OFF
-        ConnectivitySettingsManager.setPrivateDnsMode(mContext, PRIVATE_DNS_MODE_OFF);
-
-        mPreference.mMode = PRIVATE_DNS_MODE_OPPORTUNISTIC;
-        mPreference.onClick(null, DialogInterface.BUTTON_POSITIVE);
 
         // Change to OPPORTUNISTIC
-        assertThat(ConnectivitySettingsManager.getPrivateDnsMode(mContext)).isEqualTo(
+        assertThat(ConnectivitySettingsManager.getPrivateDnsMode(mContext))
-                PRIVATE_DNS_MODE_OPPORTUNISTIC);
+                .isEqualTo(PRIVATE_DNS_MODE_OFF);
     }
 
     @Test
-    public void testOnClick_negativeButtonClicked_doNothing() {
+    public void doSaveButton_changeToOpportunisticMode_saveData() {
         // Set the default settings to OFF
         ConnectivitySettingsManager.setPrivateDnsMode(mContext, PRIVATE_DNS_MODE_OFF);
 
         mPreference.mMode = PRIVATE_DNS_MODE_OPPORTUNISTIC;
-        mPreference.onClick(null, DialogInterface.BUTTON_NEGATIVE);
+        mPreference.doSaveButton();
 
-        // Still equal to OFF
+        // Change to OPPORTUNISTIC
-        assertThat(ConnectivitySettingsManager.getPrivateDnsMode(mContext)).isEqualTo(
+        assertThat(ConnectivitySettingsManager.getPrivateDnsMode(mContext))
-                PRIVATE_DNS_MODE_OFF);
+                .isEqualTo(PRIVATE_DNS_MODE_OPPORTUNISTIC);
+    }
+
+    @Test
+    public void doSaveButton_changeToProviderHostnameMode_saveData() {
+        // Set the default settings to OFF
+        ConnectivitySettingsManager.setPrivateDnsMode(mContext, PRIVATE_DNS_MODE_OFF);
+
+        mPreference.onCheckedChanged(null, R.id.private_dns_mode_provider);
+        mPreference.mHostnameText.setText(HOST_NAME);
+        mPreference.doSaveButton();
+
+        // Change to PROVIDER_HOSTNAME
+        assertThat(ConnectivitySettingsManager.getPrivateDnsMode(mContext))
+                .isEqualTo(PRIVATE_DNS_MODE_PROVIDER_HOSTNAME);
+        assertThat(ConnectivitySettingsManager.getPrivateDnsHostname(mContext))
+                .isEqualTo(HOST_NAME);
+    }
+
+    @Test
+    public void doSaveButton_providerHostnameIsEmpty_setHostnameError() {
+        ConnectivitySettingsManager.setPrivateDnsMode(mContext, PRIVATE_DNS_MODE_PROVIDER_HOSTNAME);
+        ConnectivitySettingsManager.setPrivateDnsHostname(mContext, HOST_NAME);
+        mPreference.onCheckedChanged(null, R.id.private_dns_mode_provider);
+
+        mPreference.mHostnameText.setText("");
+        mPreference.doSaveButton();
+
+        assertThat(mPreference.mHostnameLayout.isErrorEnabled()).isTrue();
+    }
+
+    @Test
+    public void doSaveButton_providerHostnameIsInvalid_setHostnameError() {
+        ConnectivitySettingsManager.setPrivateDnsMode(mContext, PRIVATE_DNS_MODE_PROVIDER_HOSTNAME);
+        ConnectivitySettingsManager.setPrivateDnsHostname(mContext, HOST_NAME);
+        mPreference.onCheckedChanged(null, R.id.private_dns_mode_provider);
+
+        mPreference.mHostnameText.setText(INVALID_HOST_NAME);
+        mPreference.doSaveButton();
+
+        assertThat(mPreference.mHostnameLayout.isErrorEnabled()).isTrue();
     }
 }