update : videos - text , carousel , columns , embed superhero#636
Open
BaskarMitrah wants to merge 4 commits into
Open
update : videos - text , carousel , columns , embed superhero#636BaskarMitrah wants to merge 4 commits into
BaskarMitrah wants to merge 4 commits into
Conversation
|
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
|
|
❌ Test ResultsStatus: Some tests failed! 🔍 Click to view failed testsTest Coverage ReportOverall Coverage Summary
Coverage by File/Directory
Coverage report generated at 2026-05-29T08:25:55.868Z |
| return getYouTubeEmbedUrl(url) || url.href; | ||
| } | ||
|
|
||
| const VIDEO_URL_PATTERN = /(?:youtube\.com|youtu\.be|video\.tv\.adobe\.com|vimeo\.com|tiktok\.com|instagram\.com|twitter\.com|x\.com|\.mp4(?:$|\?|&))/i; |
Collaborator
There was a problem hiding this comment.
isVideoUrl over- and under-matches because of VIDEO_URL_PATTERN.
The regex looks for x.com anywhere in the string, with no host boundary. So any URL whose host ends in "x.com" — netflix.com, vox.com, max.com — is treated as a video. In the text block, that means a normal link like [our pricing](https://netflix.com/…) gets its whole paragraph replaced by a broken video embed.
It also only matches .mp4, so direct .webm and .ogg links are not detected as videos here — even though isDirectVideoUrl does accept them. So the two helpers disagree.
Could we detect by parsing the URL and checking the actual host (plus isDirectVideoUrl for direct files) rather than substring-matching the raw string? Two things to keep if you go that route:
video.tv.adobe.comshould still count as a video (getVideoProviderdoesn't currently recognize it).- Link text isn't always a full URL, so it'll need to handle non-parseable values gracefully.
Collaborator
Author
There was a problem hiding this comment.
instead of using regex I've used the isDirectVideoUrl, getVideoProvider function
| } | ||
| } else { | ||
| const iframeSrc = getEmbeddableVideoUrl(videoUrl); | ||
| wrapper.innerHTML = `<iframe src="${iframeSrc}" title="${title}" allowfullscreen loading="lazy"></iframe>`; |
| } else { | ||
| block.innerHTML = getDefaultEmbed(url, loop, controls, vidTitle, isShort, autoplay); | ||
| block.classList.add('block', 'embed'); | ||
| const renderResult = renderEmbedContent(link, { |
Collaborator
There was a problem hiding this comment.
vidTitle here is sourced only from block.getAttribute('data-videotitle') (just above), and link is a.href — so the author's descriptive link text is dropped. That's the ticket's headline observation: embed uses literal paths, leaving no way to provide alt text. Every other block in this PR already uses link text; embed is the lone holdout.
The fix exists in-PR: resolveVideoLabel (used by superhero) returns link text when it isn't itself a URL. Could vidTitle fall back to it — block.getAttribute('data-videotitle') || resolveVideoLabel(block.querySelector('a'))?
Scope caveat: this fixes DevBiz only — DevDocs authors a bare URL with no anchor, so DevDocs alt-text needs a separate devsite-runtime-connector change + docs update. So 2134 can't be fully closed by this hlx_statics-only PR — confirm scope + that the connector follow-up is tracked?
Collaborator
Author
There was a problem hiding this comment.
Thanks, agreed on all of this.
vidTitle fallback: Yes. We now resolve the title before block.textContent = '' clears the anchor:
block.getAttribute('data-videotitle') || resolveVideoLabel(anchor)
That value is passed through to loadEmbed (including the placeholder click path). resolveEmbedBlockVideo uses the same resolveVideoLabel logic for carousel embeds so link text is preserved there too.
| } | ||
|
|
||
| function processTextBlockVideos(block) { | ||
| block.querySelectorAll('a').forEach((anchor) => { |
Collaborator
There was a problem hiding this comment.
This is a behavior change from stage, where only standalone button-container links became videos. Now every anchor matching the video pattern gets its enclosing <p> replaced (container?.replaceWith(slot) below), which silently destroys content in two cases the old code never touched:
- Inline links in prose — e.g.
Watch [our keynote](https://youtube.com/…) for details.The whole sentence is one<p>, so the surrounding copy is deleted and replaced with a player. - Social/profile links — twitter/x/instagram/tiktok links become embeds (and inject third-party scripts). Amplified by the loose
VIDEO_URL_PATTERN(see the regex comment): links to hosts ending in "x.com" likenetflix.comalso get pulled out and replaced with a broken iframe.
Since the block JS is shared, this hits DevDocs-authored text too, not just DevBiz.
Suggested direction: gate on getVideoProvider(url) !== null (precise hosts) instead of the substring regex, and only convert standalone-link paragraphs — skip any anchor whose <p> has other text or sibling content. Could you confirm the standalone case is the only intended one?
Collaborator
Author
There was a problem hiding this comment.
Yes, standalone video links are the only intended case for the text block, matching stage behavior (p.button-container after decorateButtons).
Standalone-only conversion — We only embed when the link is in a p.button-container, or the parent
has no other meaningful content (only the anchor plus optional whitespace). Inline prose like “Watch our keynote for details” is left unchanged.
Host-based URL detection — We replaced the substring VIDEO_URL_PATTERN with getVideoProvider / isDirectVideoUrl (plus video.tv.adobe.com), so false positives like netflix.com matching x.com no longer happen.
Text block allow list — Text blocks only auto-embed YouTube, Vimeo, direct video files, and Adobe TV. Twitter/X, Instagram, and TikTok profile links are not converted in text blocks (embed/carousel blocks can still handle those where appropriate).
Collaborator
There was a problem hiding this comment.
Thanks — the code confirms all three points. One follow-up: hasTextBlockMedia returns true for any p.button-container, not just video slots. Can you confirm existing text blocks with CTA buttons but no video still render correctly?
melissag-ensemble
requested changes
May 29, 2026
❌ Test ResultsStatus: Some tests failed! 🔍 Click to view failed testsTest Coverage ReportOverall Coverage Summary
Coverage by File/Directory
Coverage report generated at 2026-06-01T14:57:40.533Z |
| embedBlock.classList.add('embed-is-loaded'); | ||
| embedBlock.dataset.carouselVideo = 'true'; | ||
| } | ||
| } catch { |
Collaborator
There was a problem hiding this comment.
catch still doesn't bind error — change } catch { to } catch (error) { or the console.warn will throw instead of log.
Suggested change
| } catch { | |
| } catch (error) { |
| wrapper.classList.add(renderResult.className); | ||
| } | ||
| } else { | ||
| wrapper.innerHTML = buildFlatYouTubeIframeHtml(videoUrl, title, true); |
Collaborator
There was a problem hiding this comment.
else calls buildFlatYouTubeIframeHtml for non-YouTube URLs — getYouTubeEmbedUrl returns null for those so the slot renders empty and the video is silently dropped. The else branch should be removed; if no provider matched, return early rather than rendering nothing.
❌ Test ResultsStatus: Some tests failed! 🔍 Click to view failed testsTest Coverage ReportOverall Coverage Summary
Coverage by File/Directory
Coverage report generated at 2026-06-04T05:08:26.043Z |
Merged
❌ Test ResultsStatus: Some tests failed! 🔍 Click to view failed testsTest Coverage ReportOverall Coverage Summary
Coverage by File/Directory
Coverage report generated at 2026-06-08T05:48:09.030Z |
melissag-ensemble
requested changes
Jun 8, 2026
melissag-ensemble
left a comment
melissag-ensemble
left a comment
Collaborator
There was a problem hiding this comment.
Found a regression with Carousel. Before:
After:
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
I’ve updated embedded video support in the Text, Carousel, Columns, and Super Hero blocks for both DevBiz and DevDocs.
I also updated all supported video formats in the above-mentioned blocks, including MP4, RawGitHub URLs, YouTube videos, and local videos (in DevDocs).
Jira
https://jira.corp.adobe.com/browse/DEVSITE-2134
Test URL
Devbiz
Previous : https://stage--adp-devsite-stage--adobedocs.aem.page/test/baskar/video-controls
Updated: https://devsite-2134--adp-devsite-stage--adobedocs.aem.page/test/baskar/video-controls
Devdocs
Previous : https://stage--adp-devsite-stage--adobedocs.aem.page/github-actions-test/test/mp4
Updated: https://devsite-2134--adp-devsite-stage--adobedocs.aem.page/github-actions-test/test/mp4