Gallery page performance improvements

[sgrebnov]

No description provided.

[johnbender]

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]

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]

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

@toddparker, thoughts?

[jblas]

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

[toddparker]

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

[sgrebnov]

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

[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]

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

[johnbender]

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

[sgrebnov]

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]

@johnbender, the improvement mentioned above has been done

[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]

Interesting. The mini sliders and flip switches aren't mini anymore. Can't tell if this is the cause or if Gabriel's button stuff is it. Bitt were in play yesterday.

.................................. . . . .
Todd Parker
Partner, Filament Group Inc.
102 South Street #3 Boston, MA 02111
todd@filamentgroup.com // 617.953.1617

On Feb 15, 2012, at 6:30 AM, "Sergei Grebnov" reply@reply.github.com wrote:

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),

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

   // 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));

   // Shouldn’t it be attached to ‘handle’ instead of ‘slider’?
   $(sliderImg).prependTo(slider);
}

--- 
Reply to this email directly or view it on GitHub:
https://github.com/jquery/jquery-mobile/pull/3289#issuecomment-3978754