Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relocate inline <body> scripts to their own tag in the <head> #2110

Closed
wants to merge 3 commits into from

Conversation

audiodude
Copy link
Member

Fixes #2096

In the abstract Renderer, we create a method, collectInlineJs, which grabs any <script> tags that are in the body of the article, takes the actual JavaScript inside, and collects it into a string. The original <body> <script> tags are removed in the process. This string is passed to the callback of processHtml, where the concrete Renderers convert the intermediate <body> HTML into a full HTML document, and put in the <head>.

We introduce a new template, inline_js_to_head_wrapper.html, which contains a single script tag, with the body scripts wrapped in a DOMContentLoaded listener. This means that the scripts will still execute, in supported environments, after the page has loaded. It is unclear the proper function of these scripts, but the thought is that we might as well try to run them since we're already including scripts such as pcs.js in the page.

@audiodude audiodude requested a review from Jaifroid December 4, 2024 17:57
Copy link

codecov bot commented Dec 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.44%. Comparing base (1763a23) to head (a7fc05a).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2110   +/-   ##
=======================================
  Coverage   75.43%   75.44%           
=======================================
  Files          41       41           
  Lines        3188     3205   +17     
  Branches      703      707    +4     
=======================================
+ Hits         2405     2418   +13     
- Misses        666      670    +4     
  Partials      117      117           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Jaifroid
Copy link
Collaborator

Jaifroid commented Dec 4, 2024

@audiodude Just to clarify, the idea is that they are collected and put into a separate js file which is then called on DOMContentLoaded? The issue being that in certain environments we can only run scripts that are attached as files, rather than raw JS code included in <script> tags in the document (whether in the head or the body). If that is what you're doing in the PR, then it should solve the problem.

@Jaifroid
Copy link
Collaborator

Jaifroid commented Dec 4, 2024

I'll take a look at the code asap.

@audiodude
Copy link
Member Author

audiodude commented Dec 4, 2024

@audiodude Just to clarify, the idea is that they are collected and put into a separate js file which is then called on DOMContentLoaded? The issue being that in certain environments we can only run scripts that are attached as files, rather than raw JS code included in <script> tags in the document (whether in the head or the body). If that is what you're doing in the PR, then it should solve the problem.

Nope that's not it. I guess I misunderstood the requirements, so thanks for looking!

This collects all of the inline <body> scripts and shoves them in a <script> tag in the head. I don't see any reasonable way (though it is technically possible) to collect them into their own file and load that. So I think we should pare back this PR to just remove them altogether.

EDIT: See the test for the example of what happens: https://github.com/openzim/mwoffliner/pull/2110/files#diff-3e8990a262b08f8ee68d62817c1c36e46727c30f9068b3468ec3c8d802cbe8b7

@Jaifroid
Copy link
Collaborator

Jaifroid commented Dec 4, 2024

Ah OK. It used to be the case that mwOffliner could include its own script files (overrides). It would probably be complicated to do. If these scripts are not doing anything useful now that all sections are open anyway (their function was to open the sections), then removing them would be the simpler option.

@audiodude
Copy link
Member Author

Ah OK. It used to be the case that mwOffliner could include its own script files (overrides). It would probably be complicated to do.

mwoffliner can, and still does, that. However, it's for generic scripts that apply to all pages. We would have to assume every page has onBodyStart() and onBodyEnd() only, and include a script that calls those, using the current method of just attaching a single script file to every page, while stripping them from the body. This is possible, but not resilient to new scripts being added to the body, which would be stripped but not have their functionality replaced.

If these scripts are not doing anything useful now that all sections are open anyway (their function was to open the sections), then removing them would be the simpler option.

Yes, as I said on the bug, the README for these scripts say they are for lazy loading images, "themeing" (whatever that is), and obviously for unhiding sections (though the README doesn't mention that).

@audiodude
Copy link
Member Author

Closing this, as it doesn't meet requirements. Will try again with a version that only strips inline scripts, but doesn't relocate them.

@audiodude audiodude closed this Dec 5, 2024
@audiodude audiodude deleted the relocate-inline-js branch December 5, 2024 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

REGRESSION: Remove new inline JS from ZIMs produced by dev (at least 1.14.0) to comply with restrictive CSPs
2 participants