Skip to content

[MWPW-190095] Video Carousel is not working as expected -Tablet#5699

Merged
milo-pr-merge[bot] merged 16 commits intoadobecom:stagefrom
drashti1712:carousel-play-pause
Apr 9, 2026
Merged

[MWPW-190095] Video Carousel is not working as expected -Tablet#5699
milo-pr-merge[bot] merged 16 commits intoadobecom:stagefrom
drashti1712:carousel-play-pause

Conversation

@drashti1712
Copy link
Copy Markdown
Contributor

@drashti1712 drashti1712 commented Mar 26, 2026

  • Preserve user-paused video state for carousel video slides in hinting tablet/mobile layouts
  • Prevent carousel and viewport auto-play logic from restarting videos that were explicitly paused by the user when slides come back into view
  • Rename the persisted pause-state attribute to data-user-paused and apply the guard in both shared video decoration and carousel slide activation logic

Resolves: MWPW-190095

Test URLs:

Before: https://main--milo--adobecom.aem.page/?martech=off
After: https://carousel-play-pause--milo--drashti1712.aem.page/?martech=off

Dev Validation:

@drashti1712 drashti1712 requested a review from rgclayton as a code owner March 26, 2026 17:35
@drashti1712 drashti1712 added the needs-verification PR requires E2E testing by a reviewer label Mar 26, 2026
Comment thread libs/blocks/carousel/carousel.js Outdated
TAB: 'Tab',
};
const FOCUSABLE_SELECTOR = 'a, :not(.video-container, .pause-play-wrapper) > video';
const USER_PAUSED_ATTR = 'data-user-paused';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

USER_PAUSED_ATTR = 'data-user-paused' is duplicated in both carousel.js and decorate.js in this PR.

Since carousel.js already imports from decorate.js, I suppose the easiest option could be to just export this from decorate.js and import it in carousel.js

const video = activeSlide?.querySelector('video');
/* c8 ignore start */
if (video?.paused && video.readyState >= 2) {
if (video?.paused
Copy link
Copy Markdown
Contributor

@nkthakur48 nkthakur48 Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up: Once this PR is merged, please ensure that UTs for this use-case are covered either as part of #5644 or new PR

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure will do

Comment thread libs/utils/decorate.js Outdated
if (isVideoReady(video)) { video.play(); }
} else {
video.pause();
if (isManualToggle) video.setAttribute(USER_PAUSED_ATTR, 'true');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using setAttribute(USER_PAUSED_ATTR, 'true') combined with getAttribute(USER_PAUSED_ATTR) === 'true' is more verbose than needed.

For a boolean flag, the HTML convention is to use attribute presence. You could use something like:

// Setter
video.setAttribute(USER_PAUSED_ATTR, ''); // or keep 'true' - either is fine

// Getter
const isUserPaused = video.hasAttribute(USER_PAUSED_ATTR);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have made changes accordingly. Please check @nkthakur48

@drashti1712 drashti1712 requested a review from nkthakur48 April 1, 2026 13:16
Copy link
Copy Markdown
Contributor

@JasonHowellSlavin JasonHowellSlavin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, agree with nishant re unit tests.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 4, 2026

This PR is currently in the needs-verification state. Please assign a QA engineer to verify the changes.

@spadmasa spadmasa added verified PR has been E2E tested by a reviewer Ready for Stage and removed needs-verification PR requires E2E testing by a reviewer labels Apr 8, 2026
@milo-pr-merge milo-pr-merge Bot merged commit e02c017 into adobecom:stage Apr 9, 2026
20 checks passed
@milo-pr-merge milo-pr-merge Bot mentioned this pull request Apr 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ready for Stage verified PR has been E2E tested by a reviewer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants