Show a warning when PDF does not have selectable text #2623
Conversation
28948db to
df229ae
Compare
src/annotator/anchoring/pdf.js
Outdated
| resolve(pdfViewer.getPageView(pageIndex)); | ||
| }; | ||
| document.addEventListener('pagesloaded', onPagesLoaded); | ||
| pdfViewer.eventBus.on('pagesloaded', onPagesLoaded); |
There was a problem hiding this comment.
This fixes a compatibility issue with new versions of PDF.js that we hadn't noticed before. PDF viewer events are no longer emitted as DOM events. It should be extracted into a separate PR and shipped first.
df229ae to
8307002
Compare
Codecov Report
@@ Coverage Diff @@
## master #2623 +/- ##
==========================================
+ Coverage 97.40% 97.44% +0.03%
==========================================
Files 200 200
Lines 7503 7540 +37
Branches 1653 1656 +3
==========================================
+ Hits 7308 7347 +39
+ Misses 195 193 -2
Continue to review full report at Codecov.
|
|
@robertknight I've pushed some commits that apply styling to the warning message. As of 152212e it looks like this:
The border width (borders are top and bottom only) is |
- Add `documentHasText` helper in `anchoring/pdf.js` which checks if the document appears to have selectable text - Add logic in `plugin/pdf.js` which uses the `documentHasText` helper to check whether the PDF has selectable text and display a banner above the PDF viewer if not Fixes hypothesis/product-backlog#1081
This icon is an MIT-licensed SVG sourced from https://feathericons.com/
Divide up `toast-message` styling for resuability. Use its base styles as a foundation for a full-width "banner" pattern.
152212e to
d435588
Compare
| const hasText = await pdfAnchoring.documentHasText(); | ||
| this._showNoSelectableTextWarning(!hasText); | ||
| } catch (err) { | ||
| /* istanbul ignore next */ |
There was a problem hiding this comment.
I suppose I could have used this approach to what we were discussing regarding #2646 and the guarding against arrays being of different lengths...
| } | ||
|
|
||
| const mainContainer = /** @type {HTMLElement} */ (document.querySelector( | ||
| '#mainContainer' |
There was a problem hiding this comment.
Is this OK as a literal here (i.e. is this selector used elsewhere and could it be maintained as a constant somewhere?)
There was a problem hiding this comment.
This selector isn't referenced anywhere else. It could do with a comment explaining where the element comes from though.
| if (!this._warningBanner) { | ||
| return; | ||
| } | ||
| const rect = this._warningBanner.getBoundingClientRect(); |
There was a problem hiding this comment.
A comment here might not hurt—explaining that we're moving the main container down to accommodate the warning banner, right?
| }; | ||
|
|
||
| this._warningBanner = document.createElement('div'); | ||
| this._warningBanner.className = |
There was a problem hiding this comment.
I'll acknowledge that these classnames could be...tidier.
| // Update the banner when the window is resized or the Hypothesis | ||
| // sidebar is opened. | ||
| // @ts-ignore | ||
| new ResizeObserver(updateBannerHeight).observe(this._warningBanner); |
There was a problem hiding this comment.
ResizeObserver is supported in, it looks like, all of our target browsers save for Android's Firefox. Historically, we've used MutationObserver (broader/older support but more complex API) as a polyfill, see src/sidebar/util/observe-element-size — do we have a plan going forward?
There was a problem hiding this comment.
ResizeObserver was only recently introduced in Safari (13.1) which means it isn't available on older iOS devices. In older browsers the banner height will simply be static based on the window size when the client first loads. I think that's an acceptable failure mode.
So in summary - whether we need a fallback depends on the use case. For annotation cards it is more important to have a fallback for when ResizeObserver is not available.
Add comments to explain where the `#mainContainer` selector come from, when the warning banner size may change and what happens in older browsers.
This is an initial solution for hypothesis/product-backlog#1081. It adds a banner to the top of the PDF viewer when the current PDF does not contain any selectable text.
Current design (updated 2020-10-16):
The "OCR-optimized" text links to https://web.hypothes.is/help/how-to-ocr-optimize-pdfs/.