Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Editor] Add a way to extract the outlines of a union of rectangles #17291

Merged
merged 1 commit into from
Nov 21, 2023

Conversation

calixteman
Copy link
Contributor

The goal is to be able to get these outlines to fill the shape corresponding to a text selection in order to highlight some text contents. The outlines will be used either to show selected/hovered highlights.

src/display/draw_layer.js Fixed Show fixed Hide fixed
@calixteman
Copy link
Contributor Author

Here's an example of what we get thanks to this new code:
1

@calixteman calixteman force-pushed the editor_highlight1 branch 2 times, most recently from 9905e16 to f9d255c Compare November 17, 2023 18:29
Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving a few comments based on cursory first look, since this is a lot of somewhat complicated code to wrap my head around.

src/display/draw_layer.js Outdated Show resolved Hide resolved
src/display/draw_layer.js Outdated Show resolved Hide resolved
src/display/draw_layer.js Outdated Show resolved Hide resolved
src/display/draw_layer.js Outdated Show resolved Hide resolved
src/display/editor/outliner.js Outdated Show resolved Hide resolved
test/draw_layer_test.css Outdated Show resolved Hide resolved
test/driver.js Outdated Show resolved Hide resolved
web/draw_layer_builder.css Outdated Show resolved Hide resolved
web/draw_layer_builder.css Outdated Show resolved Hide resolved
web/draw_layer_builder.css Outdated Show resolved Hide resolved
@calixteman
Copy link
Contributor Author

@Snuffleupagus If you want to try the outline algo with some pdfs and your mouse you can get this patch:
calixteman@f0ee554

src/display/draw_layer.js Show resolved Hide resolved
src/display/draw_layer.js Outdated Show resolved Hide resolved
web/draw_layer_builder.js Outdated Show resolved Hide resolved
src/display/editor/outliner.js Outdated Show resolved Hide resolved
src/display/editor/outliner.js Show resolved Hide resolved
web/draw_layer_builder.js Outdated Show resolved Hide resolved
@Snuffleupagus
Copy link
Collaborator

It seems that the two test-cases added in this PR both contains left-to-right text, so please also add tests (we should hopefully have such files) for right-to-left and top-to-bottom text.

@calixteman
Copy link
Contributor Author

It seems that the two test-cases added in this PR both contains left-to-right text, so please also add tests (we should hopefully have such files) for right-to-left and top-to-bottom text.

The goal of the two tests is to just test the algorithm to get the outlines and in case of failures, we'll have a way to visualize what's wrong which should be useful.
So I don't think it's so useful to have something for RTL or TTB text because for the algorithm only sees rectangles nothing else.
But for sure we'll need to have some integration tests testing RTL/TTB text highlighting.

The goal is to be able to get these outlines to fill the shape corresponding
to a text selection in order to highlight some text contents.
The outlines will be used either to show selected/hovered highlights.
@Snuffleupagus
Copy link
Collaborator

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.241.84.105:8877/3c347fb64e4b3ad/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

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

Live output at: http://54.193.163.58:8877/646cb0274c03bb2/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/3c347fb64e4b3ad/output.txt

Total script time: 24.87 mins

  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 18
  different first/second rendering: 2

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

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/646cb0274c03bb2/output.txt

Total script time: 34.51 mins

  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 27

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

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've not looked in too much detail at the algorithm, but given that we have reference tests it should help ensure that things work as expected.

r=me, thank you!

@calixteman calixteman merged commit 02ccdbb into mozilla:master Nov 21, 2023
9 checks passed
@calixteman
Copy link
Contributor Author

/botio makeref

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_makeref from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/5ce032ee02e5b5e/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_makeref from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/7ef6de1335c91d5/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/5ce032ee02e5b5e/output.txt

Total script time: 10.27 mins

  • Lint: Passed
  • Make references: FAILED

@calixteman
Copy link
Contributor Author

/botio-windows makeref

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_makeref from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/30649cb5ef0e06d/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/7ef6de1335c91d5/output.txt

Total script time: 18.14 mins

  • Lint: Passed
  • Make references: FAILED

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/30649cb5ef0e06d/output.txt

Total script time: 10.38 mins

  • Lint: Passed
  • Make references: FAILED

@calixteman
Copy link
Contributor Author

I think there is something wrong with the test xfa_bug1722030_1.
There is a lot of pages in this xfa. I don't manage to reproduce the issue locally.
/botio makeref

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_makeref from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/2221eeac5f3fde8/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_makeref from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/3bec6ee0b8df9a1/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/2221eeac5f3fde8/output.txt

Total script time: 6.30 mins

  • Lint: Passed
  • Make references: FAILED

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/3bec6ee0b8df9a1/output.txt

Total script time: 17.05 mins

  • Lint: Passed
  • Make references: FAILED

@calixteman
Copy link
Contributor Author

/botio-windows makeref

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_makeref from @calixteman received. Current queue size: 0

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

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

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

Total script time: 11.11 mins

  • Lint: Passed
  • Make references: FAILED

@calixteman
Copy link
Contributor Author

/botio-windows makeref

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_makeref from @calixteman received. Current queue size: 0

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

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

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

Total script time: 12.49 mins

  • Lint: Passed
  • Make references: FAILED

@calixteman
Copy link
Contributor Author

I finally managed to reproduce the issue locally on Windows but just one time: it's an intermittent failure with xfa tests.
On the windows bot, the failing xfa isn't always the same...
Since we don't touch that much xfa stuff, I propose to disable the xfa tests and try from time to time to re-enable them until the underlying issue is fixed. If we've to work on xfa then we'll have to figure out what's wrong.
For information, I captured a profile when it was stuck and unfortunately, the profile was empty...
@Snuffleupagus wdyt about disabling xfa tests ?

@Snuffleupagus
Copy link
Collaborator

I finally managed to reproduce the issue locally on Windows but just one time:

Looking above didn't makeref fail on Linux as well, meaning that it's now consistently broken?
Given that Firefox was just updated to version 122 on the bots, could this be a browser regression?

@calixteman
Copy link
Contributor Author

Yes I think it's a browser regression (especially because as you pointed it out: it's failing on linux either) and I really hope that someone will have a similar issue but with a real STR.
I don't really feel like using mozregression and gulp makeref on the bot to figure out what's the regressor.
What make me sad is that there is nothing helpful to figure out what's could be wrong.

@Snuffleupagus
Copy link
Collaborator

Since we don't touch that much xfa stuff, I propose to disable the xfa tests and try from time to time to re-enable them until the underlying issue is fixed. If we've to work on xfa then we'll have to figure out what's wrong.

That could work as a temporary band-aid, assuming that's where the problem lies. The following diff is probably the smallest/easiest way to disable XFA reference tests, but please remember to open a tracking issue if we end up doing this.

diff --git a/test/driver.js b/test/driver.js
index 6805017d8..312abd64b 100644
--- a/test/driver.js
+++ b/test/driver.js
@@ -446,6 +446,12 @@ class Driver {
         md5FileMap.set(task.md5, task.file);
       }
 
+      if (task.enableXfa) {
+        this.currentTask++;
+        this._nextTask();
+        return;
+      }
+
       // Support *linked* test-cases for the other suites, e.g. unit- and
       // integration-tests, without needing to run them as reference-tests.
       if (task.type === "other") {

@calixteman
Copy link
Contributor Author

yep I was writing almost the same thing with a line for logging.

@calixteman
Copy link
Contributor Author

/botio makeref

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_makeref from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/1c7836dcdf78445/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_makeref from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/1b78b580f6dafff/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/1c7836dcdf78445/output.txt

Total script time: 19.53 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Success

Full output at http://54.193.163.58:8877/1b78b580f6dafff/output.txt

Total script time: 22.84 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants