Fix PageInfo UX issues

Set android:includeFontPadding="false" to better align favicon with URL.
Use padding instead of margin to get the right background color.
Show details link if there is a connection message.
Show red warning triangle if there is a connection security issue.
Also includes some general cleanup for margins.

Bug: 1135124
Change-Id: Iff1f880b51a2a8bd60ef0606afec03fea82e9318
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2449372Reviewed-by: Ehimare Okoyomon <eokoyomon@chromium.org>
Commit-Queue: Christian Dullweber <dullweber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#813785}

chrome/android/javatests/src/org/chromium/chrome/browser/page_info/PageInfoViewTest.java

@@ -15,8 +15,6 @@ import static androidx.test.espresso.matcher.ViewMatchers.withText;
 
 import static org.hamcrest.CoreMatchers.allOf;
 import static org.hamcrest.CoreMatchers.containsString;
-import static org.hamcrest.Matchers.instanceOf;
-import static org.hamcrest.Matchers.is;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
 
@@ -25,7 +23,6 @@ import static org.chromium.components.content_settings.PrefNames.COOKIE_CONTROLS
 
 import android.os.Build;
 import android.view.View;
-import android.widget.Button;
 
 import androidx.test.filters.MediumTest;
 
@@ -404,7 +401,7 @@ public class PageInfoViewTest {
         onView(withId(R.id.page_info_permissions_row)).perform(click());
         // Clear permissions in page info.
         onView(withText("Reset permissions")).perform(click());
-        onView(allOf(is(instanceOf(Button.class)), withText("Reset permissions"))).perform(click());
+        onView(withText("Reset")).perform(click());
         // Wait until the UI navigates back and check permissions are reset.
         onViewWaiting(allOf(withId(R.id.page_info_row_wrapper), isDisplayed()));
         // Make sure that the permission section is gone because there are no longer exceptions.

components/browser_ui/site_settings/android/java/src/org/chromium/components/browser_ui/site_settings/SingleWebsiteSettings.java

@@ -1038,12 +1038,12 @@ public class SingleWebsiteSettings extends SiteSettingsPreferenceFragment
         int confirmationResId = mHideNonPermissionPreferences
                 ? R.string.page_info_permissions_reset_confirmation
                 : R.string.website_reset_confirmation;
-        preference.setTitle(titleResId);
+        int buttonResId = mHideNonPermissionPreferences ? R.string.reset : titleResId;
         // Handle the Clear & Reset preference click by showing a confirmation.
         new AlertDialog.Builder(getActivity(), R.style.Theme_Chromium_AlertDialog)
                 .setTitle(titleResId)
                 .setMessage(confirmationResId)
-                .setPositiveButton(titleResId,
+                .setPositiveButton(buttonResId,
                         (dialog, which) -> {
                             if (mHideNonPermissionPreferences) {
                                 mSiteDataCleaner.resetPermissions(

components/page_info/android/java/res/layout/page_info_container.xml

@@ -14,28 +14,38 @@
     android:background="@color/sheet_bg_color"
     android:orientation="vertical">
 
-    <TextView
-        android:id="@+id/page_info_truncated_url"
-        android:layout_width="wrap_content"
-        android:layout_height="wrap_content"
-        android:layout_gravity="center_horizontal"
-        android:layout_marginVertical="16dp"
-        android:ellipsize="end"
-        android:lineSpacingExtra="6dp"
-        android:textAlignment="center"
-        android:textAppearance="@style/TextAppearance.TextLarge.Primary" />
-
-    <view
-        android:id="@+id/page_info_url"
-        class="org.chromium.components.page_info.PageInfoView$ElidedUrlTextView"
+    <LinearLayout
+        android:id="@+id/page_info_url_wrapper"
         android:layout_width="match_parent"
         android:layout_height="wrap_content"
-        android:ellipsize="end"
-        android:lineSpacingExtra="6dp"
-        android:paddingVertical="16dp"
-        android:textAlignment="center"
-        android:textAppearance="@style/TextAppearance.TextLarge.Primary"
-        android:visibility="gone" />
+        android:minHeight="48dp"
+        android:gravity="center"
+        android:paddingVertical="@dimen/page_info_popup_padding_vertical"
+        android:paddingHorizontal="@dimen/page_info_popup_padding_sides">
+
+        <org.chromium.components.browser_ui.widget.text.TextViewWithCompoundDrawables
+            android:id="@+id/page_info_truncated_url"
+            android:layout_width="wrap_content"
+            android:layout_height="wrap_content"
+            android:drawablePadding="@dimen/page_info_popup_button_padding_sides"
+            android:includeFontPadding="false"
+            android:lineSpacingExtra="6dp"
+            android:textAppearance="@style/TextAppearance.TextLarge.Primary"
+            app:drawableHeight="@dimen/page_info_favicon_size"
+            app:drawableWidth="@dimen/page_info_favicon_size" />
+
+        <view
+            android:id="@+id/page_info_url"
+            class="org.chromium.components.page_info.PageInfoView$ElidedUrlTextView"
+            android:layout_width="match_parent"
+            android:layout_height="wrap_content"
+            android:ellipsize="end"
+            android:includeFontPadding="false"
+            android:lineSpacingExtra="6dp"
+            android:textAlignment="center"
+            android:textAppearance="@style/TextAppearance.TextLarge.Primary"
+            android:visibility="gone" />
+    </LinearLayout>
 
     <LinearLayout
         android:id="@+id/page_info_wrapper"
@@ -49,7 +59,7 @@
             android:layout_height="wrap_content"
             android:gravity="center_vertical"
             android:orientation="horizontal"
-            android:paddingHorizontal="16dp">
+            android:paddingHorizontal="@dimen/page_info_popup_padding_sides">
 
             <org.chromium.ui.widget.ChromeImageButton
                 android:id="@+id/subpage_back_button"
@@ -67,7 +77,7 @@
                 android:id="@+id/page_info_subpage_title"
                 android:layout_width="match_parent"
                 android:layout_height="wrap_content"
-                android:layout_marginVertical="12dp"
+                android:layout_marginVertical="@dimen/page_info_popup_padding_vertical"
                 android:textAppearance="@style/TextAppearance.TextLarge.Primary" />
         </LinearLayout>
 

components/page_info/android/java/res/layout/page_info_row.xml

@@ -14,9 +14,9 @@
     android:layout_height="wrap_content"
     android:background="?attr/selectableItemBackground"
     android:paddingHorizontal="@dimen/page_info_popup_padding_sides"
-    android:paddingVertical="12dp">
+    android:paddingVertical="@dimen/page_info_popup_padding_vertical">
 
-    <ImageView
+    <org.chromium.ui.widget.ChromeImageView
         android:id="@+id/page_info_row_icon"
         android:layout_width="wrap_content"
         android:layout_height="wrap_content"

components/page_info/android/java/res/layout/page_info_v2.xml

@@ -12,7 +12,7 @@
     android:layout_height="match_parent"
     android:background="@color/sheet_bg_color"
     android:orientation="vertical"
-    android:layout_marginBottom="8dp">
+    android:paddingBottom="8dp">
 
     <LinearLayout
         android:id="@+id/page_info_row_wrapper"
@@ -50,7 +50,7 @@
         android:layout_width="match_parent"
         android:layout_height="wrap_content"
         android:layout_marginHorizontal="@dimen/page_info_popup_padding_sides"
-        android:layout_marginTop="12dp"
+        android:layout_marginTop="@dimen/page_info_popup_padding_vertical"
         android:textAppearance="@style/TextAppearance.TextLarge.Primary"
         android:text="@string/page_info_preview_message"
         android:visibility="gone" />
@@ -59,7 +59,7 @@
         android:id="@+id/page_info_preview_load_original"
         android:layout_width="match_parent"
         android:layout_height="wrap_content"
-        android:layout_marginVertical="12dp"
+        android:layout_marginVertical="@dimen/page_info_popup_padding_vertical"
         android:layout_marginHorizontal="@dimen/page_info_popup_padding_sides"
         android:textAppearance="@style/TextAppearance.TextLarge.Primary"
         android:visibility="gone" />
@@ -68,7 +68,7 @@
         android:id="@+id/page_info_lite_mode_https_image_compression_message"
         android:layout_width="match_parent"
         android:layout_height="wrap_content"
-        android:layout_marginVertical="12dp"
+        android:layout_marginVertical="@dimen/page_info_popup_padding_vertical"
         android:layout_marginHorizontal="@dimen/page_info_popup_padding_sides"
         android:text="@string/page_info_lite_mode_https_image_compression"
         android:textAppearance="@style/TextAppearance.TextMedium.Primary"

components/page_info/android/java/res/values/dimens.xml

@@ -11,6 +11,7 @@
 
     <!-- Page Info Popup Dimensions -->
     <dimen name="page_info_popup_padding_sides">16dp</dimen>
+    <dimen name="page_info_popup_padding_vertical">12dp</dimen>
     <dimen name="page_info_popup_button_padding_sides">8dp</dimen>
     <dimen name="page_info_popup_permission_icon_size">24dp</dimen>
 </resources>
\ No newline at end of file

components/page_info/android/java/src/org/chromium/components/page_info/PageInfoConnectionController.java

@@ -8,7 +8,10 @@ import android.view.View;
 import android.view.ViewGroup;
 import android.widget.FrameLayout;
 
+import androidx.annotation.ColorRes;
+
 import org.chromium.components.omnibox.SecurityStatusIcon;
+import org.chromium.components.security_state.ConnectionSecurityLevel;
 import org.chromium.components.security_state.SecurityStateModel;
 import org.chromium.content_public.browser.WebContents;
 
@@ -57,17 +60,37 @@ public class PageInfoConnectionController
         mInfoView.onDismiss();
     }
 
+    private static @ColorRes int getSecurityIconColor(
+            @ConnectionSecurityLevel int securityLevel, boolean showDangerTriangleForWarningLevel) {
+        switch (securityLevel) {
+            case ConnectionSecurityLevel.DANGEROUS:
+                return R.color.default_text_color_error;
+            case ConnectionSecurityLevel.WARNING:
+                return showDangerTriangleForWarningLevel ? R.color.default_text_color_error : 0;
+            case ConnectionSecurityLevel.NONE:
+            case ConnectionSecurityLevel.SECURE_WITH_POLICY_INSTALLED_CERT:
+            case ConnectionSecurityLevel.SECURE:
+                return 0;
+            default:
+                assert false;
+                return 0;
+        }
+    }
+
     public void setConnectionInfo(PageInfoView.ConnectionInfoParams params) {
         PageInfoRowView.ViewParams rowParams = new PageInfoRowView.ViewParams();
         mTitle = params.summary != null ? params.summary.toString() : null;
         rowParams.title = mTitle;
-        rowParams.subtitle = params.message != null ? params.message.toString() : null;
+        rowParams.subtitle = params.message;
         rowParams.visible = rowParams.title != null || rowParams.subtitle != null;
         int securityLevel = SecurityStateModel.getSecurityLevelForWebContents(mWebContents);
-        rowParams.iconResId = SecurityStatusIcon.getSecurityIconResource(securityLevel,
-                SecurityStateModel.shouldShowDangerTriangleForWarningLevel(),
-                /*isSmallDevice=*/false,
-                /*skipIconForNeutralState=*/false);
+        boolean showTriangleForWarning =
+                SecurityStateModel.shouldShowDangerTriangleForWarningLevel();
+        rowParams.iconResId =
+                SecurityStatusIcon.getSecurityIconResource(securityLevel, showTriangleForWarning,
+                        /*isSmallDevice=*/false,
+                        /*skipIconForNeutralState=*/false);
+        rowParams.iconTint = getSecurityIconColor(securityLevel, showTriangleForWarning);
         if (params.clickCallback != null) rowParams.clickCallback = this::launchSubpage;
         mRowView.setParams(rowParams);
     }

components/page_info/android/java/src/org/chromium/components/page_info/PageInfoContainer.java

@@ -69,14 +69,14 @@ public class PageInfoContainer extends FrameLayout {
         mTruncatedUrlTitle = findViewById(R.id.page_info_truncated_url);
         mTruncatedUrlTitle.setText(params.truncatedUrl);
 
+        View urlWrapper = findViewById(R.id.page_info_url_wrapper);
+        urlWrapper.setVisibility(params.urlTitleShown ? VISIBLE : GONE);
+
         ChromeImageButton backButton = findViewById(R.id.subpage_back_button);
         backButton.setOnClickListener(v -> params.backButtonClickCallback.run());
     }
 
     private void initializeUrlView(View view, Params params) {
-        if (!params.urlTitleShown) {
-            view.setVisibility(GONE);
-        }
         if (params.urlTitleClickCallback != null) {
             view.setOnClickListener(v -> { params.urlTitleClickCallback.run(); });
         }
@@ -94,12 +94,6 @@ public class PageInfoContainer extends FrameLayout {
     }
 
     public void setFavicon(Drawable favicon) {
-        int padding =
-                getResources().getDimensionPixelSize(R.dimen.page_info_popup_button_padding_sides);
-        int size = getResources().getDimensionPixelSize(R.dimen.page_info_favicon_size);
-
-        favicon.setBounds(0, 0, size, size);
-        mTruncatedUrlTitle.setCompoundDrawablePadding(padding);
         mTruncatedUrlTitle.setCompoundDrawablesRelative(favicon, null, null, null);
     }
 

components/page_info/android/java/src/org/chromium/components/page_info/PageInfoController.java

@@ -413,7 +413,7 @@ public class PageInfoController implements PageInfoMainController, ModalDialogPr
             messageBuilder.append(details);
         }
 
-        if (!mIsV2Enabled && isConnectionDetailsLinkVisible()) {
+        if (isConnectionDetailsLinkVisible() && messageBuilder.length() > 0) {
             messageBuilder.append(" ");
             SpannableString detailsText =
                     new SpannableString(mContext.getString(R.string.details_link));

components/page_info/android/java/src/org/chromium/components/page_info/PageInfoRowView.java

@@ -5,16 +5,20 @@
 package org.chromium.components.page_info;
 
 import android.content.Context;
+import android.content.res.ColorStateList;
 import android.util.AttributeSet;
 import android.view.LayoutInflater;
 import android.widget.FrameLayout;
-import android.widget.ImageView;
 import android.widget.TextView;
 
+import androidx.annotation.ColorRes;
 import androidx.annotation.DrawableRes;
 import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
 
+import org.chromium.base.ApiCompatibilityUtils;
+import org.chromium.ui.widget.ChromeImageView;
+
 /**
  * View showing an icon, title and subtitle for a page info row.
  */
@@ -23,12 +27,13 @@ public class PageInfoRowView extends FrameLayout {
     public static class ViewParams {
         public boolean visible;
         public @DrawableRes int iconResId;
-        public String title;
-        public String subtitle;
+        public @ColorRes int iconTint;
+        public CharSequence title;
+        public CharSequence subtitle;
         public Runnable clickCallback;
     }
 
-    private final ImageView mIcon;
+    private final ChromeImageView mIcon;
     private final TextView mTitle;
     private final TextView mSubtitle;
 
@@ -43,6 +48,10 @@ public class PageInfoRowView extends FrameLayout {
     public void setParams(ViewParams params) {
         setVisibility(params.visible ? VISIBLE : GONE);
         mIcon.setImageResource(params.iconResId);
+        ApiCompatibilityUtils.setImageTintList(mIcon,
+                ColorStateList.valueOf(getResources().getColor(
+                        params.iconTint != 0 ? params.iconTint : R.color.default_icon_color)));
+
         mTitle.setText(params.title);
         mTitle.setVisibility(params.title != null ? VISIBLE : GONE);
         updateSubtitle(params.subtitle);
@@ -53,7 +62,7 @@ public class PageInfoRowView extends FrameLayout {
         }
     }
 
-    public void updateSubtitle(String subtitle) {
+    public void updateSubtitle(CharSequence subtitle) {
         mSubtitle.setText(subtitle);
         mSubtitle.setVisibility(subtitle != null ? VISIBLE : GONE);
     }