Fix PDF quote anchoring #261
Conversation
Make this file easier to understand for the next person who tries to read it.
This method used to exist in the `dom-anchor-text-quote` library but got lost when the class was moved to a wrapper in `types.coffee` after the upgrade to dom-anchor-text-quote v3.x. This method is used by PDF anchoring and its absence caused quote anchoring to throw an exception. The majority of PDF annotations still continued to anchor because the position anchor still worked. This commit also adds some basic documentation to the `types` module and some basic API tests.
…to anchor This test fails without the `toPositionAnchor` fix in the previous commit.
Codecov Report
@@ Coverage Diff @@
## master #261 +/- ##
==========================================
+ Coverage 75.62% 76.05% +0.42%
==========================================
Files 116 116
Lines 5793 5796 +3
Branches 946 947 +1
==========================================
+ Hits 4381 4408 +27
+ Misses 1412 1388 -24
Continue to review full report at Codecov.
|
|
Gracias! |
|
"if the quote is not found on the first page that anchoring checks then it will never be found at all." Whoa. |
|
Confirmed that I can reproduce hypothesis/product-backlog#146 on master and that this branch appears to fix it |
|
"Make this file easier to understand for the next person who tries to |
There was a problem hiding this comment.
Very nice, fixes the bug and also adds plenty of documentation and tests, steadily improving the codebase.
I've made a comment about the top-level describe() string, but I realise that this kind of description is used often in our tests (regrettably, in my opinion) so I'll approve this and leave it up to you to merge with or without making that change @robertknight
| // These are primarily basic API tests for the anchoring classes. Tests for | ||
| // anchoring a variety of HTML and PDF content exist in `html-test` and | ||
| // `pdf-test`. | ||
| describe('Anchoring classes', function () { |
There was a problem hiding this comment.
Wondering if describe('types.coffee') (or maybe just describe('types')) would be a more precise way of putting this? Or else rename types.coffee to anchoring.coffee. But there's a mismatch between the two names "types" and "anchoring" which I think makes it less immediately clear, from looking at for example the output of a failed test, what code was being tested.
There was a problem hiding this comment.
Agreed. I'll change the name here to match the module. I think the closest we have to a convention is that the top-level describe matches:
- The name of the 'default' export if the file has one (ie. if it does
module.exports = someFunctionthen the describe block is 'someFunction') - Otherwise the name is the module filename
There was a problem hiding this comment.
That seems like a pretty reasonable convention. Perhaps files should be named after their default exports too
There was a problem hiding this comment.
Perhaps files should be named after their default exports too
This is a common convention. In our code we usually do that but hyphenate the name. eg. If the default export is serviceConfig we name the file service-config.
| var quoteAnchor = new TextQuoteAnchor(container, 'some are more equal than others'); | ||
| assert.throws(function () { | ||
| quoteAnchor.toPositionAnchor(); | ||
| }); |
There was a problem hiding this comment.
This is nice - the tests are simple and easy to understand
| # | ||
| # 1. Providing a consistent interface across different types of anchor. | ||
| # 2. Insulating the rest of the code from API changes in the underyling anchoring | ||
| # libraries. |
270932b broke anchoring using quote selectors in PDFs because the re-implementation of the
TextQuoteAnchorclass was missing a method,toPositionAnchor, which PDF anchoring tried to call.This did not affect the majority of annotations because it only affected cases where the position selector failed to anchor and so anchoring fell back to the quote selector.
Fixes hypothesis/product-backlog#146 . Note that after creating the annotation as described in the issue the highlighted region is offset. This issue existed in Hypothesis v0.50 as well.
This PR:
While implementing this fix, I noticed that quote anchoring is broken in PDFs in other ways too - if the quote is not found on the first page that anchoring checks then it will never be found at all. I'm going to fix this separately.