Skip to content

Gallery page performance improvements #3289

Merged
merged 10 commits into from Feb 14, 2012

4 participants

@sgrebnov

No description provided.

@johnbender johnbender and 3 others commented on an outdated diff Dec 19, 2011
js/jquery.mobile.forms.checkboxradio.js
@@ -51,8 +51,11 @@ $.widget( "mobile.checkboxradio", $.mobile.widget, {
});
// Wrap the input + label in a div
- input.add( label )
- .wrapAll( "<div class='ui-" + inputtype + "'></div>" );
+ var wrapper = document.createElement('div');
+ wrapper.setAttribute('class','ui-'+inputtype);
@johnbender
johnbender added a note Dec 19, 2011

According to quirksmode the setAttribute implementation in ie7 is incomplete (in their test setting the style attribute fails). Have you seen any weirdness in ie7 or wp7? In general I'm leery of stepping outside the jQuery Core abstraction.

@sgrebnov
sgrebnov added a note Dec 21, 2011

setAttribute works well in WP7 (tested on HTC HD7). No weirdness were found. Also I checked it in IE9 using IE7 compatibility mode. In this mode setAttribute really doesn’t work, but there are a lot of another broken things so I am not sure if IE7 is important for us. The reason I use setAttribute is Kin’s recommendation to use DOM functions instead of jQuery parent(), attrt(), etc.

@johnbender
johnbender added a note Jan 11, 2012

We have ie 7 as A grade on our browser support page.

@toddparker, thoughts?

@jblas
jblas added a note Jan 11, 2012

I think in this particular case, we can use wrapper.className = 'ui-' + inputtype if setAttribute() causes problems.

@toddparker
toddparker added a note Jan 11, 2012

Last time I tested, IE7 worked ok and is fairly important because WP7 is based on that version of IE.

@sgrebnov
sgrebnov added a note Jan 18, 2012

