Opened 22 hours ago
Last modified 4 hours ago
#65978 reviewing defect (bug)
linkClickHandler() reads the global event instead of its own parameter
| Reported by: | khokansardar | Owned by: | peterwilsoncc |
|---|---|---|---|
| Priority: | normal | Milestone: | 7.2 |
| Component: | Embeds | Version: | 5.6 |
| Severity: | minor | Keywords: | has-patch |
| Cc: | Focuses: | javascript |
Description
In src/js/_enqueues/lib/embed-template.js, linkClickHandler( e ) checks modifier keys against the global event rather than its own e parameter:
if ( event.altKey || event.ctrlKey || event.metaKey || event.shiftKey ) {
This works only because browsers expose window.event. That property is an accessor on Window.prototype, so any script in the embed template declaring a top-level var event shadows it with an own property on the global object. The modifier check then reads undefined, passes, and preventDefault() runs — so Cmd/Ctrl + click on a link inside an embed navigates the top window instead of opening a new tab.
This defeats the behavior added in [49202] (#39097), which was specifically about making Ctrl/Cmd + click open embed links in a new tab. Plugins can enqueue scripts into the embed template via embed_head / embed_footer, so the collision is reachable.
ESLint reports 'event' is not defined on all four references.
Steps to reproduce
- Embed a post from one site into another.
- On the embedded site, enqueue a script on
embed_headthat declares a top-levelvar event = {}; - Cmd/Ctrl + click a link inside the embed.
Expected: the link opens in a new tab.
Actual: the parent window navigates to the link.
Change History (6)
This ticket was mentioned in PR #13293 on WordPress/wordpress-develop by @khokansardar.
22 hours ago
#1
#2
follow-up:
↓ 3
@
22 hours ago
- Milestone Awaiting Review → 7.2
Correct. Thanks for the report @khokansardar.
As far as I can tell, linkClickHandler already passes the event so the fix is pretty simple. Other usages within the function use e while the conditional to return early uses event (the global one).
Also, there's really no need to shorten the event parameter to e.
This works only because browsers expose
window.event.
I think only WebKit browsers do that or, at least, it used to be so. Regardless, the bug needs to be fixed.
#3
in reply to: ↑ 2
@
6 hours ago
Replying to afercia:
Correct. Thanks for the report @khokansardar.
As far as I can tell, linkClickHandler already passes the event so the fix is pretty simple. Other usages within the function use
ewhile the conditional to return early usesevent(the global one).
Also, there's really no need to shorten the
eventparameter toe.
This works only because browsers expose
window.event.
I think only WebKit browsers do that or, at least, it used to be so. Regardless, the bug needs to be fixed.
Agreed on the parameter name — PR 13293 now renames e to event in every handler in the file, matching what @westonruter and @peterwilsoncc asked for on the PR.
On the browser support though: window.event isn't WebKit-only. It's in the DOM spec as a legacy feature and ships everywhere — Chrome 1, Edge 12, Safari 1.1, Firefox 66 (2019, still before [49202]).
So this isn't vendor-specific, it's scope. A top-level var event in the embed document puts an own property on window that shadows the Window.prototype accessor, the identifier resolves to undefined, and the check silently passes. Same in every browser — and it's in the shipped minified build too, since uglify leaves the free event unmangled.
#6
@
4 hours ago
- Keywords commit removed
On the browser support though: window.event isn't WebKit-only. It's in the DOM spec as a legacy feature and ships everywhere — Chrome 1, Edge 12, Safari 1.1, Firefox 66
As I said, or, at least, it used to be so.
For history, and for the ones who are interested in knowing a little more about browsers, Firefox actively refused to support window.event for 15 years. It only introduced support in version 66 in 2018.
Today, the WHATWG marks window.event as 'legacy extension' and only keeps it for backward compatibility. Its usage is not encouraged. If other occurrences of it are in Core, they should be fixed.
![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png)
linkClickHandler() in src/js/_enqueues/lib/embed-template.js checked modifier keys against the global event rather than its own e parameter. This works only because browsers expose window.event, which is an accessor on Window.prototype — any script in the embed template declaring a top-level var event shadows it with an own property, so the check reads undefined and passes.
The result is that a Cmd/Ctrl + click on a link inside an embed calls preventDefault() and navigates the top window instead of opening a new tab, defeating the behavior added in [49202]. Using the handler's own parameter is immune to the collision and also clears four 'event' is not defined ESLint errors.
No tests are added; there is currently no QUnit coverage for this file, and adding that harness for a one-line change seemed disproportionate.
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 window.event shadowing behavior in a browser, and drafting the fix. All changes were reviewed and validated by me.