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

[HOLD for payment 2023-06-30] [$1000] Text remains selected in a PDF attachment even after clicking on a blank space. #20098

Closed
1 of 6 tasks
kavimuru opened this issue Jun 2, 2023 · 55 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@kavimuru
Copy link

kavimuru commented Jun 2, 2023

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


Action Performed:

  1. Open new.expensify.com
  2. Navigate to a chat and send a PDF attachment.
  3. Open the attachment.
  4. Highlight text within the PDF
  5. Now click on blank space in the PDF

Expected Result:

Clicking on a blank space should unselect the text within the PDF attachment.

Actual Result:

The text remains highlighted when clicking on a blank space.

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.22-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
Notes/Photos/Videos: Any additional supporting documentation

Screen.Recording.2023-06-01.at.2.29.25.PM.mov
Recording.856.mp4

Expensify/Expensify Issue URL:
Issue reported by: @Tushu17
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1685610012204549

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~011482fa67cb29b12b
  • Upwork Job ID: 1665723287863721984
  • Last Price Increase: 2023-06-12
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jun 2, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 2, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jun 2, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@Nodebrute
Copy link
Contributor

Nodebrute commented Jun 4, 2023

Proposal

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

After selecting text in pdf attachment the text remains selected even when user clicks on blank space.

What is the root cause of that problem?

ex1
This user-select:none is causing this problem because of it whenever the user clicks in a blank space the text remains selected.

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

To over write this we have to use !important in https://github.com/Expensify/App/blob/main/assets/css/pdf.css file.
ex2

What alternative solutions did you explore? (Optional)

N/A

@melvin-bot melvin-bot bot added the Overdue label Jun 5, 2023
@trjExpensify
Copy link
Contributor

I can reproduce this on the latest staging, moving on external.

@melvin-bot melvin-bot bot removed the Overdue label Jun 5, 2023
@trjExpensify trjExpensify added External Added to denote the issue can be worked on by a contributor Overdue labels Jun 5, 2023
@melvin-bot melvin-bot bot changed the title Text remains selected in a PDF attachment even after clicking on a blank space. [$1000] Text remains selected in a PDF attachment even after clicking on a blank space. Jun 5, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 5, 2023

Job added to Upwork: https://www.upwork.com/jobs/~011482fa67cb29b12b

@melvin-bot
Copy link

melvin-bot bot commented Jun 5, 2023

Current assignee @trjExpensify is eligible for the External assigner, not assigning anyone new.

@melvin-bot
Copy link

melvin-bot bot commented Jun 5, 2023

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 5, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 5, 2023

