From: Thorsten Luenborg Date: Fri, 23 Dec 2022 20:21:39 +0000 (+0100) Subject: fix(runtime-dom): ensure element attributes are always properly applied regardless... X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=b8c330db5cc4e9713cc16eab4841dbc82c522ca3;p=thirdparty%2Fvuejs%2Fcore.git fix(runtime-dom): ensure element attributes are always properly applied regardless of being passed in kebap-case or camelCase. fix: #5477 --- diff --git a/NOTES.md b/NOTES.md new file mode 100644 index 0000000000..dc47535bfb --- /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 10ef9e289b..d3f45acb26 100644 --- 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 41648ed50d..18bf221a6e 100644 --- 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 1c1ad0c16f..0b94a89d97 100644 --- 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 6d65a63a88..2b15b8dbed 100644 --- 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