Repository navigation
Deprecate which property of the event object #2337
Description
In the https://api.jquery.com/category/events/event-object/, we have
The following properties are also copied to the event object, though some of their values may be undefined depending on the event:
altKey, bubbles, button, cancelable, charCode, clientX, clientY, ctrlKey, currentTarget, data, detail, eventPhase, metaKey, offsetX, offsetY, originalTarget, pageX, pageY, relatedTarget, screenX, screenY, shiftKey, target, view, which
which property is deprecated by the DOM specification - https://w3c.github.io/uievents/#widl-KeyboardEvent-which, it seems we need to add a note about it (and possible for the other properties too), deprecated it and show message in the migrate?
/cc @dmethvin
Activity
timmywil commentedon May 20, 2015
Hmm, I'm not sure that's necessary. Doesn't hurt to copy it if it's there.
timmywil commentedon May 20, 2015
Or should this be moved to tickets in api/migrate?
dmethvin commentedon May 20, 2015
Remember there is a
whichfor both KeyboardEvent and MouseEvent. Both are pretty screwy.For a keyboard event you're supposed to use
keyinstead ofwhichbut for a long time the preferred property waskeyCode. We copy both of them over to the normalized event if they are present, but neither will be there in IE8.The W3C created another confusing set of
buttonandbuttonsproperties for MouseEvent that also took a few years for browsers to get right. IE8 only supportswhichnatively. At the time the other two properties were screwed up with other browsers we decided to normalizewhich. The docs say that we don't copybuttonsbut we do, so a lot of people may be usingbuttonandbuttonsalready if they don't care about IE8.We could deprecate
whichin 3.0 and have Migrate warn about the better choices here. The problem would be in the compat branch where IE8 needs it and we never normalizedbutton,buttons,keyCode, and/orkey. Also note that there may be a high perf penalty for that, we'll have to see. Events like mousemove or mouseover happen a lot.mgol commentedon May 20, 2015
The copying already causes performance penalty in Firefox in some cases, see mozilla/treeherder-ui-deprecated#486 that @fitzgen made me aware of. I didn't have time to measure what's exactly slowing everything down but in the linked PR the jQuery is patched to use getters instead of copying as it significantly speeds up Firefox from what I understand.
dmethvin commentedon May 20, 2015
It's probably the calculation of
pageXandpageYbecause they can force layout. Changing to a getter avoids this as long as the user's event handler doesn't ask for them. Since they're deprecated properties nobody should use them but we've supported them because they were present in IE8 and are handy. From a perf standpoint it would be best to deprecate these as well, because even if we shim them with a getter they're poison when you use them.mgol commentedon May 20, 2015
Well, if they're deprecated and they may cause performance penalty, could we remove them in 3.0? I'd be in favor in just keeping deprecated but perf issues are a bad thing to have, especially if it's so bad that 3rd parties have to patch our code to make it palatable.
jbedard commentedon May 20, 2015
Normally the copying is only bad when it forces a style recalculation and layout such as offset/position properties (see #1746). Although for very frequent events (mousemove) the copy loop can be pretty significant even without a re-layout.
timmywil commentedon May 20, 2015
I think the most we'll do for 3.0 is deprecate. I'd feel more comfortable removing once IE8 market share is low enough and we can drop it.
mgol commentedon May 20, 2015
Right, but what PR https://github.com/mozilla/treeherder-ui/pull/486/files does is just changing the copying to multiple
definePropertyso from a purely-JS stand point the replacement should be slower. It seems actually reading the property must be causing the perf penalty then.Yeah, but if we're causing perf issues there we might for example change copying to using getters in some cases where supported. This would require some actual examples we can test on, though, for now we're just guessing.
dmethvin commentedon May 20, 2015
If all our 3.0 browsers truly support
Object.defineProperty()it seems like we could definejQuery.Event.prototypeat load time to lazy-copy and lazy-evaluate everything? That removes all the copy overhead from event delivery. There are some complications because other code may definejQuery.event.fixHooksat any time during execution and expect the next event delivery to reflect their changes. It would be good to deprecate that behavior as well so we could optimize in the future.jbedard commentedon May 20, 2015
Like @dmethvin said the
definePropertys could be on the prototype so it moves all performance issues (slow getter, style recalc) to get-time and also removes the fix loop. This makese.hasOwnProperty('which')false though, didn't think of that before...See jbedard@fd61774 for a POC from #1746. This commit handled the
fixHooksissue you mentioned @dmethvin.dmethvin commentedon May 21, 2015
Hey @jbedard, this @jbedard guy had something there. 😸 I love the way it handles the dynamic adds of fixHooks! Would you be able to add a perf test on #1746? I'm wondering if
definePropertyis fast enough now.dmethvin commentedon Jan 9, 2018
This is a docs-only action for 3.3, see jquery/api.jquery.com#821 .
The actual removal will occur with gh-3235.