Suggestion: Fix for fixed persistent toolbar padding issues #4260

Merged
merged 3 commits into from Jun 15, 2012

Conversation

Projects
None yet
9 participants
Contributor

MauriceG commented May 1, 2012

Tested with Safari 5.0.5 on Windows, 5.1.5 on MacOS, iOS 5.1 on iPhone4 + iPad3, Chrome 15.0 + 18.0 on Win, FF 9.0.1 + 12.0 on Win, Opera 11.52 on Win, IE7, Android 2.2.1 on Archos 70 tablet

May it's superfluous here to outline how the fixed toolbar bindings behave, but it shows why the current code cannot work if the updatePagePadding option is true and the header and/or footer are higher than a standard toolbar.

Note: The updatePagePadding() function always searches its closest .ui-page container to calculate the necessary page-padding.

When showing the initial page, the pageshow event calls updatePagePadding() and everything works fine.

When a page navigation occurs the following happens in this order (please correct me if I'm wrong):

pagebeforehide searches the fixed toolbar/s of page-to and moves them to $.mobile.pageContainer. Then another pageshow event is bound to the page-to, which will move the toolbar/s to that to-page.

The bound animationstart event fires on the page-from and calculates the page-padding of the page that will be left now...

The bound animationstart event fires on the page-to and calls updatePagePadding() to set the padding. But, page-to has no toolbars at this moment. They're still in the $.mobile.pageContainer. The paddng could not be calculated.

The bound pageshow event fires on the page-to and do the same again. Still no toolbars in the page.

The second pageshow event (bound at pagebeforehide in one of the steps above) fires and moves the toolbar/s from the page-container to the page-to. Too late.

The next navigation will set the proper padding to the page cause of the out-transition. When this page is shown again, the padding will be ok.

I hope my deliberations were not too boring.

Addresses #4219 + may #4259

Contributor

MauriceG commented May 1, 2012

I forgot to say, the tests ran fine but may cause of #4234.

edit: corrected tests complete successfully

Awesome thanks! I'll give it a try :)

@gseguin gseguin commented on the diff May 2, 2012

js/jquery.mobile.fixedToolbar.js
@@ -173,14 +175,14 @@ define( [ "jquery", "./jquery.mobile.widget", "./jquery.mobile.core", "./jquery.
_visible: true,
// This will set the content element's top or bottom padding equal to the toolbar's height
- updatePagePadding: function() {
+ updatePagePadding: function( tbPage ) {
var $el = this.element,
header = $el.is( ".ui-header" );
@gseguin

gseguin May 2, 2012

Member

I would add: tbPage = tbPage || $el.closest( ".ui-page" ) what do you think?

@MauriceG

MauriceG May 3, 2012

Contributor

Hi @gseguin !
Thanks a lot for looking at this!

So any progress here? Y U NO MERGE? :)

What can be done to get progress and a merge/fix as final result? The problem is extremely annoying and so far I've only developed a workaround that needs to manually reinitialize the layout.

Is this merge planned for some kind of milestone?

Contributor

johnbender commented Jun 1, 2012

@franz-josef-kaiser @Belelros

If either of you want to fork from maurices branch, add tests, and submit as a separate pr that would speed things up. Otherwise you'll have to wait until one of us has time to review.

I'll check in with @gseguin

Contributor

johnbender commented Jun 1, 2012

@Belelros

I wouldn't surprised if we got this in before the next patch release.

Belelros commented Jun 5, 2012

@johnbender

I would love to but I don't know where to start. I'll do my best and will try to make a pr. Thanks :)

Contributor

johnbender commented Jun 5, 2012

@Belelros

Since you asked I'm happy to help you get started! Here's what I can think of off the top of my head:

  1. You'll need apache + php setup to run the docs and the tests
  2. test/unit/fixedToolbar is where you'll find the testing files relevant to this pull request. Specifically fixedToolbar.js and index.html
  3. fixedToolbar.js has all the fixed toolbar tests (not too many as yet).
  4. I don't think this will require an async test since you can just run updatePadding manually by getting the fixed toolbar element.

If you get totally lost, want feedback along the way, or just don't feel like putting in the effort that's fine just post back here. Thanks for your interest!

Contributor

johnbender commented Jun 5, 2012

@Belelros

Forgot to mention that once you get your server setup you run the tests by visiting /tests/unit/fixedToolbar/

Belelros commented Jun 5, 2012

@johnbender

Thank you very much, with your help I'm sure I'll find the way. I'm glad to help. All clear but don't know how to download all needed. I've cloned the repo and I see a lot of branches but don't know if it's a branch or other thing. Any help?

Contributor

MauriceG commented Jun 13, 2012

jsfiddle with patched jquery.mobile.fixedToolbar.js: http://jsfiddle.net/MauriceG/jVKyS/

Yaiks @MauriceG ! It's looking good! I'm still stuck on the tests. Any ETA for this fix? Is really annoying for me.

Member

jaspermdegroot commented Jun 13, 2012

@toddparker - See link in comment from @MauriceG above. Can you have it tested? Thanks!

Member

jaspermdegroot commented Jun 14, 2012

@MauriceG - I am confused here. Should the text in that fiddle "the first time this page is shown, the content gets under the header." be ignored, since it loads your patch that fixes it?

Contributor

MauriceG commented Jun 14, 2012

@ugomobi
Yes, it should be ignored. It was the issue which is raised if latest is used.
I've changed to ", without this patch."

Member

jaspermdegroot commented Jun 14, 2012

@MauriceG - Cool! Thanks a lot!

Contributor

toddparker commented Jun 15, 2012

Just tested this fiddle link on an iPad 1 and 3 running iOS 5 and this looks perfect.

On the Iphone (iOS 5):

-The words "One, "Two" and "Three" are too large for the yellow boxes they are in when held portrait orientation, and therefore the word extends outside of the box.
-When held landscape, the yellow box is wide enough to contain the word and looks fine, so it's just an issue of the words not scaling down with the box.
-Navigation between all the pages works fine.

Based on this, this seems good to pull in @ugomobile.Very nice work @MauriceG!

I tested out with the same application I submitted the issue for and the issue still seems to be present. Can I upload a picture here to show you?

issue still exists

This is in response to an email from Jasper de Groot's email:
@balshamali

Can you test your project using this code:

CSS: http://ugomobi.github.com/fixedtoolbars/css_new06132012.css
JS: http://ugomobi.github.com/fixedtoolbars/js_new06132012.js

This includes the changes of PR #4260 from @MauriceG

Please confirm if this solves the issues you were having with updatePagePadding. Thanks!


Reply to this email directly or view it on GitHub:
#4259 (comment)

Member

jaspermdegroot commented Jun 15, 2012

I am going to merge the cmmits and then we look into @balshamali his issue again to see why it still persists in his case.

@MauriceG - Really nice fix! Thanks a lot!!

@jaspermdegroot jaspermdegroot merged commit 2dad4d6 into jquery:master Jun 15, 2012

Member

jaspermdegroot commented Jun 15, 2012

Merged by commit 0992512

Tested on the following:

Kindle Fire 6.2.1
Nook Color Android 2.2
HP TouchPad WebOS 3.0.2
Galaxy Tab Android 3.1
Xoom Android 3.2.1
Ipad IOS 4.3
Droid 2.1, 2.2, 2.3, 4.0
Iphone ios5
HTC WP7
Trophy WP7.5
N9 MeeGo 1.2
Wave 3 bada 2.0

Only issue I ran into was on WP7, when the phone is landscape, the content goes under the header on page 2.
Other than that, they all worked and looked good.

Contributor

MauriceG commented Jun 16, 2012

Hi @toddparker, hi @ugomobi
I'm very pleased, that this got in.
Thank you very much.

Member

jaspermdegroot commented Jun 16, 2012

@zrmccarthy - Thanks for testing!

About WP7. I checked the same test page on IE7 and it happens there too.
IE7/WP7 don't support position fixed and apply default position static. Since they are not on the blacklist the fixedToolbar script gets executed. On IE7 I noticed that class "ui-fixed-hidden" is not replaced by "slidedown" in time.
Might have to do with the way @MauriceG loads the patch with the fix on that test page, because it didn't happen with this test http://ugomobi.github.com/fixedtoolbars/

@toddparker - To be sure we could add IE7/WP7 to the blacklist. Who is good at browser agent detection?

Contributor

MauriceG commented Jun 16, 2012

same fiddle from above using latest: http://jsfiddle.net/MauriceG/jVKyS/14/show/

Member

jaspermdegroot commented Jun 16, 2012

Thanks @MauriceG - The issue is not there anymore on IE7.

@jaspermdegroot jaspermdegroot added a commit that referenced this pull request Jun 16, 2012

@jaspermdegroot jaspermdegroot Revert a change that was included in the updatePagePadding fix (PR #4260
) but was not actual part of this fix. The PR #4228 for this change did not land because we decided not to change the fact updatePagePadding is always called on pageshow.
03613ea

@jaspermdegroot jaspermdegroot added a commit that referenced this pull request Jun 16, 2012

@jaspermdegroot jaspermdegroot Revert a change that was included in the updatePagePadding fix (PR #4260
) but was not actual part of this fix. The PR #4228 for this change did not land because we decided not to change the fact updatePagePadding is always called on pageshow.
24244e1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment