To avoid being able to introduce dependencies between tests this commit
makes sure that we close the document between tests so that we can't
accidentally rely on state set by a previous test.
This commit reduces the number of freetext editor integration test suite
failures, in full isolation, from 5 to 0 by fixing the following issues
in the "basic operations" block:
- Most tests relied on the first test to enable freetext editing mode.
For isolation we now do it explicitly in all tests.
- Most tests relied on the other tests having created editors. For
isolation we now create the editors explicitly in the tests themselves.
- Most tests relied on previous tests for the editor numbering. For
isolation we change the editor numbering to the one after initial
document load. Since we can't have state (editors) from a previous
test anymore we can remove various `clearAll` calls as well.
This commit reduces the number of freetext editor integration test suite
failures, in full isolation, from 6 to 5 by fixing the following issues
in the "create editor with keyboard" block:
- The second test relied on the first test to enable freetext editing
mode and put focus on the page (annotation layer). For isolation we
now do that explicitly in the second test.
- The second test relied on the first test for the editor numbering. For
isolation we change the editor numbering to the one after initial
document load.
Moreover, the test names have been updated to clarify with scenario is
being tested, which came up during comparison of the changes against
commit ea5eafa to make sure that we are still testing the originally
intended scenarios (confirmed by disabling the relevant code from the
commit per scenario and noticing the corresponding test failing).
This commit reduces the number of freetext editor integration test suite
failures, in full isolation, from 8 to 6 by fixing the following issues
in the "move editor with arrows" block:
- The second and third test relied on the first test to enable freetext
editing mode. For isolation we now do it explicitly in both tests.
- The second test relied on the first test having created an editor. For
isolation we now create the editor explicitly in the second test.
- The third test relied on the previous tests for the editor numbering.
For isolation we change the editor numbering to the one after initial
document load. Since we can't have state (editors) from a previous
test anymore we can remove the `clearAll` call as well.
This replaces the various copies of this logic with a single helper that
we template for each editor type, similar to what we already do for the
`switchToEditor` helper.
This replaces the various copies of this logic with a single helper that
we template for each editor type, similar to what we already do for the
`switchToEditor` helper.
When a drawing was moved with arrow keys and then printed or saved, the drawing wasn't moved finally.
So the fix is just about calling onTranslated once the translation is done.
This pattern was already followed quite consistently outside of the
freetext editor integration tests, so this commit aligns the remaining
places for consistency. This also helps to make the tests more compact
and to reduce the number of changes in follow-up changes.
In most integration tests we already use the pattern of defining the
editor selector once and reusing it in the rest of the test, but it's
not fully consistent everywhere yet. This commit fixes that for the
freetext editor integration tests, which has multiple advantages:
- it improves consistency between the various editor integration tests;
- it removes duplicate function calls and aligns variable definitions;
- it reduces the number of `getEditorSelector` calls that contained
hardcoded IDs, which helps to isolate the tests and to simplify
follow-up patches.
This has multiple advantages:
- it improves consistency between the various editor integration tests;
- it makes the code easier to read/understand;
- it reduces code duplication.
This has multiple advantages:
- it improves consistency between the various editor integration tests;
- it makes the code easier to read/understand;
- it reduces code duplication;
- it reduces the number of `getEditorSelector` calls that contained
hardcoded IDs, which helps to isolate the tests and to simplify
follow-up patches.
The `selectorEditor` name is used 57 times, but only in the freetext
editor integration tests file. The `editorSelector` name on the other
hand is used 241 times in all editor integration test files, so this
commit renames the former name to the latter name to achieve consistency
in variable names across all editor integration test files, which also
simplifies upcoming changes.
The ink editor integration tests already use a helper function for
committing the editor, so this commit mirrors the approach to the
freetext editor integration tests. This has multiple advantages:
- it improves consistency between the various editor integration tests;
- it makes the code easier to read/understand;
- it reduces code duplication (220 lines of code removed);
- it reduces the number of `getEditorSelector` calls (32 calls removed)
that contained hardcoded IDs, which helps to isolate the tests and to
simplify follow-up patches.
The `getSelectedEditors` function is largely a copy of the `getEditors`
function, with three small differences:
1. `getEditors` allows getting any kind of editor whereas
`getSelectedEditors` is harcoded to only getting selected editors.
2. `getEditors` returns editor selectors (strings) whereas
`getSelectedEditors` returns editor IDs (integers).
3. `getSelectedEditors` returns a sorted array of editor IDs whereas
`getEditors` does not ensure that the array is sorted.
This commit makes the `getEditors` function a drop-in replacement for
the `getSelectedEditors` function to deduplicate the code and to have a
unified way of getting editors.
Note that we don't actually use the contents of the returned array
(only its length), so we can safely change `getEditors` to return a
sorted array of integer editor IDs instead. Sorting the array makes the
return value deterministic, which is a nice property for test stability,
and integer IDs are also easier to handle in test assertions. Note that
the corresponding selector strings can also easily be obtained from the
integer IDs using the `getEditorSelector` function if needed.
When an editor is moved with the keyboard or in dragging it, it is moved in the DOM in order
to make screen readers happy. But this move is slightly postponed thanks to a setTimeout(..., 0).
The failures were very likely due to the fact that intermittently the DOM move was done in
the middle of the next key sequence which was making the move on screen failing.
It was due the resize observer which is removed thanks to this patch.
In order to reuse the dragAndDrop function in test, this patch slighty refactors it
in order to make it easier to use.
This major version contains three breaking changes that impact us:
- The `product` option has been renamed to the more suitable `browser`.
- The `page.screenshot()` API returns a `Uint8Array` instead of a
`Buffer`, but since `pngjs` requires a `Buffer` object we need to do
the conversion using `Buffer.from()` before passing data to `pngjs`.
- The browser configuration should be set using a configuration file
instead of environment variables. Note that as a bonus this allows us
to remove the `cross-env` dependency since that was only used to set
the Puppeteer environment variable equally for all operating systems.
For more information about the changes between the old and new Puppeteer
versions refer to https://github.com/puppeteer/puppeteer/releases.
The function evaluateOnNewDocument in Puppeteer allow us to execute some js before the pdf.js one
is loaded.
It allows us to stub some setters before there are used and then set some event handlers very soon.
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.
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.
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.
This commit replaces a `waitForTimeout` occurrence with an equivalent
`waitForSelector` expression, and removes two other `waitForTimeout`
occurrences that are obsolete because we already wait for an observable
event to trigger or class change to happen.
Note that the other `waitForTimeout` occurrences in this file are either
part of #17931 or remain until we find a good way to ensure that nothing
happened (because currently there is nothing we can await there).
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).
The original bug was because the parent was null when trying to show
an highlight annotation which led to an exception.
That led me to think about having some null/non-null parent when removing
an editor: it's a mess especially if a destroyed parent is still attached
to an editor. Consequently, this patch always sets the parent to null when
deleting the editor.
For arrow functions that are both simple and short, we can avoid using explicit `return` to shorten them even further without hurting readability.
For the `gulp mozcentral` build-target this reduces the overall size of the output by just under 1 kilo-byte (which isn't a lot but still can't hurt).
When the text of an annotation is extracted in using getTextContent, consecutive white spaces
are just replaced by one space and. So this patch add an option to make sure that white
spaces are preserved when appearance is parsed.
For the case where there's no appearance, we can have a fast path to get the correct string
from the Content entry.
When an existing FreeText is edited, space (0x20) are replaced by non-breakable (0xa0) ones
to make to see all of them on screen.
