New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate `which` property of the event object #2337

Closed
markelog opened this Issue May 20, 2015 · 13 comments

Comments

Projects
None yet
5 participants
@markelog
Member

markelog commented May 20, 2015

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

@markelog markelog added the Event label May 20, 2015

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 20, 2015

Member

Hmm, I'm not sure that's necessary. Doesn't hurt to copy it if it's there.

Member

timmywil commented May 20, 2015

Hmm, I'm not sure that's necessary. Doesn't hurt to copy it if it's there.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 20, 2015

Member

Or should this be moved to tickets in api/migrate?

Member

timmywil commented May 20, 2015

Or should this be moved to tickets in api/migrate?

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin May 20, 2015

Member

Remember there is a which for both KeyboardEvent and MouseEvent. Both are pretty screwy.

For a keyboard event you're supposed to use key instead of which but for a long time the preferred property was keyCode. 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 button and buttons properties for MouseEvent that also took a few years for browsers to get right. IE8 only supports which natively. At the time the other two properties were screwed up with other browsers we decided to normalize which. The docs say that we don't copy buttons but we do, so a lot of people may be using button and buttons already if they don't care about IE8.

We could deprecate which in 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 normalized button, buttons, keyCode, and/or key. 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.

Member

dmethvin commented May 20, 2015

Remember there is a which for both KeyboardEvent and MouseEvent. Both are pretty screwy.

For a keyboard event you're supposed to use key instead of which but for a long time the preferred property was keyCode. 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 button and buttons properties for MouseEvent that also took a few years for browsers to get right. IE8 only supports which natively. At the time the other two properties were screwed up with other browsers we decided to normalize which. The docs say that we don't copy buttons but we do, so a lot of people may be using button and buttons already if they don't care about IE8.

We could deprecate which in 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 normalized button, buttons, keyCode, and/or key. 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

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol May 20, 2015

Member

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.

Member

mgol commented 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

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin May 20, 2015

Member

It's probably the calculation of pageX and pageY because 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.

Member

dmethvin commented May 20, 2015

It's probably the calculation of pageX and pageY because 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

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol May 20, 2015

Member

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.

Member

mgol commented 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

This comment has been minimized.

Show comment
Hide comment
@jbedard

jbedard May 20, 2015

Contributor

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.

Contributor

jbedard commented 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

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 20, 2015

Member

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.

Member

timmywil commented 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

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol May 20, 2015

Member

Although for very frequent events (mousemove) the copy loop can be pretty significant even without a re-layout.

Right, but what PR https://github.com/mozilla/treeherder-ui/pull/486/files does is just changing the copying to multiple defineProperty so from a purely-JS stand point the replacement should be slower. It seems actually reading the property must be causing the perf penalty then.

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.

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.

Member

mgol commented May 20, 2015

Although for very frequent events (mousemove) the copy loop can be pretty significant even without a re-layout.

Right, but what PR https://github.com/mozilla/treeherder-ui/pull/486/files does is just changing the copying to multiple defineProperty so from a purely-JS stand point the replacement should be slower. It seems actually reading the property must be causing the perf penalty then.

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.

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

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin May 20, 2015

Member

If all our 3.0 browsers truly support Object.defineProperty() it seems like we could define jQuery.Event.prototype at 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 define jQuery.event.fixHooks at 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.

Member

dmethvin commented May 20, 2015

If all our 3.0 browsers truly support Object.defineProperty() it seems like we could define jQuery.Event.prototype at 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 define jQuery.event.fixHooks at 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

This comment has been minimized.

Show comment
Hide comment
@jbedard

jbedard May 20, 2015

Contributor

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 makes e.hasOwnProperty('which') false though, didn't think of that before...

See jbedard@fd61774 for a POC from #1746. This commit handled the fixHooks issue you mentioned @dmethvin.

Contributor

jbedard commented 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 makes e.hasOwnProperty('which') false though, didn't think of that before...

See jbedard@fd61774 for a POC from #1746. This commit handled the fixHooks issue you mentioned @dmethvin.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin May 21, 2015

Member

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 defineProperty is fast enough now.

Member

dmethvin commented 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 defineProperty is fast enough now.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Jan 9, 2018

Member

This is a docs-only action for 3.3, see jquery/api.jquery.com#821 .

The actual removal will occur with gh-3235.

Member

dmethvin commented 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.

@dmethvin dmethvin closed this Jan 9, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Jul 8, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.