Code review changes

Author: schu96

src/BookReader.js

@@ -1958,7 +1958,7 @@ BookReader.prototype.queryStringFromParams = function(
   urlMode = 'hash',
 ) {
   const newParams = new URLSearchParams(currQueryString);
-  let textFragmentParam = '';
+
   if (params.view) {
     // Set ?view=theater when fullscreen
     newParams.set('view', params.view);
@@ -1970,14 +1970,25 @@ BookReader.prototype.queryStringFromParams = function(
   if (params.search && urlMode === 'history') {
     newParams.set('q', params.search);
   }
+
+  let textFragmentParam = '';
+  // Need to pull out text separately to avoid the spaces becoming encoded as +, which
+  // the browser seems not to handle with the text fragment
   if (newParams.get('text')) {
     newParams.delete('text');
-    textFragmentParam = `text=${currQueryString.match(/(?<=&?text=)[^&]*/)}`;
+    textFragmentParam = `text=${this.urlPlugin.retrieveTextFragment(currQueryString)}`;
   }
+
   // https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams/toString
   // Note: This method returns the query string without the question mark.
-  const result = textFragmentParam ? newParams.toString() + textFragmentParam : newParams.toString();
-  return result ? '?' + result : '';
+  let result = newParams.toString();
+  if (textFragmentParam) {
+    if (result) result += '&';
+    result += textFragmentParam;
+  }
+  if (result) result = '?' + result;
+
+  return result;
 };
 
 /**

src/BookReader/utils/SelectionObserver.js

@@ -4,6 +4,7 @@ export class SelectionObserver {
   startedInSelector = false;
   /** @type {HTMLElement} */
   target = null;
+  /** @type {Node} */
   lastKnownFocusNode = null;
 
   /**

src/css/_TextSelection.scss

@@ -161,4 +161,8 @@
     height: 30px;
     top: inherit;
     left: inherit;
+}
+
+.textFragmentButtonBar {
+    display: none;
 }

src/plugins/url/UrlPlugin.js

@@ -155,52 +155,57 @@ export class UrlPlugin {
     }
     this.oldLocationHash = urlStrPath;
   }
+
   /**
    * Get the hash out of the current URL. Also augments it with the text
    * from the main part of the URL, since that is not readable by JS
    * from the actual hash
    * @returns
    */
-    getHash = () => {
-      const text = this.retrieveTextFragment(window.location.search);
-      const textFragment = text ? `:~:text=${text[0]}` : '';
-      return `${window.location.hash.slice(1)}${textFragment}`;
-    }
+  getHash = () => {
+    const text = this.retrieveTextFragment(window.location.search);
+    const textFragment = text ? `:~:text=${text[0]}` : '';
+    return `${window.location.hash.slice(1)}${textFragment}`;
+  }
 
-    /**
+  /**
    * Get the url and check if it has changed
    * If it was changeed, update the urlState
    */
-    listenForHashChanges() {
-      this.oldLocationHash = this.getHash();
-      if (this.urlLocationPollId) {
-        clearInterval(this.urlLocationPollId);
-        this.urlLocationPollId = null;
-      }
+  listenForHashChanges() {
+    this.oldLocationHash = this.getHash();
+    if (this.urlLocationPollId) {
+      clearInterval(this.urlLocationPollId);
+      this.urlLocationPollId = null;
+    }
 
-      // check if the URL changes
-      const updateHash = () => {
-        const newFragment = this.getHash();
-        const hasFragmentChange = newFragment != this.oldLocationHash;
+    // check if the URL changes
+    const updateHash = () => {
+      const newFragment = this.getHash();
+      const hasFragmentChange = newFragment != this.oldLocationHash;
 
-        if (!hasFragmentChange) { return; }
+      if (!hasFragmentChange) { return; }
 
-        this.urlState = this.urlStringToUrlState(newFragment);
-      };
-      this.urlLocationPollId = setInterval(updateHash, 500);
-    }
+      this.urlState = this.urlStringToUrlState(newFragment);
+    };
+    this.urlLocationPollId = setInterval(updateHash, 500);
+  }
 
-    /**
+  /**
    * Will read either the hash or URL and return the bookreader fragment
    */
-    pullFromAddressBar (location = window.location) {
-      const path = this.urlMode === 'history'
-        ? (location.pathname.substr(this.urlHistoryBasePath.length) + location.search)
-        : location.hash.substr(1);
-      this.urlState = this.urlStringToUrlState(path);
-    }
+  pullFromAddressBar(location = window.location) {
+    const path = this.urlMode === 'history'
+      ? (location.pathname.substr(this.urlHistoryBasePath.length) + location.search)
+      : location.hash.substr(1);
+    this.urlState = this.urlStringToUrlState(path);
+  }
 
-    retrieveTextFragment (urlString) {
-      return urlString.match(/(?<=&?text=)[^&]*/);
-    }
+  /**
+   * @param {string} urlString
+   * @returns {string}
+   */
+  retrieveTextFragment(urlString) {
+    return urlString.match(/(?<=[&?]?text=)[^&]*/);
+  }
 }

src/plugins/url/plugin.url.js

@@ -140,11 +140,21 @@ BookReader.prototype.urlUpdateFragment = function() {
     return validParams;
   }, {});
 
