Skip to content

[4.0] Module Custom: remove inline style#32980

Merged
chmst merged 2 commits intojoomla:4.0-devfrom
Fedik:m-custom-bg
Apr 4, 2021
Merged

[4.0] Module Custom: remove inline style#32980
chmst merged 2 commits intojoomla:4.0-devfrom
Fedik:m-custom-bg

Conversation

@Fedik
Copy link
Member

@Fedik Fedik commented Apr 3, 2021

Summary of Changes

this changes how background added to the Custom module, to avoid conflict with CSP rules.

Testing Instructions

Apply patch.
Click "Enable" in CSP component.
Create a custom module with background, and some content.
Check the module rendering.

Actual result BEFORE applying this Pull Request

Background not visible,
in browser console CSP related error.

Expected result AFTER applying this Pull Request

Background visible,
in browser console no CSP related error.

Documentation Changes Required

none

@Fedik Fedik mentioned this pull request Apr 3, 2021
@dgrammatiko
Copy link
Contributor

I have tested this item ✅ successfully on 4a14fe1


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

@sandewt
Copy link
Contributor

sandewt commented Apr 3, 2021

Apply path.

What exactly do you mean by this? Patch Tester?


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

@Fedik
Copy link
Member Author

Fedik commented Apr 3, 2021

What exactly do you mean by this? Patch Tester?

Sorry that was typo ;)
Yes, I meant Patch not Path . Just apply the changes.

@sandewt
Copy link
Contributor

sandewt commented Apr 3, 2021

Actual result BEFORE applying this Pull Request
Background not visible,
in browser console CSP related error.

The background is really visible. NOK
I can confirm that there is in the browser console a CSP related error. OK

@Quy
Copy link
Contributor

Quy commented Apr 4, 2021

Is it ok that it does not have the query string?

Before PR:
background-image: url(/joomla-cms-4.0-dev/images/banners/banner.jpg?joomla_image_width=1140&joomla_image_height=600)

After PR:
background-image: url("/joomla-cms-4.0-dev/images/banners/banner.jpg")

@Fedik
Copy link
Member Author

Fedik commented Apr 4, 2021

yes, the query does not do anything, it something that comes from mediamanager,

@chmst
Copy link
Contributor

chmst commented Apr 4, 2021

The image has repeat-x, repeat-y. Is this intended?


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

@Fedik
Copy link
Member Author

Fedik commented Apr 4, 2021

The image has repeat-x, repeat-y. Is this intended?

yeap, this is default behavior of this module, since this feature exists

if you do test without CSP you will see that "before and after" looks the same

@Quy
Copy link
Contributor

Quy commented Apr 4, 2021

I have tested this item ✅ successfully on 4a14fe1


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

@Quy
Copy link
Contributor

Quy commented Apr 4, 2021

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Apr 4, 2021
@chmst chmst added this to the Joomla 4.0 milestone Apr 4, 2021
@chmst chmst merged commit 3ff6d0c into joomla:4.0-dev Apr 4, 2021
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Apr 4, 2021
@chmst
Copy link
Contributor

chmst commented Apr 4, 2021

Thanks!

@Fedik Fedik deleted the m-custom-bg branch April 4, 2021 16:52
@sandewt
Copy link
Contributor

sandewt commented Apr 4, 2021

Did I miss something? Before and after this PR I get to see the following message in the browser in the frontend site.

issue

@Fedik
Copy link
Member Author

Fedik commented Apr 4, 2021

Did I miss something?

yeap, CSP will not work on localhost, need extra configuration or just use a virtual domain 😉

@sandewt
Copy link
Contributor

sandewt commented Apr 4, 2021

@Fedik

Thanks. 😅

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.

6 participants

Comments