Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

PageContainer: dynamically injected pages will not be removed from DOM automatically on hide #6656

Open
frequent opened this Issue · 12 comments

4 participants

@frequent

Demo

In Firefox:

  • Check DOM
  • dashboard is first page, services is dynamically generated from the deeplink
  • Open panel, click networks
  • network page is dynamically generated and transitioned to
  • In Firebug, is still in the DOM services although it should have been removed
  • Open panel, click servers
  • again, page is generated dynamically and transitioned to, but the previous page is not removed.

I checked the pageContainer widget.

Dynamically injected content is missing a call to page.page( "bindRemove" );
Normally this is called inside the pageContainer widget _include() method, which is called on _loadSuccess(), which will only be called when an Ajax call is made to fetch a page - so not in the case of dynamic injection.

So if I intercept the changePage on pagebeforechange, generate a dynamic page, append it to the DOM and then trigger a new changePage to that page, I'm stuck without the remove mechanism.

To fix inside pageContainer widget, load method, I add:

      if ( content.length && !settings.reload ) {
        this._enhance( content, settings.role );
        // FIX: add remove bindings
        content.page( "bindRemove" );
        ...

Question is whether dynamic injection also means dynamic cleanup, but I think the hide events should also trigger regardless of "who" is handling DOM maintenance.

@frequent frequent referenced this issue from a commit in frequent/jquery-mobile
Sven Franck fix #6656 pageContainer: dynamic injected pages missing remove bindings 4eccb04
@gabrielschulhof
Collaborator

Your demo doesn't work in Chrome. Chrome just shows a blank page. I wouldn't be surprised though. Navigation during startup doesn't work in Chrome.

@frequent

@gabrielschulhof: please use Firefox, because the demo uses a custom promise() implementation which we have not fixed on Chrome :-)

@gabrielschulhof
Collaborator

OK, but this should be reproducible with a simple jsbin as well ...

@frequent

@gabrielschulhof - I try to make one btw lunch and dessert :-)

@gabrielschulhof
Collaborator
@gabrielschulhof
Collaborator

Here's a workaround:

http://jsbin.com/ofuhaw/706/

@gabrielschulhof
Collaborator

The funny thing is that if we fix this using @frequent's PR, then we will have to make sure that bindRemove() only gets called once per page and that a second call has no effect, because other people may have stumbled across this problem and applied my workaround. For those people, bindRemove() will certainly get called at least twice for each generated page - once by their own code, and once by ours.

@gabrielschulhof
Collaborator

Another aspect that may be worth investigating as we fix this bug is why we're calling bindRemove() from pagecontainer at all. We could just as easily call bindRemove() from the page's _create() method and remove all bindRemove() calls from pagecontainer. Then page removal would be entirely separated from navigation.

@gabrielschulhof gabrielschulhof modified the milestone: 1.4.2, 1.4.1
@jaspermdegroot
Collaborator

@gabrielschulhof - You closed this ticket but the PR #6657 is still open. I guess you closed by accident so I am going to re-open. Let me know if I guessed wrong.

@jaspermdegroot jaspermdegroot reopened this
@jaspermdegroot jaspermdegroot modified the milestone: 1.4.1, 1.5.0
@arschmitz
Owner

Personally I don't think we should remove dynamically injected pages. These should be treated the same as any other internal page. Meaning they should be left in the DOM for the dev to deal with it. Otherwise we would have to store a list of all the internal pages that existed when the framework spins up, to be able to tell the difference between an internal page and a dynamically injected page.

@frequent

@arschmitz:
while I agree to remove by hand I thought data-external-page=true was the flag set on pages pulled in (dynamic or not) which triggers removal of external pages upon user departure. I initially hoped I just add the attribute on the dynamic page and save myself a custom handler like:

      cleanupHandler = function (e) {
        var page, id, is_enhanced;

        page = e.target;
        id = page.id;
        is_enhanced = $(page).data("mobilePage") || $(page).page("instance");

        if (page.getAttribute('data-external-page')) {
          if (is_enhanced) {
            $(this).page('destroy').remove();
          }
        }
      };
@arschmitz
Owner

@frequent one thing that comes to mind in all of this is I am planning on moving all page removal logic to pagecontainer as a page isn't going to remove it self so the logic should be where its getting removed.

So if we do something along those lines it should be in conjunction with this.

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.