Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix unable to scroll PDF files in preview #21714

Merged
merged 5 commits into from
Jul 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 2 additions & 14 deletions src/components/AttachmentView.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,30 +88,18 @@ function AttachmentView(props) {
// will appear with a source that is a blob
if (Str.isPDF(props.source) || (props.file && Str.isPDF(props.file.name || props.translate('attachmentView.unknownFilename')))) {
const sourceURL = props.isAuthTokenRequired ? addEncryptedAuthTokenToURL(props.source) : props.source;
const children = (
return (
<PDFView
onPress={props.onPress}
isFocused={props.isFocused}
sourceURL={sourceURL}
fileName={props.file.name}
style={styles.imageModalPDF}
onToggleKeyboard={props.onToggleKeyboard}
onScaleChanged={props.onScaleChanged}
onLoadComplete={() => !loadComplete && setLoadComplete(true)}
/>
);
return props.onPress ? (
<PressableWithoutFeedback
onPress={props.onPress}
disabled={loadComplete}
style={containerStyles}
accessibilityRole={CONST.ACCESSIBILITY_ROLE.IMAGEBUTTON}
accessibilityLabel={props.file.name || props.translate('attachmentView.unknownFilename')}
>
{children}
</PressableWithoutFeedback>
) : (
children
);
}

// For this check we use both source and file.name since temporary file source is a blob
Expand Down
18 changes: 17 additions & 1 deletion src/components/PDFView/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import withWindowDimensions from '../withWindowDimensions';
import withLocalize from '../withLocalize';
import Text from '../Text';
import compose from '../../libs/compose';
import PressableWithoutFeedback from '../Pressable/PressableWithoutFeedback';

class PDFView extends Component {
constructor(props) {
Expand Down Expand Up @@ -105,7 +106,7 @@ class PDFView extends Component {
this.props.onToggleKeyboard(isKeyboardOpen);
}

render() {
renderPDFView() {
const pdfContainerWidth = this.state.windowWidth - 100;
const pageWidthOnLargeScreen = pdfContainerWidth <= variables.pdfPageMaxWidth ? pdfContainerWidth : variables.pdfPageMaxWidth;
const pageWidth = this.props.isSmallScreenWidth ? this.state.windowWidth : pageWidthOnLargeScreen;
Expand Down Expand Up @@ -160,6 +161,21 @@ class PDFView extends Component {
</View>
);
}

render() {
return this.props.onPress ? (
<PressableWithoutFeedback
onPress={this.props.onPress}
style={[styles.flex1, styles.flexRow, styles.alignSelfStretch]}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding styles.flexRow caused a regression where failed to load message is not properly visible on native #21714

accessibilityRole={CONST.ACCESSIBILITY_ROLE.IMAGEBUTTON}
accessibilityLabel={this.props.fileName || this.props.translate('attachmentView.unknownFilename')}
>
{this.renderPDFView()}
</PressableWithoutFeedback>
) : (
this.renderPDFView()
);
}
}

PDFView.propTypes = pdfViewPropTypes.propTypes;
Expand Down
21 changes: 20 additions & 1 deletion src/components/PDFView/index.native.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@ import * as StyleUtils from '../../styles/StyleUtils';
import FullScreenLoadingIndicator from '../FullscreenLoadingIndicator';
import Text from '../Text';
import PDFPasswordForm from './PDFPasswordForm';
import PressableWithoutFeedback from '../Pressable/PressableWithoutFeedback';
import {propTypes as pdfViewPropTypes, defaultProps} from './pdfViewPropTypes';
import compose from '../../libs/compose';
import withWindowDimensions from '../withWindowDimensions';
import withKeyboardState, {keyboardStatePropTypes} from '../withKeyboardState';
import withLocalize from '../withLocalize';
import CONST from '../../CONST';

const propTypes = {
...pdfViewPropTypes,
Expand Down Expand Up @@ -41,6 +43,7 @@ class PDFView extends Component {
shouldShowLoadingIndicator: true,
isPasswordInvalid: false,
failedToLoadPDF: false,
successToLoadPDF: false,
password: '',
};
this.initiatePasswordChallenge = this.initiatePasswordChallenge.bind(this);
Expand Down Expand Up @@ -118,11 +121,12 @@ class PDFView extends Component {
this.setState({
shouldRequestPassword: false,
shouldShowLoadingIndicator: false,
successToLoadPDF: true,
});
this.props.onLoadComplete();
}

render() {
renderPDFView() {
const pdfStyles = [styles.imageModalPDF, StyleUtils.getWidthAndHeightStyle(this.props.windowWidth, this.props.windowHeight)];

// If we haven't yet successfully validated the password and loaded the PDF,
Expand Down Expand Up @@ -169,6 +173,21 @@ class PDFView extends Component {
</View>
);
}

render() {
return this.props.onPress && !this.state.successToLoadPDF ? (
Copy link
Contributor

@allroundexperts allroundexperts Aug 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding this condition introduced #23475 bug. this.props.onPress always returned true because it was defined in the default props.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition caused the #42103 (comment) bug. We should've disabled the component instead of conditional rendering.

<PressableWithoutFeedback
onPress={this.props.onPress}
style={[styles.flex1, styles.flexRow, styles.alignSelfStretch]}
accessibilityRole={CONST.ACCESSIBILITY_ROLE.IMAGEBUTTON}
accessibilityLabel={this.props.fileName || this.props.translate('attachmentView.unknownFilename')}
>
{this.renderPDFView()}
</PressableWithoutFeedback>
) : (
this.renderPDFView()
);
}
}

PDFView.propTypes = propTypes;
Expand Down
4 changes: 4 additions & 0 deletions src/components/PDFView/pdfViewPropTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ const propTypes = {
/** URL to full-sized image */
sourceURL: PropTypes.string,

/** PDF file name */
fileName: PropTypes.string,

/** Additional style props */
style: stylePropTypes,

Expand All @@ -29,6 +32,7 @@ const propTypes = {

const defaultProps = {
sourceURL: '',
fileName: '',
style: {},
onPress: () => {},
onToggleKeyboard: () => {},
Expand Down
Loading