Skip to content

Fix freezes when previewing a large PDF file #22760

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

Merged
merged 16 commits into from
Aug 8, 2023
Merged
Show file tree
Hide file tree
Changes from 7 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
43 changes: 36 additions & 7 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@
"react-pdf": "^6.2.2",
"react-plaid-link": "3.3.2",
"react-web-config": "^1.0.0",
"react-window": "^1.8.9",
"save": "^2.4.0",
"semver": "^7.3.8",
"shim-keyboard-event-key": "^1.0.3",
Expand Down
129 changes: 107 additions & 22 deletions src/components/PDFView/index.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import _ from 'underscore';
import React, {Component} from 'react';
import {View, Dimensions} from 'react-native';
import {View} from 'react-native';
import {Document, Page, pdfjs} from 'react-pdf/dist/esm/entry.webpack';
import pdfWorkerSource from 'pdfjs-dist/legacy/build/pdf.worker';
import {VariableSizeList as List} from 'react-window';
import FullScreenLoadingIndicator from '../FullscreenLoadingIndicator';
import styles from '../../styles/styles';
import variables from '../../styles/variables';
Expand All @@ -14,12 +15,20 @@ import withLocalize from '../withLocalize';
import Text from '../Text';
import compose from '../../libs/compose';

/**
* Each page has a default border. The app should take this size into account
* when calculates the page width and height.
*/
const PAGE_BORDER = 9;

