App icon indicating copy to clipboard operation
App copied to clipboard

[$500] Web - Chat - You can't scroll the PDF right after opening it with the "down arrow" button

Open lanitochka17 opened this issue 1 year ago • 44 comments

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 1.4.28.0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:

Action Performed:

  1. Navigate to a chat
  2. Upload a PDF
  3. Open it
  4. Press the "down arrow" keyboard button

Expected Result:

I should be able to use the arrows without clicking

Actual Result:

You can't scroll the PDF right after opening it with the "down arrow" button. You need to click once for the function to work

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • [ ] Android: Native
  • [ ] Android: mWeb Chrome
  • [ ] iOS: Native
  • [ ] iOS: mWeb Safari
  • [x] MacOS: Chrome / Safari
  • [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/c8f29f90-636c-45fa-b250-aaa508296136

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~011e97d8eae76876e6
  • Upwork Job ID: 1748379977514749952
  • Last Price Increase: 2024-01-26

lanitochka17 avatar Jan 19 '24 16:01 lanitochka17

Triggered auto assignment to @alexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] avatar Jan 19 '24 16:01 melvin-bot[bot]

Job added to Upwork: https://www.upwork.com/jobs/~011e97d8eae76876e6

melvin-bot[bot] avatar Jan 19 '24 16:01 melvin-bot[bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @situchan (External)

melvin-bot[bot] avatar Jan 19 '24 16:01 melvin-bot[bot]

Proposal

Please re-state the problem that we are trying to solve in this issue.

You can't scroll the PDF right after opening it with the down or up buttons

What is the root cause of that problem?

the document is not focused after it's loaded

What changes do you think we should make in order to solve the problem?

we need to call the focus function in the setListAttributes this function is called in the outerRef of the List component

        ref.focus();

FYI as you can notice, the list component is shown after this loader indicator is removed

POC

https://github.com/Expensify/App/assets/59809993/336ed375-b60f-4cee-a171-6b0476cfdce3

abzokhattab avatar Jan 19 '24 17:01 abzokhattab

Proposal

Please re-state the problem that we are trying to solve in this issue.

Web - Chat - You can't scroll the PDF right after opening it with the "down arrow" button

What is the root cause of that problem?

After document is load success we don't focus to the document so we can't scroll it with the keyboard

What changes do you think we should make in order to solve the problem?

We will store the document ref and focus on it after document loaded success to avoid while document still loading user can click anywhere ST like download button...

  1. Update this line
    this.setListAttributes = this.setListAttributes.bind(this);
+   this.pdfDocumentRef = React.createRef(null);
  1. When document setListAttributes we will store the ref
    setListAttributes(ref) {
        if (!ref) {
            return;
        }
+       this.pdfDocumentRef.current = ref;
        ...
    }
  1. After that when document onDocumentLoadSuccess we will focus into the document
    onDocumentLoadSuccess(pdf) {
        const {numPages} = pdf;
+       if(this.pdfDocumentRef.current && this.pdfDocumentRef.current.focus) {
+           this.pdfDocumentRef.current.focus();
+       }
        ...
    }

POC

https://github.com/Expensify/App/assets/11959869/c6e016af-7d75-44fc-a362-ea259e736c73

What alternative solutions did you explore? (Optional)

Instead of storing ref at PDFView we can store ref at WebPDFDocument and focus on it when the document loaded success after that continue to call onDocumentLoadSuccess

suneox avatar Jan 19 '24 18:01 suneox

Proposal

Please re-state the problem that we are trying to solve in this issue.

A newly opened PDF cannot be scrolled with keyboard without clicking on it first

What is the root cause of that problem?

No focus is being set on the document

What changes do you think we should make in order to solve the problem?

react-pdf has built in props to handle this. ( the currently used open source package )


 <PDFViewer innerRef={node => (this.iframe = node)}>
          <Document onRender={() => this.iframe.focus()}>
          //PDF
          </Document>
</PDFViewer>

I will test the focus on a password protected document as well to ensure there's no conflicts.

What alternative solutions did you explore? (Optional)

Alternatively, you can set your focus by creating your own refs in React.

JeremyCroff avatar Jan 19 '24 22:01 JeremyCroff

