Avoid text selection workaround in modern Chromium#21514
Conversation
ecd420f to
c8c9837
Compare
There was a problem hiding this comment.
Looks good to me, with passing CI (GitHub Actions) and browser (bot) tests. It looks like the GitHub Actions builds somehow did not start; could you perhaps re-push the change to retrigger them?
Thank you for improving this, and for the detailed analysis in the PR body (as that really helps to understand the change)!
|
/botio browsertest |
From: Bot.io (Linux m4)ReceivedCommand cmd_browsertest from @timvandermeij received. Current queue size: 1 Live output at: http://54.241.84.105:8877/09d0b2d27676b7d/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_browsertest from @timvandermeij received. Current queue size: 1 Live output at: http://54.193.163.58:8877/71039d40cd4de38/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/09d0b2d27676b7d/output.txt Total script time: 18.13 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.193.163.58:8877/71039d40cd4de38/output.txt Total script time: 25.00 mins
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #21514 +/- ##
==========================================
- Coverage 89.79% 89.08% -0.72%
==========================================
Files 260 262 +2
Lines 66282 67013 +731
==========================================
+ Hits 59520 59698 +178
- Misses 6762 7315 +553
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
I need to investigate the integration test failures, it's possible I just need to update the expected behavior |
Chromium 148+ improved their selection behavior when it comes to absolutely positioned elements, thus making text selectino in PDF.js much better. Unfortunately this does not only mean that the workaround we currently have for Chromium is unnecessary, but it actually become harmful. It conflicts with Chromium's new behavior, making text selection *worse* on mobile. As the change has been released in Chrome only a month ago, this patch keeps the workaround for older Chromium versions. There is no easy way to feture-detect is, so unfortunately we need to do user agent version detection.
c4527a1 to
ea43bb4
Compare
|
Done :) |
Also, somehow the "right click on images" layer undoes the negative effects of the workaround, so even in modern mobile Chromium if there are images text selection already works well.
This is the behavior on various Chromium versions. By "scroll jump" I meant that the page scroll position moves to the wrong place (usually top right of the page) while selection; by "selection explodes" I mean that suddenly all the page is selected while trying to select something.
The changed code is not included in the mozcentral build. In the Chromium build it now looks like this:
while in the generic build it looks like this: