Allow for managing third-party javascript integrations to be placed at the end of the head-section as well as the end of the page.

jladage commented 6 months ago

PLIP (Plone Improvement Proposal)

Responsible Persons

Proposer: Jean-Paul Ladage

Seconder: Maurits van Rees

Abstract

Some Javascript integrations require injecting js at the end of the HEAD section, others require it at the bottom of the page.

Motivation

Currently Plone only allows for including js at the bottom of each page and the label Website Statistics makes it focused on limited use. E.g. Google Tag Manager requires inclusion of the tag in the head section. Imho Plone at least should allow website managers to include in either places.

Assumptions

Plone classic websites, no not every website will be a Volto site in the future.

Proposal & Implementation

Duplicate the webstats code for the head slot and assign it to the the IScripts viewletmanager.

Deliverables

Risks

No pain, just pleasure.

Participants

Jean-Paul Ladage
Maurits van Rees

mauritsvanrees commented 6 months ago

For me this would be a small addition that would be safe to add in Plone 6.0 already. But I would want to hear opinions from others.

Main question: is two fields the right way to do this? For us it makes sense.

gforcada commented 6 months ago

LGTM 👍🏾

davisagli commented 6 months ago

I think it's fine. Should it be added at the top or bottom of the scripts viewlet manager?

jladage commented 6 months ago

Preferable as the last script-tag in de head section, yes. You have an idea how to put it last, or should we create a separate provider after the plone.scripts one?

On Mar 27, 2024, at 18:34, David Glick @.***> wrote:

I think it's fine. Should it be added at the top or bottom of the scripts viewlet manager?

— Reply to this email directly, view it on GitHub https://github.com/plone/Products.CMFPlone/issues/3931#issuecomment-2023627939, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAESTC7AKSXFDWUZHKLPGF3Y2MGLLAVCNFSM6AAAAABFIXQ22KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRTGYZDOOJTHE. You are receiving this because you were assigned.

yurj commented 6 months ago

Preferable as the last script-tag in de head section, yes. You have an idea how to put it last, or should we create a separate provider after the plone.scripts one?

https://github.com/plone/plone.app.layout/blob/1e9df41d9f325f31fd3aa3cae0a4151ef917e3f9/plone/app/layout/links/configure.zcml#L9

I think that just adding here in the right position is enough, no need for a separate zcml.

mauritsvanrees commented 6 months ago

@davisagli Just checking: Volto does not use the current webstats_js field? Probably not, I don't see it on demo.plone.org in the Site control panel.

davisagli commented 6 months ago

@mauritsvanrees That's right.

mauritsvanrees commented 6 months ago

I think this is ready. The combined Jenkins jobs passed earlier today, but I am running them again with all the latest minor updates.

I have approved all three linked PRs, but I was involved in them (Jean-Paul is my former boss, and we are still working together), so it would be good if someone else checks them.

The script is added at the end of the html head now.

To check it:

bin/develop co plone.app.layout plone.base and use branch 3931.feature of those two packages plus plone.app.upgrade which is already checked out.
In portal_setup/manage_upgrade manually apply the latest upgrade step of Products.CMFPlone:plone ("Run to_isiteschema upgrade profile") or create a fresh Plone Classic UI site.
Go to the Site settings and add a console.log or alert to the two javascript fields.

yurj commented 6 months ago

Just to note that you can insert almost anything, not just javascript. Adding:

or

 <style>
  body {background-color: powderblue;}
  h1 {color: red;}
  p {color: blue;}
</style>

or

<meta name="keywords" content="HTML, CSS, JavaScript">

is valid. <title> and <base> tags are already set in the same viewlet manager and it is recommended to don't have multiple <title> and <base> tags.

erral commented 6 months ago

Usually there's also the need to embed a JS script as a first thing in the <head> tag. It would be nice to have that too as part of this PLIP.

jladage commented 6 months ago

Just to note that you can insert almost anything, not just javascript. Adding:

<link rel="stylesheet" href="`http url with a css`">

or
 <style>
  body {background-color: powderblue;}
  h1 {color: red;}
  p {color: blue;}
</style> 
or

<meta name="keywords" content="HTML, CSS, JavaScript">

is valid. <title> and <base> tags are already set in the same viewlet manager and it is recommended to don't have multiple <title> and <base> tags.

Hi @yurj, I added some lxml code to filter only script tags to be rendered in the viewlet, see https://github.com/plone/plone.app.layout/commit/a95f03dadb171004a74c621be236a77a308da072

