Skip to content

Commit c86d4c7

Browse files
cdrinicdrini
committed
Code review feedback
1 parent 8b4eb48 commit c86d4c7

File tree

8 files changed

+89
-112
lines changed

8 files changed

+89
-112
lines changed

‎src/plugins/plugin.experiments.js‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,20 +51,20 @@ export class ExperimentsPlugin extends BookReaderPlugin {
5151
localStorageKey: 'BrExperiments',
5252

5353
/** The experiments that should be shown in the experiments panel */
54-
enabledExperiments: ['translate', 'copyToSelectionUrl'],
54+
enabledExperiments: ['translate', 'copyLinkToHighlight'],
5555
}
5656

5757
/** @type {ExperimentModel[]} */
5858
allExperiments = [
5959
new class extends ExperimentModel {
60-
name = 'copyToSelectionUrl';
60+
name = 'copyLinkToHighlight';
6161
title = 'Copy to Selection URL';
6262
description = 'Share text selection via URL';
6363
learnMore = 'none';
6464
icon = null;
6565
enabled = false;
6666
async enable ({ manual = false }) {
67-
this.br.plugins.textSelection.enableExperiment();
67+
this.br.plugins.textSelection.enableSelectionMenu();
6868
}
6969
async disable() {
7070
sleep(0).then(() => {

‎src/plugins/plugin.text_selection.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ export class TextSelectionPlugin extends BookReaderPlugin {
6363
this.textSelectionManager.init();
6464
}
6565

66-
enableExperiment() {
66+
enableSelectionMenu() {
6767
this.textSelectionManager.selectionMenuEnabled = true;
6868
this.textSelectionManager.renderSelectionMenu();
6969
}

‎src/plugins/url/UrlPlugin.js‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -156,18 +156,6 @@ export class UrlPlugin {
156156
this.oldLocationHash = urlStrPath;
157157
}
158158

159-
/**
160-
* Get the hash out of the current URL. Also augments it with the text
161-
* from the main part of the URL, since that is not readable by JS
162-
* from the actual hash
163-
* @returns
164-
*/
165-
getHash = () => {
166-
const text = this.retrieveTextFragment(window.location.search);
167-
const textFragment = text ? `:~:text=${text[0]}` : '';
168-
return `${window.location.hash.slice(1)}${textFragment}`;
169-
}
170-
171159
/**
172160
* Get the url and check if it has changed
173161
* If it was changeed, update the urlState
@@ -201,6 +189,18 @@ export class UrlPlugin {
201189
this.urlState = this.urlStringToUrlState(path);
202190
}
203191

192+
/**
193+
* Get the hash out of the current URL. Also augments it with the text
194+
* from the main part of the URL, since that is not readable by JS
195+
* from the actual hash
196+
* @returns
197+
*/
198+
getHash() {
199+
const text = this.retrieveTextFragment(window.location.search);
200+
const textFragment = text ? `:~:text=${text[0]}` : '';
201+
return `${window.location.hash.slice(1)}${textFragment}`;
202+
}
203+
204204
/**
205205
* @param {string} urlString
206206
* @returns {string}

‎src/plugins/url/plugin.url.js‎

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
/* global BookReader */
22

33
import { UrlPlugin } from "./UrlPlugin.js";
4+
import { sleep } from "../../BookReader/utils.js";
45

56
/**
67
* Plugin for URL management in BookReader
@@ -217,13 +218,9 @@ BookReader.prototype.urlUpdateFragment = function() {
217218
* @param {string} url
218219
* @return {string}
219220
* */
220-
221-
// testing with this URL http://127.0.0.1:8000/BookReaderDemo/demo-internetarchive.html?ocaid=adventureofsherl0000unse&text=Well%2C I found my plans very seriously menaced.&q=breaking the law#page/18/mode/2up
222221
BookReader.prototype.urlParamsFiltersOnlySearch = function(url) {
223222
const params = new URLSearchParams(url);
224-
let output = '';
225-
output += params.has('q') ? `?${new URLSearchParams({ q: params.get('q') })}` : '';
226-
return output;
223+
return params.has('q') ? `?${new URLSearchParams({ q: params.get('q') })}` : '';
227224
};
228225

229226

@@ -255,12 +252,10 @@ export class BookreaderUrlPlugin extends BookReader {
255252
if (location.includes("text=")) {
256253
this.on('textLayerVisible', async (_, {pageContainerEl}) => {
257254
const visiblePageNum = pageContainerEl.getAttribute('data-page-num');
258-
let renderPageDelay = 100;
259-
if (this.mode === 1) {
260-
// maybe add some more time to have the page "settle down" from user scrolling
261-
renderPageDelay = 900;
262-
}
263-
await new Promise(resolve => setTimeout(resolve, renderPageDelay));
255+
256+
// Hack: More time mode 1up page "settle down" from user scrolling
257+
await sleep(this.mode === 1 ? 900 : 100);
258+
264259
// No textFragment found or the textFragment stored doesn't match current visible page loaded
265260
if (!this.textFragment || this.textFragmentPage !== visiblePageNum) return;
266261
if (this.options.urlMode === 'history') {

‎src/util/TextSelectionManager.js‎

Lines changed: 52 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,16 @@ import { customElement } from 'lit/decorators.js';
55
import '@internetarchive/icon-share';
66

77
export class TextSelectionManager {
8-
/** @type {BRSelectMenu} */
9-
selectMenu;
10-
selectionMenuEnabled;
118
options = {
129
// Current Translation plugin implementation does not have words, will limit to one BRlineElement for now
1310
maxProtectedWords: 200,
1411
}
1512

13+
/** @type {BRSelectMenu} */
14+
selectMenu;
15+
/** @type {boolean} */
16+
selectionMenuEnabled = false;
17+
1618
/**
1719
* @param {string} layer Selector for the text layer to manage
1820
* @param {import('../BookReader.js').default} br
@@ -29,8 +31,8 @@ export class TextSelectionManager {
2931
this.selectionElement = selectionElement;
3032
this.selectionObserver = new SelectionObserver(this.layer, this._onSelectionChange);
3133
this.options.maxProtectedWords = maxWords ? maxWords : 200;
32-
this.selectMenu = new BRSelectMenu(br);
3334

35+
this.selectMenu = new BRSelectMenu(br);
3436
this.selectMenu.className = "br-select-menu__root";
3537
}
3638

@@ -66,7 +68,9 @@ export class TextSelectionManager {
6668
// Need attach + detach methods to toggle w/ Translation plugin
6769
attach() {
6870
this.selectionObserver.attach();
69-
this.renderSelectionMenu();
71+
if (this.selectionMenuEnabled) {
72+
this.renderSelectionMenu();
73+
}
7074
if (this.br.protected) {
7175
document.addEventListener('selectionchange', this._limitSelection);
7276
// Prevent right clicking when selected text
@@ -94,9 +98,7 @@ export class TextSelectionManager {
9498

9599
renderSelectionMenu() {
96100
if (document.querySelector('.br-select-menu__option')) return;
97-
if (this.selectionMenuEnabled) {
98-
document.body.append(this.selectMenu);
99-
}
101+
document.body.append(this.selectMenu);
100102
}
101103
/**
102104
* @param {'started' | 'cleared' | 'focusChanged'} type
@@ -244,18 +246,18 @@ export class TextSelectionManager {
244246
};
245247
}
246248

247-
/** TODO ->
248-
* Can import something that handles this more gracefully? see - https://web.dev/articles/text-fragments#:~:text=In%20its%20simplest%20form%2C%20the%20syntax%20of,percent%2Dencoded%20text%20I%20want%20to%20link%20to.
249-
*/
250249
/**
251-
* Builds a URL string in the format of a TextFragment within the URL params, which differs from browser "Copy to link to highlighted text" format
252-
* Does not include the fragment directive (`:~:`) and is not after the URL hash
250+
* Builds a TextFragment string from a given text selection.
251+
* Note does not include the fragment directive `:~:` or # symbol
253252
* See https://developer.mozilla.org/en-US/docs/Web/URI/Reference/Fragment/Text_fragments
254-
* @param {Selection} selection - document.getSelection()
255-
* @param {HTMLElement} pageLayer - anchorNode.parentElement.closest('.BRtextLayer')
256-
* @returns {string} - i.e. http://127.0.0.1:8000/BookReaderDemo/demo-internetarchive.html?ocaid=adventureofsherl0000unse&text=undefined,undefined#page/10/mode/2up
253+
* @param {Selection} selection currently selected text, eg `document.getSelection()`
254+
* @param {HTMLElement[]} contextElements elements providing context for the selection
255+
* @returns {string}
257256
*/
258-
export function createTextFragmentUrlParam(selection, pageLayer) {
257+
export function createTextFragmentUrlParam(selection, contextElements) {
258+
// TODO: Can import something that handles this more gracefully? see -
259+
// https://web.dev/articles/text-fragments#:~:text=In%20its%20simplest%20form%2C%20the%20syntax%20of,percent%2Dencoded%20text%20I%20want%20to%20link%20to.
260+
259261
// :~:text=[prefix-,]textStart[,textEnd][,-suffix]
260262
const highlightedText = selection.toString().replace(/[\s]+/g, " ").trim().split(" ");
261263
const direction = selection.direction;
@@ -274,37 +276,27 @@ export function createTextFragmentUrlParam(selection, pageLayer) {
274276
const endPhraseMatchRe = new RegExp(String.raw`(${textStartRe})(?=.*?(${textEndRe}))`, "gis");
275277

276278
// Duplicated spaces in pageLayer.textContent for some reason
277-
const wholePageText = Array.from(document.querySelectorAll('.BRpage-visible'))
278-
.map((item) => item.textContent)
279+
const selectionContext = contextElements
280+
.map((el) => el.textContent)
279281
.join(' ')
280-
.replace(/\s+/g, " ") || pageLayer.textContent.replace(/\s+/g, " ");
281-
const startPhraseFoundMatches = wholePageText.matchAll(startPhraseMatchRe).toArray();
282-
const endPhraseFoundMatches = wholePageText.matchAll(endPhraseMatchRe).toArray();
282+
.replace(/\s+/g, " ");
283+
const startPhraseFoundMatches = selectionContext.matchAll(startPhraseMatchRe).toArray();
284+
const endPhraseFoundMatches = selectionContext.matchAll(endPhraseMatchRe).toArray();
283285
if (startPhraseFoundMatches.length == 1 && endPhraseFoundMatches.length == 1) {
284286
// If `startWord...endWord` quote is unambiguous and only occurs once, no prefix-/-suffix is needed for the URL param
285287
return `text=${encodeURIComponent(startWord)},${encodeURIComponent(endWord)}`;
286288
}
287289

288290
// Need to add some additional context to `startWord...endWord` by including surrounding words before and after the keywords
289291
const preStartRange = document.createRange();
290-
291-
const previousPageContainer = pageLayer.parentElement?.previousElementSibling;
292-
if (previousPageContainer?.classList.contains("BRpage-visible")) {
293-
preStartRange.setStart(previousPageContainer, 0);
294-
} else {
295-
preStartRange.setStart(pageLayer.firstElementChild, 0);
296-
}
292+
preStartRange.setStart(contextElements[0].firstElementChild, 0);
297293
preStartRange.setEnd(startNode, 0);
294+
298295
const postEndRange = document.createRange();
299296
postEndRange.setStart(endNode, endNode.textContent.length);
300-
const nextPageContainer = pageLayer.parentElement.nextElementSibling;
301-
if (nextPageContainer?.classList.contains("BRpage-visible")) {
302-
const nextPageLastWord = getLastestElement(nextPageContainer);
303-
postEndRange.setEnd(nextPageLastWord, Math.max(0, nextPageLastWord.textContent.length - 1));
304-
} else {
305-
const lastWordOfPageEl = getLastestElement(pageLayer);
306-
postEndRange.setEnd(lastWordOfPageEl, Math.max(0, lastWordOfPageEl.textContent.length - 1));
307-
}
297+
const lastWordOfPageEl = getLastMostElement(contextElements[contextElements.length - 1]);
298+
postEndRange.setEnd(lastWordOfPageEl, Math.max(0, lastWordOfPageEl.textContent.length - 1));
299+
308300
// prefixes/suffixes cannot contain paragraph breaks, words that are from more than one line break away should not be included
309301
const prefix = getLastWords(3, preStartRange.toString())
310302
.replace(/[ ]+/g, " ")
@@ -446,33 +438,38 @@ class BRSelectMenu extends LitElement {
446438
`;
447439
}
448440

441+
/**
442+
* @param {MouseEvent} e
443+
*/
449444
handleCopyLinkToHighlight(e) {
450445
e.preventDefault();
451-
const currentUrl = window.location;
452-
let currentParams = this.br.readQueryString();
446+
447+
const currentParams = this.br.readQueryString();
453448
const currentSelection = window.getSelection();
454449
/** @type {HTMLElement} */
455450
const textLayer = currentSelection.anchorNode.parentElement.closest('.BRtextLayer');
456-
// To do - updateResumeValue + getCookiePath in plugin.resume.js overrides the adjustedUrlPageNumPath, check how to workaround this
457-
const adjustedUrlPageNumPath = currentUrl.pathname.toString().replace(/(?<=\/page\/)\d+(?=\/)/, textLayer.parentElement.getAttribute('data-page-num'));
451+
const textFragmentUrlParam = createTextFragmentUrlParam(currentSelection, Array.from(document.querySelectorAll('.BRpage-visible')));
452+
453+
// Note: Have to do a param construction to avoid url-encoding of commas in the text fragment param
454+
let linkToHighlightParams = currentParams;
458455
if (currentParams.includes('text=')) {
459-
currentParams = currentParams.replace(/(text=)[\w\W\d%]+/, createTextFragmentUrlParam(currentSelection, textLayer));
456+
linkToHighlightParams = currentParams.replace(/(text=)[\w\W\d%]+/, textFragmentUrlParam);
460457
} else {
461-
if (this.br.options.urlMode === 'history') {
462-
currentParams = `?${createTextFragmentUrlParam(currentSelection, textLayer)}`;
463-
} else {
464-
currentParams = `${currentParams}&${createTextFragmentUrlParam(currentSelection, textLayer)}`;
465-
}
466-
}
467-
if (this.br.options.urlMode === 'history') {
468-
navigator.clipboard.writeText(`${currentUrl.origin}${adjustedUrlPageNumPath}${currentParams}`);
469-
} else {
470-
navigator.clipboard.writeText(`${currentUrl.origin}${adjustedUrlPageNumPath}${currentParams}${currentUrl?.hash}`);
458+
const sep = linkToHighlightParams ? '&' : '?';
459+
linkToHighlightParams += `${sep}${textFragmentUrlParam}`;
471460
}
461+
462+
const currentUrl = window.location;
463+
// TODO - updateResumeValue + getCookiePath in plugin.resume.js overrides the adjustedUrlPageNumPath, check how to workaround this
464+
// TODO - won't work with hash mode
465+
const adjustedUrlPageNumPath = currentUrl.pathname.toString().replace(/(?<=\/page\/)\d+(?=\/)/, textLayer.parentElement.getAttribute('data-page-num'));
466+
467+
const linkToHighlight = `${currentUrl.origin}${adjustedUrlPageNumPath}${linkToHighlightParams}${currentUrl?.hash || ''}`;
468+
navigator.clipboard.writeText(linkToHighlight);
472469
}
473470

474471
showMenu() {
475-
if (this.br.plugins.translate) return;
472+
if (this.br.plugins.translate?.userToggleTranslate) return;
476473
const currentSelection = window.getSelection();
477474
const start = currentSelection.anchorNode.parentElement;
478475
const end = currentSelection.focusNode.parentElement; // will always be a text node
@@ -490,7 +487,7 @@ class BRSelectMenu extends LitElement {
490487
this.style.left = `${hlButtonLeft}px`;
491488
this.style.zIndex = '1';
492489
this.style.position = 'absolute';
493-
this.style.display = 'inline';
490+
this.style.display = 'block';
494491
}
495492

496493
hideMenu = () => {
@@ -527,7 +524,7 @@ export function getLastWords(numWords, text) {
527524
* @param {HTMLElement | Element} parent
528525
* @returns {Node}
529526
*/
530-
export function getLastestElement(parent) {
527+
export function getLastMostElement(parent) {
531528
while (parent.lastElementChild) {
532529
parent = parent.lastElementChild;
533530
}

‎tests/jest/BookReader.test.js‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,6 @@ test('gets index from fragment when both fragment and cookie when InitParams cal
7777

7878
test('sets prevReadMode when init called', () => {
7979
BookReader.prototype.urlUpdateFragment = jest.fn();
80-
BookReader.prototype.urlParamsFiltersOnlySearch = jest.fn();
8180
br.init();
8281
expect(br.prevReadMode).toBeTruthy();
8382
});
@@ -86,9 +85,8 @@ test('sets prevReadMode when init called', () => {
8685
// BookReader.prototype.drawLeafsThumbnail
8786
// BookReader.prototype.getPrevReadMode
8887
test('sets prevPageMode if initial mode is thumb', () => {
89-
BookReader.prototype.urlUpdateFragment = jest.fn();
9088
br.urlReadFragment = jest.fn(() => 'page/n4/mode/thumb');
91-
BookReader.prototype.urlParamsFiltersOnlySearch = jest.fn();
89+
9290
br.init();
9391
expect(br.prevReadMode).toBe(BookReader.constMode1up);
9492
});

‎tests/jest/plugins/url/plugin.url.test.js‎

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -48,10 +48,6 @@ describe('Plugin: URL controller', () => {
4848

4949
test('initializes polling for url changes if using hash', () => {
5050
BookReader.prototype.urlReadFragment = jest.fn(() => '/page/2/mode/1up');
51-
BookReader.prototype.urlParamsFiltersOnlySearch = jest.fn((url) => {
52-
const params = new URLSearchParams(url);
53-
return params.has('q') ? `?${new URLSearchParams({ q: params.get('q') })}` : '';
54-
});
5551
BookReader.prototype.urlStartLocationPolling = jest.fn();
5652
br.init();
5753

@@ -83,7 +79,6 @@ describe('Plugin: URL controller', () => {
8379
mode: 2,
8480
search: '',
8581
}));
86-
BookReader.prototype.urlParamsFiltersOnlySearch = jest.fn();
8782
br.options.urlMode = 'history';
8883
br.init();
8984
br.urlUpdateFragment();
@@ -166,19 +161,11 @@ describe('Plugin: URL controller', () => {
166161

167162
test('only q= param is selected from url query params', () => {
168163
const INTIAL_URL = "http://127.0.0.1:8080/BookReaderDemo/demo-internetarchive.html?ocaid=adventuresofoli00dick&q=foo";
169-
BookReader.prototype.urlParamsFiltersOnlySearch = jest.fn((url) => {
170-
const params = new URLSearchParams(url);
171-
return params.has('q') ? `?${new URLSearchParams({ q: params.get('q') })}` : '';
172-
});
173164
const result = br.urlParamsFiltersOnlySearch(INTIAL_URL);
174165
expect(result).toBe("?q=foo");
175166
});
176167

177168
test('only q= param is selected from url query params with special character', () => {
178-
BookReader.prototype.urlParamsFiltersOnlySearch = jest.fn((url) => {
179-
const params = new URLSearchParams(url);
180-
return params.has('q') ? `?${new URLSearchParams({ q: params.get('q') })}` : '';
181-
});
182169
const INTIAL_URL = "http://127.0.0.1:8080/BookReaderDemo/demo-internetarchive.html?ocaid=adventuresofoli00dick&q=foo%24%24";
183170
const result = br.urlParamsFiltersOnlySearch(INTIAL_URL);
184171
expect(result).toBe("?q=foo%24%24");

0 commit comments

Comments
 (0)