Skip to content

fix: don't treat non-function on* props on custom elements as event handlers#5170

Open
JoviDeCroock wants to merge 1 commit into
mainfrom
JoviDeCroock/issue-4085
Open

fix: don't treat non-function on* props on custom elements as event handlers#5170
JoviDeCroock wants to merge 1 commit into
mainfrom
JoviDeCroock/issue-4085

Conversation

@JoviDeCroock

@JoviDeCroock JoviDeCroock commented Jul 18, 2026

Copy link
Copy Markdown
Member

Custom elements may expose non-event attributes starting with "on" (e.g. only-when-overflow). Only take the event-listener path when the new or old value is a function, or the element is not a custom element (no dash in its tag name).

Fixes #4085

Co-Authored with GPT5.6-sol for tests

…andlers

Custom elements may expose non-event attributes starting with "on"
(e.g. only-when-overflow). Only take the event-listener path when the
new or old value is a function, or the element is not a custom element
(no dash in its tag name). The typeof check short-circuits first, so
regular event handlers pay no extra cost.

Fixes #4085
Comment thread src/diff/props.js
else if (
name[0] == 'o' &&
name[1] == 'n' &&
(typeof value == 'function' ||

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

short-circuit so there shouldn't be an extra cost for normal execution)

@github-actions

github-actions Bot commented Jul 18, 2026

Copy link
Copy Markdown

📊 Tachometer Benchmark Results

Summary

duration

  • create10k: unsure 🔍 -0% - +1% (-2.77ms - +4.27ms)
    preact-local vs preact-main
  • filter-list: unsure 🔍 -0% - +1% (-0.05ms - +0.13ms)
    preact-local vs preact-main
  • hydrate1k: unsure 🔍 -2% - +1% (-1.27ms - +0.44ms)
    preact-local vs preact-main
  • many-updates: unsure 🔍 -1% - +2% (-0.19ms - +0.30ms)
    preact-local vs preact-main
  • replace1k: unsure 🔍 -2% - +1% (-0.82ms - +0.76ms)
    preact-local vs preact-main
  • text-update: unsure 🔍 -4% - +4% (-0.08ms - +0.07ms)
    preact-local vs preact-main
  • todo: unsure 🔍 -2% - +1% (-0.63ms - +0.18ms)
    preact-local vs preact-main
  • update10th1k: unsure 🔍 -3% - +4% (-1.13ms - +1.16ms)
    preact-local vs preact-main

usedJSHeapSize

  • create10k: unsure 🔍 -0% - +0% (-0.00ms - +0.01ms)
    preact-local vs preact-main
  • filter-list: unsure 🔍 +0% - +0% (+0.00ms - +0.00ms)
    preact-local vs preact-main
  • hydrate1k: unsure 🔍 -2% - +4% (-0.12ms - +0.19ms)
    preact-local vs preact-main
  • many-updates: unsure 🔍 -0% - +0% (-0.00ms - +0.01ms)
    preact-local vs preact-main
  • replace1k: unsure 🔍 -0% - +0% (-0.00ms - +0.00ms)
    preact-local vs preact-main
  • text-update: unsure 🔍 -3% - +3% (-0.04ms - +0.04ms)
    preact-local vs preact-main
  • todo: unsure 🔍 +0% - +0% (+0.00ms - +0.00ms)
    preact-local vs preact-main
  • update10th1k: unsure 🔍 -0% - +0% (-0.01ms - +0.01ms)
    preact-local vs preact-main

Results

create10k

duration

VersionAvg timevs preact-localvs preact-main
preact-local809.26ms - 814.36ms-unsure 🔍
-0% - +1%
-2.77ms - +4.27ms
preact-main808.64ms - 813.48msunsure 🔍
-1% - +0%
-4.27ms - +2.77ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local19.08ms - 19.09ms-unsure 🔍
-0% - +0%
-0.00ms - +0.01ms
preact-main19.08ms - 19.08msunsure 🔍
-0% - +0%
-0.01ms - +0.00ms
-
filter-list

duration

VersionAvg timevs preact-localvs preact-main
preact-local16.53ms - 16.69ms-unsure 🔍
-0% - +1%
-0.05ms - +0.13ms
preact-main16.53ms - 16.61msunsure 🔍
-1% - +0%
-0.13ms - +0.05ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local1.55ms - 1.56ms-unsure 🔍
+0% - +0%
+0.00ms - +0.00ms
preact-main1.55ms - 1.55msunsure 🔍
-0% - -0%
-0.00ms - -0.00ms
-
hydrate1k

duration

VersionAvg timevs preact-localvs preact-main
preact-local57.25ms - 58.44ms-unsure 🔍
-2% - +1%
-1.27ms - +0.44ms
preact-main57.64ms - 58.87msunsure 🔍
-1% - +2%
-0.44ms - +1.27ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local5.00ms - 5.23ms-unsure 🔍
-2% - +4%
-0.12ms - +0.19ms
preact-main4.98ms - 5.18msunsure 🔍
-4% - +2%
-0.19ms - +0.12ms
-
many-updates

duration

VersionAvg timevs preact-localvs preact-main
preact-local16.13ms - 16.46ms-unsure 🔍
-1% - +2%
-0.19ms - +0.30ms
preact-main16.06ms - 16.42msunsure 🔍
-2% - +1%
-0.30ms - +0.19ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local3.75ms - 3.75ms-unsure 🔍
-0% - +0%
-0.00ms - +0.01ms
preact-main3.74ms - 3.75msunsure 🔍
-0% - +0%
-0.01ms - +0.00ms
-
replace1k
  • Browser: chrome-headless
  • Sample size: 100
  • Built by: CI #5699
  • Commit: a3c5d62

duration

VersionAvg timevs preact-localvs preact-main
preact-local53.40ms - 54.32ms-unsure 🔍
-2% - +1%
-0.82ms - +0.76ms
preact-main53.25ms - 54.53msunsure 🔍
-1% - +2%
-0.76ms - +0.82ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local3.01ms - 3.01ms-unsure 🔍
-0% - +0%
-0.00ms - +0.00ms
preact-main3.01ms - 3.02msunsure 🔍
-0% - +0%
-0.00ms - +0.00ms
-

run-warmup-0

VersionAvg timevs preact-localvs preact-main
preact-local28.67ms - 29.46ms-unsure 🔍
-2% - +2%
-0.54ms - +0.57ms
preact-main28.66ms - 29.44msunsure 🔍
-2% - +2%
-0.57ms - +0.54ms
-

run-warmup-1

VersionAvg timevs preact-localvs preact-main
preact-local32.94ms - 34.30ms-unsure 🔍
-1% - +5%
-0.18ms - +1.54ms
preact-main32.41ms - 33.46msunsure 🔍
-5% - +1%
-1.54ms - +0.18ms
-

run-warmup-2

VersionAvg timevs preact-localvs preact-main
preact-local28.88ms - 30.09ms-unsure 🔍
-3% - +2%
-0.99ms - +0.69ms
preact-main29.05ms - 30.22msunsure 🔍
-2% - +3%
-0.69ms - +0.99ms
-

run-warmup-3

VersionAvg timevs preact-localvs preact-main
preact-local25.42ms - 25.72ms-unsure 🔍
-0% - +1%
-0.10ms - +0.32ms
preact-main25.31ms - 25.60msunsure 🔍
-1% - +0%
-0.32ms - +0.10ms
-

run-warmup-4

VersionAvg timevs preact-localvs preact-main
preact-local23.27ms - 24.70ms-unsure 🔍
-6% - +2%
-1.54ms - +0.57ms
preact-main23.69ms - 25.25msunsure 🔍
-2% - +6%
-0.57ms - +1.54ms
-

run-final

VersionAvg timevs preact-localvs preact-main
preact-local20.09ms - 20.45ms-unsure 🔍
-3% - +2%
-0.55ms - +0.33ms
preact-main19.98ms - 20.79msunsure 🔍
-2% - +3%
-0.33ms - +0.55ms
-
text-update
  • Browser: chrome-headless
  • Sample size: 200
  • Built by: CI #5699
  • Commit: a3c5d62

duration

VersionAvg timevs preact-localvs preact-main
preact-local1.89ms - 1.98ms-unsure 🔍
-4% - +4%
-0.08ms - +0.07ms
preact-main1.88ms - 2.00msunsure 🔍
-4% - +4%
-0.07ms - +0.08ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local1.03ms - 1.08ms-unsure 🔍
-3% - +3%
-0.04ms - +0.04ms
preact-main1.03ms - 1.08msunsure 🔍
-3% - +3%
-0.04ms - +0.04ms
-
todo

duration

VersionAvg timevs preact-localvs preact-main
preact-local25.13ms - 25.67ms-unsure 🔍
-2% - +1%
-0.63ms - +0.18ms
preact-main25.32ms - 25.93msunsure 🔍
-1% - +3%
-0.18ms - +0.63ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local1.27ms - 1.27ms-unsure 🔍
+0% - +0%
+0.00ms - +0.00ms
preact-main1.27ms - 1.27msunsure 🔍
-0% - -0%
-0.00ms - -0.00ms
-
update10th1k

duration

VersionAvg timevs preact-localvs preact-main
preact-local31.80ms - 33.54ms-unsure 🔍
-3% - +4%
-1.13ms - +1.16ms
preact-main31.91ms - 33.40msunsure 🔍
-4% - +3%
-1.16ms - +1.13ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local2.97ms - 2.98ms-unsure 🔍
-0% - +0%
-0.01ms - +0.01ms
preact-main2.97ms - 2.98msunsure 🔍
-0% - +0%
-0.01ms - +0.01ms
-

tachometer-reporter-action v2 for CI

@github-actions

Copy link
Copy Markdown

Size Change: +22 B (+0.14%)

Total Size: 15.9 kB

📦 View Changed
Filename Size Change
dist/preact.mjs 4.91 kB +22 B (+0.45%)
ℹ️ View Unchanged
Filename Size
compat/dist/compat.mjs 3.91 kB
debug/dist/debug.mjs 3.89 kB
devtools/dist/devtools.mjs 274 B
hooks/dist/hooks.mjs 1.55 kB
jsx-runtime/dist/jsxRuntime.mjs 861 B
test-utils/dist/testUtils.mjs 473 B

compressed-size-action

@coveralls

Copy link
Copy Markdown

Coverage Status

coverage: 98.541%. remained the same — JoviDeCroock/issue-4085 into main

@rschristian

Copy link
Copy Markdown
Member

Custom elements may expose non-event attributes starting with "on" (e.g. only-when-overflow)

IMO, we shouldn't support this.

on... is reserved for event handlers only in the DOM, the spec group will not allow other properties to collide. This is a very explicit rule at this point. CE authors are free to do as they please, but I don't think we should incur cost when they choose to deviate from platform standards... and we already have some restrictions/limitations around CEs in that we expect them to act like standard DOM elements, i.e., we prefer properties over attributes and so if you have a setter property that you don't want us to call, tough luck.

@JoviDeCroock

JoviDeCroock commented Jul 18, 2026

Copy link
Copy Markdown
Member Author

I mean, onlyOverflow for instance is a valid name for a prop and it runs into this issue. I think it's a non obvious footgun. I guess it has propagated in the ecosystem so we can live with it, tried to see if we had some easy fixes

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.

Uncaught Error: Component's "only-when-overflow" property should be a function, but got [boolean] instead

3 participants