Performance: changePage() searches the entire document for :focus elements #1560

Closed
jblas opened this Issue May 4, 2011 · 16 comments

Projects

None yet

9 participants

@jblas
Contributor
jblas commented May 4, 2011

In changePage() I see this:

$( window.document.activeElement || "" ).add( "input:focus, textarea:focus, select:focus" ).blur();

This will cause JQM to search the entire document every time we change pages just to release keyboard focus. Instead, we should probably be using a live("focus") handler and tracking the currently focused element so we don't have to crawl the document.

@jblas jblas was assigned May 4, 2011
@jblas
Contributor
jblas commented May 4, 2011

I'll try to address this as part of the path navigation re-working I'm doing.

@jblas
Contributor
jblas commented May 4, 2011

Related changePage() performance issue:

#1562

@StevenBlack
Contributor

Hey Kin, looking into this, unless I'm missing something window.document.activeElement is determinate. It's a reference to the page's h1 element typically. So this line does no document searching at all.

On the other hand, if you are actually referring to refocus(), which looks to be the other facet of this, then I understand :-)

@jblas
Contributor
jblas commented May 5, 2011

But the add() searches the document doesn't it?

@StevenBlack
Contributor

Doh! Right you are. I was looking at the wrong alternate signatures of .add(). http://api.jquery.com/add/

Nevermind. :-)

@StevenBlack
Contributor

Something worth speed-testing: I bet $(":input:focus").blur() may be streets faster since, internally, the :input is regex-driven.
https://github.com/jquery/sizzle/blob/master/sizzle.js#L665-667

@staabm
Contributor
staabm commented May 5, 2011

jsperf comparison?

@toddparker
Contributor

Is this still a concern? Already fixed?

@jblas
Contributor
jblas commented Aug 8, 2011

We're still searching for :focus elements, so this should stay open.

@jaspermdegroot
Member

See also #1855

@jaspermdegroot
Member

Related to #4340

@ldeluca
Contributor
ldeluca commented Oct 21, 2014

Hey @jaspermdegroot @jblas This issue is super old (has it really been 3 years since 2011?!?) Can you confirm whether this is still an issue or if it has been fixed with new releases of JQM? Thanks!

@Ruffio
Ruffio commented Dec 30, 2014

@ldeluca Even though this is super old, the issue/performance loss is still valid. Seaching jquery.mobile-1.4.5.js line 5573 reveals that this is still an issue:

// Kill the keyboard.
// XXX_jblas: We need to stop crawling the entire document to kill focus.
// Instead, we should be tracking focus with a delegate()
// handler so we already have the element in hand at this
// point.

@gabrielschulhof gabrielschulhof added this to the 1.5.0 milestone Jan 14, 2015
@gabrielschulhof
Contributor
  1. Why do we need to kill focus? To get rid of the virtual keyboard?
  2. How often do we end up using the selector instead of document.activeElement?
  3. Does it really slow down our supported browsers significantly?

If we end up keeping the selector we need to change it to this.document.find( "input:focus, textarea:focus, select:focus" ).blur(). We also need to change document.activeElement to this.document[ 0 ].activeElement.

@arschmitz Could you please weigh in? Here's the code in question.

@arschmitz
Member

We discussed this today it does not look like there is any case where this will actually happen. so lets go ahead and remove this, and clean it up.

@gabrielschulhof gabrielschulhof added a commit to gabrielschulhof/jquery-mobile that referenced this issue Jan 21, 2015
@gabrielschulhof gabrielschulhof Pagecontainer: Remove unnecessary $( ":focus" ) fallback on page change
Fixes gh-1560
824b59c
@gabrielschulhof gabrielschulhof added a commit to gabrielschulhof/jquery-mobile that referenced this issue Jan 21, 2015
@gabrielschulhof gabrielschulhof Pagecontainer: Remove unnecessary $( ":focus" ) fallback on page change
Fixes gh-1560
fe787b5
@gabrielschulhof gabrielschulhof added a commit to gabrielschulhof/jquery-mobile that referenced this issue Jun 15, 2015
@gabrielschulhof gabrielschulhof Pagecontainer: Remove unnecessary $( ":focus" ) fallback on page change
Fixes gh-1560
5178395
@gabrielschulhof gabrielschulhof added a commit to gabrielschulhof/jquery-mobile that referenced this issue Jun 15, 2015
@gabrielschulhof gabrielschulhof Pagecontainer: Remove unnecessary $( ":focus" ) fallback on page change
Fixes gh-1560
25ba854
@gabrielschulhof gabrielschulhof added a commit to gabrielschulhof/jquery-mobile that referenced this issue Jun 15, 2015
@gabrielschulhof gabrielschulhof Pagecontainer: Remove unnecessary $( ":focus" ) fallback on page change
Fixes gh-1560
d10fd0e
@arschmitz
Member

This is fixed on master already

@arschmitz arschmitz closed this Jun 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment