Right now, editable annotations are using their own canvas when they're drawn, but
it induces several issues:
- if the annotation has to be composed with the page then the canvas must be correctly
composed with its parent. That means we should move the canvas under canvasWrapper
and we should extract composing info from the drawing instructions...
Currently it's the case with highlight annotations.
- we use some extra memory for those canvas even if the user will never edit them, which
the case for example when opening a pdf in Fenix.
So with this patch, all the editable annotations are drawn on the canvas. When the
user switches to editing mode, then the pages with some editable annotations are redrawn but
without them: they'll be replaced by their counterpart in the annotation editor layer.
Debugging #17931 uncovered a race condition in the way we use the
`waitForEvent` function. Currently the following happens:
1. We call `waitForEvent`, which starts execution of the function body
and immediately returns a promise.
2. We do the action that triggers the event.
3. We await the promise, which resolves if the event is triggered or
the timeout is reached.
The problem is in step 1: function body execution has started, but not
necessarily completed. Given that we don't await the promise, we
immediately trigger step 2 and it's not unlikely that the event we
trigger arrives before the event listener is actually registered in the
function body of `waitForEvent` (which is slower because it needs to be
evaluated in the page context and there is some other logic before the
actual `addEventListener` call).
This commit fixes the issue by passing the action to `waitForEvent` as
a callback so `waitForEvent` itself can call it once it's safe to do so.
This should make sure that we always register the event listener before
triggering the event, and because we shouldn't miss events anymore we
can also remove the retry logic for pasting.
The integration tests are currently not consistent in how they do
copy/pasting: some tests use the `kbCopy`/`kbPaste` functions with
waiting for the event inline, some have their own helper function to
combine those actions and some even call `kbCopy`/`kbPaste` without
waiting for the event at all (which can cause intermittent failures).
This commit fixes the issues by providing a set of four helper functions
that all tests use and that abstract e.g. waiting for the event away
from the caller. This makes the invididual tests simpler and consistent,
reduces code duplication and fixes possible intermittent failures
due to not waiting for events to trigger.
This code contains the same bug that the previous commit fixed in
`waitForEvent`, namely that we don't clear the timeout if the event
is triggered. By using the now fixed `waitForEvent` function we not
only deduplicate this code but we also fix this issue so that no
incorrect timeout logs show up anymore.
Debugging #17931, by printing all parts of the event lifecycle including
timestamps, uncovered that some events for which a timeout was logged
actually did get triggered correctly in the browser. Going over the code
and discovering https://stackoverflow.com/questions/47107465/puppeteer-how-to-listen-to-object-events#comment117661238_65534026
showed what went wrong: if the event we wait for is triggered then
`Promise.race` resolves, but that doesn't automatically cancel the
timeout. The tests didn't fail on this because `Promise.race` resolved
correctly, but slightly later once the timeout was reached we would see
spurious log lines about timeouts for the already-triggered events.
This commit fixes the issue by canceling the timeout if the event we're
waiting for has triggered.
This integration test is currently the only one that spawns a separate
browser instance. However, while it closes the browser once it's done,
it doesn't close the page (and therefore doesn't call the `testingClose`
method) like the other integration tests do.
This commit fixes this difference by closing the page before closing the
browser, thereby ensuring all regular cleanup logic gets called and we
avoid (intermittent) shutdown tracebacks in the logs. This allows
upcoming integration tests that spawn a separate browser instance to
reuse this pattern to cleanly end the test.
Given that we integrate the `closeSinglePage` code from #17962 for this
patch, @calixteman is credited as the co-author.
Co-authored-by: Calixte Denizet <calixte.denizet@gmail.com>
This commit introduces a test to ensure that:
- When zooming below the maxCanvasPixels limit, the canvas is rendered
with the correct size (the two sides are multiplied by the zoom factor).
- When zooming above the maxCanvasPixels limit, the canvas size is capped.
This has two advantages, as far as I'm concerned:
- The tests don't need to manually invoke multiple functions to properly clean-up, which reduces the risk of missing something.
- By collecting all the relevant clean-up in one method, rather than spreading it out, we get a much better overview of exactly what is being reset.
Instead of sending to the main thread an array of Objects for a list of points (or quadpoints),
we'll send just a basic float buffer.
It should slightly improve performances (especially when cloning the data) and use slightly less memory.
Over time the number of integration tests that get the rectangle for a
given selector has increased quite a bit, and the code to do so has
consequently become duplicated.
This commit refactors the integration tests to move the rectangle
fetching code to a single place, which reduces the code by over 400
lines and makes the individual tests simpler.
When seleciting on a touch screen device, whenever the finger moves to a
blank area (so over `div.textLayer` directly rather than on a `<span>`),
the selection jumps to include all the text between the beginning of the
.textLayer and the selection side that is not being moved.
The existing selection flickering fix when using the mouse cannot be
trivially re-used on mobile, because when modifying a selection on
a touchscreen device Firefox will not emit any pointer event (and
Chrome will emit them inconsistently). Instead, we have to listen to the
'selectionchange' event.
The fix is different in Firefox and Chrome:
- on Firefox, we have to make sure that, when modifying the selection,
hovering on blank areas will hover on the .endOfContent element
rather than on the .textLayer element. This is done by adjusting the
z-indexes so that .endOfContent is above .textLayer.
- on Chrome, hovering on blank areas needs to trigger hovering on an
element that is either immediately after (or immediately before,
depending on which side of the selection the user is moving) the
currently selected text. This is done by moving the .endOfContent
element around between the correct `<span>`s in the text layer.
The new anti-flickering code is also used when selecting using a mouse:
the improvement in Firefox is only observable on multi-page selection,
while in Chrome it also affects selection within a single page.
After this commit, the `z-index`es inside .textLayer are as follows:
- .endOfContent has `z-index: 0`
- everything else has `z-index: 1`
- except for .markedContent, which have `z-index: 0`
and their contents have `z-index: 1`.
`.textLayer` has an explicit `z-index: 0` to introduce a new stacking context,
so that its contents are not drawn on top of `.annotationLayer`.
The timeout was introduced in commit 402e3fe with equal timeouts around
the helper function call. In commit 55e5af2 the timeouts around the
helper function call have been removed, and it looks like the helper
function itself was not updated purely due to an oversight.
The operations here should not require any timeouts because the promises
only resolve once the action is completed, which also explains why
removing the timeouts surrounding the helper function calls went without
any problems. It should therefore be safe to remove this timeout too.
The `waitForTimeout` function should not be used anymore and only exists
for old usages that have to be rewritten, but there was nothing in place
to signal this. This commit therefore implements a linting rule, specific
to the integration tests, to make it clear that this function should no
longer be used. We exclude the old usages from it because we are already
tracking those in #17656 (so this patch is mostly to not make the scope
of that issue bigger).
This manually ignores some cases where the resulting auto-formatting would not, as far as I'm concerned, constitute a readability improvement or where we'd just end up with more overall indentation.
Please see https://eslint.org/docs/latest/rules/arrow-body-style
This test intermittently fails, likely because the auto-print is triggered fast enough
that we don't manage to get it.
So this patch aims to try to set a listener very early in order to be sure that
we'll be aware that a print has been triggered.
Some fields, somewhere under the Fields entry in Acroform, could have no name (in T)
but with a parent which has a name but which isn't somewhere under Fields.
As a side-effect, this patch prevents infinite loops because of potential cycles
under Fields.