Please review this code, I had some UTF weirdness in the doctest, but it seems to work fine.

jladage commented 6 months ago

Usually there's also the need to embed a JS script as a first thing in the <head> tag. It would be nice to have that too as part of this PLIP.

Oh really, I didn't see one yet. Do you have examples of integrations that require a script tag before loading any other script, but for hackers? ;)

erral commented 6 months ago

Google Tag Manager requires to be inserted as a first thing in the head tag:

https://developers.google.com/tag-platform/gtagjs/install

Similarly CMPs like CookieBot require the same.

jladage commented 6 months ago

Ah, okay! That's the use case I should provide. Do we really want 3 fields (head start, head end and page end) or would it suffice when I change the viewlet to render at the top of the head?

jladage commented 6 months ago

Well, I already switch to using IHtmlHead viewlet manager, so it will be loaded before the other script tags. Earlier is not really necessary, except that they would register a fraction more bounces of people clicking the cancel button really fast. ;)

yurj commented 6 months ago

Just to note that you can insert almost anything, not just javascript. Adding: <link rel="stylesheet" href="`http url with a css`"> or
 <style>
  body {background-color: powderblue;}
  h1 {color: red;}
  p {color: blue;}
</style> 
or <meta name="keywords" content="HTML, CSS, JavaScript"> is valid. <title> and <base> tags are already set in the same viewlet manager and it is recommended to don't have multiple <title> and <base> tags.
Hi @yurj, I added some lxml code to filter only script tags to be rendered in the viewlet, see plone/plone.app.layout@a95f03d

Please review this code, I had some UTF weirdness in the doctest, but it seems to work fine.

Hi!

I think it is not necessary to allow only script tags. The webstat js field can be filled only by site admins or managers, so there's no real security problem because it is supposed they know what they're doing. Also this code will do the filtering at every rendering, you can use the registry modification time (see _RESOURCE_REGISTRY_MTIME from https://github.com/plone/Products.CMFPlone/blob/master/Products/CMFPlone/resources/browser/resource.py) as a cachekey if you wish.

erral commented 6 months ago

Ah, okay! That's the use case I should provide. Do we really want 3 fields (head start, head end and page end) or would it suffice when I change the viewlet to render at the top of the head?

We have added this kind of fields several times in some of our projects.

Some site admins are really picky about this (it should be possible because our SEO guys say it is possible and bla bla bla...).

jladage commented 6 months ago

Just to note that you can insert almost anything, not just javascript. Adding: <link rel="stylesheet" href="`http url with a css`"> or
 <style>
  body {background-color: powderblue;}
  h1 {color: red;}
  p {color: blue;}
</style> 
or <meta name="keywords" content="HTML, CSS, JavaScript"> is valid. <title> and <base> tags are already set in the same viewlet manager and it is recommended to don't have multiple <title> and <base> tags.
Hi @yurj, I added some lxml code to filter only script tags to be rendered in the viewlet, see plone/plone.app.layout@a95f03d Please review this code, I had some UTF weirdness in the doctest, but it seems to work fine.
Hi!

I think it is not necessary to allow only script tags. The webstat js field can be filled only by site admins or managers, so there's no real security problem because it is supposed they know what they're doing. Also this code will do the filtering at every rendering, you can use the registry modification time (see _RESOURCE_REGISTRY_MTIME from https://github.com/plone/Products.CMFPlone/blob/master/Products/CMFPlone/resources/browser/resource.py) as a cachekey if you wish.

Hi @yurj , I changed the code so base and title tags are stripped and all remaining valid tag are rendered. I'm totally not familiar with the code you pointed at, but considering it's 4 light calls extra, it will hardly make a difference iyam. ;) In case you or others insist, feel free to help out. https://github.com/plone/plone.app.layout/pull/361/commits/5880d2ed9fc58dbcd1220ecc95af5d11985e13f4

thet commented 6 months ago

Another use case of adding something first is to set a variable other scripts depend upon. We did something like that in older versions of Quaive.

jladage commented 6 months ago

Another use case of adding something first is to set a variable other scripts depend upon. We did something like that in older versions of Quaive.

That's now working. the head js is injected before all javascripts. ;)

mauritsvanrees commented 5 months ago

All merged, and will be released this week. Thanks for pushing this one @jladage !

erral commented 5 months ago

I have updated the translation strings in plone.app.locales and tried to keep the existing translations for the languages the texts were translated it :crossed_fingers:

plone / Products.CMFPlone