Merge pull request #6509 from matrix-org/travis/voice-messages/download-2

Improve download interactions

res/css/views/messages/_MFileBody.scss

@@ -1,5 +1,5 @@
 /*
-Copyright 2015, 2016, 2021 The Matrix.org Foundation C.I.C.
+Copyright 2015 - 2021 The Matrix.org Foundation C.I.C.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -60,6 +60,8 @@ limitations under the License.
 }
 
 .mx_MFileBody_info {
+    cursor: pointer;
+
     .mx_MFileBody_info_icon {
         background-color: $message-body-panel-icon-bg-color;
         border-radius: 20px;

src/components/views/messages/DownloadActionButton.tsx

@@ -16,12 +16,13 @@ limitations under the License.
 
 import { MatrixEvent } from "matrix-js-sdk/src";
 import { MediaEventHelper } from "../../../utils/MediaEventHelper";
-import React, { createRef } from "react";
+import React from "react";
 import { RovingAccessibleTooltipButton } from "../../../accessibility/RovingTabIndex";
 import Spinner from "../elements/Spinner";
 import classNames from "classnames";
 import { _t } from "../../../languageHandler";
 import { replaceableComponent } from "../../../utils/replaceableComponent";
+import { FileDownloader } from "../../../utils/FileDownloader";
 
 interface IProps {
     mxEvent: MatrixEvent;
@@ -39,7 +40,7 @@ interface IState {
 
 @replaceableComponent("views.messages.DownloadActionButton")
 export default class DownloadActionButton extends React.PureComponent<IProps, IState> {
-    private iframe: React.RefObject<HTMLIFrameElement> = createRef();
+    private downloader = new FileDownloader();
 
     public constructor(props: IProps) {
         super(props);
@@ -56,27 +57,21 @@ export default class DownloadActionButton extends React.PureComponent<IProps, IS
 
         if (this.state.blob) {
             // Cheat and trigger a download, again.
-            return this.onFrameLoad();
+            return this.doDownload();
         }
 
         const blob = await this.props.mediaEventHelperGet().sourceBlob.value;
         this.setState({ blob });
+        await this.doDownload();
     };
 
-    private onFrameLoad = () => {
+    private async doDownload() {
-        this.setState({ loading: false });
+        await this.downloader.download({
-
-        // we aren't showing the iframe, so we can send over the bare minimum styles and such.
-        this.iframe.current.contentWindow.postMessage({
-            imgSrc: "", // no image
-            imgStyle: null,
-            style: "",
             blob: this.state.blob,
-            download: this.props.mediaEventHelperGet().fileName,
+            name: this.props.mediaEventHelperGet().fileName,
-            textContent: "",
+        });
-            auto: true, // autodownload
+        this.setState({ loading: false });
-        }, '*');
+    }
-    };
 
     public render() {
         let spinner: JSX.Element;
@@ -92,18 +87,11 @@ export default class DownloadActionButton extends React.PureComponent<IProps, IS
 
         return <RovingAccessibleTooltipButton
             className={classes}
-            title={spinner ? _t("Downloading") : _t("Download")}
+            title={spinner ? _t("Decrypting") : _t("Download")}
             onClick={this.onDownloadClick}
             disabled={!!spinner}
         >
             { spinner }
-            { this.state.blob && <iframe
-                src="usercontent/" // XXX: Like MFileBody, this should come from the skin
-                ref={this.iframe}
-                onLoad={this.onFrameLoad}
-                sandbox="allow-scripts allow-downloads allow-downloads-without-user-activation"
-                style={{ display: "none" }}
-            /> }
         </RovingAccessibleTooltipButton>;
     }
 }

src/components/views/messages/MFileBody.tsx

@@ -26,6 +26,8 @@ import { TileShape } from "../rooms/EventTile";
 import { presentableTextForFile } from "../../../utils/FileUtils";
 import { IMediaEventContent } from "../../../customisations/models/IMediaEventContent";
 import { IBodyProps } from "./IBodyProps";
+import { FileDownloader } from "../../../utils/FileDownloader";
+import TextWithTooltip from "../elements/TextWithTooltip";
 
 export let DOWNLOAD_ICON_URL; // cached copy of the download.svg asset for the sandboxed iframe later on
 
@@ -111,6 +113,7 @@ export default class MFileBody extends React.Component<IProps, IState> {
     private iframe: React.RefObject<HTMLIFrameElement> = createRef();
     private dummyLink: React.RefObject<HTMLAnchorElement> = createRef();
     private userDidClick = false;
+    private fileDownloader: FileDownloader = new FileDownloader(() => this.iframe.current);
 
     public constructor(props: IProps) {
         super(props);
@@ -118,6 +121,32 @@ export default class MFileBody extends React.Component<IProps, IState> {
         this.state = {};
     }
 
+    private get content(): IMediaEventContent {
+        return this.props.mxEvent.getContent<IMediaEventContent>();
+    }
+
+    private get fileName(): string {
+        return this.content.body && this.content.body.length > 0 ? this.content.body : _t("Attachment");
+    }
+
+    private get linkText(): string {
+        return presentableTextForFile(this.content);
+    }
+
+    private downloadFile(fileName: string, text: string) {
+        this.fileDownloader.download({
+            blob: this.state.decryptedBlob,
+            name: fileName,
+            autoDownload: this.userDidClick,
+            opts: {
+                imgSrc: DOWNLOAD_ICON_URL,
+                imgStyle: null,
+                style: computedStyle(this.dummyLink.current),
+                textContent: _t("Download %(text)s", { text }),
+            },
+        });
+    }
+
     private getContentUrl(): string {
         const media = mediaFromContent(this.props.mxEvent.getContent());
         return media.srcHttp;
@@ -129,24 +158,56 @@ export default class MFileBody extends React.Component<IProps, IState> {
         }
     }
 
-    public render() {
+    private decryptFile = async (): Promise<void> => {
-        const content = this.props.mxEvent.getContent<IMediaEventContent>();
+        if (this.state.decryptedBlob) {
-        const text = presentableTextForFile(content);
+            return;
-        const isEncrypted = this.props.mediaEventHelper.media.isEncrypted;
+        }
-        const fileName = content.body && content.body.length > 0 ? content.body : _t("Attachment");
+        try {
-        const contentUrl = this.getContentUrl();
+            this.userDidClick = true;
-        const fileSize = content.info ? content.info.size : null;
+            this.setState({
-        const fileType = content.info ? content.info.mimetype : "application/octet-stream";
+                decryptedBlob: await this.props.mediaEventHelper.sourceBlob.value,
+            });
+        } catch (err) {
+            console.warn("Unable to decrypt attachment: ", err);
+            Modal.createTrackedDialog('Error decrypting attachment', '', ErrorDialog, {
+                title: _t("Error"),
+                description: _t("Error decrypting attachment"),
+            });
+        }
+    };
 
-        let placeholder = null;
+    private onPlaceholderClick = async () => {
+        const mediaHelper = this.props.mediaEventHelper;
+        if (mediaHelper.media.isEncrypted) {
+            await this.decryptFile();
+            this.downloadFile(this.fileName, this.linkText);
+        } else {
+            // As a button we're missing the `download` attribute for styling reasons, so
+            // download with the file downloader.
+            this.fileDownloader.download({
+                blob: await mediaHelper.sourceBlob.value,
+                name: this.fileName,
+            });
+        }
+    };
+
+    public render() {
+        const isEncrypted = this.props.mediaEventHelper.media.isEncrypted;
+        const contentUrl = this.getContentUrl();
+        const fileSize = this.content.info ? this.content.info.size : null;
+        const fileType = this.content.info ? this.content.info.mimetype : "application/octet-stream";
+
+        let placeholder: React.ReactNode = null;
         if (this.props.showGenericPlaceholder) {
             placeholder = (
-                <div className="mx_MediaBody mx_MFileBody_info">
+                <AccessibleButton className="mx_MediaBody mx_MFileBody_info" onClick={this.onPlaceholderClick}>
                     <span className="mx_MFileBody_info_icon" />
-                    <span className="mx_MFileBody_info_filename">
+                    <TextWithTooltip tooltip={presentableTextForFile(this.content, _t("Attachment"), true)}>
-                        { presentableTextForFile(content, _t("Attachment"), false) }
+                        <span className="mx_MFileBody_info_filename">
-                    </span>
+                            { presentableTextForFile(this.content, _t("Attachment"), true, true) }
-                </div>
+                        </span>
+                    </TextWithTooltip>
+                </AccessibleButton>
             );
         }
 
@@ -157,20 +218,6 @@ export default class MFileBody extends React.Component<IProps, IState> {
                 // Need to decrypt the attachment
                 // Wait for the user to click on the link before downloading
                 // and decrypting the attachment.
-                const decrypt = async () => {
-                    try {
-                        this.userDidClick = true;
-                        this.setState({
-                            decryptedBlob: await this.props.mediaEventHelper.sourceBlob.value,
-                        });
-                    } catch (err) {
-                        console.warn("Unable to decrypt attachment: ", err);
-                        Modal.createTrackedDialog('Error decrypting attachment', '', ErrorDialog, {
-                            title: _t("Error"),
-                            description: _t("Error decrypting attachment"),
-                        });
-                    }
-                };
 
                 // This button should actually Download because usercontent/ will try to click itself
                 // but it is not guaranteed between various browsers' settings.
@@ -178,31 +225,14 @@ export default class MFileBody extends React.Component<IProps, IState> {
                     <span className="mx_MFileBody">
                         { placeholder }
                         { showDownloadLink && <div className="mx_MFileBody_download">
-                            <AccessibleButton onClick={decrypt}>
+                            <AccessibleButton onClick={this.decryptFile}>
-                                { _t("Decrypt %(text)s", { text: text }) }
+                                { _t("Decrypt %(text)s", { text: this.linkText }) }
                             </AccessibleButton>
                         </div> }
                     </span>
                 );
             }
 
-            // When the iframe loads we tell it to render a download link
-            const onIframeLoad = (ev) => {
-                ev.target.contentWindow.postMessage({
-                    imgSrc: DOWNLOAD_ICON_URL,
-                    imgStyle: null, // it handles this internally for us. Useful if a downstream changes the icon.
-                    style: computedStyle(this.dummyLink.current),
-                    blob: this.state.decryptedBlob,
-                    // Set a download attribute for encrypted files so that the file
-                    // will have the correct name when the user tries to download it.
-                    // We can't provide a Content-Disposition header like we would for HTTP.
-                    download: fileName,
-                    textContent: _t("Download %(text)s", { text: text }),
-                    // only auto-download if a user triggered this iframe explicitly
-                    auto: this.userDidClick,
-                }, "*");
-            };
-
             const url = "usercontent/"; // XXX: this path should probably be passed from the skin
 
             // If the attachment is encrypted then put the link inside an iframe.
@@ -218,9 +248,16 @@ export default class MFileBody extends React.Component<IProps, IState> {
                               */ }
                             <a ref={this.dummyLink} />
                         </div>
