Skip to content
This repository has been archived by the owner on Oct 8, 2021. It is now read-only.

Bugfix for #3288 #3290

Closed
wants to merge 5 commits into from
Closed

Conversation

sgrebnov
Copy link
Contributor

No description provided.


if(!hasList){
footer.last().addClass( "ui-corner-bottom" );
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I like the fact that we are searching for a listview in a dialog. If this is an optimization for a custom select menu, then perhaps a constructor config option is in order, for example $("#foo").dialog({ addCornerBottom: false }).

Also, I get what you are caching in the footer var added, but for someone new reading that code, I'm not so sure it's obvious because the collection context changes quite a bit in that chain. ( dialog > header > dialog > content + footer). If we need to cache things, I'm wondering if it will be easier to read if we broke the chain into 2 parts, followed by the config check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To follow on to what Kin is saying, this sort of "lite" coupling between widgets is something we suffer from everywhere in the library. If we want this to be a general purpose configuration for performance reasons (see kin's example) it's fine to put it in the dialog but otherwise if it's widget specific (in this case listview specific) we should find a way to move this to the listview itself.

@sgrebnov
Copy link
Contributor Author

All comments are clear and make sens for me, will try to apply/follow them.

I have some question regarding addCornerBottom parameter.

Q1. We can specify the page to be opened as dialog using data-rel="dialog" attribute:
< a href="somePage.html" data-rel="dialog" >Open page in dialog< /a >

So I think we should have the same possibility to somehow set addCornerBottom attribute in markup and then check it in dialog _create() function. I found data-close-btn-text attribute in jQM documentation, that should act in a similar way (to use as an example), but it doesn’t work for me. I found the topic about it on jQuery forum, so it seems it is a bug - http://forum.jquery.com/topic/use-of-data-close-btn-text.
Do you think this is a good idea? Could you please share some working example of custom attribute?

Q2. When custom selectmenu is opened we call page() function, not dialog(). dialog() is called after pagecreate event fired. So we don’t have a direct access to dialog control. We need the way to pass addCornerBottom to dialog through the page control.

 //Create page in custom selectmenu open() func
      self.menuPage.appendTo( $.mobile.pageContainer ).page();

 //dialog is created after page was created. We don’t know if we need to disable corners.

$( $.mobile.dialog.prototype.options.initSelector ).live( "pagecreate", function(){
$( this ).dialog();
});
Do you have any recommendations how better to solve this problem?

@johnbender
Copy link
Contributor

@sgrebnov

Q1

I agree it would be nice to have a data-* attribute for the option but putting it on the link itself seems odd in the same way that designating the page loaded should be a dialog is odd. In the case of data-rel=dialog the convenience is such a massive win that any weirdness and difficulty in implementation is forgotten. Otherwise we might end up with a bunch of dialog specific data attributes on the link.

I think we should make it a page/dialog data attribute option. The only people we're pleasing by putting the data attribute on the link is the intersection of the set of people who don't know whether the linked page should be a dialog or a page at design time and the set of people who want to alter the default bottom corner behavior (small set of folks imo).

@toddparker can/should this be off by default?

Q2

The data attributes should be available in the this.options object in the dialog widget. If we go the above route that should be a non-issue.

@toddparker
Copy link
Contributor

So the key issue is that the bottom corners of any sort of dialog is horribly slow only on WP7, right? If so, I'd rather work around this internally by simply dropping bottom corners automatically in that platform if we can do it cleanly. Adding complexity to the API doesn't seem to make sense if the main use case if this is just to workaround IE. Most people won't ad this attr anyway, so perf will still be slow. I'd rather this "just work"

@jblas
Copy link
Contributor

jblas commented Dec 20, 2011

I guess what I'm curious about is if this is a general problem, even on WP7.5, or is it just dialogs with listviews? I'd hate to turn off the rounded corners for ALL dialogs if it's just long listviews in a custom-select that is the problem.

@johnbender
Copy link
Contributor

@sgrebnov

It looks like you were able to trace the event times in your test page and @toddparker and I were wondering if we could set up a stats page so we can see if the benefits are big on other platforms too. That way we can decide if we want this to be an IE only enhancement or xplatform.

@Wilto
Copy link
Contributor

Wilto commented Dec 20, 2011

It might be worth having a look at the dialog-refactor branch to see if this rounded corners/performance issue persists. I gotta assume it has something to do with how we’re applying the rounded corner class, rather than the rendering of the corners themselves—I know I changed that up a little.

@toddparker
Copy link
Contributor

Agree that this should be tested against the re-factor. Think it might be resolved, and if not, we can look at selector use to speed this up.

@sgrebnov
Copy link
Contributor Author

