Automatically attach file extensions in the ChromeOS "Save As" dialog.

The behaviour is similar to the file pickers on other platforms.

When clicking "OK", a file extension is only added if:
 - A FileType filter is active that has a default extension (i.e.
     not the all files filter, or an extensionless filter), and
 - The user-provided filename does not already have an extension.

The file picker already has logic for adding extensions, but it's only
applied when changing the currently active filter in the UI (and only
when the filename already has an extension that isn't *any* of the
possible extensions for the current filter). Repurpose it.

The approach taken updates the UI field when clicking close, so the user
briefly sees the extension being added before the dialog dismisses.

To keep logic & tests simple/robust it was necessary to ensure filters
are applied in the dialog correctly when there is only one filter
available, which regressed recently (https://crbug.com/1097448).

Bug: 1082624, 1097448, 1097633
Change-Id: I1488880be8c289853b864d4bea0e193beb5464ac
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2255918
Commit-Queue: Trent Apted <tapted@chromium.org>
Reviewed-by: Noel Gordon <noel@chromium.org>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Cr-Commit-Position: refs/heads/master@{#781157}

chrome/browser/chromeos/file_manager/file_manager_browsertest.cc

@@ -778,6 +778,10 @@ WRAPPED_INSTANTIATE_TEST_SUITE_P(
         TestCase("saveFileDialogDriveOfflinePinned").WithBrowser().Offline(),
         TestCase("openFileDialogDefaultFilter").WithBrowser(),
         TestCase("saveFileDialogDefaultFilter").WithBrowser(),
+        TestCase("saveFileDialogSingleFilterNoAcceptAll").WithBrowser(),
+        TestCase("saveFileDialogExtensionNotAddedWithNoFilter").WithBrowser(),
+        TestCase("saveFileDialogExtensionAddedWithJpegFilter").WithBrowser(),
+        TestCase("saveFileDialogExtensionNotAddedWhenProvided").WithBrowser(),
         TestCase("openFileDialogFileListShowContextMenu").WithBrowser(),
         TestCase("openFileDialogSelectAllDisabled").WithBrowser(),
         TestCase("openMultiFileDialogSelectAllEnabled").WithBrowser()));

chromeos/components/media_app_ui/resources/js/launch.js

@@ -144,7 +144,11 @@ guestMessagePipe.registerHandler(Message.SAVE_COPY, async (message) => {
   /** @type {!ChooseFileSystemEntriesOptions} */
   const options = {
     type: 'save-file',
-    accepts: [{extension, mimeTypes: [blob.type]}]
+    accepts: [{extension, mimeTypes: [blob.type]}],
+
+    // Without this, the file picker defaults to "All files" and will refuse to
+    // provide an extension automatically. See crbug/1082624#c23.
+    excludeAcceptAllOption: true,
   };
   /** @type {!FileSystemHandle} */
   let fileSystemHandle;

ui/file_manager/file_manager/foreground/js/dialog_action_controller.js

@@ -101,6 +101,7 @@ class DialogActionController {
 
     // Save-as doesn't require a valid selection from the list, since
     // we're going to take the filename from the text input.
+    this.updateExtensionForSelectedFileType_(true);
     const filename = this.dialogFooter_.filenameInput.value;
     if (!filename) {
       throw new Error('Missing filename!');
@@ -271,36 +272,79 @@ class DialogActionController {
     }
   }
 
+  /**
+   * Returns the regex to match against files for the current filter.
+   * @return {?RegExp}
+   */
+  regexpForCurrentFilter_() {
+    // Note selectedFilterIndex indexing is 1-based. (0 is "all files").
+    const selectedIndex = this.dialogFooter_.selectedFilterIndex;
+    if (selectedIndex < 1) {
+      return null;  // No specific filter selected.
+    }
+    return new RegExp(
+        '\\.(' + this.fileTypes_[selectedIndex - 1].extensions.join('|') + ')$',
+        'i');
+  }
+
+  /**
+   * Updates the file input field to agree with the current filter.
+   * @param {boolean} forConfirm The update is for the final confirm step.
+   * @private
+   */
+  updateExtensionForSelectedFileType_(forConfirm) {
+    const regexp = this.regexpForCurrentFilter_();
+    if (!regexp) {
+      return;  // No filter selected.
+    }
+
+    let filename = this.dialogFooter_.filenameInput.value;
+    if (!filename || regexp.test(filename)) {
+      return;  // Filename empty or already satisfies filter.
+    }
+
+    const selectedIndex = this.dialogFooter_.selectedFilterIndex;
+    assert(selectedIndex > 0);  // Otherwise there would be no regex.
+    const newExtension = this.fileTypes_[selectedIndex - 1].extensions[0];
+    if (!newExtension) {
+      return;  // No default extension.
+    }
+
+    const extensionIndex = filename.lastIndexOf('.');
+    if (extensionIndex < 0) {
+      // No current extension.
+      if (!forConfirm) {
+        return;  // Add one later.
+      }
+      filename = `${filename}.${newExtension}`;
+    } else {
+      if (forConfirm) {
+        return;  // Keep the current user choice.
+      }
+      filename = `${filename.substr(0, extensionIndex)}.${newExtension}`;
+    }
+
+    this.dialogFooter_.filenameInput.value = filename;
+    this.dialogFooter_.selectTargetNameInFilenameInput();
+  }
+
   /**
    * Filters file according to the selected file type.
    * @private
    */
   onFileTypeFilterChanged_() {
     this.fileFilter_.removeFilter('fileType');
-    const selectedIndex = this.dialogFooter_.selectedFilterIndex;
-    if (selectedIndex > 0) {  // Specific filter selected.
-      const regexp = new RegExp(
-          '\\.(' + this.fileTypes_[selectedIndex - 1].extensions.join('|') +
-              ')$',
-          'i');
-      const filter = entry => {
-        return entry.isDirectory || regexp.test(entry.name);
-      };
-      this.fileFilter_.addFilter('fileType', filter);
-
-      // In save dialog, update the destination name extension.
-      if (this.dialogType_ === DialogType.SELECT_SAVEAS_FILE) {
-        const current = this.dialogFooter_.filenameInput.value;
-        const newExt = this.fileTypes_[selectedIndex - 1].extensions[0];
-        if (newExt && !regexp.test(current)) {
-          const i = current.lastIndexOf('.');
-          if (i >= 0) {
-            this.dialogFooter_.filenameInput.value =
-                current.substr(0, i) + '.' + newExt;
-            this.dialogFooter_.selectTargetNameInFilenameInput();
-          }
-        }
-      }
+    const regexp = this.regexpForCurrentFilter_();
+    if (!regexp) {
+      return;
+    }
+
+    const filter = entry => entry.isDirectory || regexp.test(entry.name);
+    this.fileFilter_.addFilter('fileType', filter);
+
+    // In save dialog, update the destination name extension.
+    if (this.dialogType_ === DialogType.SELECT_SAVEAS_FILE) {
+      this.updateExtensionForSelectedFileType_(false);
     }
   }
 

ui/file_manager/file_manager/foreground/js/ui/dialog_footer.js

@@ -365,21 +365,18 @@ class DialogFooter {
     }
 
     const options = this.fileTypeSelector.querySelectorAll('option');
-    if (options.length >= 2) {
-      // There is in fact no choice, show the selector.
-      this.fileTypeSelector.hidden = false;
-      if (util.isFilesNg()) {
-        // Make sure one of the options is selected to match real <select>.
-        let selectedOption =
-            this.fileTypeSelector.querySelector('.options .selected');
-        if (!selectedOption) {
-          selectedOption =
-              this.fileTypeSelector.querySelector('.options option');
-          this.setOptionSelected(
-              /** @type {HTMLOptionElement } */ (selectedOption));
-        }
+    if (options.length > 0 && util.isFilesNg()) {
+      // Make sure one of the options is selected to match real <select>.
+      let selectedOption =
+          this.fileTypeSelector.querySelector('.options .selected');
+      if (!selectedOption) {
+        selectedOption = this.fileTypeSelector.querySelector('.options option');
+        this.setOptionSelected(
+            /** @type {HTMLOptionElement } */ (selectedOption));
       }
     }
+    // Hide the UI if there is actually no choice to be made (0 or 1 option).
+    this.fileTypeSelector.hidden = options.length < 2;
   }
 
   /**

ui/file_manager/integration_tests/file_manager/file_dialog.js

@@ -99,6 +99,22 @@ async function openFileDialogClickOkButton(
   return result;
 }
 
+/**
+ * Clicks the OK button in the provided dialog, expecting the provided `name` to
+ * be passed into the `OnFilesImpl()` observer in the C++ test harness.
+ *
+ * @param {string} appId App window Id.
+ * @param {string} name The (single) filename passed to the EXPECT_CALL when
+ *     verifying the mocked OnFilesOpenedImpl().
+ * @param {string} openType Type of the dialog ('open' or 'saveAs').
+ */
+async function clickOkButtonExpectName(appId, name, openType) {
+  await sendTestMessage({name: 'expectFileTask', fileNames: [name], openType});
+
+  const okButton = '.button-panel button.ok:enabled';
+  await remoteCall.waitAndClickElement(appId, okButton);
+}
+
 /**
  * Adds the basic file entry sets then opens the save file dialog on the volume.
  * Once file |name| is shown, select it and click the Ok button, again clicking
@@ -111,12 +127,7 @@ async function openFileDialogClickOkButton(
 async function saveFileDialogClickOkButton(volume, name) {
   const caller = getCaller();
 
-  await sendTestMessage(
-      {name: 'expectFileTask', fileNames: [name], openType: 'saveAs'});
-
   const closer = async (appId) => {
-    const okButton = '.button-panel button.ok:enabled';
-
     await remoteCall.callRemoteTestUtil('selectFile', appId, [name]);
     await repeatUntil(async () => {
       const element =
@@ -126,9 +137,7 @@ async function saveFileDialogClickOkButton(volume, name) {
       }
     });
 
-    await remoteCall.waitForElement(appId, okButton);
-    const event = [okButton, 'click'];
-    await remoteCall.callRemoteTestUtil('fakeEvent', appId, event);
+    await clickOkButtonExpectName(appId, name, 'saveAs');
 
     const confirmOkButton = '.files-confirm-dialog .cr-dialog-ok';
     await remoteCall.waitForElement(appId, confirmOkButton);
@@ -465,6 +474,85 @@ testcase.saveFileDialogDefaultFilter = async () => {
   chrome.test.assertEq('All files', selectedFilter.text);
 };
 
+/**
+ * Tests that filtering works with { acceptsAllTypes: false } and a single
+ * filter. Regression test for https://crbug.com/1097448.
+ */
+testcase.saveFileDialogSingleFilterNoAcceptAll = async () => {
+  const params = {
+    type: 'saveFile',
+    accepts: [{extensions: ['jpg']}],
+    acceptsAllTypes: false,
+  };
+  chrome.fileSystem.chooseEntry(params, (entry) => {});
+  const dialog = await remoteCall.waitForWindow('dialog#');
+
+  // Check: 'JPEG image' should be selected.
+  const selectedFilter =
+      await remoteCall.waitForElement(dialog, '.file-type option:checked');
+  chrome.test.assertEq('1', selectedFilter.value);
+  chrome.test.assertEq('JPEG image', selectedFilter.text);
+};
+
+/**
+ * Opens a "Save As" dialog and clicks OK. Helper for the
+ * saveFileDialogExtension* tests.
+ *
+ * @param {!Object} extraParams Extra options to pass to chooseEntry().
+ * @param {string} expectName Name for the 'expectFileTask' mock expectation.
+ * @return {!Promise<string>} The name of the entry from chooseEntry().
+ */
+async function showSaveAndConfirmExpecting(extraParams, expectName) {
+  const params = {
+    type: 'saveFile',
+    accepts: [{extensions: ['jpg']}],
+  };
+  const result = new Promise(resolve => {
+    chrome.fileSystem.chooseEntry(Object.assign(params, extraParams), resolve);
+  });
+  const dialog = await remoteCall.waitForWindow('dialog#');
+
+  // Ensure the input field is ready.
+  await remoteCall.waitForElement(dialog, '#filename-input-textbox');
+
+  await clickOkButtonExpectName(dialog, expectName, 'saveAs');
+  return (await result).name;
+}
+
+/**
+ * Tests that a file extension is not automatically added upon confirmation
+ * whilst the "All Files" filter is selected on the "Save As" dialog. Note the
+ * saveFileDialogDefaultFilter test above verifies that 'All Files' is actually
+ * the default in this setup.
+ */
+testcase.saveFileDialogExtensionNotAddedWithNoFilter = async () => {
+  // Note these tests use the suggestedName field as a robust way to simulate a
+  // user typing into the input field.
+  const extraParams = {acceptsAllTypes: true, suggestedName: 'test'};
+  const name = await showSaveAndConfirmExpecting(extraParams, 'test');
+  chrome.test.assertEq('test', name);
+};
+
+/**
+ * With no "All Files" option, the JPEG filter should be applied by default, and
+ * a ".jpg" extension automatically added on confirm.
+ */
+testcase.saveFileDialogExtensionAddedWithJpegFilter = async () => {
+  const extraParams = {acceptsAllTypes: false, suggestedName: 'test'};
+  const name = await showSaveAndConfirmExpecting(extraParams, 'test.jpg');
+  chrome.test.assertEq('test.jpg', name);
+};
+
+/**
+ * An extension should only be added if the user didn't provide one, even if it
+ * doesn't match the current filter for JPEG files (i.e. /\.(jpg)$/i).
+ */
+testcase.saveFileDialogExtensionNotAddedWhenProvided = async () => {
+  const extraParams = {acceptsAllTypes: false, suggestedName: 'foo.png'};
+  const name = await showSaveAndConfirmExpecting(extraParams, 'foo.png');
+  chrome.test.assertEq('foo.png', name);
+};
+
 /**
  * Tests that context menu on File List for file picker dialog.
  * File picker dialog displays fewer menu options than full Files app. For