Bug Tracker

Ticket #6942 (closed enhancement: fixed)

Opened 4 years ago

Last modified 3 years ago

JQuery.event.fix causes unnecessary reflows in IE when handling key events

Reported by: scott_h Owned by: dmethvin
Priority: blocker Milestone: 1.7
Component: event Version: 1.4.2
Keywords: performance,reflow,1.7-discuss Cc:
Blocking: Blocked by:

Description (last modified by dmethvin) (diff)

When normalising the event object, jQuery adds pageX/pageY properties in IE based on a combination of event.clientX/Y and document.body.scrollLeft/Top.

 More Information

Accessing scrollLeft/Top on the body triggers a full-page reflow. On a large DOM, this makes jQuery's key* event handlers unnecessarily slow in IE7 and IE8, since pageX/Y are useless for key events (and so could be defined without triggering reflows).

Interestingly, I discovered that IE defines clientX/Y on key* events to be the current mouse position, whereas in Safari/Firefox they (along with pageX/Y) are both 0. This inconsistency makes me sure that no one uses jQuery's pageX/Y on key events.

Attachments

jQuery.event.fix-dynaTrace.png Download (23.2 KB) - added by scott_h 4 years ago.
dynaTrace PurePaths tab showing poor performance on a keydown event

Change History

Changed 4 years ago by scott_h

dynaTrace PurePaths tab showing poor performance on a keydown event

comment:1 Changed 4 years ago by scott_h

Here's the fix I propose for this bug:

 http://github.com/shghs/jquery/commit/b59718e2dab80a8a92d7a02a427e60e1da06bd20

I've verified that the test suite passes in Safari 5.0.1 (Mac), Firefox 3.6.8 (Mac), IE7 (WinXP) and IE8 (WinXP) with this change.

comment:2 Changed 4 years ago by snover

  • Priority set to low
  • Status changed from new to open
  • Type changed from bug to enhancement
  • Milestone changed from 1.4.3 to 1.next

I like this. Please make a  pull request for your patch if it is still unresolved in 1.4.3. If it is resolved, please let us know so we can close this ticket.

comment:3 Changed 4 years ago by snover

  • Milestone changed from 1.4.4 to 1.5

Retarget all enhancements/features to next major version.

comment:4 Changed 3 years ago by john

  • Milestone set to 1.next

Let's look at this for 1.7.

comment:5 Changed 3 years ago by john

  • Description modified (diff)

comment:6 Changed 3 years ago by john

  • Keywords performance,reflow,1.7-discuss added; performance reflow removed

Nominating ticket for 1.7 discussion.

comment:7 Changed 3 years ago by rwaldron

  • Description modified (diff)

+1, Seems like a bug, should be fixed

comment:8 Changed 3 years ago by jaubourg

+1, no brainer

comment:9 Changed 3 years ago by ajpiano

  • Description modified (diff)

+1, Ew, unnecessary reflows in IE is like kicking somone when they're down... fix!

comment:10 Changed 3 years ago by timmywil

  • Description modified (diff)

+1, ouch

comment:11 Changed 3 years ago by paul.irish

  • Description modified (diff)

+1, it'd also be worthwhile to take a hard look at the rest of the event props we are copying over. see if theres any more we can cull

comment:12 Changed 3 years ago by john

  • Description modified (diff)

+1, Let's fix this, absolutely.

comment:13 Changed 3 years ago by scott.gonzalez

+1

comment:14 Changed 3 years ago by addyosmani

+1

comment:15 Changed 3 years ago by jzaefferer

+1

comment:16 Changed 3 years ago by dmethvin

  • Description modified (diff)
  • Milestone changed from 1.next to 1.7

comment:17 Changed 3 years ago by dmethvin

  • Priority changed from low to blocker

comment:18 Changed 3 years ago by dmethvin

  • Owner set to dmethvin
  • Status changed from open to assigned

comment:19 Changed 3 years ago by anonymous

Sorry for deleting the repo linked above. I've issued a pull request with a slightly cleaner fix compared to the original one:

 https://github.com/jquery/jquery/pull/452

but I am skeptical that this is actually a bug. I think this needs to be stress tested on a page with lots of elements.

comment:20 Changed 3 years ago by timmywil

@rwaldron: can we add this to event property hooks?

comment:21 Changed 3 years ago by dmethvin

  • Status changed from assigned to closed
  • Resolution set to fixed

Fixed along with #8789 refactor.

Note: See TracTickets for help on using tickets.