Ticket #6942 (closed enhancement: fixed)
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.
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
(23.2 KB) -
added by scott_h 3 years ago.
- dynaTrace PurePaths tab showing poor performance on a keydown event
Change History
Changed 3 years ago by scott_h
-
attachment
jQuery.event.fix-dynaTrace.png
added
comment:1 Changed 3 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 3 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 3 years ago by snover
- Milestone changed from 1.4.4 to 1.5
Retarget all enhancements/features to next major version.
comment:6 Changed 2 years ago by john
- Keywords performance,reflow,1.7-discuss added; performance reflow removed
Nominating ticket for 1.7 discussion.
comment:7 Changed 2 years ago by rwaldron
- Description modified (diff)
+1, Seems like a bug, should be fixed
comment:9 Changed 2 years ago by ajpiano
- Description modified (diff)
+1, Ew, unnecessary reflows in IE is like kicking somone when they're down... fix!
comment:11 Changed 2 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:13 Changed 2 years ago by scott.gonzalez
+1
comment:14 Changed 2 years ago by addyosmani
+1
comment:15 Changed 2 years ago by jzaefferer
+1
comment:16 Changed 23 months ago by dmethvin
- Description modified (diff)
- Milestone changed from 1.next to 1.7
comment:18 Changed 22 months ago by dmethvin
- Owner set to dmethvin
- Status changed from open to assigned
comment:19 Changed 22 months 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 20 months ago by timmywil
@rwaldron: can we add this to event property hooks?
comment:21 Changed 20 months ago by dmethvin
- Status changed from assigned to closed
- Resolution set to fixed
Fixed along with #8789 refactor.
Please follow the bug reporting guidlines and use jsFiddle when providing test cases and demonstrations instead of pasting the code in the ticket.
dynaTrace PurePaths tab showing poor performance on a keydown event