fixedToolbar _destroy() #3880

Closed
gseguin opened this Issue Mar 22, 2012 · 13 comments

Projects

None yet

6 participants

@gseguin
Member
gseguin commented Mar 22, 2012

When using fixed toolbars on a platform that doesn't support them if the user calls the updatePagePadding method a padding is added to the top of the page. It shouldn't.

@scottjehl scottjehl was assigned Mar 22, 2012
@scottjehl
Contributor

Good catch. You're right, but I don't understand why the method would even be defined if called in those cases - I'd expect it to throw an exception, since the fixedtoolbar widget is destroyed on that element after failing the support test (https://github.com/jquery/jquery-mobile/blob/master/js/jquery.mobile.fixedToolbar.js#L79 ).

I guess I would think subsequent calls to updatePagePadding would need to be directed at already-enhanced toolbars, by finding them with $(".ui-fixed-header") and such. But it seems to work on a destroyed widget too...

Maybe I'm misunderstanding the expected behavior of the widget factory here. Thoughts?

@scottjehl
Contributor

Hey @gseguin, any ideas on the above? maybe @scottgonzalez could enlighten us. :)

@gseguin
Member
gseguin commented Mar 27, 2012

@scottjehl, sorry I got side tracked. In the code that discovered this bug I was actually testing if the dom node had fixed toolbar by testing its data and the test was returning true. I switched to test if the ui-fixed-header class.

I was also under the impression that the destroy method would remove the widget from the dom node's data.

@scottjehl
Contributor

hey @gseguin

Interesting, I just tested and found that this problem does not occur in jQuery UI widgets. This means either our widget factory version is missing the required fix, or our extension of the widget factory is messing things up.

In fact, destroyed widgets in jQuery UI return undefined when you try and get the element's $(selector).data("widgetnamehere"), whereas for us, we get the full widget instance.

I'm thinking we're in need of an upgrade, but it's probably not wise to do so for 1.1. Do you agree?

Since this isn't just an issue with fixedtoolbars, but all our widgets, I'm not sure where it's affecting people, or where we'd even attempt to prevent it. If it's already affecting people, they'll probably need to adjust their code because the widget factory update will only bring them into an exception.

For example, currently the method applies in places it shouldn't, but after the upgrade, it'll probably throw the "cannot call methods on xyz prior to initialization" error.

The way around it would be to select the element by its widget class before doing anything with it:

$( ".ui-fixed-header").fixedtoolbar("foo");

So, should we close this out and add a generic ticket for upgrading our widget version?

@scottjehl
Contributor

Changed to 1.2 milestone, for the time being.

@ashaegupta

Hi There,

I believe I'm having an issue relevant, or at least related to this discussion on my Chrome browser.

With:

   <div data-role="header" data-position="fixed">

My header is pushed further down the page:
http://screencast.com/t/iHmL5VmM

When:

   <div data-role="header" data-position="inline">

header is at the top.
http://screencast.com/t/LD5LUmHyL

In addition to the positioning issue, the header is not remaining fixed upon scrolling.

What do you advise?

@jaspermdegroot
Member

@ashaegupta - Can you provide a test page? Here is a template that you can you use: http://jsbin.com/otepum/edit#html
It is also very useful to know which on platform and browser including version you experienced the issue. Thanks.

@jaspermdegroot
Member

@scottjehl @gseguin

We updated the widget factory some time ago so I wonder if this is still an issue. If so, I suggest we change the code of updatePagePadding a little bit as a temporary solution.

@arschmitz
Member

@uGoMobi @gseguin @scottjehl I just checked this out and it looks like this is fixed. Not sure if this was in anyway related but previously we were not restoring the page padding after destroy so it was leaving a space. This was fixed last week. Either way though calling updatePagePadding on destroyed tooolbars no longer has any effect and responds as expected. Unless i missed something which is certainly possible it looks like we can close this one out.

@toddparker
Contributor

@arschmitz do you have a test page you used to confirm this is ok? Just wanted to double check.

@arschmitz
Member

i just ran it in the console on the docs i can throw one together though
ill do it when i get back. on thursday

On Mon, Nov 12, 2012 at 9:34 PM, Todd Parker notifications@github.comwrote:

@arschmitz https://github.com/arschmitz do you have a test page you
used to confirm this is ok? Just wanted to double check.


Reply to this email directly or view it on GitHubhttps://github.com/jquery/jquery-mobile/issues/3880#issuecomment-10313558.

@arschmitz
Member

@toddparker @uGoMobi
test page: http://jsbin.com/uzaret/500/edit. updatePagePadding has no effect after calling destroy. so this issue is fixed. HOWEVER....
There is still a problem here though i think its a separate issue. as @scottjehl mentions above calling any method after destroy should throw an error "cannot call methods on xyz prior to initialization" This is not happening with the fixed toolbar widget. however if you look at http://jsbin.com/uzaret/501/edit with list view the error is thrown as expected. So this is not our widget factory version its something in fixedtoolbars.

UPDATE: noticed this error IS thrown from fixedtoolbars in unit tests but i can't make it happen on actual page jsbin / docs ??

@jaspermdegroot
Member

I changed the topic of this issue (was: "Add safeguard to fixed header's updatePagePadding") and changed the milestone to 1.3 because I don't think this is something for a maintenance release.

Update: turned out to be a small change... milestone back at 1.2.1

@gseguin gseguin pushed a commit that closed this issue Nov 15, 2012
@arschmitz arschmitz FixedToolbar: change destroy to _destroy per ui 1.9 upgrade guide wid…
…get was not being destroyed previously. Fixes #3880 - fixedToolbar _destroy()
e1fe9b0
@gseguin gseguin closed this in e1fe9b0 Nov 15, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment