fix(grid-marquee): dedupe drawer video when face link is auto-decorated to inline <video> (MEP replacePage) - #711
Merged
Merged
Conversation
echen-adobe
marked this pull request as ready for review
August 25, 2026 23:37
echen-adobe
requested review from
maxn-adobe,
meganthecoder and
nateyolles
as code owners
August 25, 2026 23:37
nateyolles
approved these changes
Aug 25, 2026
fullcolorcoder
approved these changes
Aug 26, 2026
fullcolorcoder
left a comment
Contributor
There was a problem hiding this comment.
@echen-adobe I think this is fine to ship as is. Some follow up notes on the FOUC stuff at https://github.com/adobecom/da-express-milo/pull/711/changes#diff-d0e2f9f7bdad2655bc446c8058bc3d1c2758bb3145fec38de1b2fcd167a63741R12
I don't anticipate regression but some notes for you/agent to consider.
Problem 1 — in new-authoring mode the guard probably doesn't even prevent the FOUC it's written for
The gate is :not(:has(> .foreground)). But look at the new-mode flow (grid-marquee.js:409 onward):
el.append(foreground); // .foreground exists NOW (empty) — guard turns OFF here
...
requestAnimationFrame(() => {
restoreImages();
const cards = items.map((item) => toCard(item)); // anchors stripped HERE, a frame later
foreground.append(cardsContainer); // items moved into .foreground HERE
});
.foreground is appended empty, synchronously, and the authored item <div>s aren't moved into it until the rAF fires a frame later. So the guard's condition (.foreground exists) flips to "safe" before the content it's guarding has actually been decorated/moved. During the window where raw anchor text could paint, .foreground already exists → the rule is inert. The gate is keyed on the wrong event: "an empty container exists" instead of "the content has been decorated."
(In practice the rAF-before-paint ordering already covers most of this window on its own — which makes the guard largely redundant where it's correct and ineffective where it isn't.)
Problem 2 — it can permanently blank the LCP headline if JS fails (your §4 anti-pattern)
In legacy mode the authored <h1> is a direct > div child until init moves it into .foreground. The guard hides every > div until .foreground exists. If grid-marquee's JS ever fails to load/execute, Milo still un-hides the section — but .foreground was never created, so the guard stays active and the H1 (frequently the page's LCP on a hero grid-marquee) is visibility: hidden forever. That's exactly the "never hide authored LCP content" rule in the project's own performance doc (§4).
The better shape
The FOUC is raw link/inline-video content painting — so hide that specific element, not the whole block. A scoped rule can't blank the block or the LCP and doesn't depend on decoration-timing races:
/* hide only the un-decorated video source, in both authored forms */
.grid-marquee .face > p > a[href*=".mp4"],
.grid-marquee .face > p > video { visibility: hidden; }
(Or, if a block-wide guard is really wanted, key it on a positive success signal added at the end of decoration — .grid-marquee.is-decorated — with a CSS-animation fallback that reveals after ~Ns so a JS failure can never leave it blank. But scoping to the offending element is simpler and strictly safer.)
Merged
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
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.
Summary
Fixes duplicate videos rendering in the
grid-marqueeblock, seen on the Saudi Arabia English homepage (delivered via an MEPreplacePagefragment). A card would show its preview/drawer video twice — once as a stray inline<video>left inside the card face, and again as the drawer video the block builds itself.The block's
toCard()only knew how to strip the drawer-video link when it was authored as an<a href="*.mp4">. When the same content is served through the MEP page-replacement flow, that.mp4link gets auto-decorated into an inline<video>before the block runs, sotoCard()never removed it — and then created a second video in the drawer. This PR makes the block tolerant of both authored forms and removes whichever is present, so the face can never duplicate the drawer video.Jira Ticket
Resolves: MWPW-204683
Root cause — in depth
How the drawer video is normally authored
Each card in
grid-marqueeis authored as afacediv followed by one or more panel divs. The face carries three stacked paragraphs:toCard()is supposed to (1) read the.mp4URL off that anchor, (2) remove the anchor from the face so only the thumbnail shows, and (3) lazily build the real<video>inside the drawer on hover/open.The
#_dnbcontract that normally protects thisExpress globally auto-converts
a[href$=".mp4"]links into autoplaying inline<video>elements.grid-marqueeopts out of that so it can manage the link itself. Inscripts/utils.js(decorateArea→videoLinksToNotAutoBlock, ~line 847), every.mp4link inside agrid-marqueegets a#_dnb("do not block") suffix appended:On an ordinary page this runs over the page area before autoblocking, so the link stays an
<a>andtoCard()behaves.Why it breaks under MEP
replacePageOn the SA homepage the entire page is swapped for a personalization fragment:
When content arrives through that page-replacement path, the grid-marquee
.mp4links are not guaranteed to receive the#_dnbguard before Express's auto-video decoration runs. So the.mp4link in the face is converted into an inline<video>first.Now
toCard()runs against a face that contains a<video>, not an<a>:Result:
<video>is left in the card face (visible dupe sync stage #1).<video>(dupe Stage to Main #2).videoAnchor.hrefonnullthrows insidelazyCB, breaking drawer decoration for that card.This is the same failure the T&O team patched a year ago with a full
updateBlockCodefork of the block (scripts/mep/ace1057/grid-marquee/), whosetoCard()usedface.querySelector('a') || face.querySelector('video'). That fork has since diverged completely from the live block ("isn't fully compatible with the latest block code"), which is why the fix now lands in the real block instead of another throwaway override.The fix
toCard()now resolves the drawer-video source from either an<a>or an already-inlined<video>, and removes both from the face:Key properties:
<a>present, no inline video),faceVideoisnull,videoSrc === faceAnchor.href, and behavior is byte-for-byte identical to before. Every existing grid-marquee page is unaffected..mp4(incl. any#_dnbsuffix, which the media loader ignores — unchanged from prior behavior).<a>and a<video>) can't leave a dupe.null.hrefthrow —videoSrcdegrades gracefully toundefined(drawer still opens with title/CTAs) instead of crashing decoration.Secondary change — anti-FOUC CSS guard (separable)
grid-marquee.cssalso gains a one-line guard that hides the raw authored rows until the block builds.foreground:This addresses a separate, cosmetic symptom (raw anchor/link text briefly painting under the thumbnail during the block's staged decoration), not the dupe itself. It is fully separable from the JS fix — see Potential Regressions. Happy to drop it if we'd rather keep this PR laser-focused on the dupe.
Test URLs
Verification Steps
Cannot read properties of null (reading 'href')fromgrid-marquee.js.Potential Regressions
Reviewed carefully; the JS change is a no-op on the normal authoring path, so risk concentrates in a few edge cases:
JS (
toCard)<video>in the face (no.face videoCSS in this block or the historical override) — the face is always a static thumbnail — so removing a stray inline video is safe by design.videoSrcunresolved. If a face somehow has neither an<a>nor a resolvable video source,videoSrcisundefinedand the drawer builds<source src="undefined">(video simply won't load). This is strictly better than the previous behavior, which threw and broke drawer decoration entirely.getPosterSrc(face.querySelector('img'))still reads the first face<img>(the authored thumbnail, which precedes the video link), so removing the inline<video>does not affect the drawer poster — unless authoring ever places the video before the thumbnail image (not the case in any known authoring).#_dnbin the source URL. Unchanged from before — the anchor href already carried it; the media loader ignores non-#t=fragments.CSS (anti-FOUC guard)
initnever runs. The guard keys off.foreground, which the block appends synchronously at the start ofinit. If block JS fails to load/execute, the block staysvisibility: hidden(blank) instead of showing raw content. Rare, but a real trade-off — this is the main reason the guard is called out as separable.:has()support. Baseline across current evergreen browsers (Express's targets); no polyfill needed, but flagging since it's a relatively new selector for this codebase's block CSS.visibility: hiddenpreserves layout (no added CLS), but briefly defers paint of the authored rows until.foregroundexists. The new-authoring path already removes images up front for LCP, so net impact is negligible; worth an eyeball on a Lighthouse/CWV run if the marquee image is the LCP element.Broad regression surface to spot-check (branch builds of grid-marquee surfaces):
Additional Notes
test/blocks/grid-marquee/grid-marquee.test.js) pass unchanged. A follow-up commit can add an explicit regression test asserting exactly one<video>renders when the face is authored with (or auto-decorated into) an inline<video>.scripts/mep/ace1057/grid-marquee/fork approach; folding the<a>-or-<video>handling into the live block keeps it from rotting against future block changes.#_dnbde-autoblock pass (utils.jsvideoLinksToNotAutoBlock) also runs over MEPreplacePagefragment content, which would prevent the auto-conversion at the source.