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

[4.4] Remove unused cassiopeia inline event handler #43586

Merged

Conversation

SniperSister
Copy link
Contributor

@SniperSister SniperSister commented May 31, 2024

Summary of Changes

The current Cassiopeia version has an inline onload event handler for the fontscheme.current css file. The inline event handler produces a warning in strict CSP mode and is redundant, as the lazy-loading functionality (that was the purpose of the handler) is handled in the template.js and trigger by the rel="lazy-stylesheet" attribute.

Testing Instructions

  • Configure the "roboto (local)" font as font in cassiopeia, verify that it's loaded by the browser using the browser dev tools.
  • Configure a strict ("script-src: 'self' 'unsafe-eval'") report-only script CSP using the Joomla HTTP Header plugin:
Bildschirmfoto 2024-06-01 um 14 20 13

Actual result BEFORE applying this Pull Request

  • CSS file loaded
  • Error message about forbidden unsafe-inline JS calls, caused by an "onload" handler of the mentioned CSS file in the browser console shown
Bildschirmfoto 2024-06-01 um 14 21 00

Expected result AFTER applying this Pull Request

  • CSS file loaded
  • Error message about unsafe-inline calls about the onload handler is gone

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • [x ] No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@brianteeman
Copy link
Contributor

Please provide more test instructions and what is expected to be seen in the generated source

Maybe I am looking at this wrong but before this PR I see

<link href="/media/templates/site/cassiopeia/css/global/fonts-local_roboto.min.css?8eef51" rel="stylesheet" media="all" onload="this.media='all'">

With the PR applied I see

<link href="/media/templates/site/cassiopeia/css/global/fonts-local_roboto.min.css?447e02" rel="stylesheet" media="all" onload="this.media='all'">

@brianteeman
Copy link
Contributor

Also does the same change need to be made in
templates\cassiopeia\component.php
templates\cassiopeia\error.php

Copy link
Contributor

@dgrammatiko dgrammatiko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SniperSister please remove all the 'media' => 'print'

templates/cassiopeia/component.php Outdated Show resolved Hide resolved
templates/cassiopeia/component.php Outdated Show resolved Hide resolved
templates/cassiopeia/error.php Outdated Show resolved Hide resolved
templates/cassiopeia/error.php Outdated Show resolved Hide resolved
templates/cassiopeia/index.php Outdated Show resolved Hide resolved
templates/cassiopeia/offline.php Outdated Show resolved Hide resolved
templates/cassiopeia/offline.php Outdated Show resolved Hide resolved
@SniperSister
Copy link
Contributor Author

Lesson learned: don't commit while listening to JDay presentations ;)

Sorry guys, now all required changes (removal of inline handler, removal of unused media attribute) have been performed in all relevant files and locations.

@brianteeman
Copy link
Contributor

As stated before please provide more detailed test instructions. How can a tester replicate the error etc

@SniperSister
Copy link
Contributor Author

As stated before please provide more detailed test instructions. How can a tester replicate the error etc

Done!

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on f3bbd2d


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43586.

@richard67
Copy link
Member

@SniperSister Shouldn't it be fixed in 4.4-dev, too (or only there and later be merged up to 5.1-dev)? If it's a bug fix we should to it also in 4.4-dev, and if it's a new feature (what I don't think) it should to into 5.2-dev.

@dautrich
Copy link

dautrich commented Jun 2, 2024

I have tested this item ✅ successfully on f3bbd2d


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43586.

@richard67
Copy link
Member

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43586.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 2, 2024
@SniperSister SniperSister changed the base branch from 5.1-dev to 4.4-dev June 3, 2024 06:21
@SniperSister SniperSister changed the base branch from 4.4-dev to 5.1-dev June 3, 2024 06:22
@SniperSister SniperSister force-pushed the 5.1-cassopeia-inline-event-handler branch from f3bbd2d to 82c15b5 Compare June 3, 2024 06:23
@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester Unit/System Tests labels Jun 3, 2024
@SniperSister SniperSister changed the base branch from 5.1-dev to 4.4-dev June 3, 2024 06:23
@SniperSister
Copy link
Contributor Author

Shouldn't it be fixed in 4.4-dev, too (or only there and later be merged up to 5.1-dev)?

@richard67 fair comment. I have rebased the PR as suggested!

@SniperSister SniperSister changed the title [5.1] Remove unused cassiopeia inline event handler [4.4] Remove unused cassiopeia inline event handler Jun 3, 2024
@joomla-cms-bot joomla-cms-bot added PR-4.4-dev and removed Language Change This is for Translators Unit/System Tests labels Jun 3, 2024
@SniperSister SniperSister added Language Change This is for Translators and removed PR-5.1-dev labels Jun 3, 2024
@joomla-cms-bot joomla-cms-bot removed the NPM Resource Changed This Pull Request can't be tested by Patchtester label Jun 3, 2024
@MacJoom MacJoom self-assigned this Jun 14, 2024
@joomla-cms-bot joomla-cms-bot removed the Language Change This is for Translators label Jun 14, 2024
@MacJoom MacJoom added this to the Joomla! 4.4.6 milestone Jun 14, 2024
@MacJoom MacJoom merged commit 67cd90e into joomla:4.4-dev Jun 14, 2024
3 checks passed
@MacJoom
Copy link
Contributor

MacJoom commented Jun 14, 2024

Thank you!

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jun 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants