From 1173ca3d97c9027985ac202d05aade396a32cdc6 Mon Sep 17 00:00:00 2001 From: Harrison Healey Date: Wed, 1 May 2019 09:34:45 -0400 Subject: [PATCH] MM-14030 Don't show large gifs in posts (#2750) * MM-14030 Don't show large gifs in posts * Re-add default argument --- app/components/markdown/markdown.js | 4 +- .../markdown/markdown_image/markdown_image.js | 48 +++++++----- .../message_attachments/attachment_fields.js | 2 +- .../message_attachments/attachment_image.js | 27 ++++--- ...ement_pretext.js => attachment_pretext.js} | 2 +- .../message_attachments/attachment_text.js | 2 +- .../message_attachments/message_attachment.js | 4 +- app/components/post_attachment_image/index.js | 73 +++++++++++++++++++ .../post_attachment_opengraph.js | 14 ++-- .../post_attachment_opengraph.test.js | 2 +- app/components/post_body/post_body.js | 2 +- .../post_body_additional_content.js | 40 ++++------ app/utils/images.js | 16 ++++ app/utils/images.test.js | 48 +++++++++++- 14 files changed, 209 insertions(+), 75 deletions(-) rename app/components/message_attachments/{attachement_pretext.js => attachment_pretext.js} (96%) create mode 100644 app/components/post_attachment_image/index.js diff --git a/app/components/markdown/markdown.js b/app/components/markdown/markdown.js index 7c22b476f..6acf3e1c0 100644 --- a/app/components/markdown/markdown.js +++ b/app/components/markdown/markdown.js @@ -43,7 +43,7 @@ export default class Markdown extends PureComponent { baseTextStyle: CustomPropTypes.Style, blockStyles: PropTypes.object, channelMentions: PropTypes.object, - imageMetadata: PropTypes.object, + imagesMetadata: PropTypes.object, isEdited: PropTypes.bool, isReplyPost: PropTypes.bool, isSearchResult: PropTypes.bool, @@ -186,7 +186,7 @@ export default class Markdown extends PureComponent { return ( { - let source = props.source; + getSource = () => { + let source = this.props.source; if (source.startsWith('/')) { - source = props.serverURL + '/' + source; + source = this.props.serverURL + '/' + source; } return source; @@ -112,6 +115,7 @@ export default class MarkdownImage extends React.Component { } this.setState({ + failed: false, originalHeight: height, originalWidth: width, }); @@ -203,6 +207,10 @@ export default class MarkdownImage extends React.Component { }; render() { + if (isGifTooLarge(this.props.imagesMetadata?.[this.props.source])) { + return null; + } + let image = null; const {originalHeight, originalWidth, uri} = this.state; const {height, width} = calculateDimensions(originalHeight, originalWidth, this.getViewPortWidth()); diff --git a/app/components/message_attachments/attachment_fields.js b/app/components/message_attachments/attachment_fields.js index dd42f57f0..67d76dda4 100644 --- a/app/components/message_attachments/attachment_fields.js +++ b/app/components/message_attachments/attachment_fields.js @@ -86,7 +86,7 @@ export default class AttachmentFields extends PureComponent { baseTextStyle={baseTextStyle} textStyles={textStyles} blockStyles={blockStyles} - imageMetadata={metadata?.images} + imagesMetadata={metadata?.images} value={(field.value || '')} navigator={navigator} onPermalinkPress={onPermalinkPress} diff --git a/app/components/message_attachments/attachment_image.js b/app/components/message_attachments/attachment_image.js index 5dede251a..249c1e9ec 100644 --- a/app/components/message_attachments/attachment_image.js +++ b/app/components/message_attachments/attachment_image.js @@ -6,7 +6,7 @@ import PropTypes from 'prop-types'; import {Image, TouchableWithoutFeedback, View} from 'react-native'; import ProgressiveImage from 'app/components/progressive_image'; -import {previewImageAtIndex, calculateDimensions} from 'app/utils/images'; +import {isGifTooLarge, previewImageAtIndex, calculateDimensions} from 'app/utils/images'; import ImageCacheManager from 'app/utils/image_cache_manager'; import {changeOpacity, makeStyleSheetFromTheme} from 'app/utils/theme'; @@ -17,8 +17,8 @@ export default class AttachmentImage extends PureComponent { static propTypes = { deviceHeight: PropTypes.number.isRequired, deviceWidth: PropTypes.number.isRequired, + imageMetadata: PropTypes.object, imageUrl: PropTypes.string, - metadata: PropTypes.object, navigator: PropTypes.object.isRequired, theme: PropTypes.object.isRequired, }; @@ -34,12 +34,11 @@ export default class AttachmentImage extends PureComponent { componentDidMount() { this.mounted = true; - const {imageUrl, metadata} = this.props; + const {imageUrl, imageMetadata} = this.props; this.setViewPortMaxWidth(); - if (metadata?.images?.[imageUrl]) { - const img = metadata.images[imageUrl]; - this.setImageDimensionsFromMeta(null, img); + if (imageMetadata) { + this.setImageDimensionsFromMeta(null, imageMetadata); } if (imageUrl) { @@ -88,16 +87,16 @@ export default class AttachmentImage extends PureComponent { } }; - setImageDimensionsFromMeta = (imageUri, img) => { - const dimensions = calculateDimensions(img.height, img.width, this.maxImageWidth); - this.setImageDimensions(imageUri, dimensions, img.width, img.height); + setImageDimensionsFromMeta = (imageUri, imageMetadata) => { + const dimensions = calculateDimensions(imageMetadata.height, imageMetadata.width, this.maxImageWidth); + this.setImageDimensions(imageUri, dimensions, imageMetadata.width, imageMetadata.height); }; setImageUrl = (imageURL) => { - const {imageUrl: attachmentImageUrl, metadata} = this.props; + const {imageMetadata} = this.props; - if (metadata?.images?.[attachmentImageUrl]) { - this.setImageDimensionsFromMeta(imageURL, metadata.images[attachmentImageUrl]); + if (imageMetadata) { + this.setImageDimensionsFromMeta(imageURL, imageMetadata); return; } @@ -114,10 +113,10 @@ export default class AttachmentImage extends PureComponent { }; render() { - const {theme} = this.props; + const {imageMetadata, theme} = this.props; const {hasImage, height, imageUri, width} = this.state; - if (!hasImage) { + if (!hasImage || isGifTooLarge(imageMetadata)) { return null; } diff --git a/app/components/message_attachments/attachement_pretext.js b/app/components/message_attachments/attachment_pretext.js similarity index 96% rename from app/components/message_attachments/attachement_pretext.js rename to app/components/message_attachments/attachment_pretext.js index 48080bfac..fcb1e900d 100644 --- a/app/components/message_attachments/attachement_pretext.js +++ b/app/components/message_attachments/attachment_pretext.js @@ -40,7 +40,7 @@ export default class AttachmentPreText extends PureComponent { baseTextStyle={baseTextStyle} textStyles={textStyles} blockStyles={blockStyles} - imageMetadata={metadata?.images} + imagesMetadata={metadata?.images} value={value} navigator={navigator} onPermalinkPress={onPermalinkPress} diff --git a/app/components/message_attachments/attachment_text.js b/app/components/message_attachments/attachment_text.js index 9a3718362..66ef572ca 100644 --- a/app/components/message_attachments/attachment_text.js +++ b/app/components/message_attachments/attachment_text.js @@ -95,7 +95,7 @@ export default class AttachmentText extends PureComponent { baseTextStyle={baseTextStyle} textStyles={textStyles} blockStyles={blockStyles} - imageMetadata={metadata?.images} + imagesMetadata={metadata?.images} value={value} navigator={navigator} onPermalinkPress={onPermalinkPress} diff --git a/app/components/message_attachments/message_attachment.js b/app/components/message_attachments/message_attachment.js index 714620ade..eafba47aa 100644 --- a/app/components/message_attachments/message_attachment.js +++ b/app/components/message_attachments/message_attachment.js @@ -12,7 +12,7 @@ import AttachmentActions from './attachment_actions'; import AttachmentAuthor from './attachment_author'; import AttachmentFields from './attachment_fields'; import AttachmentImage from './attachment_image'; -import AttachmentPreText from './attachement_pretext'; +import AttachmentPreText from './attachment_pretext'; import AttachmentText from './attachment_text'; import AttachmentThumbnail from './attachment_thumbnail'; import AttachmentTitle from './attachment_title'; @@ -119,7 +119,7 @@ export default class MessageAttachment extends PureComponent { deviceHeight={deviceHeight} deviceWidth={deviceWidth} imageUrl={attachment.image_url} - metadata={metadata} + imageMetadata={metadata?.images?.[attachment.image_url]} navigator={navigator} theme={theme} /> diff --git a/app/components/post_attachment_image/index.js b/app/components/post_attachment_image/index.js new file mode 100644 index 000000000..ff6c6aeb0 --- /dev/null +++ b/app/components/post_attachment_image/index.js @@ -0,0 +1,73 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import PropTypes from 'prop-types'; +import React from 'react'; +import {StyleSheet, TouchableWithoutFeedback, View} from 'react-native'; + +import ProgressiveImage from 'app/components/progressive_image'; +import {isGifTooLarge} from 'app/utils/images'; + +export default class PostAttachmentImage extends React.PureComponent { + static propTypes = { + height: PropTypes.number.isRequired, + imageMetadata: PropTypes.object, + onError: PropTypes.func.isRequired, + onImagePress: PropTypes.func.isRequired, + uri: PropTypes.string, + width: PropTypes.number.isRequired, + }; + + static defaultProps = { + frameCount: 0, + }; + + constructor(props) { + super(props); + + this.image = React.createRef(); + } + + handlePress = () => { + this.props.onImagePress(this.image.current); + }; + + render() { + if (isGifTooLarge(this.props.imageMetadata)) { + return null; + } + + // Note that TouchableWithoutFeedback only works if its child is a View + + return ( + + + + + + ); + } +} + +const styles = StyleSheet.create({ + imageContainer: { + alignItems: 'flex-start', + justifyContent: 'flex-start', + marginBottom: 6, + marginTop: 10, + }, + image: { + alignItems: 'center', + borderRadius: 3, + justifyContent: 'center', + marginVertical: 1, + }, +}); diff --git a/app/components/post_attachment_opengraph/post_attachment_opengraph.js b/app/components/post_attachment_opengraph/post_attachment_opengraph.js index de1ddeb2c..5e50e6107 100644 --- a/app/components/post_attachment_opengraph/post_attachment_opengraph.js +++ b/app/components/post_attachment_opengraph/post_attachment_opengraph.js @@ -28,7 +28,7 @@ export default class PostAttachmentOpenGraph extends PureComponent { }).isRequired, deviceHeight: PropTypes.number.isRequired, deviceWidth: PropTypes.number.isRequired, - imageMetadata: PropTypes.object, + imagesMetadata: PropTypes.object, isReplyPost: PropTypes.bool, link: PropTypes.string.isRequired, navigator: PropTypes.object.isRequired, @@ -78,7 +78,7 @@ export default class PostAttachmentOpenGraph extends PureComponent { }; } - const {imageMetadata} = this.props; + const {imagesMetadata} = this.props; const bestDimensions = { width: this.getViewPostWidth(), height: MAX_IMAGE_HEIGHT, @@ -87,8 +87,8 @@ export default class PostAttachmentOpenGraph extends PureComponent { const bestImage = getNearestPoint(bestDimensions, data.images, 'width', 'height'); const imageUrl = bestImage.secure_url || bestImage.url; let ogImage; - if (imageMetadata && imageMetadata[imageUrl]) { - ogImage = imageMetadata[imageUrl]; + if (imagesMetadata && imagesMetadata[imageUrl]) { + ogImage = imagesMetadata[imageUrl]; } if (!ogImage) { @@ -124,12 +124,12 @@ export default class PostAttachmentOpenGraph extends PureComponent { }; getImageSize = (imageUrl) => { - const {imageMetadata, openGraphData} = this.props; + const {imagesMetadata, openGraphData} = this.props; const {openGraphImageUrl} = this.state; let ogImage; - if (imageMetadata && imageMetadata[openGraphImageUrl]) { - ogImage = imageMetadata[openGraphImageUrl]; + if (imagesMetadata && imagesMetadata[openGraphImageUrl]) { + ogImage = imagesMetadata[openGraphImageUrl]; } if (!ogImage) { diff --git a/app/components/post_attachment_opengraph/post_attachment_opengraph.test.js b/app/components/post_attachment_opengraph/post_attachment_opengraph.test.js index 146dd4fa4..d86e41e65 100644 --- a/app/components/post_attachment_opengraph/post_attachment_opengraph.test.js +++ b/app/components/post_attachment_opengraph/post_attachment_opengraph.test.js @@ -28,7 +28,7 @@ describe('PostAttachmentOpenGraph', () => { }, deviceHeight: 600, deviceWidth: 400, - imageMetadata: { + imagesMetadata: { 'https://www.mattermost.org/wp-content/uploads/2016/03/logoHorizontal_WS.png': { width: 1165, height: 265, diff --git a/app/components/post_body/post_body.js b/app/components/post_body/post_body.js index 35cab34ef..df6ad9f26 100644 --- a/app/components/post_body/post_body.js +++ b/app/components/post_body/post_body.js @@ -396,7 +396,7 @@ export default class PostBody extends PureComponent { baseTextStyle={messageStyle} blockStyles={blockStyles} channelMentions={postProps.channel_mentions} - imageMetadata={metadata?.images} + imagesMetadata={metadata?.images} isEdited={hasBeenEdited} isReplyPost={isReplyPost} isSearchResult={isSearchResult} diff --git a/app/components/post_body_additional_content/post_body_additional_content.js b/app/components/post_body_additional_content/post_body_additional_content.js index c2a62bdfd..a4891055a 100644 --- a/app/components/post_body_additional_content/post_body_additional_content.js +++ b/app/components/post_body_additional_content/post_body_additional_content.js @@ -9,13 +9,12 @@ import { Linking, Platform, StyleSheet, - TouchableWithoutFeedback, TouchableOpacity, - View, } from 'react-native'; import {YouTubeStandaloneAndroid, YouTubeStandaloneIOS} from 'react-native-youtube'; import {intlShape} from 'react-intl'; +import PostAttachmentImage from 'app/components/post_attachment_image'; import ProgressiveImage from 'app/components/progressive_image'; import CustomPropTypes from 'app/constants/custom_prop_types'; @@ -199,7 +198,7 @@ export default class PostBodyAdditionalContent extends PureComponent { link={link} navigator={navigator} openGraphData={openGraphData} - imageMetadata={metadata && metadata.images} + imagesMetadata={metadata && metadata.images} theme={theme} /> ); @@ -215,7 +214,6 @@ export default class PostBodyAdditionalContent extends PureComponent { link = shortenedLink; } const {width, height, uri} = this.state; - const imgHeight = height; if (link) { if (isYouTube) { @@ -225,14 +223,13 @@ export default class PostBodyAdditionalContent extends PureComponent { return ( - - - - + ); } } @@ -405,7 +397,7 @@ export default class PostBodyAdditionalContent extends PureComponent { this.setState({linkLoadError: true}); }; - handlePreviewImage = () => { + handlePreviewImage = (imageRef) => { const {shortenedLink} = this.state; let {link} = this.props; const {navigator} = this.props; @@ -430,7 +422,7 @@ export default class PostBodyAdditionalContent extends PureComponent { }, }]; - previewImageAtIndex(navigator, [this.refs.item], 0, files); + previewImageAtIndex(navigator, [imageRef], 0, files); }; playYouTubeVideo = () => { diff --git a/app/utils/images.js b/app/utils/images.js index 7a6ea019a..eed74595d 100644 --- a/app/utils/images.js +++ b/app/utils/images.js @@ -104,3 +104,19 @@ function getItemMeasures(index, cb) { }); }); } + +const MAX_GIF_SIZE = 100 * 1024 * 1024; + +// isGifTooLarge returns true if we think that the GIF may cause the device to run out of memory when rendered +// based on the image's dimensions and frame count. +export function isGifTooLarge(imageMetadata) { + if (imageMetadata?.format !== 'gif') { + // Not a gif or from an older server that doesn't count frames + return false; + } + + const {frame_count: frameCount, height, width} = imageMetadata; + + // Try to estimate the in-memory size of the gif to prevent the device out of memory + return width * height * frameCount > MAX_GIF_SIZE; +} diff --git a/app/utils/images.test.js b/app/utils/images.test.js index d0866603d..dc2986402 100644 --- a/app/utils/images.test.js +++ b/app/utils/images.test.js @@ -1,7 +1,7 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -import {calculateDimensions} from 'app/utils/images'; +import {calculateDimensions, isGifTooLarge} from 'app/utils/images'; import { IMAGE_MAX_HEIGHT, IMAGE_MIN_DIMENSION, @@ -88,3 +88,49 @@ describe('Images calculateDimensions', () => { expect(height).toEqual(500); }); }); + +describe('isGifTooLarge', () => { + const testCases = [ + { + name: 'no image metadata', + imageMetadata: null, + expected: false, + }, + { + name: 'not a gif', + imageMetadata: {format: 'png', frame_count: 0, height: 1000, width: 1000}, + expected: false, + }, + { + name: 'an image without a format/frame count', + imageMetadata: {height: 1000, width: 1000}, + expected: false, + }, + { + name: 'a static gif', + imageMetadata: {format: 'gif', frame_count: 1, height: 600, width: 500}, + expected: false, + }, + { + name: 'a regular animated gif', + imageMetadata: {format: 'gif', frame_count: 40, height: 600, width: 500}, + expected: false, + }, + { + name: 'a very large animated gif', + imageMetadata: {format: 'gif', frame_count: 40, height: 100000, width: 100000}, + expected: true, + }, + { + name: 'a very long animated gif', + imageMetadata: {format: 'gif', frame_count: 2000, height: 1000, width: 1000}, + expected: true, + }, + ]; + + for (const testCase of testCases) { + test(testCase.name, () => { + expect(isGifTooLarge(testCase.imageMetadata)).toBe(testCase.expected); + }); + } +});