Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix detection of anchor click events inside svg #23272

Merged
merged 7 commits into from Feb 6, 2022

Conversation

klaasman
Copy link
Contributor

@klaasman klaasman commented Mar 22, 2021

Bug

The nodeName of an anchor inside an SVG equals the lowercase a instead of the html anchor's uppercase A.
This behavior can be seen in this plain js demo: https://jsfiddle.net/L2p8f4ve/28/

fixes #23252

Bug

  • Related issues linked using fixes #number
  • Integration tests added

Feature

  • Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
  • Related issues linked using fixes #number
  • Integration tests added
  • Documentation added
  • Telemetry added. In case of a feature if it's used or not.

Documentation / Examples

  • Make sure the linting passes

@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@timneutkens
Copy link
Member

timneutkens commented Mar 23, 2021

Thanks!

I've added back the PR template as there's a missing step in this PR:

Integration tests added

@klaasman
Copy link
Contributor Author

Any pointers on where to add/write the integration test?

@timneutkens
Copy link
Member

In test/integration you can find examples of other integration tests.

@ijjk

This comment has been minimized.

@ijjk

This comment was marked as outdated.

# Conflicts:
#	package.json
#	test/lib/next-webdriver.d.ts
#	test/lib/wd-chain.js
#	yarn.lock
@ijjk
Copy link
Member

ijjk commented Feb 6, 2022

Stats from current PR

Default Build (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary klaasman/next.js fix-link-in-svg Change
buildDuration 19.6s 19.4s -173ms
buildDurationCached 7.6s 7.6s -35ms
nodeModulesSize 359 MB 359 MB ⚠️ +221 B
Page Load Tests Overall increase ✓
vercel/next.js canary klaasman/next.js fix-link-in-svg Change
/ failed reqs 0 0
/ total time (seconds) 4.041 3.972 -0.07
/ avg req/sec 618.62 629.46 +10.84
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.713 1.682 -0.03
/error-in-render avg req/sec 1459.81 1486.03 +26.22
Client Bundles (main, webpack, commons)
vercel/next.js canary klaasman/next.js fix-link-in-svg Change
450.HASH.js gzip 179 B 179 B
framework-HASH.js gzip 42.2 kB 42.2 kB
main-HASH.js gzip 27.4 kB 27.4 kB
webpack-HASH.js gzip 1.44 kB 1.44 kB
Overall change 71.2 kB 71.2 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary klaasman/next.js fix-link-in-svg Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages Overall increase ⚠️
vercel/next.js canary klaasman/next.js fix-link-in-svg Change
_app-HASH.js gzip 1.36 kB 1.36 kB
_error-HASH.js gzip 194 B 194 B
amp-HASH.js gzip 312 B 312 B
css-HASH.js gzip 326 B 326 B
dynamic-HASH.js gzip 2.37 kB 2.37 kB
head-HASH.js gzip 350 B 350 B
hooks-HASH.js gzip 919 B 919 B
image-HASH.js gzip 4.94 kB 4.94 kB
index-HASH.js gzip 263 B 263 B
link-HASH.js gzip 2.19 kB 2.2 kB ⚠️ +10 B
routerDirect..HASH.js gzip 321 B 321 B
script-HASH.js gzip 383 B 383 B
withRouter-HASH.js gzip 318 B 318 B
85e02e95b279..7e3.css gzip 107 B 107 B
Overall change 14.3 kB 14.4 kB ⚠️ +10 B
Client Build Manifests Overall increase ⚠️
vercel/next.js canary klaasman/next.js fix-link-in-svg Change
_buildManifest.js gzip 459 B 460 B ⚠️ +1 B
Overall change 459 B 460 B ⚠️ +1 B
Rendered Page Sizes
vercel/next.js canary klaasman/next.js fix-link-in-svg Change
index.html gzip 532 B 532 B
link.html gzip 546 B 546 B
withRouter.html gzip 526 B 526 B
Overall change 1.6 kB 1.6 kB

Diffs

Diff for _buildManifest.js
@@ -13,7 +13,7 @@ self.__BUILD_MANIFEST = {
   "/head": ["static\u002Fchunks\u002Fpages\u002Fhead-7100d3b2a548f0e4.js"],
   "/hooks": ["static\u002Fchunks\u002Fpages\u002Fhooks-538d621a0e670391.js"],
   "/image": ["static\u002Fchunks\u002Fpages\u002Fimage-f97943edf7ae3dd3.js"],
-  "/link": ["static\u002Fchunks\u002Fpages\u002Flink-f0a2c3bb0706d8b2.js"],
+  "/link": ["static\u002Fchunks\u002Fpages\u002Flink-27386a147a7100fa.js"],
   "/routerDirect": [
     "static\u002Fchunks\u002Fpages\u002FrouterDirect-76232dd6bc335a24.js"
   ],
Diff for link-HASH.js
@@ -151,8 +151,10 @@
         locale
       ) {
         var nodeName = e.currentTarget.nodeName;
+        // anchors inside an svg have a lowercase nodeName
+        var isAnchorNodeName = nodeName.toUpperCase() === "A";
         if (
-          nodeName === "A" &&
+          isAnchorNodeName &&
           (isModifiedEvent(e) || !(0, _router).isLocalURL(href))
         ) {
           // ignore click for browser’s default behavior
Diff for link.html
@@ -27,7 +27,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/pages/link-f0a2c3bb0706d8b2.js"
+      src="/_next/static/chunks/pages/link-27386a147a7100fa.js"
       defer=""
     ></script>
     <script src="/_next/static/BUILD_ID/_buildManifest.js" defer=""></script>

Default Build with SWC (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary klaasman/next.js fix-link-in-svg Change
buildDuration 23.4s 23.6s ⚠️ +192ms
buildDurationCached 7.7s 7.6s -36ms
nodeModulesSize 359 MB 359 MB ⚠️ +221 B
Page Load Tests Overall increase ✓
vercel/next.js canary klaasman/next.js fix-link-in-svg Change
/ failed reqs 0 0
/ total time (seconds) 3.96 4.007 ⚠️ +0.05
/ avg req/sec 631.35 623.89 ⚠️ -7.46
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.798 1.668 -0.13
/error-in-render avg req/sec 1390.05 1498.64 +108.59
Client Bundles (main, webpack, commons)
vercel/next.js canary klaasman/next.js fix-link-in-svg Change
450.HASH.js gzip 179 B 179 B
framework-HASH.js gzip 42.3 kB 42.3 kB
main-HASH.js gzip 27.4 kB 27.4 kB
webpack-HASH.js gzip 1.44 kB 1.44 kB
Overall change 71.3 kB 71.3 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary klaasman/next.js fix-link-in-svg Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages Overall increase ⚠️
vercel/next.js canary klaasman/next.js fix-link-in-svg Change
_app-HASH.js gzip 1.35 kB 1.35 kB
_error-HASH.js gzip 180 B 180 B
amp-HASH.js gzip 305 B 305 B
css-HASH.js gzip 321 B 321 B
dynamic-HASH.js gzip 2.36 kB 2.36 kB
head-HASH.js gzip 342 B 342 B
hooks-HASH.js gzip 911 B 911 B
image-HASH.js gzip 4.97 kB 4.97 kB
index-HASH.js gzip 256 B 256 B
link-HASH.js gzip 2.21 kB 2.22 kB ⚠️ +11 B
routerDirect..HASH.js gzip 314 B 314 B
script-HASH.js gzip 375 B 375 B
withRouter-HASH.js gzip 309 B 309 B
85e02e95b279..7e3.css gzip 107 B 107 B
Overall change 14.3 kB 14.3 kB ⚠️ +11 B
Client Build Manifests
vercel/next.js canary klaasman/next.js fix-link-in-svg Change
_buildManifest.js gzip 459 B 459 B
Overall change 459 B 459 B
Rendered Page Sizes
vercel/next.js canary klaasman/next.js fix-link-in-svg Change
index.html gzip 533 B 533 B
link.html gzip 546 B 546 B
withRouter.html gzip 528 B 528 B
Overall change 1.61 kB 1.61 kB

Diffs

Diff for _buildManifest.js
@@ -13,7 +13,7 @@ self.__BUILD_MANIFEST = {
   "/head": ["static\u002Fchunks\u002Fpages\u002Fhead-7100d3b2a548f0e4.js"],
   "/hooks": ["static\u002Fchunks\u002Fpages\u002Fhooks-538d621a0e670391.js"],
   "/image": ["static\u002Fchunks\u002Fpages\u002Fimage-f97943edf7ae3dd3.js"],
-  "/link": ["static\u002Fchunks\u002Fpages\u002Flink-f0a2c3bb0706d8b2.js"],
+  "/link": ["static\u002Fchunks\u002Fpages\u002Flink-27386a147a7100fa.js"],
   "/routerDirect": [
     "static\u002Fchunks\u002Fpages\u002FrouterDirect-76232dd6bc335a24.js"
   ],
Diff for link-HASH.js
@@ -151,8 +151,10 @@
         locale
       ) {
         var nodeName = e.currentTarget.nodeName;
+        // anchors inside an svg have a lowercase nodeName
+        var isAnchorNodeName = nodeName.toUpperCase() === "A";
         if (
-          nodeName === "A" &&
+          isAnchorNodeName &&
           (isModifiedEvent(e) || !(0, _router).isLocalURL(href))
         ) {
           // ignore click for browser’s default behavior
Diff for link.html
@@ -27,7 +27,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/pages/link-f0a2c3bb0706d8b2.js"
+      src="/_next/static/chunks/pages/link-27386a147a7100fa.js"
       defer=""
     ></script>
     <script src="/_next/static/BUILD_ID/_buildManifest.js" defer=""></script>
Commit: fb67e68

Copy link
Member

@ijjk ijjk left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

@kodiakhq kodiakhq bot merged commit b39e49e into vercel:canary Feb 6, 2022
@klaasman klaasman deleted the fix-link-in-svg branch February 11, 2022 14:21
@klaasman klaasman restored the fix-link-in-svg branch February 11, 2022 14:21
natew pushed a commit to natew/next.js that referenced this pull request Feb 16, 2022
## Bug

The `nodeName` of an anchor inside an SVG equals the lowercase `a` instead of the html anchor's uppercase `A`.
This behavior can be seen in this plain js demo: https://jsfiddle.net/L2p8f4ve/28/

fixes vercel#23252




## Bug

- [x] Related issues linked using `fixes #number`
- [x] Integration tests added

## Feature

- [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
- [ ] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [ ] Documentation added
- [ ] Telemetry added. In case of a feature if it's used or not.

## Documentation / Examples

- [ ] Make sure the linting passes

Co-authored-by: JJ Kasper <22380829+ijjk@users.noreply.github.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<Link /> inside an SVG won't allow user to cmd+click and open page in new tab
3 participants