fix(runtime-dom): ensure element attributes are always properly applied regardless...

fix: #5477

diff --git a/NOTES.md b/NOTES.md
new file mode 100644 (file)
index 0000000..dc47535
--- /dev/null
+++ b/NOTES.md
@@ -0,0 +1,43 @@
+# Fixing problems with hyphenated vs. camelCased element props
+
+## Situation
+
+Vue has two different ways of applying the vnode's props to an element:
+  1. apply them as element attributes (`el.settAttribute('value', 'Test')`)
+  2. apply them as element properties (`el.value = 'Test'`)
+
+Vue prefers the second way *if* it can detect that property on the element (simplified: ` if (value in el)`, plus some exceptions/special cases.)
+
+As no* regular HTML attribute contains a hyphen, kebab-case vs. camelCase is usually not an issue, but there are two important exceptions:
+
+- all `aria-` attributes.
+- any custom Attributes can contain hyphens (or be camelCased element properties). These are usually used on custom elements ("web components")
+
+## The problem
+
+When a hyphenated or camelCased vnode prop is processed in `patchProp`, we can experience a few related but distinct undesirable bugs.
+
+|prop|Has DOM prop?|handled correctly?|behavior
+|-|-|-|-|
+|`id`|✅|✅|applied as DOM property `id`|
+|`aria-label`|❌|✅|applied as attribute `aria-label`|
+|`ariaControls`|❌ |❌| 🚸 applied as attribute `ariacontrols` (1)|
+|`custom-attr`|✅ `customAttr`|❌| 🚸 applied as attribute `custom-attr` (2a), though |
+|`customAttr`|✅|✅| 🚸 applied as el property `customAttr` (2b)|
+
+Problem (1):
+
+a `camelCase` prop is applied as lowercase attribute (missing hyphen)
+
+Problem (2): 
+
+- `kebap-case`prop applied as attribute even though matching camelCase DOM property exists.
+- while `camelCase` prop is applied to element via the matching DOM property.
+
+This can lead to problems with custom elements. For example, if the custom element's prop `post` expects a post object, that has to be passed as a DOMprop. Applying it as an attribute will result in `posts="[Object object]"`.
+
+## Things things to consider / Open questions.
+
+- SVGs can have `camelCase` attributes. That should be handled properly by the implementation, though - I think it's covered.
+- `tabindex` attribute vs `tabIndex` DOMProp. Don't think this is a problem either but it feels worth mentioning as it's the only instance I can think of where a regular HTML attribute has a camelCase counterpart.
+- `aria-haspopup` vs. `ariaHasPopup`: Chrome has the latter as a DOMProp (FF doesn't). That domProp's name is *not* the camelCase Version of the `aria-haspopup` attribute. Kinda like the tabindex situation.
\ No newline at end of file

diff --git a/packages/runtime-dom/__tests__/patchProps.spec.ts b/packages/runtime-dom/__tests__/patchProps.spec.ts
index 10ef9e289b4e8e2b867e3a247adca6b5161d6850..d3f45acb268800b8d07b7829fc3927a14083e9ba 100644 (file)
--- a/packages/runtime-dom/__tests__/patchProps.spec.ts
+++ b/packages/runtime-dom/__tests__/patchProps.spec.ts
@@ -140,6 +140,44 @@ describe('runtime-dom: props patching', () => {
     expect(fn).toHaveBeenCalled()
   })
 
+  test('kebap-case vs. camelCase props', async () => {
+    const el = document.createElement('div')
+    let _fooBar: string | undefined = undefined
+    Object.defineProperty(el, 'fooBar', {
+      get() {
+        return _fooBar
+      },
+      set(v: string | undefined) {
+        _fooBar = v
+      }
+    })
+    Object.defineProperty(el, 'fooBaz', {
+      get() {
+        return _fooBar
+      },
+      set(v: string | undefined) {
+        _fooBar = v
+      }
+    })
+    // existing DOMprops
+    patchProp(el, 'fooBar', null, 'foo')
+    expect(el.getAttribute('foobar')).toBe(null)
+    expect((el as any).fooBar).toBe('foo')
+    patchProp(el, 'foo-baz', null, 'baz')
+    expect(el.getAttribute('foobaz')).toBe(null)
+    expect((el as any).fooBar).toBe('baz')
+
+    // missing DOMProp
+    patchProp(el, 'ariaControls', null, 'someId')
+    expect('ariaControls' in el).toBe(false)
+    expect('aria-controls' in el).toBe(false)
+    expect(el.getAttribute('aria-controls')!).toBe('someId')
+    patchProp(el, 'aria-label', null, 'someId')
+    expect('aria-label' in el).toBe(false)
+    expect('ariaLabel' in el).toBe(false)
+    expect(el.getAttribute('aria-label')!).toBe('someId')
+  })
+
   // #1049
   test('set value as-is for non string-value props', () => {
     const el = document.createElement('video')

diff --git a/packages/runtime-dom/src/modules/attrs.ts b/packages/runtime-dom/src/modules/attrs.ts
index 41648ed50dc80e15cc22f48ccf846e44d0712dd2..18bf221a6e539cd1d0927012b180783aefe23286 100644 (file)
--- a/packages/runtime-dom/src/modules/attrs.ts
+++ b/packages/runtime-dom/src/modules/attrs.ts
@@ -1,4 +1,5 @@
 import {
+  hyphenate,
   includeBooleanAttr,
   isSpecialBooleanAttr,
   makeMap,
@@ -33,6 +34,7 @@ export function patchAttr(
     // note we are only checking boolean attributes that don't have a
     // corresponding dom prop of the same name here.
     const isBoolean = isSpecialBooleanAttr(key)
+    key = isSVG ? key : hyphenate(key)
     if (value == null || (isBoolean && !includeBooleanAttr(value))) {
       el.removeAttribute(key)
     } else {

diff --git a/packages/runtime-dom/src/modules/props.ts b/packages/runtime-dom/src/modules/props.ts
index 1c1ad0c16f7a072e97ac94618a5d27bda1102d07..0b94a89d978be40380909d820f7c27573bc9e7b8 100644 (file)
--- a/packages/runtime-dom/src/modules/props.ts
+++ b/packages/runtime-dom/src/modules/props.ts
@@ -3,7 +3,7 @@
 // This can come from explicit usage of v-html or innerHTML as a prop in render
 
 import { warn, DeprecationTypes, compatUtils } from '@vue/runtime-core'
-import { includeBooleanAttr } from '@vue/shared'
+import { camelize, includeBooleanAttr } from '@vue/shared'
 
 // functions. The user is responsible for using them with only trusted content.
 export function patchDOMProp(
@@ -18,6 +18,7 @@ export function patchDOMProp(
   parentSuspense: any,
   unmountChildren: any
 ) {
+  key = camelize(key)
   if (key === 'innerHTML' || key === 'textContent') {
     if (prevChildren) {
       unmountChildren(prevChildren, parentComponent, parentSuspense)

diff --git a/packages/runtime-dom/src/patchProp.ts b/packages/runtime-dom/src/patchProp.ts
index 6d65a63a88c4d513a3b824196b04e14521e106e0..2b15b8dbed18baacd343735f4a457591f271a4a4 100644 (file)
--- a/packages/runtime-dom/src/patchProp.ts
+++ b/packages/runtime-dom/src/patchProp.ts
@@ -3,7 +3,13 @@ import { patchStyle } from './modules/style'
 import { patchAttr } from './modules/attrs'
 import { patchDOMProp } from './modules/props'
 import { patchEvent } from './modules/events'
-import { isOn, isString, isFunction, isModelListener } from '@vue/shared'
+import {
+  isOn,
+  isString,
+  isFunction,
+  isModelListener,
+  camelize
+} from '@vue/shared'
 import { RendererOptions } from '@vue/runtime-core'
 
 const nativeOnRE = /^on[a-z]/
@@ -62,10 +68,11 @@ export const patchProp: DOMRendererOptions['patchProp'] = (
 
 function shouldSetAsProp(
   el: Element,
-  key: string,
+  key: string, // always camelized
   value: unknown,
   isSVG: boolean
 ) {
+  key = key.includes('-') ? camelize(key) : key
   if (isSVG) {
     // most keys must be set as attribute on svg elements to work
     // ...except innerHTML & textContent