Open links in the browser for every click gesture, not just a plain click - #358
Open
amitraj2203 wants to merge 4 commits into
Open
Open links in the browser for every click gesture, not just a plain click#358amitraj2203 wants to merge 4 commits into
amitraj2203 wants to merge 4 commits into
Conversation
The site links each cancel their own navigation in an onClick handler. A middle click fires auxclick, which a click handler never sees, so Chromium's "open in a new window" default ran and the site loaded inside a bare app window with no address bar and no way back. Refuse navigation and window creation once for the app window instead, and hand the address to the system browser. Every link is covered, including ones added later, and the address goes out through the same external-url.js gate the renderer already uses. Fixes WordPress#284
Two findings from the review pass on this branch. The window guard only intercepted http and https, so anything else was left to navigate the main window. That window is the one with the preload bridge attached, and it renders content the app does not author — captured email bodies among it — where a link can point at a local path or at one relative to the app's own file: origin. It now refuses every navigation except a reload, which is the one case that has to keep working; a plain deny breaks reloading the window. The onFailed reporter also only covered the link path. openExternalUrl ignored it and let a rejecting openExternal propagate out of the url:open handler, where none of the renderer's 23 call sites catches anything — so an ordinary click on an address the OS cannot open was still a link that did nothing with nothing in the log. Reporting moved into openExternalUrl, which is where both paths meet. Tests for both fail on the previous code.
Both were out of scope for WordPress#284. The navigation hole they closed — the app window following a file: or relative link out of its own page — is on trunk already and is not something this fix introduced, and the missing failure log on the url:open path is a gap in external-url.js rather than in the link handling. Neither belongs in a PR that fixes one reported bug. Both are worth doing on their own; noted in the PR description so they are not lost.
Trunk moved every test into `tests/unit/` in WordPress#377, and this branch predates that, so the merge raises a `CONFLICT (file location)` for the new `test/window-links.test.cjs`. Git relocates the file on its own; it does not rewrite what is inside it. Accepting the move alone leaves `require('../src/window-links.js')` pointing one directory too high, and the test throws `MODULE_NOT_FOUND` the first time it runs. This resolution accepts the relocation and takes the require to `../../src/window-links.js`, the depth every other file in `tests/unit/` already uses. --- Generated with the help of Claude Code, https://code.claude.com Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
|
@amitraj2203 thank you for this PR. You'll notice I just 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
2 participants
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.
Why
Middle-clicking the site link, wp-admin or Adminer opens the site inside an app window with no address bar and no back button. Only closing it gets you out. See #284.
Each link cancels its own navigation in
onClick. A middle click firesauxclick, which a click handler never sees, so Chromium's "open in a new window" default runs.What changes
src/window-links.jsdenies window creation and http/https navigation on the app window, and sends the address to the system browser instead. One place, so every link is covered including new ones.file:navigation is left alone on purpose. The app's own page is afile:URL and blocking it stopslocation.reload().The links keep their
onClickhandlers. They're redundant now, but removing seven of them is a separate change.How to test this
Platforms: any. Needs a mouse with a clickable scroll wheel; macOS trackpads have no middle button.
Starting state: a site that has finished setting up.
What must not have happened:
For the old behaviour, check out
trunkand repeat steps 1-3.Risks and limitations
Cmd/Ctrl+click is not affected by this bug, though #284 says it is. Chromium sends it as a normal
click, so the existingpreventDefault()catches it. I tested all four gestures on trunk: only middle click reproduces. Worth knowing before testing, or the gesture the issue names looks like it disproves the fix.The review found three separate bugs sitting next to this code. They're listed below and left alone.
Related
Fixes #284
Review outcome (required — see AGENTS.md)
Lint clean.
npm testandnpm run test:electronboth 1034 pass.Fixed: an empty
catchin the new module swallowed a failedopenExternal.Not fixed, all pre-existing and none caused by this change:
file:or page-relative link. One route is the email body atindex.jsx:5551, rendered withdangerouslySetInnerHTML. Trunk has no guard on this window at all. If you fix it, note that blocking every navigation breakslocation.reload(), so the exemption has to be the current URL.openExternalUrlhas no way to report a failed open, and lets the rejection propagate into a renderer that catches nothing at 23 call sites. So a click on an address the OS can't open is a link that did nothing, with nothing in the log.openLinksExternallycall and the suite stays green.test/ipc-wiring.test.cjscan't reachcreateWindow:app.whenReady()is stubbed as a promise that never settles andmain.jshas no exports.I had fixed the first two, then reverted them as out of scope, which is why there's a revert in the history. The code matches the first commit.
Screenshots
Nothing new appears on screen; the fix is a window that stops appearing. I have a screenshot of the bug on trunk if it's useful.