Allow mixed case events

[developit]

No description provided.

[coveralls]

Coverage remained the same at 100.0% when pulling 4288e18 on event-case into e026d9e on master.

[KeithHenry]

I think this would break React style camelCase events (like onClick).

[developit]

@KeithHenry you're right! not sure what I was thinking there. As for other options, maybe checking for the existence of an on${x} property to signal lowercased events...

if (name.toLowerCase() in node) name = name.toLowerCase();

[coveralls]

Coverage remained the same at 100.0% when pulling 5cb4276 on event-case into 5645573 on master.

[jeremenichelli]

I was thinking about this today after Rob Dodson's talk on Polymer summit. The only way I can imagine this to work is if you have a whilelist of native DOM events and bail from lowercase if the string is not found. Sad because this would increase lib size.

[developit]

@jeremenichelli you don't think my in check would work? Seems like it'd fix some things.

[jeremenichelli]

@developit can you point the line you are doing the in check? Can't find it 😟

The issue here I think is the fact that after L67 we need to somehow know if the event name needs to be normalized to lowercase, before the listener is added.

I don't know for sure how safe this approach is but checking the full event name against the body object might help, somethink like:

if (document.body.name !== 'undefined') {
  // is native event
  name = name.toLowerCase();
}

name = name.substring(2);

[developit]

@jeremenichelli - it wasn't in this PR, was just an idea I came up with. It would look very similar to what you suggested, but rather than using document.body, it would check the current Node being mutated (since different nodes support different events):

let nameLower = name.toLowerCase();
if (node[nameLower]!=undefined) {
  name = nameLower;
}
name = name.substring(2);

[jeremenichelli]

Oh I get it now @developit. Let me know if there's anything I can help with, really interested in helping to push this forward 🎉

[developit]

Same here! I'll update the PR with the fix and we can see what bundlesize says it costs haha

[coveralls]

Coverage remained the same at 100.0% when pulling 716e4bf on event-case into e29de9d on master.

[coveralls]

Coverage remained the same at 100.0% when pulling 5a8d256 on event-case into 7e49a91 on master.

[jeremenichelli]

Looks cool, would be nice to add tests for custom elements event if current Preact's test suit allows it. If not it would be a nice future PR.

[developit]

Yup. We'll need to switch the tests from running in PhantomJS to Chrome Headless, since Phantom doesn't support web components.

[lozandier]

@developit Was this blocked at some point since the analysis on what this change cost as far as bundlesize & switching to Puppeteer (Headless Chrome)?

[robdodson]

btw I've been meaning to write a blog post on this...

In building custom-elements-everywhere, I've seen issues in various libraries around mixed case events. I'm increasingly of the opinion that, as a best practice, event names should just be all one word, all lowercase. That's how the platform handles things today: beforeunload, pointerdown, etc. and I've only found a few exceptions (which seem like anachronisms) e.g. DOMContentLoaded.

By sticking to all lowercase, all one-word, you free up the libraries from each needing to invent their own heuristic to grok what event name you were actually trying to use.

[developit]

@lozandier yup - we can't do whitelists, so detection is the only way to go here, and I'm not sure that can even work. Maybe we'll get browsers to switch those last few event types to lowercase ;)

[developit]

Fixed in X.