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 the "Text selection using mouse doesn't jump when hovering on an empty area in a single page" integration test #18774

Closed
timvandermeij opened this issue Sep 22, 2024 · 3 comments · Fixed by #18923

Comments

@timvandermeij
Copy link
Contributor

This integration test consistently fails locally for me on Arch Linux with the following traceback:

Failures:
1) Text layer Text selection using mouse doesn't jump when hovering on an empty area in a single page
  Message:
    In chrome:
        Expected '' to roughly match 'code sequences, records
        them, and compiles them to fast native code. We call suc'.
  Stack:
        at <Jasmine>
        at file:///home/timvandermeij/Documenten/Ontwikkeling/pdf.js/Code/test/integration/text_layer_spec.mjs:138:18
        at async Promise.all (index 1)
        at async UserContext.<anonymous> (file:///home/timvandermeij/Documenten/Ontwikkeling/pdf.js/Code/test/integration/text_layer_spec.mjs:116:11)

I haven't seen this happening on the bots. We should figure out what could cause this.

@nicolo-ribaudo
Copy link
Contributor

I take a look, if I can reproduce it

@timvandermeij
Copy link
Contributor Author

timvandermeij commented Oct 6, 2024

I have bisected this problem (using git bisect) and found the commit in which this integration test broke:

68332ec23670c020c8aade17d24256b86ccd8a6d is the first bad commit
commit 68332ec23670c020c8aade17d24256b86ccd8a6d (HEAD)
Author: Calixte Denizet <[email protected]>
Date:   Thu Sep 5 17:05:22 2024 +0200

    Avoid to have a white line around the canvas
    
    The canvas must have the same dims as the page in order to avoid to see the page
    background.

 src/display/display_utils.js     |   8 ++++++--
 test/integration/viewer_spec.mjs |  74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 test/pdfs/.gitignore             |   1 +
 test/pdfs/issue18694.pdf         | Bin 0 -> 7531 bytes
 test/unit/ui_utils_spec.js       |  14 ++++++++++++++
 web/pdf_page_view.js             |  31 ++++++++++++++++++++++++++-----
 web/pdf_viewer.css               |   5 +++++
 web/ui_utils.js                  |  20 ++++++++++++++++++++
 8 files changed, 146 insertions(+), 7 deletions(-)
 create mode 100755 test/pdfs/issue18694.pdf

I have confirmed that if I revert this commit for the current master branch (with the dependencies installed from the current master branch too) that the problem is indeed resolved, so it doesn't appear to be related to e.g. the Puppeteer version or the test itself but rather to the contents of this patch.

I'm not sure why this only fails on Chrome and only locally (on Arch Linux), but fortunately it does fail consistently which makes it easier to reproduce and debug. I have found that changing https://github.com/mozilla/pdf.js/blob/master/test/integration/text_layer_spec.mjs#L128 from belowEndPosition to middlePosition does make the text selection work, so it's probably something related to the calculated coordinates having changed since this patch.

/cc @calixteman Do you have an idea what could have caused this issue?

@timvandermeij
Copy link
Contributor Author

timvandermeij commented Oct 6, 2024

I have reduced it to 68332ec#diff-148dd18f1e2ce2f7b3ed63d4a36922f87ebb1103ff2eaf2e9854adee72d280e5R1106-R1111 causing the issue. It looks like rounding down the canvas size somehow causes this because if I change down to up (or leave it out) the test works again.

Given that this issue and #18775 only happen in Chrome it could indicate a few things: a bug in this commit that only triggers in Chrome, a bug in Chrome itself or a bug in Puppeteer in combination with Chrome. However, they also both use the getSpanRectFromText helper function. I played with that a bit and found that adding Math.round() around either the x/y values or the width/height values in the return value also seems to fix the issue, so it looks like it has to do with rounding in Chrome specifically somehow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants