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(compat): click modifiers #11702

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alexanderturinske
Copy link

@alexanderturinske alexanderturinske commented Aug 23, 2024

 - when the click "native" modifier is used in conjunction
   with the click "capture" modifier, the click is not
   registered in tests
 - see https://gitlab.com/groups/gitlab-org/-/epics/14951
   for more details

More information

https://gitlab.com/groups/gitlab-org/-/epics/14951

Workaround

-          @click.native.capture.stop="onItemClick(role)"
+          @click.capture.native.stop="onItemClick(role)"

Screenshot

image

     - when the click "native" modifier is used in conjunction
       with the click "capture" modifier, the click is not
       registered in tests
     - see https://gitlab.com/groups/gitlab-org/-/epics/14951
       for more details
@alexanderturinske
Copy link
Author

alexanderturinske commented Aug 23, 2024

May be related to #4566 (comment)

This PR does not fix the solution, but starts TDD with a test

@markrian
Copy link

markrian commented Aug 28, 2024

@alexanderturinske I spent a bit more time looking into this and believe I have a fix.

Roughly speaking, the problem is that the compiler and runtime have different expectations about how "native" events are annotated. The compiler constructs a string of modifiers that matches the source order. So, @click.native.capture gives onClickNativeCapture, while @click.capture.native gives onClickCaptureNative.

The runtime, however, expects the Native substring to only be at the end of the event listener string (removing it if present).

So, there are two possible fixes:

  1. Update the compiler to ensure the native modifier is always last, OR
  2. Update the runtime to look anywhere in the event listener string for Native.

I'm more inclined to update the compiler, since the latter means doing a bit more work at runtime.

Here's a patch that fixes the compiler and the failing test:

diff --git a/packages/compiler-dom/src/transforms/vOn.ts b/packages/compiler-dom/src/transforms/vOn.ts
index e2bf1573b..2c7feb657 100644
--- a/packages/compiler-dom/src/transforms/vOn.ts
+++ b/packages/compiler-dom/src/transforms/vOn.ts
@@ -42,6 +42,7 @@ const resolveModifiers = (
   const keyModifiers = []
   const nonKeyModifiers = []
   const eventOptionModifiers = []
+  let hasNativeModifier = false
 
   for (let i = 0; i < modifiers.length; i++) {
     const modifier = modifiers[i]
@@ -55,7 +56,7 @@ const resolveModifiers = (
         loc,
       )
     ) {
-      eventOptionModifiers.push(modifier)
+      hasNativeModifier = true
     } else if (isEventOptionModifier(modifier)) {
       // eventOptionModifiers: modifiers for addEventListener() options,
       // e.g. .passive & .capture
@@ -83,6 +84,11 @@ const resolveModifiers = (
     }
   }
 
+  if (__COMPAT__ && hasNativeModifier) {
+    // Ensure native modifer is last
+    eventOptionModifiers.push('native')
+  }
+
   console.log('resolveModifiers', {
     keyModifiers,
     nonKeyModifiers,

For completeness, here's a patch that fixes the runtime, but I don't think it's the right approach.

Patch for runtime
diff --git a/packages/runtime-core/src/componentProps.ts b/packages/runtime-core/src/componentProps.ts
index 79434471b..ce39f150e 100644
--- a/packages/runtime-core/src/componentProps.ts
+++ b/packages/runtime-core/src/componentProps.ts
@@ -409,8 +409,8 @@ function setFullProps(
         // into a separate `attrs` object for spreading. Make sure to preserve
         // original key casing
         if (__COMPAT__) {
-          if (isOn(key) && key.includes('Native')) {
-            key = key.replace('Native', '')
+          if (isOn(key) && key.endsWith('Native')) {
+            key = key.slice(0, -6) // remove Native postfix
           } else if (shouldSkipAttr(key, instance)) {
             continue
           }

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.

2 participants