Skip to content

Title: Embeds: Use the click handler's own event parameter - #13293

Open
itzmekhokan wants to merge 2 commits into
WordPress:trunkfrom
itzmekhokan:fix/embed-global-event-param
Open

Title: Embeds: Use the click handler's own event parameter#13293
itzmekhokan wants to merge 2 commits into
WordPress:trunkfrom
itzmekhokan:fix/embed-global-event-param

Conversation

@itzmekhokan

@itzmekhokan itzmekhokan commented Aug 27, 2026

Copy link
Copy Markdown

linkClickHandler() in src/js/_enqueues/lib/embed-template.js read modifier keys from the global event rather than the handler's own parameter. A Cmd/Ctrl + click on a link inside an embed therefore called preventDefault() and navigated the top window instead of opening a new tab, defeating r49202.

window.event is an accessor on Window.prototype. The embed template inlines its script into the document's global scope, so any script there declaring a top-level var event shadows the accessor with an own property and the check reads undefined. Naming the handler's parameter event binds the check to the real event object.

Every handler in the file is renamed from e to event for consistency, as requested in review. There is no QUnit coverage for this file to extend.

Trac ticket: https://core.trac.wordpress.org/ticket/65978

Use of AI Tools

  • AI assistance: Yes
  • Tool(s): Claude Code
  • Model(s): Claude Opus 5
  • Used for: Identifying the issue while reviewing PR 13244, verifying the shadowing behavior in a browser and in the minified build, and drafting the fix. All changes were reviewed and validated by me.

`linkClickHandler()` checked modifier keys against the global `event`
rather than its `e` parameter. Any script in the embed template that
declares a top-level `var event` shadows `window.event`, so the check
reads `undefined` and passes — a Cmd/Ctrl + click on a link inside an
embed then navigates the top window instead of opening a new tab.
@github-actions

github-actions Bot commented Aug 27, 2026

Copy link
Copy Markdown

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props khokansardar, westonruter, peterwilsoncc.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@github-actions

Copy link
Copy Markdown

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

Comment thread src/js/_enqueues/lib/embed-template.js Outdated
Comment thread src/js/_enqueues/lib/embed-template.js Outdated

@peterwilsoncc peterwilsoncc left a comment

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.

I agree with @westonruter that it would be good to rename the variable to the self documenting event. There are some other event handlers within the file that also use e, so it would be good to change them all to be consistent.

Rename the `e` parameter to `event` in every handler in
`embed-template.js`, rather than reading the modifier keys off `e` in
`linkClickHandler()`.

The parameter now shadows `window.event`, so the modifier key check is
no longer affected by a top-level `var event` in the embed template, and
the name matches the existing `onMessage( event )` handler.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Fixes modifier-key detection in the embed template’s link click handler by using the handler’s own event object (instead of the global event accessor), preventing Cmd/Ctrl+click from being incorrectly preventDefault()’d and forcing top-window navigation.

Changes:

  • Update linkClickHandler() to read modifier keys from its event parameter and use that parameter consistently.
  • Rename other event handler parameters from e to event for consistency across the file.
  • Add a JSDoc @param annotation to linkClickHandler().

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants