[SafetyCheck] Update text of aria label and header text

This CL aligns the text for the ARIA label of the entry point button and the header text with the UX design. Additionally, it fixes an issue with the composition of the CSS flexbox of the header. Bug: 1345920, 1374908 Change-Id: Ifd968cf8f0c429b1582ca1d24f95ecc946618ba3 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4040134 Reviewed-by: Side YILMAZ <sideyilmaz@chromium.org> Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org> Commit-Queue: Tom Van Goethem <tov@chromium.org> Reviewed-by: Theodore Olsauskas-Warren <sauski@google.com> Cr-Commit-Position: refs/heads/main@{#1075410}
chromium · Nov 24, 2022 · bd78ae5 · bd78ae5
1 parent d84d615
commit bd78ae5
Show file tree

Hide file tree

Showing 12 changed files with 55 additions and 59 deletions.
diff --git a/chrome/app/settings_strings.grdp b/chrome/app/settings_strings.grdp
@@ -2044,11 +2044,16 @@
   </message>
   <message name="IDS_SETTINGS_SAFETY_CHECK_REVIEW_NOTIFICATION_PERMISSIONS_PRIMARY_LABEL" desc="'Review Notification Permissions' shows the websites that send a lot of notifications. This label is the header of the module in the site settings notification page. The number represents the number of notification permissions to be reviewed.">
     {NUM_SITES, plural,
-     =1 {Review 1 site that recently sent a lot of notifications}
-     other {Review {NUM_SITES} sites that recently sent a lot of notifications}}
+     =1 {Review 1 site that sent a lot of notifications}
+     other {Review {NUM_SITES} sites that sent a lot of notifications}}
+  </message>
+  <message name="IDS_SETTINGS_SAFETY_CHECK_NOTIFICATION_PERMISSIONS_REVIEW_BUTTON_ARIA_LABEL" desc="'Review Notification Permissions' shows the websites that send a lot of notifications. This label is used for the ARIA label of the button of the module in the site settings notification page.">
+    Review notification permissions
   </message>
   <message name="IDS_SETTINGS_SAFETY_CHECK_REVIEW_NOTIFICATION_PERMISSIONS_SECONDARY_LABEL" desc="'Review Notification Permissions' shows the websites that send a lot of notifications. This label is the description of the module in site settings notification page.">
-    These sites sent a lot of notifications recently. You can stop them from sending future notifications.
+    {NUM_SITES, plural,
+     =1 {This site sent a lot of notifications recently. You can stop it from sending future notifications.}
+     other {These sites sent a lot of notifications recently. You can stop them from sending future notifications.}}
   </message>
   <message name="IDS_SETTINGS_SAFETY_CHECK_NOTIFICATION_PERMISSION_REVIEW_IGNORED_TOAST_LABEL" desc="'Review Notification Permissions' shows the websites that send a lot of notifications. This label is to indicate for which site the user has explicitly allowed permissions.">
     Notifications allowed for <ph name="SITE">$1<ex>www.example.com</ex></ph>
@@ -2236,7 +2241,7 @@
   <message name="IDS_SETTINGS_PERMISSIONS" desc="Name of the settings page which allows users to manage permissions and site content settings">
     Permissions and content settings
   </message>
-  <message name="IDS_SETTINGS_PERMISSIONS_DESCRIPTION" desc="Description of the controls available on the permisisons and site content settings page">
+  <message name="IDS_SETTINGS_PERMISSIONS_DESCRIPTION" desc="Description of the controls available on the permissions and site content settings page">
     Controls what information sites can use and show (location, camera, pop-ups, and more)
   </message>
   <message name="IDS_SETTINGS_SECURITY" desc="Name of the settings page which allows

