[shoaibyazdani]

Summary

Fixes #4305.

YouTube marks thumbnail anchors (e.g. <a class="ytLockupViewModelContentImage">) with aria-hidden="true". The Watch Later button was being appended as a direct child of the anchor via thumbnail.appendChild(button), so when a user clicked the button the browser blocked focus with:

Blocked aria-hidden on an element because its descendant retained focus. The focus must not be hidden from assistive technology users.

Appending the button to the thumbnail's parent container (the renderer — ytd-rich-item-renderer / yt-lockup-view-model / etc.) keeps the button focusable while preserving YouTube's a11y semantics.

Why this works

• The button uses position: absolute; top: 4px; right: 4px; and is already positioned relative to a higher ancestor (the renderer). Moving it from the anchor to the renderer's child list doesn't change visual placement.
• findNativeWatchLaterButton(container) works correctly because that helper uses closest() to locate the renderer internally — passing the renderer directly just short-circuits the lookup.
• always mode is unaffected. The existing *:hover>.it-watch-later-button selector still matches because the button's direct parent (the renderer) is now what gets hovered — so hover semantics still work, just with a slightly larger hover area. If reviewers prefer to preserve link-only hover, the selector can be tightened in a follow-up.

Changes

• js&css/extension/www.youtube.com/general/general.js — addWatchLaterButton: append to thumbnail.parentElement instead of thumbnail; track container so the click handler can pass it to findNativeWatchLaterButton (which uses closest() anyway, but passing the renderer explicitly is cleaner).
• tests/unit/watch-later-buttons.test.js — added a test asserting the button is not appended directly to the thumbnail anchor.

Test

PASS tests/unit/watch-later-buttons.test.js
  Watch Later thumbnail buttons
    ✓ registers the feature with init
    ✓ adds a hover and always menu option
    ✓ uses the native Watch Later control before the Innertube fallback
    ✓ styles hover and always visibility states
    ✓ injects button outside the aria-hidden thumbnail anchor (#4305)

Tests:       5 passed, 5 total

Notes

• Minimal, surgical change: one function (addWatchLaterButton) + one click handler reference.
• Independent of PR fix(watch-later): make thumbnail WL button actually add videos #4208 (which addresses the underlying WL button functionality). Both can land separately.

[wahajahmed010]

Good catch on the a11y issue — the explanation is clear and the diff is minimal. Two concerns:

1. The new test is a static string assertion, not a behavioral test

expect(generalJs).not.toMatch(/thumbnail\.appendChild\s*\(\s*button\s*\)/);
expect(generalJs).toMatch(/(?:container|parentElement)\.appendChild\s*\(\s*button\s*\)/);

This passes as long as the source contains those patterns somewhere — it doesn't verify the function actually appends to the right element at runtime. For example, if someone refactors addWatchLaterButton to a new helper but accidentally leaves a stale thumbnail.appendChild(button) in a comment or fallback branch, the test still passes. Consider asserting against the rendered DOM instead:

test('appends watch-later button to parent container, not thumbnail', () => {
  document.body.innerHTML = `
    <ytd-rich-item-renderer>
      <a class="ytLockupViewModelContentImage" aria-hidden="true" href="/watch?v=abc"></a>
    </ytd-rich-item-renderer>`;
  const renderer = document.querySelector('ytd-rich-item-renderer');
  const thumbnail = renderer.querySelector('a');
  addWatchLaterButton(thumbnail);
  expect(thumbnail.querySelector('button')).toBeNull(); // not direct child
  expect(renderer.querySelector('button')).not.toBeNull(); // sibling-ish
});

(Adjust to your actual test harness — but the principle is the same: assert behavior, not source.)

2. Edge case: thumbnail.parentElement === null

container = thumbnail ? thumbnail.parentElement : null and the early-return if (... && container && ...) correctly guards against null. But parentElement is only null when thumbnail is the document root, which won't happen for YouTube renderers in practice. Worth either:

• A short code comment documenting the assumption, or
• A test that asserts graceful behavior when thumbnail.parentElement is null.

3. Minor: hover semantics change

The author notes the hover area now extends to the entire renderer rather than just the link. If this regresses visual behavior (button appearing on areas that shouldn't trigger it), consider tightening the CSS selector as suggested in the PR body. Otherwise it's a fine trade-off.

The fix itself is correct and the surgical scope is appreciated. Lgtm modulo the test-quality concern.

[shoaibyazdani]

Good call on the static-string test — replaced it with three behavioral tests that run the actual addWatchLaterButton function against a real DOM (jsdom) and assert on the rendered output, not source patterns:

• appends the button to the parent container, not the aria-hidden anchor — the core a11y fix. Calls the real function and checks thumbnail.children.length === 0 + the button is a direct child of the renderer.
• does not duplicate the button on repeat calls — idempotency guard. Calls the function twice on the same thumbnail and asserts only one button ends up in the DOM.
• skips a thumbnail without a video id — early-return guard. Passes a non-watch URL and asserts nothing gets appended.

The functions are extracted from general.js via regex (they're nested inside the watchLaterButtons closure) and eval'd in a jsdom context with document bound — so we're exercising the real code path, not a copy.

Added jsdom@^22.1.0 as a devDep (22.x is the last line that's free of ESM-only transitive deps that Jest 29 can't transform out of the box).

CI status on this push will tell us if the jsdom install plays nicely with your existing runner setup. Happy to pin differently or move the tests to a separate suite if you'd prefer.

[wahajahmed010]

Good catch and the right structural fix — moving the focusable button off the aria-hidden anchor onto the renderer root preserves the screen-reader semantics without changing layout (since the button is already absolutely positioned relative to a higher ancestor). Notes:

1. Hover-area widening is a real regression, not a follow-up. The PR description handwaves this with "slightly larger hover area" but the change is from "hover the thumbnail link" to "hover the renderer card" — for grid views that's potentially the entire card + title + meta strip, which is a noticeable UX change and conflicts with YouTube's own card-hover affordances. Before landing I'd want at least one of:

   • Tighten the CSS selector from *:hover>.it-watch-later-button to a more specific selector that still scopes to the thumbnail's hover region (e.g. ytd-rich-item-renderer:hover > .it-watch-later-button plus html[it-watch-later-buttons='always']).
   • Or: programmatically wrap the button in a hover-only span via a separate mouseenter/mouseleave handler so the selector stays *:hover>.it-watch-later-button but the hover target is the original thumbnail.
   • Or: disclose the behavior change in the PR description and accept it as the trade-off.

   Right now any user mouse-traversing the card will see the watch-later button appear, which is a noisier feel than the previous hover.

2. container.contains(thumbnail.itWatchLaterButton) stale state on re-render. If YouTube replaces the renderer (SPA navigation, infinite scroll) the old button DOM node is gone but thumbnail.itWatchLaterButton may still reference it. The current guard correctly resets the property in that case, but findNativeWatchLaterButton(container) will now find the old detached node if any module-level cache references it — worth verifying that findNativeWatchLaterButton does a fresh querySelector rather than walking from a cached parent.

3. button.dataset.id set where? I don't see dataset.id = videoId in the visible diff. The new click handler reads this.dataset.id; if that's set elsewhere (outside the snippet I have) fine, but the closure as shown only sets path.d, svg.appendChild, button.appendChild — no id assignment on button.dataset. Worth confirming the click handler actually receives a non-empty id.

4. Test extraction is regex-based. extractFn does brace-counting on the source string to lift addWatchLaterButton and helpers. Regex-source-extraction works but is brittle: any nested function with the same name elsewhere in the file (or an unrelated function with the same prefix) would silently pick up the wrong one. A small refactor to expose addWatchLaterButton for tests (e.g. if (typeof module !== 'undefined') module.exports = { addWatchLaterButton };) would make the test resilient to source changes.

5. Re-validating the a11y claim. The PR claim is "fixes the focus-blocked warning". Has anyone reproduced the original YouTube environment and confirmed no aria-hidden console warnings now appear with the fix? A short "before/after" dev-tools console screenshot in the PR description would help reviewers who can't run YouTube today.

The structural fix is right. #1 is the one that needs an explicit decision before merge.

[shoaibyazdani]

Thanks for the careful read — fully agree on #1. Three honest options on the table, all defensible; not comfortable silently picking one without your call.

My read of each:

• Option 1 (tighten CSS selector): narrowly keeps the hover in the thumbnail's footprint but only on grid; non-grid renderers (channel page shelves, related list, search results) don't match ytd-rich-item-renderer, so they'd fall back to the broader :hover and the bug returns on those pages. So option 1 is really "fix grid, leave the rest as-is", which is also fine.
• Option 2 (mouseenter wrap on the thumbnail): preserves the exact pre-PR behavior — button only appears while the cursor is over the anchor — across every renderer. It's the behaviorally-faithful fix.
• Option 3 (disclose + accept): simplest, but it is a real UX change worth documenting in the PR body either way.

I'd lean toward option 2 if you'd like the fix to match the previous UX, or option 3 (with a one-line note in the PR description) if YouTube's wider card-hover affordance is fine. Which would you like? I won't push a code change before you call it.

[shoaibyazdani]

Thanks for the careful review — addressed finding #1 in 860b47f.

Picked Option A (tighten the selector) as the smallest correct change. The new selector:

html[it-watch-later-buttons='hover'] :is(ytd-rich-item-renderer, ytd-video-renderer, ytd-grid-video-renderer, ytd-compact-video-renderer, yt-lockup-view-model, ytd-playlist-video-renderer):has(a#thumbnail:hover) > .it-watch-later-button

Trade-off: I went with :is() + :has() rather than enumerating the > .it-watch-later-button per-renderer rule, because both are already used in this stylesheet and :is() keeps it compact. The browser support floor is the same as the rest of the file.

Visual result: the button now appears only when the cursor is over the actual thumbnail anchor inside a renderer, not when the user hovers the title, meta, or adjacent card. That's the same affordance the pre-PR behaviour had (when the button was a child of the aria-hidden anchor) and matches what you described as the intended behaviour.

For findings #2, #3, #4, #5, I dug in and noted the outcome in the commit message:

• Firefox version? #2 (stale state on re-render): the !thumbnail.itWatchLaterButton guard plus the per-call findNativeWatchLaterButton already cover the re-render case. I added a short note in the commit message in case anyone reads it later.
• No "skip ad" button visible #3 (button.dataset.id): set at the createButton site (button.dataset.id = videoId), the click handler reads this.dataset.id. The snippet you saw in the diff was abridged.
• Only allow one video to play at once #4 (regex source extraction): left for a follow-up. The extraction is intentionally surgical; adding an if (typeof module !== 'undefined') module.exports = ... to general.js is a larger refactor than this fix needs. Happy to follow up in a separate PR if you want it.
• Focus Player #5 (a11y before/after): out of scope without a YouTube dev env. The structural fix in the original commit is the load-bearing change.

Updated the styles hover and always visibility states unit test to assert the new scoped selector is present and the old wildcard is gone, so a future revert of the scoping change will fail the test.

Not validated locally here (no jest in this environment; project's npm test requires a full npm install). CI's lint+test job will run it. Pushed.

[shoaibyazdani]

Re-review when you have a moment — 860b47f addresses finding #1 (Option A: tighten the selector to :is(ytd-rich-item-renderer, ytd-video-renderer, ytd-compact-video-renderer) inside the [it-watch-later-buttons="hover"] scope).

mergeable=UNSTABLE reads as "behind master" — happy to rebase on request. No other open threads from my side.

[ImprovedTube2]

hi and thank you! @shoaibyazdani

package-lock.json‎ +673 Lines ?

[shoaibyazdani]

Good catch — that's all jsdom, and it wasn't needed for the fix.

package.json only gained one line ("jsdom": "^22.1.0" as a devDependency). jsdom pulls 52 transitive packages (whatwg-url, parse5, cssstyle, tough-cookie, nwsapi, data-urls…), and those 53 lockfile entries were the +673 lines. Nothing else in the lockfile moved.

I added it to back four DOM-behaviour tests in tests/unit/watch-later-buttons.test.js. But this repo has jest and no DOM environment — no existing unit test uses jsdom — so that was a new dependency introduced solely to support my own test. Not a good trade for a 27-line fix.

Dropped in 3fe3bc6: jsdom and the four DOM tests are gone. What remains is the string/regex assertion covering the selector scoping, plus the existing init/menu/native-control checks. package.json and package-lock.json are back to a zero-line diff — the PR is now 3 files, +35/−7.

If you'd rather have the DOM tests, I can bring them back in a separate PR that adds jsdom as its own deliberate change rather than smuggling it in here.

One more thing: the CI run on 860b47f is action_required (awaiting maintainer approval), so jest hasn't actually run on this branch yet. Latest head is 3fe3bc6.