+  // eg 'page/3/mode/2up'; no query params (in hash mode, it might have /search/term)
+  // Does NOT have the :~:text fragment
   const newFragment = this.fragmentFromParams(params, this.options.urlMode);
+  // eg 'page/3/mode/2up'; no query params
+  // WILL CONTAIN the :~:text fragment in hash mode (!)
   const currFragment = this.urlReadFragment();
+  // This should have both ?q=foo&text=bar (and any other params) as an encoded string
   const currQueryString = this.getLocationSearch();
+  // Eg ?q=foo&text=bar; only query params, no fragment
   const newQueryString = this.queryStringFromParams(params, currQueryString, this.options.urlMode);
-  const hasTextParam = currQueryString.includes("text");
+
+  // NOTE: If ?text is in the URL, we will fire fragment change events on every render; which is
+  // not desireable, but currently don't have a way to handle re-writing ?text to the hash text
+  // fragment form, :~:text=foo.
+  const hasTextParam = this.urlPlugin.retrieveTextFragment(currQueryString);
   if (currFragment === newFragment && currQueryString === newQueryString && !hasTextParam) {
     return;
   }
@@ -155,21 +165,14 @@ BookReader.prototype.urlUpdateFragment = function() {
     } else {
       const baseWithoutSlash = this.options.urlHistoryBasePath.replace(/\/+$/, '');
       const newFragmentWithSlash = newFragment === '' ? '' : `/${newFragment}`;
-      let textFragment = "";
-      if (this.urlPlugin.retrieveTextFragment(newQueryString)) {
-        textFragment = this.urlParamsFiltersOnlySearch(this.readQueryString());
-        // newQueryString = newQueryString.replace(this.urlPlugin.retrieveTextFragment(newQueryString)[0], "").replace(/(\?text=)/, "")
-      }
-      console.log("this is newQueryString", newQueryString);
+      const textFragment = this.urlPlugin.retrieveTextFragment(newQueryString);
       const newUrlPath = `${baseWithoutSlash}${newFragmentWithSlash}${newQueryString}`;
-      console.log("this is newURLPath", newUrlPath);
       try {
         window.history.replaceState({}, null, newUrlPath);
-        this.oldLocationHash = newFragment + newQueryString + textFragment;
-        // if (textFragment) {
-        //   window.location.replace('#' + textFragment);
-        //   this.oldLocationHash = textFragment;
-        // }
+        this.oldLocationHash = newFragment + newQueryString;
+        if (textFragment) {
+          this.oldLocationHash += `:~:text=${textFragment[0]}`;
+        }
       } catch (e) {
         // DOMException on Chrome when in sandboxed iframe
         this.options.urlMode = 'hash';
@@ -179,8 +182,12 @@ BookReader.prototype.urlUpdateFragment = function() {
 
   if (this.options.urlMode === 'hash')  {
     const newQueryStringSearch = this.urlParamsFiltersOnlySearch(this.readQueryString());
-    window.location.replace('#' + newFragment + newQueryStringSearch);
-    this.oldLocationHash = newFragment + newQueryStringSearch;
+    let textFragment = this.urlPlugin.retrieveTextFragment(this.readQueryString());
+    if (textFragment) {
+      textFragment = `:~:text=${textFragment[0]}`;
+    }
+    window.location.replace('#' + newFragment + newQueryStringSearch + textFragment);
+    this.oldLocationHash = newFragment + newQueryStringSearch + textFragment;
   }
 };
 
@@ -194,11 +201,9 @@ BookReader.prototype.urlUpdateFragment = function() {
 
 // 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
 BookReader.prototype.urlParamsFiltersOnlySearch = function(url) {
-  const text = this.urlPlugin.retrieveTextFragment(url);
   const params = new URLSearchParams(url);
   let output = '';
   output += params.has('q') ? `?${new URLSearchParams({ q: params.get('q') })}` : '';
-  output += text ? `:~:text=${text[0]}` : '';
   return output;
 };
 
@@ -230,10 +235,15 @@ export class BookreaderUrlPlugin extends BookReader {
       const location = this.getLocationSearch();
       if (location.includes("text=")) {
         this.on('textLayerRendered', (_, {pageIndex, container}) => {
-          window.location.replace(`#${this.oldLocationHash}`);
-        });
-        this.on('pageVisible', (_) => {
-          window.location.replace(`#${this.oldLocationHash}`);
+          if (this.options.urlMode === 'history') {
+            // only want the text fragment forwarded
+            if (this.oldLocationHash.includes(':~:')) {
+              const newHash = this.oldLocationHash.split(':~:')[1];
+              window.location.replace(`#:~:${newHash}`);
+            }
+          } else {
+            window.location.replace(`#${this.oldLocationHash}`);
+          }
         });
       }
       this.bind(BookReader.eventNames.PostInit, () => {

src/util/TextSelectionManager.js

@@ -108,10 +108,12 @@ export class TextSelectionManager {
    * @param {HTMLElement} target
    */
   _onSelectionChange = (type, target) => {
-    if (type === 'started' || type === 'focusChanged') {
+    if (type === 'started') {
       this.textSelectingMode(target);
     } else if (type === 'cleared') {
       this.defaultMode(target);
+    } else if (type === 'focusChanged') {
+      // do nothing, just wait for the mouseup to trigger the styling change
     } else {
       throw new Error(`Unknown type ${type}`);
     }