Remove all links, stylesheets and scripts added by the boot script when the client is unloaded #2782

robertknight · 2020-12-02T11:55:51Z

This PR fixes an issue where deactivating and re-activating the browser extension did not work properly after the recent addition of a new link[type="application/annotator+html"] element for the notebook. The problem is that the boot script checks for the existence of such a link and if found assumes the client is already active and bails out. There used to be a single link that was removed when the boot script was unloaded. Now there are multiple such links and only one gets removed when the client is unloaded.

The fix here is a general improvement to unloading of <link> and <script> tags added by the boot script when the client is unloaded. All such elements are tagged with a hypothesis-asset attribute by the boot script and then elements with this attribute are removed when the client is unloaded.

I also added a "Load client" / "Unload client" toggle button to the dev server to make it easier to test this without having to build the extension.

Ensure that all links, script and stylesheet elements added to the page by the boot script are removed when the client is unloaded. This fixes a problem where the recently added `<link>` for the notebook app was not removed when the page was unloaded, causing attempts to activate the browser extension on a page where the client had previously been loaded and then removed, to fail. The implementation works by tagging all elements with a `hypothesis-asset` attribute in the boot script and then removing these elements when the client is unloaded.

Add a button to the dev server to facilitate easy testing of unloading and re-loading the client, as done by the browser extension and also some third-party pages.

codecov · 2020-12-02T11:57:49Z

Codecov Report

Merging #2782 (365c89a) into master (09a586c) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2782      +/-   ##
==========================================
- Coverage   97.74%   97.74%   -0.01%     
==========================================
  Files         202      202              
  Lines        7660     7659       -1     
  Branches     1693     1693              
==========================================
- Hits         7487     7486       -1     
  Misses        173      173

Impacted Files	Coverage Δ
src/boot/boot.js	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09a586c...365c89a. Read the comment docs.

lyzadanger

Glad you are addressing this (slash, whoops!) and it's headed in a good direction, but there are a few things that cropped up for me that I thought I'd get your feedback/response on before doing a full review:

On this branch running the dev server, the HTML test pages no longer work—they're throwing a TypeError on the missing toggle button reference
The custom attribute hypothesis-asset results in HTML that won't validate. Might we use a data- attribute or something else?
One of the goals of refactoring the dev server was to keep the serve-dev module relatively free of template literal strings. You might consider creating a new template file for the client-loading script that can be rendered/brought into context in the templateContext function and then referenced in the renderScript function string/literal template similarly to {{{hypothesisConfig}}} — I can provide some sample code for this if it helps?

The toggle button only exists on the index page. Also I fixed an issue where an incorrect variable name went unnoticed due to DOM element IDs creating global variables (sigh).

Adding a custom attribute without a `data-` prefix could potentially cause headaches for publishers running their code through HTML validation tools.

Per discussion on the PR, it is preferable to put markup and JS in the dev server in external files rather than as string literals in `serve-dev.js`. This enables them to be edited without having to restart the dev server. - Extract shared functions for loading and unloading client into `scripts/util.js` - Move logic specific to the index template into `scripts/index.js`.

robertknight · 2020-12-03T09:30:18Z

On this branch running the dev server, the HTML test pages no longer work—they're throwing a TypeError on the missing toggle button reference

🤦‍♂️ - Fixed! The logic for the toggle button is now in dev-server/static/scripts/index.js and is only loaded on the index page.

The custom attribute hypothesis-asset results in HTML that won't validate. Might we use a data- attribute or something else?

Yes, I've changed it to data-hypothesis-asset. While we don't apply HTML validation to the loaded application directly I can imagine that just maybe some user will want to run it on a page that has Hypothesis installed.

One of the goals of refactoring the dev server was to keep the serve-dev module relatively free of template literal strings.

Makes sense. I've moved the code for the toggle button into static/scripts/index.js and the shared code for loading and unloading the client into static/scripts/util.js.

This feature is now enabled by default.

robertknight · 2020-12-03T09:39:28Z

On a related note, modern web specs support using <link> elements directly within a shadow root: whatwg/html#1572. I don't know which of the browsers we support actually support that though.

lyzadanger

Thanks for perservering!

lyzadanger · 2020-12-03T13:52:53Z

dev-server/static/scripts/index.js

@@ -0,0 +1,16 @@
+import { activeClientUrl, loadClient, unloadClient } from './util.js';
+
+const toggleClientButton = document.querySelector('.js-toggle-client');


Can you add a comment/JSdocstring to this module to document its purpose?

lyzadanger · 2020-12-03T13:54:45Z

src/boot/boot.js

+  const link = doc.createElement('link');
+  link.rel = rel;
+  link.href = url;
+  link.type = 'application/annotator+' + type;


Curious if we have a rough convention for when to use template literals or if it's up to the dev. Because of the literal + in this string, I'd personally find it easier to parse as

`application/annotator+${type}`

but this is quite trivial...

Curious if we have a rough convention for when to use template literals or if it's up to the dev.

We don't have a strong convention here. I'm happy to make this change.

And I want to emphasize that we don't need to :). Getting that micro-managey isn't something all that useful.

- Add comment to explain purpose of `index.js` - Use a template string for readability

robertknight added 2 commits December 2, 2020 11:55

Add "Load client" / "Unload client" toggle button to dev server

57dd19e

Add a button to the dev server to facilitate easy testing of unloading and re-loading the client, as done by the browser extension and also some third-party pages.

robertknight requested a review from lyzadanger December 2, 2020 11:55

robertknight mentioned this pull request Dec 2, 2020

Fix error when deactivating extension a second time in a tab hypothesis/browser-extension#453

Merged

lyzadanger suggested changes Dec 2, 2020

View reviewed changes

robertknight added 3 commits December 2, 2020 16:02

Fix client load on dev server pages without toggle button

af61f60

The toggle button only exists on the index page. Also I fixed an issue where an incorrect variable name went unnoticed due to DOM element IDs creating global variables (sigh).

Change hypothesis-asset attribute to data-hypothesis-asset

1a21b37

Adding a custom attribute without a `data-` prefix could potentially cause headaches for publishers running their code through HTML validation tools.

Remove obsolete experimental.pdfSideBySide config

3b562fc

This feature is now enabled by default.

robertknight requested a review from lyzadanger December 3, 2020 10:05

lyzadanger approved these changes Dec 3, 2020

View reviewed changes

Address minor PR feedback

365c89a

- Add comment to explain purpose of `index.js` - Use a template string for readability

robertknight merged commit 91f3648 into master Dec 3, 2020

robertknight deleted the remove-all-links-on-unload branch December 3, 2020 15:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove all links, stylesheets and scripts added by the boot script when the client is unloaded #2782

Remove all links, stylesheets and scripts added by the boot script when the client is unloaded #2782

robertknight commented Dec 2, 2020 •

edited

Loading

codecov bot commented Dec 2, 2020 •

edited

Loading

lyzadanger left a comment

robertknight commented Dec 3, 2020

robertknight commented Dec 3, 2020

lyzadanger left a comment

lyzadanger Dec 3, 2020

lyzadanger Dec 3, 2020

robertknight Dec 3, 2020

lyzadanger Dec 3, 2020

		@@ -0,0 +1,16 @@
		import { activeClientUrl, loadClient, unloadClient } from './util.js';

		const toggleClientButton = document.querySelector('.js-toggle-client');

Remove all links, stylesheets and scripts added by the boot script when the client is unloaded #2782

Remove all links, stylesheets and scripts added by the boot script when the client is unloaded #2782

Conversation

robertknight commented Dec 2, 2020 • edited Loading

codecov bot commented Dec 2, 2020 • edited Loading

Codecov Report

lyzadanger left a comment

Choose a reason for hiding this comment

robertknight commented Dec 3, 2020

robertknight commented Dec 3, 2020

lyzadanger left a comment

Choose a reason for hiding this comment

lyzadanger Dec 3, 2020

Choose a reason for hiding this comment

lyzadanger Dec 3, 2020

Choose a reason for hiding this comment

robertknight Dec 3, 2020

Choose a reason for hiding this comment

lyzadanger Dec 3, 2020

Choose a reason for hiding this comment

robertknight commented Dec 2, 2020 •

edited

Loading

codecov bot commented Dec 2, 2020 •

edited

Loading