[5.2] 5.2-Styles and Script element in head
Pull Request for Issue # .
Summary of Changes
Code be better...
Testing Instructions
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
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
I removed itemprop...
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
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.
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
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?
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>
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".
FWIW Joomla is already adding the nonce for the head inline css/js tags.
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.
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
I read a page from the MDN website. And I understood such a concept. Anyway, I trust your words. Are the current changes acceptable?
trailing slash and needless attribute in debug bar removed :-)
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.
This pull request has been automatically rebased to 5.3-dev.
@brianteeman title and code edited.
@brianteeman title and code edited.
the title still says this is for the head
@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 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?
@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?
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.
This pull request has been automatically rebased to 6.0-dev.
are you able to resolve the conflicts and then we can get this merged please
I've allowed myself to fix the conflict.
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.
I've allowed myself to fix the conflict.
thanks