I don't think it is a bug, even in mac pdf preview and chrome pdf viewer you can't scroll without clicking on the pdf.

Krishna2323 avatar Jan 19 '24 23:01 Krishna2323

@sofi-a It works fine without the timer just make sure to put this ref.focus()after this line

plus there is some lagging when putting the time .. the user has to wait 1 second to use the scroll

abzokhattab avatar Jan 21 '24 12:01 abzokhattab

@abzokhattab It didn't work for me without the timer and I set the duration randomly. It can be decreased if necessary.

sofi-a avatar Jan 21 '24 13:01 sofi-a

@abzokhattab you're right, calling ref.focus() after the ref.tabIndex = -1 statement works 👍.

sofi-a avatar Jan 21 '24 13:01 sofi-a

I'll remove my proposal since it's redundant.

sofi-a avatar Jan 21 '24 13:01 sofi-a

@situchan - any update here regarding these proposals?

alexpensify avatar Jan 22 '24 19:01 alexpensify

reviewing

situchan avatar Jan 22 '24 19:01 situchan

My original.proposal is currenly hidden because my github account is currently suspended while it's being recovered. The proposal should be in an email history.

The proposal was to use the react-pdf built in props to maintain focus

Proposal

Please re-state the problem that we are trying to solve in this issue.

PDFs opened in a new tab don't focus

What is the root cause of that problem?

This is not default behaviour, and must be coded for.

What changes do you think we should make in order to solve the problem?

We should leverage the react-pdf components and it's existing props to manage a focus callback on ready state of the document

 <PDFViewer innerRef={node => (this.iframe = node)}>
          <Document onRender={() => this.iframe.focus()}>
            // PDF implementation
          </Document>
        </PDFViewer>

What alternative solutions did you explore? (Optional)

jeremy-croff avatar Jan 22 '24 20:01 jeremy-croff

Sounds like a plan, keep us posted @situchan. Thanks!

alexpensify avatar Jan 24 '24 17:01 alexpensify

I think we should hold this for:

  • https://github.com/Expensify/App/pull/32800

situchan avatar Jan 25 '24 19:01 situchan

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] avatar Jan 26 '24 16:01 melvin-bot[bot]

Thanks, moving this to weekly and putting it on hold.

alexpensify avatar Jan 26 '24 18:01 alexpensify

Still holding

situchan avatar Jan 29 '24 14:01 situchan

🤦🏼 looks like my tags didn't stick last time. Moving this to Weekly!

alexpensify avatar Jan 31 '24 20:01 alexpensify

Weekly update: On Hold

alexpensify avatar Feb 07 '24 23:02 alexpensify

Weekly Update: On Hold

alexpensify avatar Feb 13 '24 22:02 alexpensify

Weekly Update: On Hold

alexpensify avatar Feb 21 '24 01:02 alexpensify

Weekly Update: @situchan it looks like #32800 is going to close. Should we close this one too or take this one off hold?

alexpensify avatar Feb 27 '24 22:02 alexpensify

@situchan can you please update here if we should close this one too? Thanks!

alexpensify avatar Mar 05 '24 22:03 alexpensify

I have checked this issue still happens, so i think we should focus after pdf file loaded successfully

suneox avatar Mar 06 '24 11:03 suneox

The holding PR was closed but the root cause still remains. There's another attempt to use focus trap in https://github.com/Expensify/App/issues/36476.

situchan avatar Mar 06 '24 11:03 situchan

@situchan - should we hold for https://github.com/Expensify/App/issues/36476 or update the focus like @suneox suggested? Thanks!

alexpensify avatar Mar 06 '24 14:03 alexpensify

Just to clarify my proposal, this line inside the setListAttributes function is gonna be reached only when the document is loaded successfully,

as per the source code of the Document component in the react-pdf library, the children components will be only rendered when the pdf is loaded successfully which means the setListAttributes should be called after the doc is loaded.

abzokhattab avatar Mar 06 '24 15:03 abzokhattab

Just to clarify my proposal, this line inside the setListAttributes function is gonna be reached only when the document is loaded successfully,

Based on the code base the event for PDFDocument loaded successfully at onDocumentLoadSuccess callback, and setListAttributes at this line is only keeping the ref for the List has import from react-window at this line

suneox avatar Mar 06 '24 15:03 suneox