-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Gallery page performance improvements #3289
Changes from 9 commits
59abc6f
83765f0
ca2ada8
9afaf8b
fcbf106
8293af5
9236f20
31b5fe4
a77ed1d
f86eb66
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,7 +19,7 @@ | |
"<div class='ui-title'>" + label.getEncodedText() + "</div>"+ | ||
"</div>"+ | ||
"<div data-" + $.mobile.ns + "role='content'></div>"+ | ||
"</div>" ).appendTo( $.mobile.pageContainer ).page(), | ||
"</div>" ), | ||
|
||
listbox = $("<div>", { "class": "ui-selectmenu ui-selectmenu-hidden ui-overlay-shadow ui-corner-all ui-body-" + widget.options.overlayTheme + " " + $.mobile.defaultDialogTransition } ).insertAfter(screen), | ||
|
||
|
@@ -44,9 +44,9 @@ | |
"class": "ui-btn-left" | ||
}).attr( "data-" + $.mobile.ns + "iconpos", "notext" ).attr( "data-" + $.mobile.ns + "icon", "delete" ).appendTo( header ).buttonMarkup(), | ||
|
||
menuPageContent = menuPage.find( ".ui-content" ), | ||
|
||
menuPageClose = menuPage.find( ".ui-header a" ); | ||
menuPageContent, | ||
menuPageClose; | ||
|
||
|
||
$.extend( widget, { | ||
|
@@ -316,6 +316,11 @@ | |
} | ||
|
||
if ( menuHeight > screenHeight - 80 || !$.support.scrollTop ) { | ||
|
||
self.menuPage.appendTo( $.mobile.pageContainer ).page(); | ||
self.menuPageContent = menuPage.find( ".ui-content" ); | ||
self.menuPageClose = menuPage.find( ".ui-header a" ); | ||
|
||
// prevent the parent page from being removed from the DOM, | ||
// otherwise the results of selecting a list item in the dialog | ||
// fall into a black hole | ||
|
@@ -408,49 +413,63 @@ | |
|
||
self.list.empty().filter( ".ui-listview" ).listview( "destroy" ); | ||
|
||
// Populate menu with options from select element | ||
self.select.find( "option" ).each( function( i ) { | ||
var $this = $( this ), | ||
$parent = $this.parent(), | ||
text = $this.getEncodedText(), | ||
anchor = "<a href='#'>"+ text +"</a>", | ||
classes = [], | ||
extraAttrs = []; | ||
|
||
// Are we inside an optgroup? | ||
if ( $parent.is( "optgroup" ) ) { | ||
var optLabel = $parent.attr( "label" ); | ||
|
||
// has this optgroup already been built yet? | ||
if ( $.inArray( optLabel, optgroups ) === -1 ) { | ||
lis.push( "<li data-" + $.mobile.ns + "role='list-divider'>"+ optLabel +"</li>" ); | ||
optgroups.push( optLabel ); | ||
var $options = self.select.find("option"), | ||
numOptions = $options.length, | ||
select = this.select[ 0 ], | ||
dataPrefix = 'data-' + $.mobile.ns, | ||
dataIndexAttr = dataPrefix + 'option-index', | ||
dataIconAttr = dataPrefix + 'icon', | ||
dataRoleAttr = dataPrefix + 'role', | ||
fragment = document.createDocumentFragment(), | ||
optGroup; | ||
|
||
for (var i = 0; i < numOptions;i++){ | ||
var option = $options[i], | ||
$option = $(option), | ||
parent = option.parentNode, | ||
text = $option.text(), | ||
anchor = document.createElement('a'); | ||
classes = []; | ||
|
||
anchor.setAttribute('href','#'); | ||
anchor.appendChild(document.createTextNode(text)); | ||
|
||
// Are we inside an optgroup? | ||
if (parent !== select && parent.nodeName.toLowerCase() === "optgroup"){ | ||
var optLabel = parent.getAttribute('label'); | ||
if ( optLabel != optGroup) { | ||
var divider = document.createElement('li'); | ||
divider.setAttribute(dataRoleAttr,'list-divider'); | ||
divider.setAttribute('role','option'); | ||
divider.setAttribute('tabindex','-1'); | ||
divider.appendChild(document.createTextNode(optLabel)); | ||
fragment.appendChild(divider); | ||
optGroup = optLabel; | ||
} | ||
} | ||
|
||
// Find placeholder text | ||
// TODO: Are you sure you want to use getAttribute? ^RW | ||
if ( !this.getAttribute( "value" ) || text.length == 0 || $this.jqmData( "placeholder" ) ) { | ||
} | ||
|
||
if (!placeholder && (!option.getAttribute( "value" ) || text.length == 0 || $option.jqmData( "placeholder" )) ) { | ||
if ( o.hidePlaceholderMenuItems ) { | ||
classes.push( "ui-selectmenu-placeholder" ); | ||
} | ||
placeholder = self.placeholder = text; | ||
} | ||
placeholder = self.placeholder = text; | ||
} | ||
|
||
// support disabled option tags | ||
if ( this.disabled ) { | ||
var item = document.createElement('li'); | ||
if ( option.disabled ) { | ||
classes.push( "ui-disabled" ); | ||
extraAttrs.push( "aria-disabled='true'" ); | ||
item.setAttribute('aria-disabled',true); | ||
} | ||
|
||
lis.push( "<li data-" + $.mobile.ns + "option-index='" + i + "' data-" + $.mobile.ns + "icon='"+ dataIcon +"' class='"+ classes.join(" ") + "' " + extraAttrs.join(" ") +">"+ anchor +"</li>" ); | ||
}); | ||
|
||
self.list.html( lis.join(" ") ); | ||
|
||
self.list.find( "li" ) | ||
.attr({ "role": "option", "tabindex": "-1" }) | ||
.first().attr( "tabindex", "0" ); | ||
item.setAttribute(dataIndexAttr,i); | ||
item.setAttribute(dataIconAttr,dataIcon); | ||
item.setAttribute('class',classes.join(" ")); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't recall but will this cause an issue in ie7? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: I'm like | | close to merging this into master. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @johnbender, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @johnbender, the improvement mentioned above has been done |
||
item.setAttribute('role','option'); | ||
item.setAttribute('tabindex','-1'); | ||
item.appendChild(anchor); | ||
fragment.appendChild(item); | ||
} | ||
fragment.firstChild.setAttribute('tabindex',0); | ||
self.list[0].appendChild(fragment); | ||
|
||
// Hide header close link for single selects | ||
if ( !this.isMultiple ) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
<!DOCTYPE html> | ||
<html> | ||
<head> | ||
<meta charset="utf-8"> | ||
<meta name="viewport" content="width=device-width, initial-scale=1"> | ||
<title>jQuery Mobile Docs - Checkboxradio</title> | ||
<link rel="stylesheet" href="../../css/themes/default/" /> | ||
<script src="../../js/jquery.js"></script> | ||
<script src="../../js/"></script> | ||
</head> | ||
<body> | ||
|
||
<div data-role="page"> | ||
|
||
<div data-role="header"> | ||
<h1>Checkboxradio</h1> | ||
</div><!-- /header --> | ||
|
||
<div data-role="content" data-filter="true"> | ||
|
||
<input type="checkbox" name="checkbox1" id="checkbox1" /> | ||
<label for="checkbox1">Checkbox</label> | ||
|
||
<input type="checkbox" name="checkbox2" id="checkbox2" /> | ||
<label for="checkbox2">Checkbox</label> | ||
|
||
<input type="checkbox" name="checkbox3" id="checkbox3" /> | ||
<label for="checkbox3">Checkbox</label> | ||
|
||
<input type="checkbox" name="checkbox4" id="checkbox4" /> | ||
<label for="checkbox4">Checkbox</label> | ||
|
||
<input type="checkbox" name="checkbox5" id="checkbox5" /> | ||
<label for="checkbox5">Checkbox</label> | ||
|
||
<input type="checkbox" name="checkbox6" id="checkbox6" /> | ||
<label for="checkbox6">Checkbox</label> | ||
|
||
<input type="checkbox" name="checkbox7" id="checkbox7" /> | ||
<label for="checkbox7">Checkbox</label> | ||
|
||
<input type="checkbox" name="checkbox8" id="checkbox8" /> | ||
<label for="checkbox8">Checkbox</label> | ||
|
||
<input type="checkbox" name="checkbox9" id="checkbox9" /> | ||
<label for="checkbox9">Checkbox</label> | ||
|
||
<input type="checkbox" name="checkbox10" id="checkbox10" /> | ||
<label for="checkbox10">Checkbox</label> | ||
|
||
</div><!-- /content --> | ||
</div><!-- /page --> | ||
|
||
</body> | ||
</html> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
<!DOCTYPE html> | ||
<html> | ||
<head> | ||
<meta charset="utf-8"> | ||
<meta name="viewport" content="width=device-width, initial-scale=1"> | ||
<title>jQuery Mobile Docs - Checkboxradio</title> | ||
<link rel="stylesheet" href="../../css/themes/default/" /> | ||
<script src="../../js/jquery.js"></script> | ||
<script type="text/javascript" src="stats/perf.js"></script> | ||
<script type="text/javascript" src="stats/startup.js"></script> | ||
<script src="../../js/"></script> | ||
</head> | ||
<body> | ||
|
||
<div data-role="page" id="perf-test-page"> | ||
|
||
<div data-role="header"> | ||
<h1>Checkboxradio</h1> | ||
</div><!-- /header --> | ||
|
||
<div data-role="content" data-filter="true"> | ||
|
||
<input type="checkbox" name="checkbox1" id="checkbox1" /> | ||
<label for="checkbox1">Checkbox</label> | ||
|
||
<input type="checkbox" name="checkbox2" id="checkbox2" /> | ||
<label for="checkbox2">Checkbox</label> | ||
|
||
<input type="checkbox" name="checkbox3" id="checkbox3" /> | ||
<label for="checkbox3">Checkbox</label> | ||
|
||
<input type="checkbox" name="checkbox4" id="checkbox4" /> | ||
<label for="checkbox4">Checkbox</label> | ||
|
||
<input type="checkbox" name="checkbox5" id="checkbox5" /> | ||
<label for="checkbox5">Checkbox</label> | ||
|
||
<input type="checkbox" name="checkbox6" id="checkbox6" /> | ||
<label for="checkbox6">Checkbox</label> | ||
|
||
<input type="checkbox" name="checkbox7" id="checkbox7" /> | ||
<label for="checkbox7">Checkbox</label> | ||
|
||
<input type="checkbox" name="checkbox8" id="checkbox8" /> | ||
<label for="checkbox8">Checkbox</label> | ||
|
||
<input type="checkbox" name="checkbox9" id="checkbox9" /> | ||
<label for="checkbox9">Checkbox</label> | ||
|
||
<input type="checkbox" name="checkbox10" id="checkbox10" /> | ||
<label for="checkbox10">Checkbox</label> | ||
|
||
</div><!-- /content --> | ||
</div><!-- /page --> | ||
|
||
</body> | ||
</html> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there ever a case where the parent might be a select node and also have a
nodeName
of optgroup? Or is this just to short circuit quickly before a string comparison where possible?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We iterate only through elements in the loop and their parent might be < select > or < optgroup >. nodeName is a read only property that always returns tag name for a nodes so we have ‘select’ or ‘optgroup’.
Thus basically we can just perform 'parent !== select' comparison which should run faster. The second part is for the possible future changes. I will double check the performance improvement using this statement and add additional comment to code.