Triggered auto assignment to @puneetlath (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@Thanos30
Copy link
Contributor

Thanos30 commented Jun 5, 2023

Proposal

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

We can't deselect the text highlighted inside a PDF when we click on an empty area

What is the root cause of that problem?

The root cause of the issue is the styling of the PageCanvas which contains the following part:

userSelect: 'none'

, a styling option that prevents users from affecting the document.

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

Since I don't want to interfere with imported modules' code, my solution would be to add a onMouseDown event on the Document of our PDFView component, that will clear our previous selection.

 handleDocumentMouseDown() {
        const selection = window.getSelection();
        if (selection && !selection.isCollapsed) {
          selection.removeAllRanges();
        }
      }

and then append this to the Document props:

    onMouseDown={this.handleDocumentMouseDown} // Added onMouseDown event handler

Video of solution:

Screen.Recording.2023-06-05.at.7.12.29.PM.mov

@parasharrajat
Copy link
Member

Reviewing...

@will0225
Copy link

will0225 commented Jun 6, 2023

Can I work on this?

@parasharrajat
Copy link
Member

Thanks for both of the proposals @Nodebrute @Thanos30.

@Nodebrute We don't prefer CSS-based solutions.
@Thanos30 This is just a hack.

Let's dig more to find a better solution that fits well with react.

@parasharrajat
Copy link
Member

parasharrajat commented Jun 7, 2023

@will0225 If you are interested, feel free to send a proposal.


This issue is open for proposals.

@Nodebrute
Copy link
Contributor

Nodebrute commented Jun 7, 2023

Updated

Proposal

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

Clicking on a blank space should unselect the text within the PDF attachment.

What is the root cause of that problem?

Open the pdf and select text. Notice when you click outside the text it remains selected but, when your pointer is over any text and you click on it, the text gets unselected.
ex4
Notice that all the child 's have pointer-events:all on them. This is why when you click on text it gets selected and a second click on the text will unselect the text. The canvas and parent div have user-select and pointer-events properties set to none. This is why, when we click outside the text it does not get unselected.

These styles are coming from textLayer of react pdf
ex5

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

To make this work properly, we have to set only one property to 'auto'.
React-pdf gives us a canvasRef and onLoadSuccess prop on Page.

canvasRef={(ref) => { this.state.pageRef = ref; }}
and onLoadSucces we change its style to 'auto'

This solution works.

ex7

What alternative solutions did you explore? (Optional)

We can overwrite styles in CSS files using !important or we can unselect text listening pointer events. React pdf also gives us inputRef that is passed to the main div rendered by page we can also use that approach

@parasharrajat
Copy link
Member

To make this work properly, we have to set only one property to 'auto'.
React-pdf gives us a canvasRef and onLoadSuccess prop on Page.

canvasRef={(ref) => { this.state.pageRef = ref; }}
and onLoadSucces we change its style to 'auto'

where do we set the property? Which property? How do we set the property?

@Nodebrute
Copy link
Contributor

Nodebrute commented Jun 7, 2023

This is the picture of working solution
ex8
Where do we set the property? On Page component in this file https://github.com/Expensify/App/blob/bd5db762cd07a0258d92462d8ddaffc02315854e/src/components/PDFView/index.js
Which property? here I am using canvasRef so we'll change userSelect property
How do we set property? onLoadSuccess prop.
ex9

@parasharrajat
Copy link
Member

parasharrajat commented Jun 7, 2023

Ok. Thanks for the clarification.

Still not sold on this. It comes down to manipulating styles directly on DOM (CSS). Isn't there any React-native/react way to solve this issue?

Why the user-select:none in the first place? No one answered that in the root cause. I feel like this user applied and we can get rid of this by configuration.

@allroundexperts
Copy link
Contributor

allroundexperts commented Jun 11, 2023

Proposal

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

Text remains selected in PDF attachment event after clicking on a blank space.

What is the root cause of that problem?

The root cause of this issue lies in the usage of an older version of react-pdf library. I'm not exactly sure about the exact line in the library is causing this issue but the later versions improve the canvas text rendering of the pdf and seem to fix this issue as well. This can be read in the next major version (6.0.0) change log.

I don't think that the problem is with user-select because even the latest version is applying the same styles as seen here. Also, the styles are not being applied due to a specific user configuration. As seen above, this style gets applied regardless of the configuration.

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

We need to update react-pdf from v5.7.2 to either the latest 7.x or 6.x. The latest release of the library improves the canvas rendering of pdf's a lot and also fixes this issue. Here's an example video with v6.0:

Screen.Recording.2023-06-12.at.3.46.05.AM.mov

What alternative solutions did you explore? (Optional)

None

@tylerkaraszewski
Copy link
Contributor

@puneetlath - giving this one back to you as I'm heading out on sabbatical.

@trjExpensify
Copy link
Contributor

👋 @parasharrajat @allroundexperts can you guys get to the checklist so we can proceed to payments? Thanks!

@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Jun 30, 2023
@trjExpensify
Copy link
Contributor

No change since Friday, awaiting the checklist!

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jul 3, 2023
@trjExpensify
Copy link
Contributor

Same, require the checklist to be filled out ahead of payments!

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jul 5, 2023
@trjExpensify
Copy link
Contributor

@parasharrajat @allroundexperts can you guys complete the checklist so we can proceed with payments, please?

@melvin-bot melvin-bot bot removed the Overdue label Jul 10, 2023
@parasharrajat
Copy link
Member

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@parasharrajat] The PR that introduced the bug has been identified. Link to the PR: NA. This was caused directly by the library.

  • [@parasharrajat] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: NA

  • [@parasharrajat] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: NA

  • [@parasharrajat] Determine if we should create a regression test for this bug. yes

  • [@parasharrajat] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Steps

  1. Open the Expensify App on Web.
  2. Navigate to a chat and send a PDF attachment.
  3. Open the PDF attachment.
  4. Highlight text within the PDF via cursor.
  5. Now click on the blank space in the PDF.
  6. Highlighted text should be unhighlighted.

Do you agree 👍 or 👎 ?

@parasharrajat
Copy link
Member

@trjExpensify Please hold my payments for now. Thanks. I will update you within 5 days.

@melvin-bot melvin-bot bot added the Overdue label Jul 13, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 14, 2023

@puneetlath, @trjExpensify, @parasharrajat, @allroundexperts Whoops! This issue is 2 days overdue. Let's get this updated quick!

@parasharrajat
Copy link
Member

@trjExpensify Payment requested 1K.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jul 15, 2023
@trjExpensify
Copy link
Contributor

Cool, thanks! Outside of that, I've proceeded on Upwork with the following:

@Tushu17 - $250 for the bug report (Offer sent)
@allroundexperts - $1,000 for the bug fix (Paid!)

As for the regression test, I think we can modify one of the existing test scripts to accommodate this (bold for emphasis). Issue created here.

  1. On a conversation, click on the + button in the compose box
  2. Click on add attachment
  3. Upload a PDF
  4. Verify a preview of the PDF is displayed before uploading to the conversation
  5. Click on upload
  6. Verify you can see the PDF in the conversation
  7. Click on the PDF preview
  8. Verify a larger preview of the PDF is displayed
  9. Verify the width of the send button doesn't extend to the far edges of the screen on desktop, web and wide devices
  10. Verify the modal background color matches the header dark green color (both should be the same shade)
  11. Highlight text within the PDF via cursor.
  12. Click on the blank space in the PDF.
  13. Verify the highlighted text is unhighlighted
  14. Close the PDF preview

@melvin-bot melvin-bot bot removed the Overdue label Jul 17, 2023
@Tushu17
Copy link
Contributor

Tushu17 commented Jul 17, 2023

@trjExpensify offer accepted. Thank you.

@trjExpensify
Copy link
Contributor

@Tushu17 paid!

@melvin-bot melvin-bot bot added the Overdue label Jul 19, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 20, 2023

@puneetlath, @trjExpensify, @parasharrajat, @allroundexperts Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot melvin-bot bot removed the Overdue label Jul 20, 2023
@anmurali
Copy link

Approved $1000 to Rajat based on #20098 (comment) (checklist completed since)

@trjExpensify
Copy link
Contributor

Neeeat! Thanks, closing! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests