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

[api-minor] Split highlight annotation div into multiple divs #12505

Merged
merged 1 commit into from
Nov 4, 2020

Conversation

calixteman
Copy link
Contributor

This patch aims to fix issue #12504.
Instead of using Annotation::Rect for the div dimensions, we use several divs where dimensions are in QuadPoints and then set mouse handlers on each of them.

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 added a couple of quick comments; please also add more context to the commit message here as well :-)

src/display/annotation_layer.js Outdated Show resolved Hide resolved
src/display/annotation_layer.js Outdated Show resolved Hide resolved
src/display/annotation_layer.js Outdated Show resolved Hide resolved
src/display/annotation_layer.js Outdated Show resolved Hide resolved
src/display/annotation_layer.js Outdated Show resolved Hide resolved
src/display/annotation_layer.js Outdated Show resolved Hide resolved
Copy link
Contributor

@timvandermeij timvandermeij left a comment

Choose a reason for hiding this comment

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

Looks good with these comments addressed. This is most likely related to #6811; note that it applies not only to highlights, but to all markup annotations and link annotations. That can be done in a follow-up though.

src/display/annotation_layer.js Show resolved Hide resolved
src/display/annotation_layer.js Outdated Show resolved Hide resolved
src/display/annotation_layer.js Outdated Show resolved Hide resolved
src/display/annotation_layer.js Outdated Show resolved Hide resolved
src/display/annotation_layer.js Outdated Show resolved Hide resolved
src/display/annotation_layer.js Show resolved Hide resolved
Fix for issue mozilla#12504.
Highlight annotation may have several rectangles so we must have several divs to add mouse events handlers.
@brendandahl
Copy link
Contributor

/botio-linux test

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/d49c5ffb1707677/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

Full output at http://54.67.70.0:8877/d49c5ffb1707677/output.txt

Total script time: 24.78 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: FAILED

Image differences available at: http://54.67.70.0:8877/d49c5ffb1707677/reftest-analyzer.html#web=eq.log

@brendandahl brendandahl merged commit 1de2bc4 into mozilla:master Nov 4, 2020
@timvandermeij
Copy link
Contributor

timvandermeij commented Nov 4, 2020

Unfortunately the reference tests above show a regression for annotation-highlight-without-appearance and issue9552. I'll open a new issue for this: #12576

@timvandermeij timvandermeij changed the title Split highlight annotation div into multiple divs [api-minor] Split highlight annotation div into multiple divs Nov 4, 2020
@timvandermeij
Copy link
Contributor

Aside from the things mentioned in follow-up issue #12576, this patch does look exactly like what I would expect code-wise and works fine for the original issue and most other files, so thank you for that!

@calixteman calixteman deleted the 12504 branch November 5, 2020 13:55
calixteman added a commit to calixteman/pdf.js that referenced this pull request Nov 5, 2020
 * remove 1st param of _createPopup (almost useless for a method)
 * prepend popup div to avoid to have them on top of some highlights (and so "disable" partially mouse events)
 * add a ref test for issue mozilla#12504
calixteman added a commit to calixteman/pdf.js that referenced this pull request Nov 5, 2020
 * remove 1st param of _createPopup (almost useless for a method)
 * prepend popup div to avoid to have them on top of some highlights (and so "disable" partially mouse events)
 * add a ref test for issue mozilla#12504
calixteman added a commit to calixteman/pdf.js that referenced this pull request Nov 5, 2020
 * remove 1st param of _createPopup (almost useless for a method)
 * prepend popup div to avoid to have them on top of some highlights (and so "disable" partially mouse events)
 * add a ref test for issue mozilla#12504
calixteman added a commit to calixteman/pdf.js that referenced this pull request Nov 10, 2020
 * remove 1st param of _createPopup (almost useless for a method)
 * prepend popup div to avoid to have them on top of some highlights (and so "disable" partially mouse events)
 * add a ref test for issue mozilla#12504
brendandahl added a commit that referenced this pull request Nov 10, 2020
Fix popup for highlights without popup (follow-up of #12505)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants