[ios][composebox] Pre-select attached images in gallery picker [chromium/src : main]

0 views
Skip to first unread message

Ameur Hosni (Gerrit)

unread,
Jul 15, 2026, 10:14:58 AM (9 days ago) Jul 15
to Elmehdi Rahmaoui, chromium...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
Attention needed from Elmehdi Rahmaoui

Ameur Hosni added 3 comments

Commit Message
Line 15, Patchset 3 (Latest):https://drive.google.com/file/d/1JKRj2gVf2b_6cKo0tGI1Puy46kb6AAzH/view?usp=sharing&resourcekey=0-kplCicxzlHZrgWpXBcflkg
Ameur Hosni . unresolved

Can we also deselect images in the picker?

  • If we can, we should remove composebox items when we deselect a preselected image.
  • If we can't, we shouldn't update the selectionLimit as the gallery picker title will be wrong (e.g "select up to 10 images" while we should only select up to 9).
File ios/chrome/browser/composebox/shared/coordinator/composebox_picker_presenter.mm
Line 64, Patchset 3 (Latest): if ([self.dataSource
respondsToSelector:@selector(attachedImageAssetIDsForPresenter:)]) {
Ameur Hosni . unresolved

`attachedImageAssetIDsForPresenter` isn't optional, I don't think this check is necessary.

Line 72, Patchset 3 (Latest): config.selectionLimit = limit + preselectedAssetIDs.count;
Ameur Hosni . unresolved

As mentioned above, we need to update this logic only if we are able to deselect preselected images.

Open in Gerrit

Related details

Attention is currently required from:
  • Elmehdi Rahmaoui
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Id36c6ee8f0a038fbceb356532047e609ccc5e553
Gerrit-Change-Number: 8098405
Gerrit-PatchSet: 3
Gerrit-Owner: Elmehdi Rahmaoui <erah...@google.com>
Gerrit-Reviewer: Ameur Hosni <ameur...@google.com>
Gerrit-Attention: Elmehdi Rahmaoui <erah...@google.com>
Gerrit-Comment-Date: Wed, 15 Jul 2026 14:14:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Elmehdi Rahmaoui (Gerrit)

unread,
Jul 15, 2026, 10:58:02 AM (9 days ago) Jul 15
to Ameur Hosni, chromium...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
Attention needed from Ameur Hosni

Elmehdi Rahmaoui added 3 comments

Can we also deselect images in the picker?

  • If we can, we should remove composebox items when we deselect a preselected image.
  • If we can't, we shouldn't update the selectionLimit as the gallery picker title will be wrong (e.g "select up to 10 images" while we should only select up to 9).
Elmehdi Rahmaoui

Good catch, I've updated the code to handle deselecting images accordingly.

File ios/chrome/browser/composebox/shared/coordinator/composebox_picker_presenter.mm
Line 64, Patchset 3: if ([self.dataSource
respondsToSelector:@selector(attachedImageAssetIDsForPresenter:)]) {
Ameur Hosni . resolved

`attachedImageAssetIDsForPresenter` isn't optional, I don't think this check is necessary.

Elmehdi Rahmaoui

Done

Line 72, Patchset 3: config.selectionLimit = limit + preselectedAssetIDs.count;
Ameur Hosni . resolved

As mentioned above, we need to update this logic only if we are able to deselect preselected images.

Elmehdi Rahmaoui

Done

Open in Gerrit

Related details

Attention is currently required from:
  • Ameur Hosni
Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Owners
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Id36c6ee8f0a038fbceb356532047e609ccc5e553
    Gerrit-Change-Number: 8098405
    Gerrit-PatchSet: 5
    Gerrit-Owner: Elmehdi Rahmaoui <erah...@google.com>
    Gerrit-Reviewer: Ameur Hosni <ameur...@google.com>
    Gerrit-Attention: Ameur Hosni <ameur...@google.com>
    Gerrit-Comment-Date: Wed, 15 Jul 2026 14:57:45 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Ameur Hosni <ameur...@google.com>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Ameur Hosni (Gerrit)

    unread,
    Jul 20, 2026, 11:22:26 AM (4 days ago) Jul 20
    to Elmehdi Rahmaoui, chromium...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
    Attention needed from Elmehdi Rahmaoui

    Ameur Hosni added 4 comments

    File ios/chrome/browser/composebox/coordinator/composebox_input_plate_coordinator.mm
    Line 769, Patchset 5 (Latest):- (void)composeboxPickerPresenter:(ComposeboxPickerPresenter*)presenter
    didPickImages:
    (NSArray<ComposeboxPickerImageResult*>*)results {
    Ameur Hosni . unresolved

    Double checking, is this also called when the user taps cancel in the photo picker?
    If so would this incorrectly remove items? (e.g Deselect and then tap cancel.)

    File ios/chrome/browser/composebox/coordinator/composebox_input_plate_mediator.mm
    Line 482, Patchset 5 (Latest):- (void)removeImageWithAssetID:(NSString*)assetID {
    Ameur Hosni . unresolved

    ```suggestion

    • (void)removeItemWithAssetID:(NSString*)assetID {
    • ```
    Line 567, Patchset 5 (Latest): [self removeItem:item];
    Ameur Hosni . unresolved

    I think this would also remove image items taken from Camera.

    File ios/chrome/browser/composebox/menu/coordinator/composebox_menu_mediator.mm
    Line 66, Patchset 5 (Parent): NSMutableArray<ComposeboxPickerImageResult*>* updatedImageResults =
    [[NSMutableArray alloc] init];

    if (_preselection.images) {
    updatedImageResults = [_preselection.images mutableCopy];
    }

    [updatedImageResults addObjectsFromArray:imageItems];
    Ameur Hosni . unresolved

    What about the images taken from a Camera. (not the gallery)

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Elmehdi Rahmaoui
    Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement is not satisfiedCode-Owners
      • requirement is not satisfiedCode-Review
      • requirement is not satisfiedNo-Unresolved-Comments
      • requirement is not satisfiedReview-Enforcement
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: Id36c6ee8f0a038fbceb356532047e609ccc5e553
      Gerrit-Change-Number: 8098405
      Gerrit-PatchSet: 5
      Gerrit-Owner: Elmehdi Rahmaoui <erah...@google.com>
      Gerrit-Reviewer: Ameur Hosni <ameur...@google.com>
      Gerrit-Attention: Elmehdi Rahmaoui <erah...@google.com>
      Gerrit-Comment-Date: Mon, 20 Jul 2026 15:22:08 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: No
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Elmehdi Rahmaoui (Gerrit)

      unread,
      Jul 23, 2026, 5:21:41 AM (yesterday) Jul 23
      to Ameur Hosni, chromium...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
      Attention needed from Ameur Hosni

      Elmehdi Rahmaoui added 4 comments

      File ios/chrome/browser/composebox/coordinator/composebox_input_plate_coordinator.mm
      Line 769, Patchset 5:- (void)composeboxPickerPresenter:(ComposeboxPickerPresenter*)presenter
      didPickImages:
      (NSArray<ComposeboxPickerImageResult*>*)results {
      Ameur Hosni . resolved

      Double checking, is this also called when the user taps cancel in the photo picker?
      If so would this incorrectly remove items? (e.g Deselect and then tap cancel.)

      Elmehdi Rahmaoui

      It is not called the behavior here is correct.

      File ios/chrome/browser/composebox/coordinator/composebox_input_plate_mediator.mm
      Line 482, Patchset 5:- (void)removeImageWithAssetID:(NSString*)assetID {
      Ameur Hosni . resolved

      ```suggestion

      • (void)removeItemWithAssetID:(NSString*)assetID {
      • ```
      Elmehdi Rahmaoui
      The goal of the function is to remove images not any item as per the check `if (item.type == ComposeboxInputItemType::kComposeboxInputItemTypeImage &&
              [item.assetID isEqualToString:assetID]) `
      Line 567, Patchset 5: [self removeItem:item];
      Ameur Hosni . resolved

      I think this would also remove image items taken from Camera.

      Elmehdi Rahmaoui

      Good catch, the code is now updated, and the behavior is correct.

      File ios/chrome/browser/composebox/menu/coordinator/composebox_menu_mediator.mm
      Line 66, Patchset 5 (Parent): NSMutableArray<ComposeboxPickerImageResult*>* updatedImageResults =
      [[NSMutableArray alloc] init];

      if (_preselection.images) {
      updatedImageResults = [_preselection.images mutableCopy];
      }

      [updatedImageResults addObjectsFromArray:imageItems];
      Ameur Hosni . resolved

      What about the images taken from a Camera. (not the gallery)

      Elmehdi Rahmaoui

      Same as the previous comment.

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Ameur Hosni
      Submit Requirements:
        • requirement satisfiedCode-Coverage
        • requirement is not satisfiedCode-Owners
        • requirement is not satisfiedCode-Review
        • requirement is not satisfiedReview-Enforcement
        Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
        Gerrit-MessageType: comment
        Gerrit-Project: chromium/src
        Gerrit-Branch: main
        Gerrit-Change-Id: Id36c6ee8f0a038fbceb356532047e609ccc5e553
        Gerrit-Change-Number: 8098405
        Gerrit-PatchSet: 6
        Gerrit-Owner: Elmehdi Rahmaoui <erah...@google.com>
        Gerrit-Reviewer: Ameur Hosni <ameur...@google.com>
        Gerrit-Attention: Ameur Hosni <ameur...@google.com>
        Gerrit-Comment-Date: Thu, 23 Jul 2026 09:21:26 +0000
        Gerrit-HasComments: Yes
        Gerrit-Has-Labels: No
        Comment-In-Reply-To: Ameur Hosni <ameur...@google.com>
        satisfied_requirement
        unsatisfied_requirement
        open
        diffy

        Ameur Hosni (Gerrit)

        unread,
        Jul 23, 2026, 10:42:19 AM (20 hours ago) Jul 23
        to Elmehdi Rahmaoui, chromium...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
        Attention needed from Elmehdi Rahmaoui

        Ameur Hosni voted and added 3 comments

        Votes added by Ameur Hosni

        Code-Review+1

        3 comments

        Patchset-level comments
        File-level comment, Patchset 6 (Latest):
        Ameur Hosni . resolved

        Lgtm with comments.

        File ios/chrome/browser/composebox/coordinator/composebox_input_plate_coordinator.mm
        Line 772, Patchset 6 (Latest): BOOL isCamera =
        Ameur Hosni . unresolved

        Please add a comment explaining logic here.

        File ios/chrome/browser/composebox/menu/coordinator/composebox_menu_mediator.mm
        Line 81, Patchset 6 (Latest): if (imageResult.assetID.length == 0) {
        Ameur Hosni . unresolved

        I'm not sure what this checks, does it check if this is an image from the camera? If so is there a better way to check for this?

        Open in Gerrit

        Related details

        Attention is currently required from:
        • Elmehdi Rahmaoui
        Submit Requirements:
        • requirement satisfiedCode-Coverage
        • requirement satisfiedCode-Owners
        • requirement satisfiedCode-Review
        • requirement is not satisfiedNo-Unresolved-Comments
        • requirement satisfiedReview-Enforcement
        Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
        Gerrit-MessageType: comment
        Gerrit-Project: chromium/src
        Gerrit-Branch: main
        Gerrit-Change-Id: Id36c6ee8f0a038fbceb356532047e609ccc5e553
        Gerrit-Change-Number: 8098405
        Gerrit-PatchSet: 6
        Gerrit-Owner: Elmehdi Rahmaoui <erah...@google.com>
        Gerrit-Reviewer: Ameur Hosni <ameur...@google.com>
        Gerrit-Attention: Elmehdi Rahmaoui <erah...@google.com>
        Gerrit-Comment-Date: Thu, 23 Jul 2026 14:42:00 +0000
        Gerrit-HasComments: Yes
        Gerrit-Has-Labels: Yes
        satisfied_requirement
        unsatisfied_requirement
        open
        diffy

        Elmehdi Rahmaoui (Gerrit)

        unread,
        5:23 AM (2 hours ago) 5:23 AM
        to Ameur Hosni, chromium...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

        Elmehdi Rahmaoui added 2 comments

        File ios/chrome/browser/composebox/coordinator/composebox_input_plate_coordinator.mm
        Line 772, Patchset 6: BOOL isCamera =
        Ameur Hosni . resolved

        Please add a comment explaining logic here.

        Elmehdi Rahmaoui

        Done

        File ios/chrome/browser/composebox/menu/coordinator/composebox_menu_mediator.mm
        Line 81, Patchset 6: if (imageResult.assetID.length == 0) {
        Ameur Hosni . resolved

        I'm not sure what this checks, does it check if this is an image from the camera? If so is there a better way to check for this?

        Elmehdi Rahmaoui

        Done

        Open in Gerrit

        Related details

        Attention set is empty
        Submit Requirements:
          • requirement satisfiedCode-Coverage
          • requirement satisfiedCode-Owners
          • requirement satisfiedCode-Review
          • requirement satisfiedReview-Enforcement
          Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
          Gerrit-MessageType: comment
          Gerrit-Project: chromium/src
          Gerrit-Branch: main
          Gerrit-Change-Id: Id36c6ee8f0a038fbceb356532047e609ccc5e553
          Gerrit-Change-Number: 8098405
          Gerrit-PatchSet: 7
          Gerrit-Owner: Elmehdi Rahmaoui <erah...@google.com>
          Gerrit-Reviewer: Ameur Hosni <ameur...@google.com>
          Gerrit-Comment-Date: Fri, 24 Jul 2026 09:23:16 +0000
          satisfied_requirement
          open
          diffy

          Elmehdi Rahmaoui (Gerrit)

          unread,
          5:23 AM (2 hours ago) 5:23 AM
          to Ameur Hosni, chromium...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

          Elmehdi Rahmaoui voted Commit-Queue+2

          Commit-Queue+2
          Open in Gerrit

          Related details

          Attention set is empty
          Submit Requirements:
          • requirement satisfiedCode-Coverage
          • requirement satisfiedCode-Owners
          • requirement satisfiedCode-Review
          • requirement satisfiedReview-Enforcement
          Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
          Gerrit-MessageType: comment
          Gerrit-Project: chromium/src
          Gerrit-Branch: main
          Gerrit-Change-Id: Id36c6ee8f0a038fbceb356532047e609ccc5e553
          Gerrit-Change-Number: 8098405
          Gerrit-PatchSet: 7
          Gerrit-Owner: Elmehdi Rahmaoui <erah...@google.com>
          Gerrit-Reviewer: Ameur Hosni <ameur...@google.com>
          Gerrit-Reviewer: Elmehdi Rahmaoui <erah...@google.com>
          Gerrit-Comment-Date: Fri, 24 Jul 2026 09:23:24 +0000
          Gerrit-HasComments: No
          Gerrit-Has-Labels: Yes
          satisfied_requirement
          open
          diffy

          Chromium LUCI CQ (Gerrit)

          unread,
          6:37 AM (23 minutes ago) 6:37 AM
          to Elmehdi Rahmaoui, Ameur Hosni, chromium...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

          Chromium LUCI CQ submitted the change with unreviewed changes

          Unreviewed changes

          6 is the latest approved patch-set.
          The change was submitted with unreviewed changes in the following files:

          ```
          The name of the file: ios/chrome/browser/composebox/coordinator/composebox_input_plate_coordinator.mm
          Insertions: 5, Deletions: 0.

          @@ -769,6 +769,11 @@

           - (void)composeboxPickerPresenter:(ComposeboxPickerPresenter*)presenter
                               didPickImages:
                                   (NSArray<ComposeboxPickerImageResult*>*)results {
          +  // Gallery picker results (PHPickerViewController) return the complete set of
          +  // selected gallery items. Reconcile preselected asset IDs so that any gallery
          +  // photo deselected by the user is removed from attachments. Camera picker
          +  // captures lack asset IDs and represent single new photos, so skip gallery
          +  // reconciliation.
             BOOL isCamera =
                 results.firstObject.source == ComposeboxInputItemSource::kCameraPicker;
             NSArray<NSString*>* attachedAssetIDs = [_mediator attachedImageAssetIDs];
          ```
          ```
          The name of the file: ios/chrome/browser/composebox/menu/coordinator/composebox_menu_mediator.mm
          Insertions: 1, Deletions: 1.

          @@ -78,7 +78,7 @@
               // Gallery picker results. Retain existing non-gallery images from
               // preselection, and replace gallery images with `imageItems`.
               for (ComposeboxPickerImageResult* imageResult in _preselection.images) {
          -      if (imageResult.assetID.length == 0) {
          +      if (imageResult.source != ComposeboxInputItemSource::kGalleryPicker) {
                   [updatedImages addObject:imageResult];
                 }
               }
          ```

          Change information

          Commit message:
          [ios][composebox] Pre-select attached images in gallery picker

          Pass preselected image asset identifiers to `PHPickerConfiguration` when
          launching the gallery picker from the Composebox input plate and menu.
          This ensures that previously attached photos display checkmarks when the
          user reopens the system photo picker.

          Demo
          https://drive.google.com/file/d/1qgh7AkZdGTNFD0lSc8PTfZywBR1960PE/view?usp=sharing&resourcekey=0-rLXzxbFBiaXM4MiYLjxYvQ
          Bug: 534829829
          Change-Id: Id36c6ee8f0a038fbceb356532047e609ccc5e553
          Reviewed-by: Ameur Hosni <ameur...@google.com>
          Commit-Queue: Elmehdi Rahmaoui <erah...@google.com>
          Cr-Commit-Position: refs/heads/main@{#1667764}
          Files:
          Change size: M
          Delta: 8 files changed, 154 insertions(+), 25 deletions(-)
          Branch: refs/heads/main
          Submit Requirements:
          • requirement satisfiedCode-Review: +1 by Ameur Hosni
          Open in Gerrit
          Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
          Gerrit-MessageType: merged
          Gerrit-Project: chromium/src
          Gerrit-Branch: main
          Gerrit-Change-Id: Id36c6ee8f0a038fbceb356532047e609ccc5e553
          Gerrit-Change-Number: 8098405
          Gerrit-PatchSet: 8
          Gerrit-Owner: Elmehdi Rahmaoui <erah...@google.com>
          Gerrit-Reviewer: Ameur Hosni <ameur...@google.com>
          Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
          open
          diffy
          satisfied_requirement
          Reply all
          Reply to author
          Forward
          0 new messages