-
Notifications
You must be signed in to change notification settings - Fork 180
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
MWPW-164093: Adobe Home "Try Buy Widget" merch card #3651
base: stage
Are you sure you want to change the base?
Conversation
Co-Authored-By: Angelo Statescu <[email protected]>
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
Commits |
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.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
eslint
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
libs/deps/mas/commerce.js
Outdated
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.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
libs/deps/mas/mas.js
Outdated
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.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
|
libs/deps/mas/merch-card.js
Outdated
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.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
'light', | ||
el.classList.contains('light'), | ||
); | ||
targetContainer.innerHTML = `<h4>Demo: </h4><div class="demo-container">${el.textContent}</div>`; |
Check warning
Code scanning / CodeQL
DOM text reinterpreted as HTML Medium documentation
DOM text
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 18 days ago
To fix the problem, we need to ensure that any text content inserted into the DOM is properly escaped to prevent XSS attacks. This can be achieved by using a text node instead of setting innerHTML
directly. By creating a text node, we ensure that any special characters in the text content are automatically escaped.
The best way to fix this issue is to create a text node for the content of el.textContent
and append it to the target container. This way, the text content is safely inserted into the DOM without being interpreted as HTML.
-
Copy modified lines R587-R591
@@ -586,3 +586,7 @@ | ||
); | ||
targetContainer.innerHTML = `<h4>Demo: </h4><div class="demo-container">${el.textContent}</div>`; | ||
const demoContainer = document.createElement('div'); | ||
demoContainer.className = 'demo-container'; | ||
demoContainer.textContent = el.textContent; | ||
targetContainer.innerHTML = '<h4>Demo: </h4>'; | ||
targetContainer.appendChild(demoContainer); | ||
el.parentElement.after(targetContainer); |
'light', | ||
el.classList.contains('light'), | ||
); | ||
targetContainer.innerHTML = `<h4>Demo: </h4><div class="demo-container">${el.textContent}</div>`; |
Check warning
Code scanning / CodeQL
DOM text reinterpreted as HTML Medium documentation
DOM text
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 18 days ago
To fix the problem, we need to ensure that any text content assigned to innerHTML
is properly escaped to prevent the interpretation of HTML meta-characters. This can be achieved by using a function that escapes HTML special characters before assigning the text to innerHTML
.
The best way to fix this without changing existing functionality is to create a utility function that escapes HTML special characters and use this function to sanitize el.textContent
before assigning it to innerHTML
.
-
Copy modified lines R583-R595 -
Copy modified line R602
@@ -582,2 +582,15 @@ | ||
<script type="module"> | ||
function escapeHTML(str) { | ||
return str.replace(/[&<>"']/g, function (match) { | ||
const escapeMap = { | ||
'&': '&', | ||
'<': '<', | ||
'>': '>', | ||
'"': '"', | ||
"'": ''', | ||
}; | ||
return escapeMap[match]; | ||
}); | ||
} | ||
|
||
document.querySelectorAll('code.demo').forEach((el) => { | ||
@@ -588,3 +601,3 @@ | ||
); | ||
targetContainer.innerHTML = `<h4>Demo: </h4><div class="demo-container">${el.textContent}</div>`; | ||
targetContainer.innerHTML = `<h4>Demo: </h4><div class="demo-container">${escapeHTML(el.textContent)}</div>`; | ||
el.parentElement.after(targetContainer); |
'light', | ||
el.classList.contains('light'), | ||
); | ||
targetContainer.innerHTML = `<h4>Demo: </h4><div class="demo-container">${el.textContent}</div>`; |
Check warning
Code scanning / CodeQL
DOM text reinterpreted as HTML Medium documentation
DOM text
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 18 days ago
To fix the problem, we need to ensure that the text content is properly escaped before being inserted into the HTML. This can be achieved by creating a text node and appending it to the target container, which ensures that any special characters in the text content are properly escaped.
- Replace the direct insertion of
el.textContent
into the HTML with a safer method that escapes the text content. - Specifically, create a text node for the demo content and append it to the target container.
- Modify the relevant lines in the file
libs/features/mas/docs/inline-price.html
.
-
Copy modified lines R523-R527
@@ -522,3 +522,7 @@ | ||
); | ||
targetContainer.innerHTML = `<h4>Demo: </h4><div class="demo-container">${el.textContent}</div>`; | ||
const demoContainer = document.createElement('div'); | ||
demoContainer.className = 'demo-container'; | ||
demoContainer.textContent = el.textContent; | ||
targetContainer.innerHTML = '<h4>Demo: </h4>'; | ||
targetContainer.appendChild(demoContainer); | ||
el.parentElement.after(targetContainer); |
'light', | ||
el.classList.contains('light'), | ||
); | ||
targetContainer.innerHTML = `<h4>Demo: </h4><div class="demo-container">${el.textContent}</div>`; |
Check warning
Code scanning / CodeQL
DOM text reinterpreted as HTML Medium documentation
DOM text
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 18 days ago
To fix the problem, we need to ensure that the text content is properly escaped before being inserted into the HTML. This can be achieved by creating a text node and appending it to the container, which ensures that any special characters in the text content are treated as text rather than HTML.
- Replace the direct assignment of
el.textContent
toinnerHTML
with a method that safely appends the text content. - Specifically, create a text node with the content of
el.textContent
and append it to the container.
-
Copy modified lines R270-R274
@@ -269,3 +269,7 @@ | ||
); | ||
targetContainer.innerHTML = `<h4>Demo: </h4><div class="demo-container">${el.textContent}</div>`; | ||
const demoContainer = document.createElement('div'); | ||
demoContainer.className = 'demo-container'; | ||
demoContainer.textContent = el.textContent; | ||
targetContainer.innerHTML = '<h4>Demo: </h4>'; | ||
targetContainer.appendChild(demoContainer); | ||
el.parentElement.after(targetContainer); |
'light', | ||
el.classList.contains('light'), | ||
); | ||
targetContainer.innerHTML = `<h4>Demo: </h4><div class="demo-container">${el.textContent}</div>`; |
Check warning
Code scanning / CodeQL
DOM text reinterpreted as HTML Medium documentation
DOM text
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 18 days ago
To fix the problem, we need to ensure that any text content extracted from the DOM and inserted back as HTML is properly escaped to prevent it from being interpreted as HTML. This can be achieved by using a function to escape HTML special characters before assigning the text content to innerHTML
.
The best way to fix this without changing existing functionality is to create a utility function that escapes HTML special characters and use this function to sanitize el.textContent
before inserting it into the innerHTML
.
-
Copy modified lines R786-R793 -
Copy modified line R800
@@ -785,2 +785,10 @@ | ||
<script type="module"> | ||
function escapeHtml(unsafe) { | ||
return unsafe | ||
.replace(/&/g, "&") | ||
.replace(/</g, "<") | ||
.replace(/>/g, ">") | ||
.replace(/"/g, """) | ||
.replace(/'/g, "'"); | ||
} | ||
document.querySelectorAll('code.demo').forEach((el) => { | ||
@@ -791,3 +799,3 @@ | ||
); | ||
targetContainer.innerHTML = `<h4>Demo: </h4><div class="demo-container">${el.textContent}</div>`; | ||
targetContainer.innerHTML = `<h4>Demo: </h4><div class="demo-container">${escapeHtml(el.textContent)}</div>`; | ||
el.parentElement.after(targetContainer); |
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.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
eslint
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
libs/features/mas/dist/mas.js
Outdated
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.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
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.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
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.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
libs/features/mas/src/global.css.js
Outdated
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.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
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.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
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.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
This pull request is not passing all required checks. Please see this discussion for information on how to get all checks passing. Inconsistent checks can be manually retried. If a test absolutely can not pass for a good reason, please add a comment with an explanation to the PR. |
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.
I think this is not in the navigation menu(lost in previous PRs) can you add it back?
libs/features/mas/src/hydrate.js
Outdated
try { | ||
await customElements.whenDefined('checkout-button'); | ||
const CheckoutButtonEl = customElements.get('checkout-button'); | ||
const checkoutBtn = CheckoutButtonEl.createCheckoutButton({}, cta.innerHTML); |
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.
I think this block causes runtime errors in ah gallery:
mas.js:2370 Failed to initialize checkout-button logic: TypeError: Cannot read properties of null (reading 'setAttribute')
at mas.js:2370:30403
libs/features/mas/src/hydrate.js
Outdated
const checkoutBtn = CheckoutButtonEl.createCheckoutButton({}, cta.innerHTML); | ||
|
||
for (const attr of cta.attributes) { | ||
checkoutBtn.setAttribute(attr.name, attr.value); |
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.
I wonder why this is needed?
libs/features/mas/src/hydrate.js
Outdated
/* c8 ignore next 3 */ | ||
(async () => { | ||
try { | ||
await customElements.whenDefined('checkout-button'); |
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.
I think it is safe to assume that checkout-button
is already defined before we hit here, if not wee need to check how mas.js is bundled.
I would avoid adding checks for web components that we know are in the bundle.
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.
Disregard my previous comment, I found the root cause for the race condition. It was that in my test page, mas-commerce-service was actually defined after mas.js so the checkout button was not created properly since there was no service.
libs/features/mas/src/hydrate.js
Outdated
e.stopPropagation(); | ||
cta.click(); | ||
if (typeof checkoutBtn.clickHandler === 'function') { |
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.
isn't checkoutBtn.click()
not enough?
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, fixed
@@ -0,0 +1,34 @@ | |||
import { LitElement } from 'lit'; | |||
|
|||
export default class MatchMediaController { |
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.
is this different than https://opensource.adobe.com/spectrum-web-components/tools/match-media/ ?
can we use the lit one instead?
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.
We talked about it a few weeks ago. You said if MatchMediaController
from SWC is small we could use it, otherwise we can implement the logic in js. And this is it, a local implementation of the same thing.
However, it's pretty much the same as the one from SWC to be honest. Let me know if you want me to remove this and just use the one from SWC.
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.
Actually since MatchMediaController
from SWC is so small we can just use that. I changed it back everywhere too.
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.
true true, I thought this portion was from Axel and I didn't remember our discussion.
if putting MatchMediaController
doesn't explode our bundle size, let's use it as it.
libs/features/mas/src/merch-stock.js
Outdated
@@ -1,6 +1,6 @@ | |||
import { LitElement, css, html } from 'lit'; | |||
import { EVENT_MERCH_STOCK_CHANGE } from './constants.js'; | |||
import { MatchMediaController } from '@spectrum-web-components/reactive-controllers/src/MatchMedia.js'; | |||
import MatchMediaController from './match-media-controller.js'; |
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.
I would like to understand the difference
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.
See above
flex-direction: row; | ||
} | ||
:host([variant='ah-try-buy-widget']) .footer .spectrum-Button-label { |
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 we don't support Spectrum CSS anymore for AH, we should remove such rules.
`; | ||
} | ||
|
||
customElements.define('ah-try-buy-widget', AHTryBuyWidget); |
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 cleanup Spectrum CSS related rules since we switched to SWC.
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.
I removed, hopefully all of them
@@ -100,7 +100,6 @@ export function CheckoutMixin(Base) { | |||
} | |||
|
|||
async render(overrides = {}) { | |||
if (!this.isConnected) return false; |
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.
is it to fix some issue? do you have more details?
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.
@3ch023 I asked them. Since sp-button
doesn't have checkout-link capabilities, instead we will render a checkout-link but not in the DOM and just forward clicks to it.
But the DOM element must be rendered even if it is not connected to DOM which is fine IMO.
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.
i am good if there is no impact on events we throw for CCD
let title = fields.cardTitle; | ||
const { maxCount } = titleConfig; | ||
if (maxCount) { | ||
const [truncatedTitle, cleanTitle] = getTruncatedTextData(fields.cardTitle, maxCount); |
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.
we should have a nala card with 'too long' description and a test checking truncation on /libs/features/mas/docs/adobe-home.html
lets do it as a follow up ticket though, @Axelcureno could you create one?
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, created: https://jira.corp.adobe.com/browse/MWPW-167766
@Axelcureno Can you plz fix following , Thanks
![]() 2)Plz update the first card in triple plan to have background - Grey
![]()
![]()
![]()
![]()
![]() |
@Roycethan For 1 and 7 we'll look into it. cc: @Axelcureno |
![]() ![]() 10)Figma mentions widjet should have shadow on Desktop and tablet and not in mobile: do we have a separate ticket to address this ? Actual: Shadow is not rendered in the widject for Desktop/tablet: @Axelcureno cc @st-angelo-adobe |
@Roycethan we are not delivering "widgets", we are only delivering the cards. The page in milo is just for demonstration purposes, it does not represent the actual widget. The widget containing the cards is built in adobe home code. |
@Roycethan item #9 is a mistake in the color of figma, we are not overriding colors in dark theme and even in figma it says "Dark mode follows the same S2 guidelines as Light mode." |
I've fixed the remaining items, please check @Roycethan |
Thanks 1 is fixed, for 6 and 7 will create a separate followup ticket, approving this to proceed to stage |
Skipped 3651: "MWPW-164093: Adobe Home "Try Buy Widget" merch card" due to file "libs/deps/mas/commerce.js" overlap. Merging will be attempted in the next batch |
|
@Axelcureno , @Roycethan - it is almost impossible for someone outside the Merch team to test the changes. I had to go through comments to find a URL where the elements are rendered. The testing URL should be in the PR description. |
Yes tracking here https://jira.corp.adobe.com/browse/MWPW-168322 |
@Roycethan 6 and 7 are working as expected. I have validated on my end. |
Cool thanks @Axelcureno for fixing them, closing as fixed |
This PR introduces Adobe Home 'Pricing Widget' merch card with 3 variations:



Resolves: MWPW-164093
Test URLs:
Before: https://main--milo--adobecom.hlx.live/docs/library/kitchen-sink/merch-card?martech=off
After: https://mwpw-164093--milo--adobecom.hlx.live/docs/library/kitchen-sink/merch-card?martech=off