diff --git a/...grdp/IDS_SETTINGS_SAFETY_CHECK_NOTIFICATION_PERMISSIONS_REVIEW_BUTTON_ARIA_LABEL.png.sha1 b/...grdp/IDS_SETTINGS_SAFETY_CHECK_NOTIFICATION_PERMISSIONS_REVIEW_BUTTON_ARIA_LABEL.png.sha1
@@ -0,0 +1 @@
+bf146724fac63bd8d116d4095bcab10b5d66e998
diff --git a/...ngs_grdp/IDS_SETTINGS_SAFETY_CHECK_REVIEW_NOTIFICATION_PERMISSIONS_PRIMARY_LABEL.png.sha1 b/...ngs_grdp/IDS_SETTINGS_SAFETY_CHECK_REVIEW_NOTIFICATION_PERMISSIONS_PRIMARY_LABEL.png.sha1
@@ -1 +1 @@
-2046a724d4a713393a02bf4fd77dc28d90fec4ae
+8b5b7c8d959bea2a8bf7d31243aba0d0466da239
diff --git a/...s_grdp/IDS_SETTINGS_SAFETY_CHECK_REVIEW_NOTIFICATION_PERMISSIONS_SECONDARY_LABEL.png.sha1 b/...s_grdp/IDS_SETTINGS_SAFETY_CHECK_REVIEW_NOTIFICATION_PERMISSIONS_SECONDARY_LABEL.png.sha1
@@ -1 +1 @@
-0bcca247ab3656ade7ed536b7a41c7fa2a986be3
+31d9b8d63ca573006b132cf2992dc31ba0ddc376
diff --git a/...e/browser/resources/settings/safety_check_page/safety_check_notification_permissions.html b/...e/browser/resources/settings/safety_check_page/safety_check_notification_permissions.html
@@ -4,7 +4,7 @@
     icon-status="[[iconStatus_]]"
     label="[[headerString_]]"
     button-label="$i18n{safetyCheckReview}"
-    button-aria-label="[[buttonAriaLabel_]]"
+    button-aria-label="$i18n{safetyCheckNotificationPermissionReviewButtonAriaLabel}"
     on-button-click="onButtonClick_"
     role="presentation"
     class="two-line">

diff --git a/chrome/browser/resources/settings/safety_check_page/safety_check_notification_permissions.ts b/chrome/browser/resources/settings/safety_check_page/safety_check_notification_permissions.ts
@@ -50,35 +50,25 @@ export class SettingsSafetyCheckNotificationPermissionsElement extends
         },
       },
 
-      sites_: {
-        type: Array,
-        observer: 'onSitesChanged_',
-      },
-
       headerString_: String,
-
-      buttonAriaLabel_: String,
     };
   }
 
   private iconStatus_: SafetyCheckIconStatus;
   private headerString_: string;
-  private buttonAriaLabel_: string;
-  private sites_: NotificationPermission[] = [];
   private siteSettingsBrowserProxy_: SiteSettingsPrefsBrowserProxy =
       SiteSettingsPrefsBrowserProxyImpl.getInstance();
 
-  override async connectedCallback() {
+  override connectedCallback() {
     super.connectedCallback();
 
     // Register for review notification permission list updates.
     this.addWebUIListener(
         'notification-permission-review-list-maybe-changed',
-        (sites: NotificationPermission[]) =>
-            this.onReviewNotificationPermissionListChanged_(sites));
+        (sites: NotificationPermission[]) => this.onSitesChanged_(sites));
 
-    this.sites_ =
-        await this.siteSettingsBrowserProxy_.getNotificationPermissionReview();
+    this.siteSettingsBrowserProxy_.getNotificationPermissionReview().then(
+        this.onSitesChanged_.bind(this));
   }
 
   private onButtonClick_() {
@@ -87,20 +77,10 @@ export class SettingsSafetyCheckNotificationPermissionsElement extends
         /* removeSearch= */ true);
   }
 
-  private async onReviewNotificationPermissionListChanged_(
-      sites: NotificationPermission[]) {
-    this.sites_ = sites;
-  }
-
-  private async onSitesChanged_() {
+  private async onSitesChanged_(sites: NotificationPermission[]) {
     this.headerString_ =
         await PluralStringProxyImpl.getInstance().getPluralString(
-            'safetyCheckNotificationPermissionReviewHeaderLabel',
-            this.sites_.length);
-    this.buttonAriaLabel_ =
-        await PluralStringProxyImpl.getInstance().getPluralString(
-            'safetyCheckNotificationPermissionReviewPrimaryLabel',
-            this.sites_!.length);
+            'safetyCheckNotificationPermissionReviewHeaderLabel', sites.length);
   }
 }
 

diff --git a/chrome/browser/resources/settings/site_settings/review_notification_permissions.html b/chrome/browser/resources/settings/site_settings/review_notification_permissions.html
@@ -29,6 +29,7 @@
   }
 
   .header-group-wrapper {
+    flex: 1;
     margin-inline-start: 15px;
   }
 
@@ -113,12 +114,12 @@
 <template is="dom-if" if="[[!shouldShowCompletionInfo_]]">
   <div id="review-header" class="header-with-icon">
     <iron-icon role="img" icon="settings:notifications-none"></iron-icon>
-    <cr-expand-button id="expandButton" class="header-group-wrapper" no-hover
-        expanded="{{notificationPermissionReviewListExpanded_}}">
+    <div class="header-group-wrapper">
       <h2>[[headerString_]]</h2>