+                        { /*
+                            TODO: Move iframe (and dummy link) into FileDownloader.
+                            We currently have it set up this way because of styles applied to the iframe
+                            itself which cannot be easily handled/overridden by the FileDownloader. In
+                            future, the download link may disappear entirely at which point it could also
+                            be suitable to just remove this bit of code.
+                         */ }
                         <iframe
                             src={url}
-                            onLoad={onIframeLoad}
+                            onLoad={() => this.downloadFile(this.fileName, this.linkText)}
                             ref={this.iframe}
                             sandbox="allow-scripts allow-downloads allow-downloads-without-user-activation" />
                     </div> }
@@ -259,7 +296,7 @@ export default class MFileBody extends React.Component<IProps, IState> {
 
                         // We have to create an anchor to download the file
                         const tempAnchor = document.createElement('a');
-                        tempAnchor.download = fileName;
+                        tempAnchor.download = this.fileName;
                         tempAnchor.href = blobUrl;
                         document.body.appendChild(tempAnchor); // for firefox: https://stackoverflow.com/a/32226068
                         tempAnchor.click();
@@ -268,7 +305,7 @@ export default class MFileBody extends React.Component<IProps, IState> {
                 };
             } else {
                 // Else we are hoping the browser will do the right thing
-                downloadProps["download"] = fileName;
+                downloadProps["download"] = this.fileName;
             }
 
             return (
@@ -277,16 +314,16 @@ export default class MFileBody extends React.Component<IProps, IState> {
                     { showDownloadLink && <div className="mx_MFileBody_download">
                         <a {...downloadProps}>
                             <span className="mx_MFileBody_download_icon" />
-                            { _t("Download %(text)s", { text: text }) }
+                            { _t("Download %(text)s", { text: this.linkText }) }
                         </a>
                         { this.props.tileShape === TileShape.FileGrid && <div className="mx_MImageBody_size">
-                            { content.info && content.info.size ? filesize(content.info.size) : "" }
+                            { this.content.info && this.content.info.size ? filesize(this.content.info.size) : "" }
                         </div> }
                     </div> }
                 </span>
             );
         } else {
-            const extra = text ? (': ' + text) : '';
+            const extra = this.linkText ? (': ' + this.linkText) : '';
             return <span className="mx_MFileBody">
                 { placeholder }
                 { _t("Invalid file%(extra)s", { extra: extra }) }

src/i18n/strings/en_EN.json

@@ -1888,7 +1888,7 @@
     "Saturday": "Saturday",
     "Today": "Today",
     "Yesterday": "Yesterday",
-    "Downloading": "Downloading",
+    "Decrypting": "Decrypting",
     "Download": "Download",
     "View Source": "View Source",
     "Messages here are end-to-end encrypted. Verify %(displayName)s in their profile - tap on their avatar.": "Messages here are end-to-end encrypted. Verify %(displayName)s in their profile - tap on their avatar.",
@@ -1903,9 +1903,9 @@
     "Retry": "Retry",
     "Reply": "Reply",
     "Message Actions": "Message Actions",
+    "Download %(text)s": "Download %(text)s",
     "Error decrypting attachment": "Error decrypting attachment",
     "Decrypt %(text)s": "Decrypt %(text)s",
-    "Download %(text)s": "Download %(text)s",
     "Invalid file%(extra)s": "Invalid file%(extra)s",
     "Error decrypting image": "Error decrypting image",
     "Show image": "Show image",

src/utils/FileDownloader.ts

@@ -0,0 +1,103 @@
+/*
+Copyright 2021 The Matrix.org Foundation C.I.C.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+export type getIframeFn = () => HTMLIFrameElement; // eslint-disable-line @typescript-eslint/naming-convention
+
+export const DEFAULT_STYLES = {
+    imgSrc: "",
+    imgStyle: null, // css props
+    style: "",
+    textContent: "",
+};
+
+type DownloadOptions = {
+    blob: Blob;
+    name: string;
+    autoDownload?: boolean;
+    opts?: typeof DEFAULT_STYLES;
+};
+
+// set up the iframe as a singleton so we don't have to figure out destruction of it down the line.
+let managedIframe: HTMLIFrameElement;
+let onLoadPromise: Promise<void>;
+function getManagedIframe(): { iframe: HTMLIFrameElement, onLoadPromise: Promise<void> } {
+    if (managedIframe) return { iframe: managedIframe, onLoadPromise };
+
+    managedIframe = document.createElement("iframe");
+
+    // Need to append the iframe in order for the browser to load it.
+    document.body.appendChild(managedIframe);
+
+    // Dev note: the reassignment warnings are entirely incorrect here.
+
+    // @ts-ignore
+    // noinspection JSConstantReassignment
+    managedIframe.style = { display: "none" };
+    // @ts-ignore
+    // noinspection JSConstantReassignment
+    managedIframe.sandbox = "allow-scripts allow-downloads allow-downloads-without-user-activation";
+
+    onLoadPromise = new Promise(resolve => {
+        managedIframe.onload = () => {
+            resolve();
+        };
+        managedIframe.src = "usercontent/"; // XXX: Should come from the skin
+    });
+
+    return { iframe: managedIframe, onLoadPromise };
+}
+
+// TODO: If we decide to keep the download link behaviour, we should bring the style management into here.
+
+/**
+ * Helper to handle safe file downloads. This operates off an iframe for reasons described
+ * by the blob helpers. By default, this will use a hidden iframe to manage the download
+ * through a user content wrapper, but can be given an iframe reference if the caller needs
+ * additional control over the styling/position of the iframe itself.
+ */
+export class FileDownloader {
+    private onLoadPromise: Promise<void>;
+
+    /**
+     * Creates a new file downloader
+     * @param iframeFn Function to get a pre-configured iframe. Set to null to have the downloader
+     * use a generic, hidden, iframe.
+     */
+    constructor(private iframeFn: getIframeFn = null) {
+    }
+
+    private get iframe(): HTMLIFrameElement {
+        const iframe = this.iframeFn?.();
+        if (!iframe) {
+            const managed = getManagedIframe();
+            this.onLoadPromise = managed.onLoadPromise;
+            return managed.iframe;
+        }
+        this.onLoadPromise = null;
+        return iframe;
+    }
+
+    public async download({ blob, name, autoDownload = true, opts = DEFAULT_STYLES }: DownloadOptions) {
+        const iframe = this.iframe; // get the iframe first just in case we need to await onload
+        if (this.onLoadPromise) await this.onLoadPromise;
+        iframe.contentWindow.postMessage({
+            ...opts,
+            blob: blob,
+            download: name,
+            auto: autoDownload,
+        }, '*');
+    }
+}

src/utils/FileUtils.ts

@@ -26,12 +26,14 @@ import { _t } from '../languageHandler';
  * @param {IMediaEventContent} content The "content" key of the matrix event.
  * @param {string} fallbackText The fallback text
  * @param {boolean} withSize Whether to include size information. Default true.
+ * @param {boolean} shortened Ensure the extension of the file name is visible. Default false.
  * @return {string} the human readable link text for the attachment.
  */
 export function presentableTextForFile(
     content: IMediaEventContent,
     fallbackText = _t("Attachment"),
     withSize = true,
+    shortened = false,
 ): string {
     let text = fallbackText;
     if (content.body && content.body.length > 0) {
@@ -40,6 +42,21 @@ export function presentableTextForFile(
         text = content.body;
     }
 
+    // We shorten to 15 characters somewhat arbitrarily, and assume most files
+    // will have a 3 character (plus full stop) extension. The goal is to knock
+    // the label down to 15-25 characters, not perfect accuracy.
+    if (shortened && text.length > 19) {
+        const parts = text.split('.');
+        let fileName = parts.slice(0, parts.length - 1).join('.').substring(0, 15);
+        const extension = parts[parts.length - 1];
+
+        // Trim off any full stops from the file name to avoid a case where we
+        // add an ellipsis that looks really funky.
+        fileName = fileName.replace(/\.*$/g, '');
+
+        text = `${fileName}...${extension}`;
+    }
+
     if (content.info && content.info.size && withSize) {
         // If we know the size of the file then add it as human readable
         // string to the end of the link text so that the user knows how