Don't open two browser tabs when clicking the pull request number - #8962
Open
Lukas (L4XB) wants to merge 1 commit into
Open
Lukas (L4XB) wants to merge 1 commit into
Lukas (L4XB) wants to merge 1 commit into
Conversation
02ac4de gave the pull request number links in the overview header and the sticky header an onClick that calls preventDefault and opens the item on GitHub, so that the link also works in the Agents window. The webview host opens any anchor with an href as well, and its click handler does not check defaultPrevented, so in a regular window the click is handled twice and the browser gets two tabs for the same URL. Stop the click from reaching the host, the way the code reference link handler in webviews/editorWebview/app.tsx already does. Fixes microsoft#8955
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
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.
Fixes #8955
Cause
Not #8919, which the issue guesses at — that PR does not touch link handling. The
regression is 02ac4de ("Fix various things in the webview that don't work correctly in
Agents window", #8935), which fixed #8934 by giving the
#<number>links inwebviews/components/header.tsxandwebviews/components/stickyHeader.tsxanonClick:Both anchors keep their
href, and the webview host opens any anchor with anhrefon itsown.
handleInnerClickinsrc/vs/workbench/contrib/webview/browser/pre/index.html(microsoft/vscode) is registeredwith
contentWindow.addEventListener('click', handleInnerClick)and postsdid-click-linkfor the first anchor in the click's composed path. It does not check
event.defaultPrevented— thecontextmenuhandler registered a few lines below it does.React 16 delegates
onClickatdocument, sopreventDefault()runs and the event thencarries on bubbling to the content
window, where the host handler runs regardless.One click therefore produces two opens:
did-click-link→IOpenerService.openpr.openOnGitHub→openItemOnGitHub(src/commands.ts) →openWithDefaultExternalOpenerWith
githubPullRequests.openPullLinksoff — its default since #8942 — nothing claims thefirst one, so both reach the browser: the two tabs in the report. With the setting on, the
extension's own external URI opener claims the
did-click-linkand re-reveals the overviewinstead, so the duplicate is less visible but still there.
These two anchors are the only ones under
webviews/that carry both anhrefand anonClick; every other<a onClick>in the tree has nohref, which is why they were neveraffected.
Fix
Stop the click from reaching the host, so exactly one code path opens the URL.
webviews/editorWebview/app.tsxalready does this for code reference links, with acapture-phase listener that calls
preventDefault()andstopPropagation().Keeping the
hrefpreserves the hover target, keyboard activation and the copy-link contextmenu; dropping the
onClickinstead would bring #8934 back, so the Agents-window path staysexactly as #8935 left it.
Arguably the deeper defect is that
handleInnerClickshould honourdefaultPreventedtheway the neighbouring
contextmenuhandler does. That lives in microsoft/vscode, so thischange does not wait on it.
Test
webviews/editorWebview/test/overview.test.tsxgainsopens a PR number link exactly once.It registers the host's behaviour on
window— open any anchor with anhrefthat the clickreaches, without checking
defaultPrevented— clicks each number link and asserts oneopenOnGitHubcall and no host open, i.e. exactly one external open per click.At the merge base (2f06150), with only the test applied:
With the fix:
applies deferred pull request updatesfails the same way on both sides and is unrelated tothis change:
PRContext.updatePRcallsupdateState, which doesObject.assign(vscode.getState(), data), and the test constructsnew PRContext(pr)withoutanything ever calling
setState, sogetState()isundefinedand it throwsTypeError: Cannot convert undefined or null to object.These tests are not currently executed anywhere
webviews/**/test/*.test.tsxis type-checked throughtsconfig.webviews.json, but nothingruns it.
tsconfig.test.jsonincludes onlysrc/@types,src/common,src/githubandsrc/test;src/test/index.tsloadsrequire.context('./', …); and afternpm run compileno webview test appears in
dist/— neither the new test nor the existingapplies sticky class when scrolled. That also explains how the failure above wentunnoticed.
I have left the test next to the code it covers, with the three sibling webview test files,
rather than growing this fix into a test-infrastructure change — turning the directory on
today would redden CI on pre-existing failures in
overview.test.tsxandapp.test.tsx.Happy to split that out into its own PR if you want it.
I ran the file with mocha and ts-node against
tsconfig.webviews.json(module: commonjs),with a setup file that installs
jsdom-globalplusrequestAnimationFrame,matchMediaandIntersectionObserverstubs and anacquireVsCodeApimirroringsrc/test/mocks/mockWebviewEnvironment.ts, andrequire.extensionshooks standing in for thesvg-inline-loaderand CSS webpack loaders.Validation
npm run compile— success (extension:node,extension:webworker,webviews)npm run lint— exit 0, no files changednpm run hygiene— exit 0npm run check:commands— all declared commands are registerednpm test— 559 passing, 0 failing (test:scripts75 passing, extension host suite exit 0)npx tsc --noEmitfortsconfig.json,tsconfig.webviews.jsonandtsconfig.test.json— all cleanFile and line references are against
main@2f06150a, and microsoft/vscodemain@f80869ac.