Skip to content
This repository

1.2.0 beta 1 Popups: Hash ui-state dialog and refreshing kills popups #4994

Closed
Racquetballer opened this Issue · 6 comments

4 participants

Racquetballer Gabriel "_|Nix|_" Schulhof Todd Parker John Bender
Racquetballer

When a popup is open on a page and the user performs a browser refresh the user is no longer able to trigger any popups.

#defaultPage&ui-state=dialog

The hash is still visible in the url on refresh.

What is the fix for this? Thanks!

Todd Parker

I can't seem to reproduce this in the latest code. If I go here, open a popup, then refresh, the next popup button I press opens a new popup.
http://jquerymobile.com/test/docs/pages/popup/index.html

Popups are closed when you reload because they aren't designed to be deep-linkable.

Racquetballer

I'm new to jsfiddle but here is an example of what I'm talking about.

http://jsfiddle.net/ActionJackson/aEzzx/1/

It only happens when you give the page an id value. In this case I named my data role page with an id defaultPage. I did confirm if there is no id it works.

Gabriel "_|Nix|_" Schulhof gabrielschulhof closed this in 8b52631
John Bender johnbender referenced this issue from a commit
John Bender Revert "[popup] Reuse dialog hash if already present, and tack on ano…
…ther if it is part of the initial hash -- Fixes #4994"

This reverts commit 8b52631.
2f8ef2f
John Bender johnbender referenced this issue from a commit
John Bender simply tack on another dialog state Fixes #4994
The alternate implementation here is to check if the current pages
data-url is already a part of the hash and replace it with just
the dialog hash key. This is dangerous though because it doesn't
account for dialog states either. This solution is simple and works
with the only downside being an odd url in an extreme corner case.
e625c95
John Bender

@gabrielschulhof

See the second commit above in the fix-4994 branch. I think in this case a simpler approach for the corner case is better to reduce complexity and get this out the door with the minimum of impact on the release.

Also see my comments on the commit

8b52631#comments

Definitely let me know if I missed a corner case. If it's good I can add a simple test for this one tomorrow.

Gabriel "_|Nix|_" Schulhof
Collaborator

This does not address the following sequence:

initial state -> open popup -> close popup -> forward -> open popup. Dialogs handle this correctly by reusing the dialog hash state instead of creating a new history entry. Popups need to do that too.

With this fix, a new history entry is created, which is not too bad, because when you dismiss the popup the second time, it goes back two history entries. It is worse, however, if the initial url is of the form http://domain/path/to/page1.html/path/to/page2.html&ui-state=dialog, because then, after you dismiss the popup the second time, it only goes back one history entry. So, you dismiss the popup, and, according to the location, you're still in a dialog hash state, even though you shouldn't be.

Also, it's imperative that we get the initial url from $.mobile.urlHistory.getActive().url instead of activePage.jqmData( "url" ), because otherwise the id of the page can end up in the URL even if it's the first page of a multi-page document.

I've added a manual test environment for such navigational combinations in 59be4b9. This should allow us to test all possible steps and funky initial URLs as well.

I've also tried to create a qunit test generator for all possible combinations of page -> popup, page -> dialog, page -> dialog -> popup, page -> popup -> page, etc., but, needless to say, it's a combinatorial explosion. I stopped at around 1200 unit tests.

John Bender

@gabrielschulhof

This only occurs on the initial state, that is a refresh. For all other back and forward combinations it does re-use the dialog hash key. I guess I'm not convinced that there's an enormous amount of value to making this kind of change this late in the release cycle.

If we want to add this after the release I think I'd be more open to it. I'm in #jquerymobile-dev to chat about it when you have a minute.

John Bender

@gabrielschulhof

Also, as I pointed out in the commit message, we can actually make use of the id reference that's currently included to ensure a hashchange when we want to re-use the current dialog state. That is if the default when a popup is clicked looks something like:

http://foo.com/ --- click popup link ---> http://foo.com/#foo&ui-state=dialog

We can check if we're on the initial page and the location.hash is a match for the hash we're creating and just strip the id from it:

http://foo.com/#foo&ui-state=dialog --- click popup link ---> http://foo.com/#&ui-state=dialog

That would, at most, be a 2 to 3 line change, and accommodates the "reuse" of the existing hash. Also, it's not clear to me that we shouldn't use the identifier for popup hash state when we can since the semantics add value to the url.

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.