Just to clarify the issue:

  1. We have a perf problem with the bottom round corners only in WP7. But there is a strange thing in other platforms: then I open custom selectmenu (not dialog with listview) in iphone or android I don’t see round corners in the bottom! They are only in IE(both mobile and PC) and FF. Dialog with listview/other controls has round corners in all platforms.
  2. There is a lag during opening and scrolling for custom selectmenu and dialog with listview. Dialog with other controls works like normal page without any lags in all platforms.

So please give me the direction for further actions:
a) We should try to apply Kin and John’s approach and add addCornerBottom attribute in the page/dialog options.
b) Need to investigate if we can move setting corners in other place or apply it in a different way.
c) Try to disable dialog corners inside listview since it seems that this control causes the problem.
d) Find the way to disable corners only for WP7.
e) Spend more time investigating why only listview causes the problem (I already spent some time on this but have not found the reason)

@jblas
Copy link
Contributor

jblas commented Dec 21, 2011

Regarding #1, some browsers don't clip the content inside of an element to it's border-radius, so if content inside a rounded element specifies a background, the corners of the content render over the rounded corners of the element. I believe this is why things like the custom-select pop-up (used when items fit completely within the viewport) use padding on the element to make sure the rounded corners show. In this particular case, the .ui-content container that is the parent of the listview specifies a 15px padding all around, but the non-inset listview by default specifies a -15px margin which basically negates the .ui-content's padding. We need to consult @toddparker, @Wilto, and/or @scottjehl as to what the proper thing to do here is? Live with it? Add padding? I noticed that a listview inside custom-select dialog actually gets a unique class placed on it ui-selectmenu-list, but didn't see any rules defined for it.

Regarding #2, it would be nice if we could figure out what exactly is causing the slow down, and what contributes to it? Is it the number of items in the list? Is it a specific style that's in use? What are you using as your test case for this? When you mention scroll lag, are you talking about when your finger is down and you are dragging things around? Or is it after you drag and lift your finger to scroll with momentum? I ask because I'm wondering if the lag is due to a redraw of the hover/down state on the list item you touched to initiate the drag?

Regarding #a, I think this should be done regardless. We don't want to introduce interdependencies between or knowledge of other components if we can help it.

Regarding #b, not sure what you meant by a "different place", but if there is another way to apply it that doesn't introduce this lag I'd be willing to hear about it.

Regarding #c, we don't want to do this. The rounded corners are a property of an ancestor of the listview so we don't want the listview dictating to it's ancestor containers/components how they should be styled.

Regarding #d we should do this if there is no solution/workaround. I also see this related to #1 ... if in the custom-select dialog case we see square buttons everywhere except WP7 and FF, then maybe we should disable it for all custom-select dialogs ... this is where the option in #a comes into play. It would be passed to the dialog() constructor in the custom-select code.

Regarding #e, yes it would be nice to know the "why" for listviews. Once again, is it because of the number of elements involved? Or is it because of some style that is used on EVERY listitem?

@sgrebnov
Copy link
Contributor Author

Kin, thank you for very detailed explanation. Today I performed the following experiment, most interesting is Part3.

Part1. I've opened custom selectmenu with many elements in IE9 PC (http://m.akvelon.net:8182/55a/ ). It seems to work fast, BUT if you run Developers Tools (F12) and try to select any element you observe many lags. So performance problem exists in PC too which is actually great because you can use dev tools for investigation.

Part2. I disabled rounded corners for the 'ui-corner-bottom' class (main div, see below) and observe that lags gone away. This proves that removing bottom corners can resolve this issue. Below is html layout structure.

< div class="ui-overlay-shadow ui-corner-bottom ui-content ui-body-c" role="main" data-role="content">
< ul..>

< li class="ui-btn ui-btn-icon-right ui-li ui-btn-up-c" role="option" tabIndex="-1" aria-selected="false" data-theme="c" data-icon="false" data-option-index="43">
< div aria-hidden="true" class="ui-btn-inner ui-li">
< div class="ui-btn-text">
< a class="ui-link-inherit" href="#">Texas</ a>
/ div>
</ div>

Part3. I was experimenting with styles and found out that removing 'position: relative' style for list elements (li and all internal divs) resolves performance problem. In this case dialog and all elements are rendered correctly and I don't see any lags. I could guess that it hard for IE to calculate rounded-corners position, that is why there is no same issue with rounded corners on dialog top.

I'm not sure about layout bugs if we remove positioning properties, but it seems listview is rendered exactly the same. Will continue my research of this problem in this direction...

@sgrebnov
Copy link
Contributor Author

Posted alternative solution via positioning as pull request below
#3342

@gseguin gseguin closed this Jan 19, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants