-
Notifications
You must be signed in to change notification settings - Fork 10k
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
[api-minor] XFA - Support text search in XFA documents. #13908
Conversation
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @brendandahl received. Current queue size: 0 Live output at: http://54.67.70.0:8877/5791d5bb0647658/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @brendandahl received. Current queue size: 0 Live output at: http://3.101.106.178:8877/ea30e5dd1da048b/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/5791d5bb0647658/output.txt Total script time: 33.89 mins
Image differences available at: http://54.67.70.0:8877/5791d5bb0647658/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://3.101.106.178:8877/ea30e5dd1da048b/output.txt Total script time: 40.15 mins
Image differences available at: http://3.101.106.178:8877/ea30e5dd1da048b/reftest-analyzer.html#web=eq.log |
Looks like the bots are slow enough that we catch them while still typing. There are 51 matches of "ci", so at least it's correctly wrong. Now how to wait until find is done... |
web/app_options.js
Outdated
defaultOptions.enableXfa = { | ||
/** @type {boolean} */ | ||
value: false, | ||
kind: OptionKind.API + OptionKind.PREFERENCE, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but why is this being duplicated; please remove this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The chrome lint was failing because it finds enableXfa = true. Is there a better way to fix this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, this is my fault since I didn't consider all of the ways that the TESTING
define could be used. Please see PR #13911 which should fix this.
this._updateMatches(); | ||
} | ||
|
||
disable() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the comment above, for the enable
-method, but for the !this.enabled
case instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We currently call disable without enabling since we cancel things out before starting. Since this is safe to call without enabling, I think we should just leave it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just add a if (!this.enabled) { return; }
at the top then, since none of the code in this method needs to run if it's already disabled?
beb7818
to
24058e1
Compare
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @brendandahl received. Current queue size: 0 Live output at: http://54.67.70.0:8877/3eaacbf8136ff8d/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @brendandahl received. Current queue size: 0 Live output at: http://3.101.106.178:8877/04732ab914269e1/output.txt |
From: Bot.io (Windows)FailedFull output at http://3.101.106.178:8877/04732ab914269e1/output.txt Total script time: 32.04 mins
Image differences available at: http://3.101.106.178:8877/04732ab914269e1/reftest-analyzer.html#web=eq.log |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/3eaacbf8136ff8d/output.txt Total script time: 33.67 mins
Image differences available at: http://54.67.70.0:8877/3eaacbf8136ff8d/reftest-analyzer.html#web=eq.log |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, with one final optional comment; thank you!
This is kind of how I imagined/hoped that the solution to issue #13908 would look like, with re-using the existing find functionality.
It's especially nice, in my opinion, that you extracted the TextHighlighter
code into its own class and were able to re-use it for both "normal" and XFA find :-)
this._updateMatches(); | ||
} | ||
|
||
disable() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just add a if (!this.enabled) { return; }
at the top then, since none of the code in this method needs to run if it's already disabled?
Moves the logic out of TextLayerBuilder to handle highlighting matches into a new separate class `TextHighlighter` that can be used with regular PDFs and XFA PDFs. To mimic the current find functionality in XFA, two arrays from the XFA rendering are created to get the text content and map those to DOM nodes. Fixes mozilla#13878
Moves the logic out of TextLayerBuilder to handle
highlighting matches into a new separate class
TextHighlighter
that can be used with regular PDFs and XFA PDFs.
To mimic the current find functionality in XFA, two arrays
from the XFA rendering are created to get the text content
and map those to DOM nodes.
Fixes #13878