From b3938a0244548272f504a6fd5693cdd6595bd182 Mon Sep 17 00:00:00 2001 From: Weng Su Date: Thu, 13 Feb 2025 10:48:25 +0800 Subject: [PATCH] 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 | 29 +++-- res/values/strings.xml | 6 + .../PrivateDnsModeDialogPreference.java | 116 ++++++++++-------- .../PrivateDnsPreferenceController.java | 2 +- .../PrivateDnsModeDialogPreferenceTest.java | 105 +++++++++------- 5 files changed, 151 insertions(+), 107 deletions(-) diff --git a/res/layout/private_dns_mode_dialog.xml b/res/layout/private_dns_mode_dialog.xml index 96ebd2c3955..bee949a906e 100644 --- a/res/layout/private_dns_mode_dialog.xml +++ b/res/layout/private_dns_mode_dialog.xml @@ -16,8 +16,10 @@ + android:layout_height="wrap_content" + android:theme="@style/Theme.AppCompat.DayNight"> - + android:hint="@string/private_dns_title" + android:theme="@style/Theme.Settings" + app:endIconMode="clear_text" + app:errorEnabled="true"> + + + Emergency address Used as your location when you make an emergency call over Wi\u2011Fi + + Hostname + + The field is required + + The hostname you typed isn\u2019t valid Learn more about Private DNS features diff --git a/src/com/android/settings/network/PrivateDnsModeDialogPreference.java b/src/com/android/settings/network/PrivateDnsModeDialogPreference.java index 3b99777720a..fc517ac4495 100644 --- a/src/com/android/settings/network/PrivateDnsModeDialogPreference.java +++ b/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; - - public static String getHostnameFromSettings(ContentResolver cr) { - return Settings.Global.getString(cr, HOSTNAME_KEY); - } - - @VisibleForTesting - EditText mEditText; + EditText mHostnameText; @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 - 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); - } + updateDialogInfo(); } @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) { - mEditText.setEnabled(modeProvider); - } - final Button saveButton = getSaveButton(); - if (saveButton != null) { - saveButton.setEnabled(modeProvider - ? InternetDomainName.isValid(mEditText.getText().toString()) - : true); + if (mHostnameLayout != null) { + mHostnameLayout.setEnabled(modeProvider); + mHostnameLayout.setErrorEnabled(false); } } + + 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(); + } } diff --git a/src/com/android/settings/network/PrivateDnsPreferenceController.java b/src/com/android/settings/network/PrivateDnsPreferenceController.java index 21e4926f490..291ce3307fd 100644 --- a/src/com/android/settings/network/PrivateDnsPreferenceController.java +++ b/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); } diff --git a/tests/robotests/src/com/android/settings/network/PrivateDnsModeDialogPreferenceTest.java b/tests/robotests/src/com/android/settings/network/PrivateDnsModeDialogPreferenceTest.java index 458000292df..c1811915a90 100644 --- a/tests/robotests/src/com/android/settings/network/PrivateDnsModeDialogPreferenceTest.java +++ b/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() { - final String[] INVALID_HOST_NAMES = new String[] { - INVALID_HOST_NAME, - "2001:db8::53", // IPv6 string literal - "192.168.1.1", // IPv4 string literal - }; + public void doSaveButton_changeToOffMode_saveData() { + // Set the default settings to OPPORTUNISTIC + ConnectivitySettingsManager.setPrivateDnsMode(mContext, PRIVATE_DNS_MODE_OPPORTUNISTIC); - for (String invalid : INVALID_HOST_NAMES) { - // Set invalid hostname - 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); + mPreference.mMode = PRIVATE_DNS_MODE_OFF; + mPreference.doSaveButton(); // Change to OPPORTUNISTIC - assertThat(ConnectivitySettingsManager.getPrivateDnsMode(mContext)).isEqualTo( - PRIVATE_DNS_MODE_OPPORTUNISTIC); + assertThat(ConnectivitySettingsManager.getPrivateDnsMode(mContext)) + .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 - assertThat(ConnectivitySettingsManager.getPrivateDnsMode(mContext)).isEqualTo( - PRIVATE_DNS_MODE_OFF); + // Change to OPPORTUNISTIC + assertThat(ConnectivitySettingsManager.getPrivateDnsMode(mContext)) + .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(); } }