class PDFView extends Component {
constructor(props) {
super(props);
this.state = {
numPages: null,
windowWidth: Dimensions.get('window').width,
pageViewports: [],
containerWidth: props.windowWidth,
containerHeight: props.windowHeight,
shouldRequestPassword: false,
isPasswordInvalid: false,
isKeyboardOpen: false,
Expand All @@ -28,6 +37,9 @@ class PDFView extends Component {
this.initiatePasswordChallenge = this.initiatePasswordChallenge.bind(this);
this.attemptPDFLoad = this.attemptPDFLoad.bind(this);
this.toggleKeyboardOnSmallScreens = this.toggleKeyboardOnSmallScreens.bind(this);
this.calculatePageHeight = this.calculatePageHeight.bind(this);
this.calculatePageWidth = this.calculatePageWidth.bind(this);
this.renderPage = this.renderPage.bind(this);

const workerBlob = new Blob([pdfWorkerSource], {type: 'text/javascript'});
pdfjs.GlobalWorkerOptions.workerSrc = URL.createObjectURL(workerBlob);
Expand All @@ -45,21 +57,66 @@ class PDFView extends Component {
}

/**
* Upon successful document load, set the number of pages on PDF,
* Upon successful document load, combine an array of page viewports,
* set the number of pages on PDF,
* hide/reset PDF password form, and notify parent component that
* user input is no longer required.
*
* @param {*} {numPages} No of pages in the rendered PDF
* @param {*} pdf The PDF file instance
* @memberof PDFView
*/
onDocumentLoadSuccess({numPages}) {
this.setState({
numPages,
shouldRequestPassword: false,
isPasswordInvalid: false,
onDocumentLoadSuccess(pdf) {
const numPages = pdf.numPages;

Promise.all(
_.times(numPages, (index) => {
const pageNumber = index + 1;

return pdf.getPage(pageNumber).then((page) => page.getViewport({scale: 1}));
}),
).then((pageViewports) => {
this.setState({
pageViewports,
numPages,
shouldRequestPassword: false,
isPasswordInvalid: false,
});
});
}

/**
* Calculates a proper page height.
* It is based on a ratio between the specific page viewport width and provided page width.
* Also, the app should take into account the page borders.
* @param {*} pageIndex
* @returns {Number}
*/
calculatePageHeight(pageIndex) {
if (this.state.pageViewports.length === 0) {
throw new Error('calculatePageHeight() called too early');
}

const pageViewport = this.state.pageViewports[pageIndex];
const pageWidth = this.calculatePageWidth();
const scale = pageWidth / pageViewport.width;
const actualHeight = pageViewport.height * scale + PAGE_BORDER * 2;
Copy link
Member

@rushatgabhane rushatgabhane Aug 3, 2023

Choose a reason for hiding this comment

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

Looks like we're using the virtual list concept here. Just out of curiosity, what's stopping us from using this solution on native?

If I'm not wrong, react-window is a web only library. Why not use FlatList and render x pages only?

Copy link
Contributor

@situchan situchan Aug 4, 2023

Choose a reason for hiding this comment

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

AFAIK:

Looks like we're using the virtual list concept here. Just out of curiosity, what's stopping us from using this solution on native?

The libraries web and native are using are completely different.
As this issue happens only on mSafari, it's fine to optimize only react-pdf (web).
react-native-pdf (native) is "perfect" library and we haven't found any need of customization yet.

If I'm not wrong, react-window is a web only library. Why not use FlatList and render x pages only?

FlatList can only be used on web combined with react-native-web. As react-pdf is a pure web library (not depending on react-native-web), no need to integrate with FlatList.
And we're going to publish our own npm module to be used for web-only projects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In short, FlatList does not work the same way on web as it does on native platforms.


return actualHeight;
}

/**
* Calculates a proper page width.
* It depends on a screen size. Also, the app should take into account the page borders.
* @returns {Number}
*/
calculatePageWidth() {
const pdfContainerWidth = this.state.containerWidth;
const pageWidthOnLargeScreen = pdfContainerWidth <= variables.pdfPageMaxWidth ? pdfContainerWidth : variables.pdfPageMaxWidth;
const pageWidth = this.props.isSmallScreenWidth ? this.state.containerWidth : pageWidthOnLargeScreen;

return pageWidth + PAGE_BORDER * 2;
}

/**
* Initiate password challenge process. The react-pdf/Document
* component calls this handler to indicate that a PDF requires a
Expand Down Expand Up @@ -105,10 +162,32 @@ class PDFView extends Component {
this.props.onToggleKeyboard(isKeyboardOpen);
}

/**
* Renders a specific page based on its index.
* It includes a wrapper to apply virtualized styles.
* @param {Number} index
* @param {Object} style
* @returns {JSX.Element}
*/
renderPage({index, style}) {
const pageWidth = this.calculatePageWidth();

return (
<View style={style}>
<Page
key={`page_${index}`}
width={pageWidth}
pageIndex={index}
// This needs to be empty to avoid multiple loading texts which show per page and look ugly
// See https://github.com/Expensify/App/issues/14358 for more details
loading=""
/>
</View>
);
}

render() {
const pdfContainerWidth = this.state.windowWidth - 100;
const pageWidthOnLargeScreen = pdfContainerWidth <= variables.pdfPageMaxWidth ? pdfContainerWidth : variables.pdfPageMaxWidth;
const pageWidth = this.props.isSmallScreenWidth ? this.state.windowWidth : pageWidthOnLargeScreen;
const pageWidth = this.calculatePageWidth();
const outerContainerStyle = [styles.w100, styles.h100, styles.justifyContentCenter, styles.alignItemsCenter];

// If we're requesting a password then we need to hide - but still render -
Expand All @@ -122,7 +201,11 @@ class PDFView extends Component {
<View
focusable
style={pdfContainerStyle}
onLayout={(event) => this.setState({windowWidth: event.nativeEvent.layout.width})}
onLayout={({
nativeEvent: {
layout: {width, height},
},
}) => this.setState({containerWidth: width, containerHeight: height})}
>
<Document
error={<Text style={[styles.textLabel, styles.textLarge]}>{this.props.translate('attachmentView.failedToLoadPDF')}</Text>}
Expand All @@ -136,16 +219,18 @@ class PDFView extends Component {
onLoadSuccess={this.onDocumentLoadSuccess}
onPassword={this.initiatePasswordChallenge}
>
{_.map(_.range(this.state.numPages), (v, index) => (
<Page
{this.state.pageViewports.length > 0 && (
<List
style={styles.PDFViewList}
width={pageWidth}
key={`page_${index + 1}`}
pageNumber={index + 1}
// This needs to be empty to avoid multiple loading texts which show per page and look ugly
// See https://github.com/Expensify/App/issues/14358 for more details
loading=""
/>
))}
height={this.state.containerHeight}
estimatedItemSize={this.calculatePageHeight(0)}
itemCount={this.state.numPages}
itemSize={this.calculatePageHeight}
>
{this.renderPage}
</List>
)}
</Document>
</View>
{this.state.shouldRequestPassword && (
Expand Down
5 changes: 4 additions & 1 deletion src/styles/styles.js
Original file line number Diff line number Diff line change
Expand Up @@ -2043,10 +2043,13 @@ const styles = {
height: '100%',
justifyContent: 'center',
overflow: 'hidden',
overflowY: 'auto',
alignItems: 'center',
},

PDFViewList: {
overflowX: 'hidden',
},

pdfPasswordForm: {
wideScreenWidth: {
width: 350,
Expand Down