[Regression] Text-selection boxes visually overlap after PR 17533

[Snuffleupagus]

Attach (recommended) or Link to PDF file here: https://github.com/mozilla/pdf.js/blob/master/test/pdfs/pdf.pdf.link

(This can be reproduce even with tracemonkey.pdf, but it's less pronounced since the lines don't overlap as much.)

Configuration:

• Web browser and its version: Firefox 123.0a1
• Operating system and its version: Windows 11
• PDF.js version: 5d2e7cf, i.e. PR Make the caret visible in the text layer in caret browsing mode #17533
• Is a browser extension: N/A

Steps to reproduce the problem:

1. Select text, and look at the intersection between different text-selection boxes.

What is the expected behavior? (add screenshot)
The intersection should not be visible, as previously:

What went wrong? (add screenshot)
The intersection is visible:

Link to a viewer (if hosted on a site other than mozilla.github.io/pdf.js or as Firefox/Chrome extension): N/A

/cc @calixteman

[marco-c]

This has not made its way to Firefox yet, right?

[Snuffleupagus]

This has not made its way to Firefox yet, right?

Correct.

[calixteman]

I think this regression is acceptable with regard to the improvement we've in HCM and it helps to see the caret to make highlighting possible with the keyboard.
It's a visual annoyance, but I'd say that it doesn't avoid to use text selection and I checked few pdfs and in general there are no issues, so maybe this case is just a corner case.
That said, as far as I know there is no way to fix it without reverting the regressor patch BUT the height of the selection depends of the font, its size, the line height, ... which aren't correct because the font we use (serif/sans serif) is very likely not the one we've in the pdf. So the correct fix should be to use the font from the pdf in the text layer (I still have a WIP locally to do that).

[timvandermeij]

Given the above, should we still mark this as a release blocker? I'm planning to make a new release tomorrow, and if this does block a new release I'll postpone that action.

Edit: Given the other release blocker ticket, I'll postpone the release with a week anyway and then see where we are.

[Snuffleupagus]

I think this regression is acceptable with regard to the improvement we've in HCM and it helps to see the caret to make highlighting possible with the keyboard.

I don't really agree with that assessment, since we have potential regressions for the majority of users this way.

It's a visual annoyance, but I'd say that it doesn't avoid to use text selection and I checked few pdfs and in general there are no issues, so maybe this case is just a corner case.

Unfortunately it can be a lot worse than just a minor annoyance in some cases, please see an example further below.

That said, as far as I know there is no way to fix it without reverting the regressor patch

Would it be feasible to only apply the new styles in HCM, to avoid affecting "normal" browsing?

So the correct fix should be to use the font from the pdf in the text layer (I still have a WIP locally to do that).

The problem is that it still won't fix these types of issues generally, since glyphs are often positioned absolutely and that means that we'll never get rid of all overlapping text-selection boxes. The example that I used in this issue contained vertical overlap, but it could just as well be horizontal overlap which can look a lot worse.

The following is an especially problematical example, since it uses Type3 fonts, but it does illustrate (much better) why we "need" to do something about it: https://github.com/mozilla/pdf.js/files/1274923/SMPembedded.2006.10.09b.pdf

[calixteman]

In general we can't ship something which isn't accessible (it used to be the opposite but nowadays it's the policy), so we need to make highlighting keyboard accessible if we want to ship it.
The "annoyance" you mentioned here is less important for us than fixing a11y issues.
About the pdf you mentioned in your comment: it was already awful (in term of text selection) and I fully agree that now it's worse but between "awful" and "slightly more awful" and I don't see that much difference, so I still consider that it's a minor annoyance in general.
That said, I usually try to improve things instead of making them worse, so I've a WIP locally to draw the selection in using the svg layer we recently added , for example:

and it'll fix this bug and it'll let us really improve text selection in HCM (in using a filter) which is, right now, awful. In term of performance, I didn't see any noticeable regressions.

[calixteman]

And for your information, I have a potential fix for the selection issue in Chrome.
I need to think about that globally but I've the feeling it should be possible to fix most of the selection issues we've.

[marco-c]

I think this regression is acceptable with regard to the improvement we've in HCM and it helps to see the caret to make highlighting possible with the keyboard.

I don't really agree with that assessment, since we have potential regressions for the majority of users this way.

I agree we should avoid shipping this regression to users. I have filed https://bugzilla.mozilla.org/show_bug.cgi?id=1878258 to track this regression so that we make sure it is fixed before we release Firefox 124 (which currently includes the regression).

That said, we are considering the regression less severe than the improvement because while it's true that the regression affects a larger number of users, the improvement has a huge impact for users that currently are completely unable to select text and/or highlight text in the reader.

[timvandermeij]

I agree we should avoid shipping this regression to users. I have filed https://bugzilla.mozilla.org/show_bug.cgi?id=1878258 to track this regression so that we make sure it is fixed before we release Firefox 124 (which currently includes the regression).

Thank you for the feedback. Given this information I'll hold off on releasing a new PDF.js version until this is fixed so there is enough time and we avoid any bug reports about this.