joomla-cms icon indicating copy to clipboard operation
joomla-cms copied to clipboard

[5.2] 5.2-Styles and Script element in head

Open sinahaghparast opened this issue 1 year ago • 13 comments

Pull Request for Issue # .

Summary of Changes

Code be better...

Testing Instructions

https://validator.w3.org/

Actual result BEFORE applying this Pull Request

Validator message: Info: The type attribute is unnecessary for JavaScript resources. Info: Trailing slash on void elements has no effect and interacts badly with unquoted attribute values.

Expected result AFTER applying this Pull Request

Validator message: Document checking completed. No errors or warnings to show.

Link to documentations

Please select:

  • [ ] Documentation link for docs.joomla.org:

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

  • [ ] Pull Request link for manual.joomla.org:

  • [ ] No documentation changes for manual.joomla.org needed

sinahaghparast avatar Apr 09 '24 08:04 sinahaghparast

Before jumping in and "fixing" the code you really need to spend a little more time both using Joomla dn looking through the code history.

For example you will find that the "itemprop" you are adding here has only just been removed because we switched to use json rich snippets see #41151 so adding it here is completely wrong.

Secondly a pull request should address a single issue. Here you are addressing two unrelated issues

brianteeman avatar Apr 09 '24 08:04 brianteeman

I removed itemprop...

sinahaghparast avatar Apr 09 '24 09:04 sinahaghparast

I removed itemprop...

Now you need to make sure the pull request title and description are still correct and if not then you will need to edit those. The title is particularly important as that is what people will seein the changelog

brianteeman avatar Apr 09 '24 10:04 brianteeman

the javascript renderer for the debug plugin is not in the head. It is just before the closing body tag. You probably should also fix the css for that plugin as well.

image

brianteeman avatar Apr 09 '24 11:04 brianteeman

The code for the debug css is here: https://github.com/joomla/joomla-cms/blob/45adb61f8306b945f43b4326305fafe87c154aa2/plugins/system/debug/src/JavascriptRenderer.php#L63 For the JS is here: https://github.com/joomla/joomla-cms/blob/45adb61f8306b945f43b4326305fafe87c154aa2/plugins/system/debug/src/JavascriptRenderer.php#L71

Also just reviewing the code the nonce attribute needs to be applied for each stylesheet/script, right now debug is probably broken with CSP enabled

dgrammatiko avatar Apr 10 '24 16:04 dgrammatiko

I apologize for the delay. It was related to my personal life.

According to this document, external files can also have the nonce attribute. what is your opinion? be added?

sinahaghparast avatar Apr 16 '24 15:04 sinahaghparast

Also, I don't agree with adding the nonce attribute to the inline style. Because according to this document, the code inside the elements are also blocked.

Inline style attributes are also blocked: <div style="display:none">Foo</div>

sinahaghparast avatar Apr 16 '24 15:04 sinahaghparast

Also, I don't agree with adding the nonce attribute to the inline style. Because according to this document, the code inside the elements are also blocked.

You misunderstood what the inline style it refers to. It's not the <style> tag but rather the attribute style="display: none".

Screenshot 2024-04-16 at 6 48 11 PM

FWIW Joomla is already adding the nonce for the head inline css/js tags.

dgrammatiko avatar Apr 16 '24 15:04 dgrammatiko

If I have misunderstood, then you guide me so that I can be informed. Just stating that I got it wrong won't help me. It may be repeated until the offender realizes his mistake. Please introduce new document or interpret that document.

sinahaghparast avatar Apr 16 '24 16:04 sinahaghparast

CSP has different levels. The strict level is the one that does not allow inline css (attribute style). I'm not sure how I can explain this better as the CSP chapter is pretty big and needs some studying to grasp the fundamentals. MDN and web.dev are good starting points, then more googling are my only suggestions here if the comments are not sufficient

dgrammatiko avatar Apr 16 '24 16:04 dgrammatiko

I read a page from the MDN website. And I understood such a concept. Anyway, I trust your words. Are the current changes acceptable?

sinahaghparast avatar Apr 16 '24 16:04 sinahaghparast

trailing slash and needless attribute in debug bar removed :-)

angieradtke avatar Aug 24 '24 12:08 angieradtke

I have tested this item :white_check_mark: successfully on b609e14ef386f863502465a5317386eeea1a79d7


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

angieradtke avatar Aug 24 '24 12:08 angieradtke

This pull request has been automatically rebased to 5.3-dev.

HLeithner avatar Sep 02 '24 08:09 HLeithner

@brianteeman title and code edited.

sinahaghparast avatar Sep 25 '24 21:09 sinahaghparast

@brianteeman title and code edited.

the title still says this is for the head

brianteeman avatar Sep 29 '24 09:09 brianteeman

@brianteeman title and code edited.

the title still says this is for the head

@brianteeman renderHead is function's name... What do you recommend?

sinahaghparast avatar Sep 29 '24 09:09 sinahaghparast

@sinahaghparast the debugbar injects the assets at the end of the body (although the class says head) so maybe instead of head should say debugbar or just debug?

dgrammatiko avatar Sep 29 '24 09:09 dgrammatiko

@brianteeman title and code edited.

the title still says this is for the head

@brianteeman renderHead is function's name... What do you recommend?

I suggest to name the function "renderDebugAssets". Is okay?

sinahaghparast avatar Sep 29 '24 10:09 sinahaghparast

I suggest to name the function "renderDebugAssets". Is okay?

@sinahaghparast I would not change the method name for b/c (backwards compatibility) reasons. But the title of your pull request is better now.

richard67 avatar Sep 29 '24 10:09 richard67

This pull request has been automatically rebased to 6.0-dev.

HLeithner avatar Mar 04 '25 17:03 HLeithner

are you able to resolve the conflicts and then we can get this merged please

brianteeman avatar Apr 11 '25 08:04 brianteeman

I've allowed myself to fix the conflict.

richard67 avatar Apr 11 '25 09:04 richard67

I have tested this item :white_check_mark: successfully on 08d60b5f3c9e44ba41a72b827012af93eadfdfc7


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

brianteeman avatar Apr 11 '25 09:04 brianteeman

I've allowed myself to fix the conflict.

thanks

brianteeman avatar Apr 11 '25 09:04 brianteeman