[api-minor] Expanding divs to improve selection #7539
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,4 +12,4 @@ | |
| "disableTextLayer": false, | ||
| "useOnlyCssZoom": false, | ||
| "externalLinkTarget": 0 | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,6 +50,8 @@ var TEXT_LAYER_RENDER_DELAY = 200; // ms | |
| * @property {PDFRenderingQueue} renderingQueue - The rendering queue object. | ||
| * @property {IPDFTextLayerFactory} textLayerFactory | ||
| * @property {IPDFAnnotationLayerFactory} annotationLayerFactory | ||
| * @property {boolean} enhanceTextSelection - Turns on the text selection | ||
| * enhancement. The default is `false`. | ||
| */ | ||
|
|
||
| /** | ||
|
|
@@ -69,6 +71,7 @@ var PDFPageView = (function PDFPageViewClosure() { | |
| var renderingQueue = options.renderingQueue; | ||
| var textLayerFactory = options.textLayerFactory; | ||
| var annotationLayerFactory = options.annotationLayerFactory; | ||
| var enhanceTextSelection = options.enhanceTextSelection || false; | ||
|
There was a problem hiding this comment. The intention is to enable this by default (see previous comments), so let's change this to There was a problem hiding this comment. Given the performance/correctness issues currently present, refer to #7539 (comment), I don't think that we want to enable this by default at this point. Hence More to the point though, please note that we agreed to #7539 (comment), to avoid changing the defaults for the viewer components. Hence this comment is not applicable. There was a problem hiding this comment. Agreed. We can also land this disabled and file a follow-up issue for the optimizations. |
||
|
|
||
| this.id = id; | ||
| this.renderingId = 'page' + id; | ||
|
|
@@ -78,6 +81,7 @@ var PDFPageView = (function PDFPageViewClosure() { | |
| this.viewport = defaultViewport; | ||
| this.pdfPageRotate = defaultViewport.rotation; | ||
| this.hasRestrictedScaling = false; | ||
| this.enhanceTextSelection = enhanceTextSelection; | ||
|
|
||
| this.eventBus = options.eventBus || domEvents.getGlobalEventBus(); | ||
| this.renderingQueue = renderingQueue; | ||
|
|
@@ -395,9 +399,9 @@ var PDFPageView = (function PDFPageViewClosure() { | |
| div.appendChild(textLayerDiv); | ||
| } | ||
|
|
||
| textLayer = this.textLayerFactory. | ||
| createTextLayerBuilder(textLayerDiv, this.id - 1, this.viewport, | ||
| this.enhanceTextSelection); | ||
| } | ||
| this.textLayer = textLayer; | ||
|
|
||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,6 +76,8 @@ var DEFAULT_CACHE_SIZE = 10; | |
| * queue object. | ||
| * @property {boolean} removePageBorders - (optional) Removes the border shadow | ||
| * around the pages. The default is false. | ||
| * @property {boolean} enhanceTextSelection - (optional) Enables the improved | ||
| * text selection behaviour. The default is `false`. | ||
| */ | ||
|
|
||
| /** | ||
|
|
@@ -127,6 +129,7 @@ var PDFViewer = (function pdfViewer() { | |
| this.linkService = options.linkService || new SimpleLinkService(); | ||
| this.downloadManager = options.downloadManager || null; | ||
| this.removePageBorders = options.removePageBorders || false; | ||
| this.enhanceTextSelection = options.enhanceTextSelection || false; | ||
|
There was a problem hiding this comment. Same here, let's make this There was a problem hiding this comment. Please note that #7539 (comment) applies here too, hence this shouldn't change. |
||
|
|
||
|
There was a problem hiding this comment. Please remove this newline. |
||
| this.defaultRenderingQueue = !options.renderingQueue; | ||
| if (this.defaultRenderingQueue) { | ||
|
|
@@ -352,7 +355,8 @@ var PDFViewer = (function pdfViewer() { | |
| defaultViewport: viewport.clone(), | ||
| renderingQueue: this.renderingQueue, | ||
| textLayerFactory: textLayerFactory, | ||
| annotationLayerFactory: this, | ||
| enhanceTextSelection: this.enhanceTextSelection, | ||
| }); | ||
| bindOnAfterAndBeforeDraw(pageView); | ||
| this._pages.push(pageView); | ||
|
|
@@ -798,13 +802,16 @@ var PDFViewer = (function pdfViewer() { | |
| * @param {PageViewport} viewport | ||
| * @returns {TextLayerBuilder} | ||
| */ | ||
| createTextLayerBuilder: function (textLayerDiv, pageIndex, viewport, | ||
| enhanceTextSelection) { | ||
| return new TextLayerBuilder({ | ||
| textLayerDiv: textLayerDiv, | ||
| eventBus: this.eventBus, | ||
| pageIndex: pageIndex, | ||
| viewport: viewport, | ||
| findController: this.isInPresentationMode ? null : this.findController, | ||
| enhanceTextSelection: this.isInPresentationMode ? false : | ||
| enhanceTextSelection, | ||
| }); | ||
| }, | ||
|
|
||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,8 @@ | |
| * @property {number} pageIndex - The page index. | ||
| * @property {PageViewport} viewport - The viewport of the text layer. | ||
| * @property {PDFFindController} findController | ||
| * @property {boolean} enhanceTextSelection - Option to turn on improved | ||
| * text selection. | ||
| */ | ||
|
|
||
| /** | ||
|
|
@@ -57,16 +59,19 @@ var TextLayerBuilder = (function TextLayerBuilderClosure() { | |
| this.textDivs = []; | ||
| this.findController = options.findController || null; | ||
| this.textLayerRenderTask = null; | ||
| this.enhanceTextSelection = options.enhanceTextSelection; | ||
| this._bindMouse(); | ||
| } | ||
|
|
||
| TextLayerBuilder.prototype = { | ||
| _finishRendering: function TextLayerBuilder_finishRendering() { | ||
| this.renderingDone = true; | ||
|
|
||
| if (!this.enhanceTextSelection) { | ||
| var endOfContent = document.createElement('div'); | ||
| endOfContent.className = 'endOfContent'; | ||
| this.textLayerDiv.appendChild(endOfContent); | ||
| } | ||
|
|
||
| this.eventBus.dispatch('textlayerrendered', { | ||
| source: this, | ||
|
|
@@ -96,7 +101,8 @@ var TextLayerBuilder = (function TextLayerBuilderClosure() { | |
| container: textLayerFrag, | ||
| viewport: this.viewport, | ||
| textDivs: this.textDivs, | ||
| timeout: timeout, | ||
| enhanceTextSelection: this.enhanceTextSelection, | ||
| }); | ||
| this.textLayerRenderTask.promise.then(function () { | ||
| this.textLayerDiv.appendChild(textLayerFrag); | ||
|
|
@@ -314,7 +320,12 @@ var TextLayerBuilder = (function TextLayerBuilderClosure() { | |
| */ | ||
| _bindMouse: function TextLayerBuilder_bindMouse() { | ||
| var div = this.textLayerDiv; | ||
| var self = this; | ||
| div.addEventListener('mousedown', function (e) { | ||
| if (self.enhanceTextSelection && self.textLayerRenderTask) { | ||
| self.textLayerRenderTask.expandTextDivs(true); | ||
| return; | ||
| } | ||
|
There was a problem hiding this comment. here and in mouseup, return from the function to avoid div-workaround logic. There was a problem hiding this comment. Then I'd assume that we also want to change https://github.com/mozilla/pdf.js/blob/master/web/text_layer_builder.js#L67-L69 to something like: if (!this.enhanceTextSelection) {
var endOfContent = document.createElement('div');
endOfContent.className = 'endOfContent';
this.textLayerDiv.appendChild(endOfContent);
} |
||
| var end = div.querySelector('.endOfContent'); | ||
| if (!end) { | ||
| return; | ||
|
|
@@ -338,6 +349,10 @@ var TextLayerBuilder = (function TextLayerBuilderClosure() { | |
| end.classList.add('active'); | ||
| }); | ||
| div.addEventListener('mouseup', function (e) { | ||
| if (self.enhanceTextSelection && self.textLayerRenderTask) { | ||
| self.textLayerRenderTask.expandTextDivs(false); | ||
| return; | ||
| } | ||
| var end = div.querySelector('.endOfContent'); | ||
| if (!end) { | ||
| return; | ||
|
|
@@ -362,13 +377,16 @@ DefaultTextLayerFactory.prototype = { | |
| * @param {HTMLDivElement} textLayerDiv | ||
| * @param {number} pageIndex | ||
| * @param {PageViewport} viewport | ||
| * @param {boolean} enhanceTextSelection | ||
| * @returns {TextLayerBuilder} | ||
| */ | ||
| createTextLayerBuilder: function (textLayerDiv, pageIndex, viewport, | ||
| enhanceTextSelection) { | ||
| return new TextLayerBuilder({ | ||
| textLayerDiv: textLayerDiv, | ||
| pageIndex: pageIndex, | ||
| viewport: viewport, | ||
| enhanceTextSelection: enhanceTextSelection | ||
| }); | ||
| } | ||
| }; | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Nit: please change this to: