-
Notifications
You must be signed in to change notification settings - Fork 18
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
Add the Sandbox for the iFrame #189
Conversation
KambleSonam
commented
Feb 9, 2024
•
edited
Loading
edited
- Add the sandbox for the iframe created for Web Popup and Exit Intents
- Pass html directly as srcdoc to iframe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please delete all the commented out code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the description with relevant details.
|
||
iframe.setAttribute('style', 'z-index: 2147483647; display:block; width: 100% !important; border:0px !important; border-color:none !important;') | ||
// made height : 100% which will help in adjustIframe height | ||
iframe.setAttribute('style', 'z-index: 2147483647; display:block; width: 100% !important; height:100%; border:0px !important; border-color:none !important;') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check for regressions
src/util/tr.js
Outdated
@@ -436,6 +436,9 @@ const _tr = (msg, { | |||
iframe.marginwidth = '0px' | |||
iframe.scrolling = 'no' | |||
iframe.id = 'wiz-iframe' | |||
if (displayObj['custom-editor']) { // sandboxing the iframe only for custom html | |||
iframe.sandbox = 'allow-scripts allow-popups allow-popups-to-escape-sandbox' // allow popup to open url in new page |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have you tested a custom html with form? https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe#allow-forms
src/util/tr.js
Outdated
adjustIFrameHeight() | ||
const contentDiv = document.getElementById('wiz-iframe').contentDocument.getElementById('contentDiv') | ||
setupClickUrl(onClick, targetingMsgJson, contentDiv, divId, legacy) | ||
if (displayObj['custom-editor']) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with your current changes looks like we are not calling adjustIFrameHeight() fn for form based templates. Is that the case ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added support for form based templates
src/util/tr.js
Outdated
const contentDiv = document.getElementById('wiz-iframe').contentDocument.getElementById('contentDiv') | ||
setupClickUrl(onClick, targetingMsgJson, contentDiv, divId, legacy) | ||
iframe.onload = () => { | ||
if (displayObj['custom-editor']) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lot of code duplication. please optimise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes done!
src/util/tr.js
Outdated
contentDiv = document.getElementById('contentDiv') | ||
let contentHeight = contentDiv.scrollHeight | ||
contentDiv.style.height = '100%' | ||
// if (displayObj['custom-editor'] !== true && !isBanner) { //It will always be custom-editor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove commented code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
src/util/tr.js
Outdated
} | ||
}) | ||
} | ||
setupClickUrl(onClick, targetingMsgJson, '', divId, legacy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you verified if the clicks get tracked correctly for form based templates? I think it might break coz we are passing an empty string as a part of contentDiv
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added support for form based templates as well
src/util/tr.js
Outdated
const contentDiv = document.getElementById('wiz-iframe-intent').contentDocument.getElementById('contentDiv') | ||
setupClickUrl(onClick, targetingMsgJson, contentDiv, 'intentPreview', legacy) | ||
iframe.onload = () => { | ||
window.addEventListener('message', event => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If multiple custom html campaigns are rendered on a page, by doing the above, wouldn't we end up adding multiple message listeners on the page, in turn raising duplicate notification clicked events??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed duplicate notification clicked events issue!
…ending html in different way for custom editor