-      <div class="secondary">
-        $i18n{safetyCheckNotificationPermissionReviewSecondaryLabel}
-      </div>
+      <div class="secondary">[[subtitleString_]]</div>
+    </div>
+    <cr-expand-button id="expandButton" no-hover
+        expanded="{{notificationPermissionReviewListExpanded_}}">
     </cr-expand-button>
   </div>
   <iron-collapse class="notification-permissions-list"
@@ -168,7 +169,7 @@ <h2>[[headerString_]]</h2>
 </template>
 <cr-toast id="undoToast" duration="5000">
   <div id="undoNotification">[[toastText_]]</div>
-  <cr-button aria-label="$i18n{safetyCheckNotificationPermissionReviewUndo}" 
+  <cr-button aria-label="$i18n{safetyCheckNotificationPermissionReviewUndo}"
       on-click="onUndoButtonClick_">
     $i18n{safetyCheckNotificationPermissionReviewUndo}
   </cr-button>

diff --git a/chrome/browser/resources/settings/site_settings/review_notification_permissions.ts b/chrome/browser/resources/settings/site_settings/review_notification_permissions.ts
@@ -102,6 +102,9 @@ export class SettingsReviewNotificationPermissionsElement extends
       /* The string for the primary header label. */
       headerString_: String,
 
+      /* The string for the subtitle. */
+      subtitleString_: String,
+
       /**
        * The text that will be shown in the toast element upon clicking one of
        * the actions.
@@ -113,18 +116,19 @@ export class SettingsReviewNotificationPermissionsElement extends
   private sites_: NotificationPermission[];
   private notificationPermissionReviewListExpanded_: boolean;
   private shouldShowCompletionInfo_: boolean;
-  private browserProxy_: SiteSettingsPrefsBrowserProxy =
-      SiteSettingsPrefsBrowserProxyImpl.getInstance();
   private lastOrigins_: string[] = [];
   private lastUserAction_: Actions|null;
   private headerString_: string;
+  private subtitleString_: string;
   private sitesLoaded_: boolean = false;
   private modelUpdateDelayMsForTesting_: number|null = null;
   private toastText_: string|null;
+  private eventTracker_: EventTracker = new EventTracker();
+  private shouldRefocusExpandButton_: boolean = false;
+  private browserProxy_: SiteSettingsPrefsBrowserProxy =
+      SiteSettingsPrefsBrowserProxyImpl.getInstance();
   private metricsBrowserProxy_: MetricsBrowserProxy =
       MetricsBrowserProxyImpl.getInstance();
-  private shouldRefocusExpandButton_: boolean = false;
-  private eventTracker_: EventTracker = new EventTracker();
 
   override async connectedCallback() {
     super.connectedCallback();
@@ -427,6 +431,10 @@ export class SettingsReviewNotificationPermissionsElement extends
         await PluralStringProxyImpl.getInstance().getPluralString(
             'safetyCheckNotificationPermissionReviewPrimaryLabel',
             this.sites_.length);
+    this.subtitleString_ =
+        await PluralStringProxyImpl.getInstance().getPluralString(
+            'safetyCheckNotificationPermissionReviewSecondaryLabel',
+            this.sites_.length);
     /**
      * Focus on the expand button after the undo button is clicked and sites are
      * loaded again.

diff --git a/chrome/browser/ui/webui/settings/settings_localized_strings_provider.cc b/chrome/browser/ui/webui/settings/settings_localized_strings_provider.cc
@@ -2005,8 +2005,6 @@ void AddSafetyCheckStrings(content::WebUIDataSource* html_source) {
      IDS_SETTINGS_SAFETY_CHECK_EXTENSIONS_PRIMARY_LABEL},
     {"safetyCheckExtensionsButtonAriaLabel",
      IDS_SETTINGS_SAFETY_CHECK_EXTENSIONS_BUTTON_ARIA_LABEL},
-    {"safetyCheckNotificationPermissionReviewSecondaryLabel",
-     IDS_SETTINGS_SAFETY_CHECK_REVIEW_NOTIFICATION_PERMISSIONS_SECONDARY_LABEL},
     {"safetyCheckNotificationPermissionReviewIgnoredToastLabel",
      IDS_SETTINGS_SAFETY_CHECK_NOTIFICATION_PERMISSION_REVIEW_IGNORED_TOAST_LABEL},
     {"safetyCheckNotificationPermissionReviewBlockedToastLabel",
@@ -2035,6 +2033,8 @@ void AddSafetyCheckStrings(content::WebUIDataSource* html_source) {
      IDS_SETTINGS_SAFETY_CHECK_NOTIFICATION_PERMISSION_REVIEW_BLOCK_ALL_LABEL},
     {"safetyCheckUnusedSitePermissionsHeaderAriaLabel",
      IDS_SETTINGS_SAFETY_CHECK_UNUSED_SITE_PERMISSIONS_HEADER_ARIA_LABEL},
+    {"safetyCheckNotificationPermissionReviewButtonAriaLabel",
+     IDS_SETTINGS_SAFETY_CHECK_NOTIFICATION_PERMISSIONS_REVIEW_BUTTON_ARIA_LABEL},
 #if BUILDFLAG(IS_WIN) && BUILDFLAG(GOOGLE_CHROME_BRANDING)
     {"safetyCheckChromeCleanerPrimaryLabel",
      IDS_SETTINGS_SAFETY_CHECK_CHROME_CLEANER_PRIMARY_LABEL},

diff --git a/chrome/browser/ui/webui/settings/settings_ui.cc b/chrome/browser/ui/webui/settings/settings_ui.cc
@@ -406,6 +406,9 @@ SettingsUI::SettingsUI(content::WebUI* web_ui)
   plural_string_handler->AddLocalizedString(
       "safetyCheckUnusedSitePermissionsHeaderLabel",
       IDS_SETTINGS_SAFETY_CHECK_UNUSED_SITE_PERMISSIONS_HEADER_LABEL);
+  plural_string_handler->AddLocalizedString(
+      "safetyCheckNotificationPermissionReviewSecondaryLabel",
+      IDS_SETTINGS_SAFETY_CHECK_REVIEW_NOTIFICATION_PERMISSIONS_SECONDARY_LABEL);
   web_ui->AddMessageHandler(std::move(plural_string_handler));
 
   // Add the metrics handler to write uma stats.

diff --git a/chrome/test/data/webui/settings/review_notification_permissions_test.ts b/chrome/test/data/webui/settings/review_notification_permissions_test.ts
@@ -465,9 +465,6 @@ suite('CrSettingsReviewNotificationPermissionsTest', function() {
   });
 
   /**
-   * TODO(crbug/1374908): Re-enable the commented parts. This is failing on
-   * buildbots.
-   *
    * Tests whether header string updated based on the notification permission
    * list size for plural and singular case.
    */
