Calculate number of above-fold destinations more reliably.

Currently, the number of above-fold destinations is calculated by
summing into a PreferenceKey, based on .onAppear or .onDisappear
handlers. Unfortunately, this approach has not been reliably working in
iOS 16. For more context, please see:
https://developer.apple.com/forums/thread/717057.

This change introduces a new, more reliable way for calculating the
number of above-fold destinations. It introduces a new static function
numDestinationsVisibleWithoutHorizontalScrolling which performs
calculations on internal constants, instead of view handlers being
fired to sum the number of visible items.

Change-Id: If96d6817b5fae8bf28ed4d72dcd239a7cd0c5b2d
Bug: 1409976
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4190367
Commit-Queue: Benjamin Williams <bwwilliams@google.com>
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Reviewed-by: Robbie Gibson <rkgibson@google.com>
Cr-Commit-Position: refs/heads/main@{#1096872}

ios/chrome/browser/ui/popup_menu/BUILD.gn

@@ -137,10 +137,7 @@ source_set("constants") {
 
 source_set("metrics_protocols") {
   configs += [ "//build/config/compiler:enable_arc" ]
-  sources = [
-    "popup_menu_carousel_metrics_delegate.h",
-    "popup_menu_metrics_handler.h",
-  ]
+  sources = [ "popup_menu_metrics_handler.h" ]
 }
 
 source_set("unit_tests") {

ios/chrome/browser/ui/popup_menu/overflow_menu/overflow_menu_destination_list.swift

@@ -103,13 +103,10 @@ struct OverflowMenuDestinationList: View {
   private func scrollView(in geometry: GeometryProxy) -> some View {
     ScrollViewReader { proxy in
       ScrollView(.horizontal, showsIndicators: false) {
-        let spacing = destinationSpacing(forScreenWidth: geometry.size.width)
-        let layoutParameters: OverflowMenuDestinationView.LayoutParameters =
-          sizeCategory >= .accessibilityMedium
-          ? .horizontal(itemWidth: geometry.size.width - Constants.largeTextSizeSpace)
-          : .vertical(
-            iconSpacing: spacing.iconSpacing,
-            iconPadding: spacing.iconPadding)
+        let spacing = OverflowMenuDestinationList.destinationSpacing(
+          forScreenWidth: geometry.size.width)
+        let layoutParameters = OverflowMenuDestinationList.layoutParameters(
+          forScreenWidth: geometry.size.width, forSizeCategory: sizeCategory)
         let alignment: VerticalAlignment = sizeCategory >= .accessibilityMedium ? .center : .top
 
         ZStack {
@@ -151,7 +148,7 @@ struct OverflowMenuDestinationList: View {
   ///
   /// Returns `nil` for either end if `width` is above or below the largest or
   /// smallest breakpoint.
-  private func findBreakpoints(forScreenWidth width: CGFloat) -> (CGFloat?, CGFloat?) {
+  private static func findBreakpoints(forScreenWidth width: CGFloat) -> (CGFloat?, CGFloat?) {
     // Add extra sentinel values to either end of the breakpoint array.
     let x = zip(
       Constants.lowerWidthBreakpoints, Constants.upperWidthBreakpoints
@@ -170,7 +167,7 @@ struct OverflowMenuDestinationList: View {
   }
 
   /// Calculates the icon spacing and padding for the given `width`.
-  private func destinationSpacing(forScreenWidth width: CGFloat) -> (
+  private static func destinationSpacing(forScreenWidth width: CGFloat) -> (
     iconSpacing: CGFloat, iconPadding: CGFloat
   ) {
     let (lowerBreakpoint, upperBreakpoint) = findBreakpoints(
@@ -203,9 +200,34 @@ struct OverflowMenuDestinationList: View {
     return (iconSpacing: iconSpacing, iconPadding: iconPadding)
   }
 
+  private static func layoutParameters(
+    forScreenWidth width: CGFloat, forSizeCategory sizeCategory: ContentSizeCategory
+  ) -> OverflowMenuDestinationView.LayoutParameters {
+    let spacing = OverflowMenuDestinationList.destinationSpacing(forScreenWidth: width)
+
+    return sizeCategory >= .accessibilityMedium
+      ? .horizontal(itemWidth: width - Constants.largeTextSizeSpace)
+      : .vertical(
+        iconSpacing: spacing.iconSpacing,
+        iconPadding: spacing.iconPadding)
+  }
+
+  public static func numDestinationsVisibleWithoutHorizontalScrolling(
+    forScreenWidth width: CGFloat, forSizeCategory sizeCategory: ContentSizeCategory
+  )
+    -> CGFloat
+  {
+    let layoutParameters = OverflowMenuDestinationList.layoutParameters(
+      forScreenWidth: width, forSizeCategory: sizeCategory)
+    let destinationWidth = OverflowMenuDestinationButton.destinationWidth(
+      forLayoutParameters: layoutParameters)
+
+    return (width / destinationWidth).rounded(.up)
+  }
+
   /// Maps the given `number` from its relative position in `inRange` to its
   /// relative position in `outRange`.
-  private func mapNumber<F: FloatingPoint>(
+  private static func mapNumber<F: FloatingPoint>(
     _ number: F, from inRange: ClosedRange<F>, to outRange: ClosedRange<F>
   ) -> F {
     let scalingFactor =

ios/chrome/browser/ui/popup_menu/overflow_menu/overflow_menu_destination_view.swift

@@ -5,14 +5,6 @@
 import SwiftUI
 import ios_chrome_common_ui_colors_swift
 
-/// PreferenceKey to listen to visibility changes for a given destination.
-struct DestinationVisibilityPreferenceKey: PreferenceKey {
-  static var defaultValue: Int = 0
-  static func reduce(value: inout Int, nextValue: () -> Int) {
-    value += nextValue()
-  }
-}
-
 /// Style based on state for an OverflowMenuDestinationView.
 @available(iOS 15, *)
 struct OverflowMenuDestinationButton: ButtonStyle {
@@ -57,40 +49,32 @@ struct OverflowMenuDestinationButton: ButtonStyle {
   /// The layout parameters for this view.
   var layoutParameters: OverflowMenuDestinationView.LayoutParameters
 
-  /// Tracks if the destination icon is visible in the carousel.
-  @State var isIconVisible = false
-
-  /// Tracks if the destination name is visible in the carousel.
-  @State var isTextVisible = false
-
   weak var metricsHandler: PopupMenuMetricsHandler?
 
   func makeBody(configuration: Configuration) -> some View {
+    let destinationWidth = OverflowMenuDestinationButton.destinationWidth(
+      forLayoutParameters: layoutParameters)
     Group {
       switch layoutParameters {
-      case .vertical(let iconSpacing, let iconPadding):
+      case .vertical:
         VStack {
           icon(configuration: configuration)
           text
         }
-        .frame(width: Dimensions.imageWidth + 2 * iconSpacing + 2 * iconPadding)
-      case .horizontal(let itemWidth):
+        .frame(width: destinationWidth)
+      case .horizontal:
         HStack {
           icon(configuration: configuration)
           Spacer().frame(width: Dimensions.horizontalLayoutIconSpacing)
           text
         }
-        .frame(width: itemWidth, alignment: .leading)
+        .frame(width: destinationWidth, alignment: .leading)
         // In horizontal layout, the item itself has leading and trailing
         // padding.
         .padding([.leading, .trailing], Dimensions.horizontalLayoutViewPadding)
       }
     }
     .contentShape(Rectangle())
-    .preference(
-      key: DestinationVisibilityPreferenceKey.self,
-      value: (isIconVisible || isTextVisible) ? 1 : 0
-    )
   }
 
   /// Background color for the icon.
@@ -186,12 +170,6 @@ struct OverflowMenuDestinationButton: ButtonStyle {
       // VoiceOver will occasionally read out icons it thinks it can
       // recognize.
       .accessibilityHidden(true)
-      .onAppear {
-        isIconVisible = true
-      }
-      .onDisappear {
-        isIconVisible = false
-      }
 
     if !destination.symbolName.isEmpty {
       configuredImage
@@ -220,12 +198,17 @@ struct OverflowMenuDestinationButton: ButtonStyle {
       .padding([.leading, .trailing], textSpacing)
       .multilineTextAlignment(.center)
       .lineLimit(maximumLines)
-      .onAppear {
-        isTextVisible = true
-      }
-      .onDisappear {
-        isTextVisible = false
-      }
+  }
+
+  static public func destinationWidth(
+    forLayoutParameters layoutParameters: OverflowMenuDestinationView.LayoutParameters
+  ) -> CGFloat {
+    switch layoutParameters {
+    case .vertical(let iconSpacing, let iconPadding):
+      return Dimensions.imageWidth + 2 * iconSpacing + 2 * iconPadding
+    case .horizontal(let itemWidth):
+      return itemWidth
+    }
   }
 }
 

ios/chrome/browser/ui/popup_menu/overflow_menu/overflow_menu_mediator.h

@@ -10,7 +10,6 @@
 #import "ios/chrome/browser/follow/follow_action_state.h"
 #import "ios/chrome/browser/ui/browser_container/browser_container_consumer.h"
 #import "ios/chrome/browser/ui/popup_menu/overflow_menu/overflow_menu_swift.h"
-#import "ios/chrome/browser/ui/popup_menu/popup_menu_carousel_metrics_delegate.h"
 
 namespace bookmarks {
 class BookmarkModel;
@@ -37,8 +36,7 @@ class FollowBrowserAgent;
 
 // Mediator for the overflow menu. This object is in charge of creating and
 // updating the items of the overflow menu.
-@interface OverflowMenuMediator
-    : NSObject <BrowserContainerConsumer, PopupMenuCarouselMetricsDelegate>
+@interface OverflowMenuMediator : NSObject <BrowserContainerConsumer>
 
 // The data model for the overflow menu.
 @property(nonatomic, readonly) OverflowMenuModel* overflowMenuModel;
@@ -95,6 +93,10 @@ class FollowBrowserAgent;
 // The FollowBrowserAgent used to manage web channels subscriptions.
 @property(nonatomic, assign) FollowBrowserAgent* followBrowserAgent;
 
+// The number of destinations immediately visible to the user when opening the
+// new overflow menu (i.e. the number of "above-the-fold" destinations).
+@property(nonatomic, assign) int numAboveFoldDestinations;
+
 // Disconnect the mediator.
 - (void)disconnect;
 

ios/chrome/browser/ui/popup_menu/overflow_menu/overflow_menu_mediator.mm

@@ -205,10 +205,6 @@ @interface OverflowMenuMediator () <BookmarkModelBridgeObserver,
 // Whether the web content is currently being blocked.
 @property(nonatomic, assign) BOOL contentBlocked;
 
-// The number of destinations immediately visible to the user when opening the
-// new overflow menu (i.e. the number of "above-the-fold" destinations).
-@property(nonatomic, assign) int numAboveFoldDestinations;
-
 @property(nonatomic, strong) OverflowMenuDestination* bookmarksDestination;
 @property(nonatomic, strong) OverflowMenuDestination* downloadsDestination;
 @property(nonatomic, strong) OverflowMenuDestination* historyDestination;
@@ -1865,17 +1861,4 @@ - (void)openSpotlightDebugger {
   [self.dispatcher showSpotlightDebugger];
 }
 
-#pragma mark - PopupMenuCarouselMetricsDelegate
-
-- (void)visibleDestinationCountDidChange:(NSInteger)numVisibleDestinations {
-  // Exit early if numAboveFoldDestinations is already set to a non-zero value;
-  // this class only cares about the first time this method is called with a
-  // non-zero value; for that reason, exit early on subsequent calls to this
-  // method if numAboveFoldDestinations is already set.
-  if (self.numAboveFoldDestinations)
-    return;
-
-  self.numAboveFoldDestinations = numVisibleDestinations;
-}
-
 @end

ios/chrome/browser/ui/popup_menu/overflow_menu/overflow_menu_swift_bridge.h

@@ -10,7 +10,6 @@
 
 #include "ios/chrome/grit/ios_strings.h"
 
-#import "ios/chrome/browser/ui/popup_menu/popup_menu_carousel_metrics_delegate.h"
 #import "ios/chrome/browser/ui/popup_menu/popup_menu_constants.h"
 #import "ios/chrome/browser/ui/popup_menu/popup_menu_metrics_handler.h"
 #include "ui/base/l10n/l10n_util_mac_bridge.h"

ios/chrome/browser/ui/popup_menu/overflow_menu/overflow_menu_ui_configuration.swift

@@ -14,6 +14,16 @@ import SwiftUI
   /// The destination list's frame in screen coordinates.
   public var destinationListScreenFrame: CGRect = .zero
 
+  @available(iOS 15, *)
+  static public func numDestinationsVisibleWithoutHorizontalScrolling(
+    forScreenWidth width: CGFloat, forContentSizeCategory sizeCategory: UIContentSizeCategory
+  ) -> CGFloat {
+    let contentSizeCategory = ContentSizeCategory(sizeCategory) ?? .medium
+
+    return OverflowMenuDestinationList.numDestinationsVisibleWithoutHorizontalScrolling(
+      forScreenWidth: width, forSizeCategory: contentSizeCategory)
+  }
+
   public init(
     presentingViewControllerHorizontalSizeClass: UIUserInterfaceSizeClass,
     presentingViewControllerVerticalSizeClass: UIUserInterfaceSizeClass

ios/chrome/browser/ui/popup_menu/overflow_menu/overflow_menu_view.swift

@@ -16,8 +16,6 @@ struct OverflowMenuView: View {
 
   weak var metricsHandler: PopupMenuMetricsHandler?
 
-  weak var carouselMetricsDelegate: PopupMenuCarouselMetricsDelegate?
-
   var body: some View {
     VStack(
       alignment: .leading,
@@ -28,12 +26,7 @@ struct OverflowMenuView: View {
       OverflowMenuDestinationList(
         destinations: model.destinations, metricsHandler: metricsHandler,
         uiConfiguration: uiConfiguration
-      ).onPreferenceChange(
-        DestinationVisibilityPreferenceKey.self
-      ) {
-        (value: DestinationVisibilityPreferenceKey.Value) in
-        carouselMetricsDelegate?.visibleDestinationCountDidChange(value)
-      }.frame(height: Dimensions.destinationListHeight)
+      ).frame(height: Dimensions.destinationListHeight)
       Divider()
       OverflowMenuActionList(actionGroups: model.actionGroups, metricsHandler: metricsHandler)
       // Add a spacer on iPad to make sure there's space below the list.

ios/chrome/browser/ui/popup_menu/overflow_menu/overflow_menu_view_provider.swift

@@ -12,13 +12,11 @@ import UIKit
   public static func makeViewController(
     withModel model: OverflowMenuModel,
     uiConfiguration: OverflowMenuUIConfiguration,
-    metricsHandler: PopupMenuMetricsHandler,
-    carouselMetricsDelegate: PopupMenuCarouselMetricsDelegate
+    metricsHandler: PopupMenuMetricsHandler
   ) -> UIViewController {
     return OverflowMenuHostingController(
       rootView: OverflowMenuView(
-        model: model, uiConfiguration: uiConfiguration, metricsHandler: metricsHandler,
-        carouselMetricsDelegate: carouselMetricsDelegate),
+        model: model, uiConfiguration: uiConfiguration, metricsHandler: metricsHandler),
       uiConfiguration: uiConfiguration)
   }
 }

ios/chrome/browser/ui/popup_menu/popup_menu_coordinator.mm

@@ -390,6 +390,19 @@ - (void)presentPopupOfType:(PopupMenuType)type
     if (IsNewOverflowMenuEnabled()) {
       if (@available(iOS 15, *)) {
         self.overflowMenuMediator = [[OverflowMenuMediator alloc] init];
+
+        CGFloat screenWidth = self.baseViewController.view.frame.size.width;
+        UIContentSizeCategory contentSizeCategory =
+            self.baseViewController.traitCollection
+                .preferredContentSizeCategory;
+
+        self.overflowMenuMediator
+            .numAboveFoldDestinations = [OverflowMenuUIConfiguration
+            numDestinationsVisibleWithoutHorizontalScrollingForScreenWidth:
+                screenWidth
+                                                    forContentSizeCategory:
+                                                        contentSizeCategory];
+
         self.overflowMenuMediator.dispatcher = static_cast<
             id<ActivityServiceCommands, ApplicationCommands, BrowserCommands,
                BrowserCoordinatorCommands, FindInPageCommands,
@@ -444,8 +457,7 @@ - (void)presentPopupOfType:(PopupMenuType)type
             makeViewControllerWithModel:self.overflowMenuMediator
                                             .overflowMenuModel
                         uiConfiguration:uiConfiguration
-                         metricsHandler:self
-                carouselMetricsDelegate:self.overflowMenuMediator];
+                         metricsHandler:self];
 
         LayoutGuideCenter* layoutGuideCenter =
             LayoutGuideCenterForBrowser(self.browser);