/botio-linux preview

I'm not sure if it's possible to add an integration test for this kind of change because even though we can probably do the selection with Puppeteer I don't really see what we could assert in the test. In any case, let's first check if all existing tests still pass with this change applied:

/botio test

It looks like the integration tests did notice the change, so we fortunately at least have some coverage on this (although we don't specifically test this selection issue, but it's most likely noticed as a side-effect of other selection/scrolling tests). However, sadly they seem to fail on both Linux and Windows.

Tip: what usually helps for quick debugging is enabling one of failing tests from the logs by changing it to fit in the test function definition (which tells the Jasmine test runner to only run that particular test instead of the entire suite which takes a much longer time) and then running npx gulp integrationtest to find out what's breaking or changed compared to the master branch.

@timvandermeij Thanks for the tip! The flickering behavior is very consistent/predictable, so if with puppeteer I could simulate "long-tap at (x1,y2) to select, then hold the selection bubble and drag to (x2,y2), and check what is selected" I would be able to add a test.

The problem is the "drag the selection bubble" part, since it's not dragging on the page (the selection bubble is browser UI and doesn't trigger touch events)

I pushed two commits:

• the first one unregisters the selectionchange event listeners in .cancel(), so that it doesn't leak (fixing Fix flickering on text selection #17923 (comment))
• the second one fixes flickering also in the case of <span>s nested inside .markedContent, and removes the old anti-flickering code for Chrome when using a mouse (I added a before/after video in the issue description), as I noticed that the mobile fix also improves the selection on desktop. This de-duplicates the isFirefox ??= ... check, solving Fix flickering on text selection #17923 (comment).

I still need to investigate testing.

The problem is the "drag the selection bubble" part, since it's not dragging on the page (the selection bubble is browser UI and doesn't trigger touch events)

I have looked that the Puppeteer API documentation, but I can't really find anything related to this. However, if the flickering effects are also consistently visible in Firefox or Chrome on desktop then we might be able to write a test that starts a selection, moves the mouse to certain coordinates and gets the selected text. We'd have to try it out to see if it actually works, but it sounds like it might be doable.

(By the way, thank you for providing such detailed issue/PR descriptions, visuals and inline comments! That really helps to understand the change, especially for these kinds of bugs that are usually a bit tricky and difficult to fully test in an automated way, and therefore require additional manual testing.)

@timvandermeij I managed to add a test when using a mouse (which before this PR fails in Chrome), but unfortunately puppeteer's API doesn't seem to allow selecting text emulating a touchscreen input.

In Chrome this test will trigger the selectionchange event, so it covers some of the code paths of Firefox mobile too (except that obviously there is some Chrome-specific additional code).

In Firefox (desktop), it's possible to reproduce the flickering in setting the pref layout.accessiblecaret.hide_carets_for_mouse_input to false and in using the blue anchors to modify the selection.

Oh thank you :) Can I somehow set it in Puppeteer?

Yes, Firefox-specific preferences are configured here: https://github.com/mozilla/pdf.js/blob/master/test/test.mjs#L907-L942. If it helps, I also found some more documentation about this preference at https://firefox-source-docs.mozilla.org/layout/AccessibleCaret.html.

I pushed 7bb30a4 (#17923) to do what suggested by @calixteman in #17923 (comment). Commit description:

Handle selectionchange events globally

This commit moves the selectionchange and pointerup event
listeners to be shared across multiple TextLayerBuilder
instances. This has two effects:

• we only need to register one global listener that then "dispatches"
  the DOM updates in the correct .textLayer, rather than having one
  per instance that needs to be careful about only handling selection
  changes that affect it.
• the fix now works with multi-page selections, which were currently
  also broken on Firefox desktop

I am working on a test for this multi-page flickering on Firefox desktop, then I'll try if layout.accessiblecaret.hide_carets_for_mouse_input lets me test the flickering behavior when using the carets. I also still need to go through the snapshot failures.

All tests are passing locally, except for Chrome reftests because one of them kept timing out while I was trying to generate the reference images from master.

I had to add one more commit to fix a regression: de1e73b (#17923)

Explicitly add z-index: 0 to .textLayer

This creates a "stacking context", making sure that the elements
inside .textLayer (on the z axis) remain inside .textLayer.
Without this change, the <span>s inside .textLayer end up being
on top of .annotationLayer.

This was caught by the freetext editor integration tests, that
failed because we were trying to draw on top of some text.

This is ready for re-review -- I will squash once it's ready :)

/botio-linux preview

From: Bot.io (Linux m4)

Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/a85aa629944d00c/output.txt

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/a85aa629944d00c/output.txt

Total script time: 1.17 mins

Published

• Viewer: http://54.241.84.105:8877/a85aa629944d00c/web/viewer.html
• Viewer (legacy): http://54.241.84.105:8877/a85aa629944d00c/legacy/web/viewer.html

/botio test

From: Bot.io (Linux m4)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/4205770be581237/output.txt

From: Bot.io (Windows)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://54.193.163.58:8877/e6e46cd33c757e6/output.txt

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/4205770be581237/output.txt

Total script time: 27.80 mins

• Unit tests: Passed
• Integration Tests: FAILED
• Regression tests: FAILED

  different ref/snapshot: 16
  different first/second rendering: 2

Image differences available at: http://54.241.84.105:8877/4205770be581237/reftest-analyzer.html#web=eq.log

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/e6e46cd33c757e6/output.txt

Total script time: 42.28 mins

• Unit tests: Passed
• Integration Tests: FAILED
• Regression tests: FAILED

  different ref/snapshot: 6

Image differences available at: http://54.193.163.58:8877/e6e46cd33c757e6/reftest-analyzer.html#web=eq.log

The Linux bot is green now (the reported failures are known intermittents that we already track in #17931 and #16934, so they are unrelated to this patch), but the Windows bot still contains some assertion errors for the integration tests. You can find the assertion errors at the bottom of the logs at http://54.193.163.58:8877/e6e46cd33c757e6/output.txt.

Looking at the logs I noticed that there are some newline differences in the expected outputs. If I have to guess (but I don't have a Windows machine at the moment to verify), I suspect at least one of the assertions to fail on Windows because the tests use \n while Windows probably uses \r\n. I haven't checked if this explains all Windows failures, but perhaps this helps as a first observation (I've left a comment below with more pointers).

I don't have further comments on this patch, so once the assertions pass on Windows, and the commits are squashed and rebased onto the current master so the CI is also green again, I think we can merge this :-)

/botio integrationtest

From: Bot.io (Linux m4)

Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/ea1720bee7fdf1e/output.txt

From: Bot.io (Windows)

Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.193.163.58:8877/0c6537d92fea187/output.txt

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/ea1720bee7fdf1e/output.txt

Total script time: 7.39 mins

• Integration Tests: FAILED

The Linux integration tests passed (the only failure is a known intermittent), but the Windows bot has not started yet because it seems stuck on a job in another PR (where I've asked to give the bot a reboot), so after that it should start running and otherwise I'll retrigger it manually.

In the meantime, could you squash the commits so that once the Windows tests passed we should be ready to land this?

Rebased!

/botio-windows integrationtest

From: Bot.io (Windows)

Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.193.163.58:8877/edfbf086f943cfc/output.txt

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/edfbf086f943cfc/output.txt

Total script time: 1.73 mins

/botio-windows integrationtest

From: Bot.io (Windows)

Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.193.163.58:8877/3ad9ebab6ed3957/output.txt