@@ -480,11 +477,13 @@ suite('CrSettingsReviewNotificationPermissionsTest', function() {
     assertEquals(2, entries.length);
 
     const headerElement =
-        testElement.shadowRoot!.querySelector('#expandButton h2');
+        testElement.shadowRoot!.querySelector('#review-header h2');
     assertTrue(headerElement !== null);
-    // assertEquals(
-    //     'Review 2 sites that recently sent a lot of notifications',
-    //     headerElement.textContent!.trim());
+
+    const headerStringTwo =
+        await PluralStringProxyImpl.getInstance().getPluralString(
+            'safetyCheckNotificationPermissionReviewPrimaryLabel', 2);
+    assertEquals(headerStringTwo, headerElement.textContent!.trim());
 
     // Check header string for singular case.
     webUIListenerCallback(
@@ -497,8 +496,9 @@ suite('CrSettingsReviewNotificationPermissionsTest', function() {
     entries = getEntries();
     assertEquals(1, entries.length);
 
-    // assertEquals(
-    //     'Review 1 site that recently sent a lot of notifications',
-    //     headerElement.textContent!.trim());
+    const headerStringOne =
+        await PluralStringProxyImpl.getInstance().getPluralString(
+            'safetyCheckNotificationPermissionReviewPrimaryLabel', 1);
+    assertEquals(headerStringOne, headerElement.textContent!.trim());
   });
 });
diff --git a/chrome/test/data/webui/settings/safety_check_permissions_test.ts b/chrome/test/data/webui/settings/safety_check_permissions_test.ts
@@ -137,8 +137,7 @@ suite('SafetyCheckNotificationPermissionsUiTests', function() {
       iconStatus: SafetyCheckIconStatus.NOTIFICATION_PERMISSIONS,
       label: 'Review <b>2 sites</b> that recently sent a lot of notifications',
       buttonLabel: 'Review',
-      buttonAriaLabel:
-          'Review 2 sites that recently sent a lot of notifications',
+      buttonAriaLabel: 'Review notification permissions',
     });
 
     // User clicks review button.
@@ -174,8 +173,7 @@ suite('SafetyCheckNotificationPermissionsUiTests', function() {
       iconStatus: SafetyCheckIconStatus.NOTIFICATION_PERMISSIONS,
       label: 'Review <b>1 site</b> that recently sent a lot of notifications',
       buttonLabel: 'Review',
-      buttonAriaLabel:
-          'Review 1 site that recently sent a lot of notifications',
+      buttonAriaLabel: 'Review notification permissions',
     });
 
     // User clicks review button.