@toddparker, @johnbender, I 've replaced setAttribute with wrapper.className statement. Could you please take a look? - last commit in this pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@johnbender johnbender and 1 other commented on an outdated diff Dec 19, 2011
js/jquery.mobile.forms.select.custom.js
}
+ }
+
+ if (!placeholder){
+ if ( !option.getAttribute( "value" ) || text.length == 0 || $option.jqmData( "placeholder" ) ) {
@johnbender
johnbender added a note Dec 19, 2011

These two guards can be consolidated with an &&

if (!placeholder && (!option.getAttribute( "value" ) || text.length == 0 || $option.jqmData( "placeholder" )) ) {
@sgrebnov
sgrebnov added a note Dec 21, 2011

Thank you, will replace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@johnbender johnbender commented on the diff Dec 19, 2011
js/jquery.mobile.forms.select.custom.js
+ 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"){
@johnbender
johnbender added a note Dec 19, 2011

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?

@sgrebnov
sgrebnov added a note Dec 21, 2011

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@johnbender

Aside from the questions posted inline with the source I was wondering if you checked into the test suite for coverage of the areas that you've changed. I know the slider test coverage is mediocre and could use a bit of love. Right now the stability of many modules is riding on the strength of the "code freeze" running up to 1.0 and a lot of end user testing. That won't work in the face of an alteration like this.

Test suggestions

  1. Custom select: optgroup elements end up as headers
  2. Custom select: placeholders receive the right class
  3. Slider: markup structure matches some predefined expectation

I also wonder what others think about reverting to the DOM api here. It's clearly good for performance but there are drawbacks in terms of readability and ease of contribution.

Finally, it would be HUGE if we could add perf testing pages for each of the widgets that you've altered, preferably before merging this change, so that we can track the difference here and on other platforms. You can find a sample at tests/speed/lists-ul-record.php which references tests/speed/stats/perf.js and tests/speed/stats/startup.js. perf.js is just a collection of helpers for recording stats, and startup.js is an example for recording them.

Other than the aforementioned I think this looks really great and I look forward to merging it in!

@sgrebnov

I have started running unit tests. All the checkboxradio tests are passed, but many slider and selectmenu tests failed for both original and modified version. Investigating this...
Also I have started working on perf pages. John, please confirm that I should only create three pages for each improved control – checkboxradio, slider and custom selectmenu and then add them to ’tests/speed’ folder on github.

@sgrebnov

Hi John, still unable to pass the following tests:
1. slider control (both original and modified, all platforms).
"refresh should force val to nearest step"
"empty string value results defaults to slider min value"
"flip toggle switch title should be current selected value attr"
2. custom selectmenu (tests fail on iphone and android, original and modified):
"adding options and refreshing a custom select changes the options list"
"menupage is removed when the parent page is removed"
"dialog size select title should match the label when changed after the dialog markup is added to the DOM"
"dialog sized select shouldn't rebind its parent page remove handler when closing, if the parent page domCache option is true"

Could you please also try to run the tests for the current (original) version - probably I run them incorrectly or there is some issue with my copy of source code.

@sgrebnov

Created performance pages for improved controls. I was able to check that results are saved in sqlite database and they are in correct form (I used sqlite manager). I don't know why but results aren’t shown in stats/visualize pages (same behavior for already existing list-view test page, so I believe the tests are correct, but there is some misconfiguration in my side.

@sgrebnov

I also found the reason why some slider tests failed - I used wrong way of plugging tests files. Now all the slider tests are passed, but custom selectmenu tests still fail on mobile platforms (both original and improved version).

@sgrebnov

Now original and modified version of code pass/fail same tests.

@johnbender johnbender and 1 other commented on an outdated diff Jan 24, 2012
js/jquery.mobile.forms.select.custom.js
}
-
- 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(" "));
@johnbender
johnbender added a note Jan 24, 2012

I can't recall but will this cause an issue in ie7?

@johnbender
johnbender added a note Jan 24, 2012

Note: I'm like | | close to merging this into master.

@sgrebnov
sgrebnov added a note Jan 24, 2012

Hi @johnbender,
According to this information - http://www.quirksmode.org/dom/w3c_core.html setAttribute should work fine in IE7 except some cases when we set styles and events. I checked slider and custom selectmenu in IE9 under IE7 compatibility mode and can confirm that all attributes are applied correctly. So I will replace setting class attribute with className property like we decided for checkboxradio (will add this as part of this pull request).

@sgrebnov
sgrebnov added a note Jan 25, 2012

@johnbender, the improvement mentioned above has been done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@johnbender johnbender merged commit f86eb66 into jquery:master Feb 14, 2012
@johnbender

@sgrebnov

Merged! I moved the perf additions over to the website repo (which is private) since we made some changes to how jquerymobile.com/test is handled. All the tests appear to be passing but there was a nasty merge conflict so it might be work going though at a couple spots:

https://github.com/jquery/jquery-mobile/blob/master/js/jquery.mobile.forms.slider.js#L58
https://github.com/jquery/jquery-mobile/blob/master/js/jquery.mobile.forms.slider.js#L115

And the merge commit 480cdea#diff-2

Thanks again for all the hard work and hopefully I didn't mess anything up during the merge!

@sgrebnov

Hi @johnbender,
The merged commit looks correct, but when I look on slider.js in master branch I see the following issues
.
#1. sliderLabel is initialized but the code that adds it to slider was removed. If it is not necessary now I think we can just remove variable. In other case it have to be added to control, I suppose.

#2. sliderImg is now attached to ‘slider’ instead of ‘handle’.

for (var i = 0, optionsCount = options.length; i < optionsCount; i++) {
    var side = !i ? "b" : "a",
         sliderTheme = !i ? " ui-btn-down-" + trackTheme : (" " + $.mobile.activeBtnClass),

     // #1 sliderLabel isn’t used now
     sliderLabel = document.createElement('div'),
     sliderImg = document.createElement('span');

    // #1 The Next two lines are absent now    
    sliderLabel.className = ['ui-slider-labelbg ui-slider-labelbg-', side, theme, " ui-btn-corner-", corners].join("");
    $(sliderLabel).prependTo(slider);           

    sliderImg.className = ['ui-slider-label ui-slider-label-', side, sliderTheme, " ui-btn-corner-all"].join("");
    sliderImg.setAttribute('role', 'img');
    sliderImg.appendChild(document.createTextNode(options[i].innerHTML));

    // #2 Shouldn’t it be attached to ‘handle’ instead of ‘slider’?
    $(sliderImg).prependTo(slider);
}
@toddparker
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.