-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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] Invalidate an annotation with no quadPoints (when it's required) #12503
Conversation
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.
Is there a PDF file that can be used to (manually) test this patch?
Also, please provide slightly more complete commit messages, such that it's easier to understand what a patch does (and why) e.g. when looking at it using the git
command line.
There's a bunch more information in #12503 (comment), so it'd be nice if that was included in the commit message too.
src/core/annotation.js
Outdated
@@ -2245,6 +2250,10 @@ class HighlightAnnotation extends MarkupAnnotation { | |||
}, | |||
}); | |||
} | |||
} else { | |||
// QuadPoints is required | |||
this._isInvalid = true; |
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.
This name feels very "generic", while it's however only used together with QuadPoints.
Couldn't we simply set this.data.quadPoints = null;
explicitly here, and then add a this.data.quadPoints !== null
check to the _isViewable
/_isPrintable
helpers 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.
So for any reason, I can't attach a file here so I filed an issue #12506.
Some pdf softwares don't remove highlight annotations but make the QuadPoints array empty. And the Rect for the annotation can be [-32768, -32768, 32768, 32768] so it leads to have a giant div which catches all the mouse events and make the pdf unusable when there are some forms elements.
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/77e2fc84530e200/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/e471f2e4df9fe74/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/77e2fc84530e200/output.txt Total script time: 25.77 mins
Image differences available at: http://54.67.70.0:8877/77e2fc84530e200/reftest-analyzer.html#web=eq.log |
/botio-windows test |
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/e2d647553c184e0/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/e2d647553c184e0/output.txt Total script time: 4.08 mins Published |
Looks good! |
Some pdf softwares don't remove highlight annotations but make the QuadPoints array empty.
And the Rect for the annotation can be [-32768, -32768, 32768, 32768] so it leads to have a giant div which catches all the mouse events and make the pdf unusable when there are some forms elements.
I aims to fix issue #12506.