Navigating to root "/" does not generate clean URL if root page has id="*" #2644

Closed
ahutch opened this Issue Oct 7, 2011 · 16 comments

Comments

Projects
None yet
4 participants
@ahutch

ahutch commented Oct 7, 2011

Scenario:
I navigate to site.com/ then click a link to site.com/page. Then try to return home by clicking on a link site.com/. But I get site.com/page#root

The link is:

<li ><a href="/" data-icon="home" data-theme="c" ><p  class="ui-li-heading" >Home</p></a></li>

Comments from IRC:
If you remove the id off of your first-page you will get the result you are expecting.

Result:
Removing the ID of the root page did fix the URL. However the javascript and page* events that target this page no longer work because the page has no ID.

@ghost ghost assigned jblas Oct 7, 2011

@jblas

This comment has been minimized.

Show comment
Hide comment
@jblas

jblas Oct 7, 2011

Contributor

NOTE:

I know we are placing the #root due to the fact that the first-page has an id on it. The more concerning thing is that the hash is appended to the wrong URL. We need to check if this is due to some code that makes assumptions about location which could be different if push-state is enabled.

Contributor

jblas commented Oct 7, 2011

NOTE:

I know we are placing the #root due to the fact that the first-page has an id on it. The more concerning thing is that the hash is appended to the wrong URL. We need to check if this is due to some code that makes assumptions about location which could be different if push-state is enabled.

@johnbender

This comment has been minimized.

Show comment
Hide comment
@johnbender

johnbender Oct 8, 2011

Contributor

@jblas

https://github.com/jquery/jquery-mobile/blob/master/js/jquery.mobile.init.js#L79

It seems like that assumption was made before the relative path changes went in. Shouldn't we prefer the pathname + search? I confess I'm getting to the point where I feel like its extremely hard to keep all the implications straight in my mind.

Contributor

johnbender commented Oct 8, 2011

@jblas

https://github.com/jquery/jquery-mobile/blob/master/js/jquery.mobile.init.js#L79

It seems like that assumption was made before the relative path changes went in. Shouldn't we prefer the pathname + search? I confess I'm getting to the point where I feel like its extremely hard to keep all the implications straight in my mind.

@johnbender

This comment has been minimized.

Show comment
Hide comment
@johnbender

johnbender Oct 8, 2011

Contributor

@jblas

Clarification: If the hash was a path here, the pushstate plugin would reconcile it with the current url and remove the hash altogether. Because the hash isn't a path it leaves it.

This is why I linked to where we set the data-url.

Contributor

johnbender commented Oct 8, 2011

@jblas

Clarification: If the hash was a path here, the pushstate plugin would reconcile it with the current url and remove the hash altogether. Because the hash isn't a path it leaves it.

This is why I linked to where we set the data-url.

@jblas

This comment has been minimized.

Show comment
Hide comment
@jblas

jblas Oct 9, 2011

Contributor

@johnbender

For consistency sake I think it needs to remain an id ... we already have a utility function that can figure out if it is the first page or not based on id, document URL, or base URL ... what puzzles me is why this is the only case where we are getting the hash wrong? I haven't actually debugged it yet to figure out if this is a nav problem or a pushstate problem.

Contributor

jblas commented Oct 9, 2011

@johnbender

For consistency sake I think it needs to remain an id ... we already have a utility function that can figure out if it is the first page or not based on id, document URL, or base URL ... what puzzles me is why this is the only case where we are getting the hash wrong? I haven't actually debugged it yet to figure out if this is a nav problem or a pushstate problem.

@serandel

This comment has been minimized.

Show comment
Hide comment
@serandel

serandel Oct 9, 2011

It happens exactly the same if the links points to "index.html" instead of "/". And I've found that the pageinit event isn't fired neither. :(

serandel commented Oct 9, 2011

It happens exactly the same if the links points to "index.html" instead of "/". And I've found that the pageinit event isn't fired neither. :(

@jblas

This comment has been minimized.

Show comment
Hide comment
@jblas

jblas Oct 10, 2011

Contributor

@johnbender

I haven't actually debugged this because I'm on an iPad at the moment, but looking at the onhashchange callback:

https://github.com/jquery/jquery-mobile/blob/master/js/jquery.mobile.navigation.pushstate.js#L75

It seems to assume that it can just resolve an id hash against the current location.href when it should be resolving against the $.mobile.documentUrl.

Contributor

jblas commented Oct 10, 2011

@johnbender

I haven't actually debugged this because I'm on an iPad at the moment, but looking at the onhashchange callback:

https://github.com/jquery/jquery-mobile/blob/master/js/jquery.mobile.navigation.pushstate.js#L75

It seems to assume that it can just resolve an id hash against the current location.href when it should be resolving against the $.mobile.documentUrl.

@jblas

This comment has been minimized.

Show comment
Hide comment
@jblas

jblas Oct 10, 2011

Contributor

I have a fix for this that I'm running against the unit tests. I'll check in once the results return all green/passing.

As a side note, I'm a bit hesitant about forcing all hash Urls with:

http://site.com/#root

into:

http://site.com/

Mainly because I'm not sure of the intent of the developer in all cases. That said, it's easy to work around thr problem if you really want that case to always result in a clean URL:

$( document ).bind( "pagebeforechange", function( e, data ) {
    var toPage = data.toPage;
    if ( typeof toPage === "object" && !data.options.dataUrl ) {
        if ( toPage[ 0 ] === $.mobile.firstPage[ 0 ] ) {
            data.options.dataUrl = $.mobile.getDocumentUrl();
        }
    }
});

The code snippet above makes use of the dataUrl option to changePage() which is a mechanism that can be used to tell the framework what to actually use when setting the location. In the case above we're saying, if we're actually going to go to the first page, always make sure you use the original documentUrl.

Contributor

jblas commented Oct 10, 2011

I have a fix for this that I'm running against the unit tests. I'll check in once the results return all green/passing.

As a side note, I'm a bit hesitant about forcing all hash Urls with:

http://site.com/#root

into:

http://site.com/

Mainly because I'm not sure of the intent of the developer in all cases. That said, it's easy to work around thr problem if you really want that case to always result in a clean URL:

$( document ).bind( "pagebeforechange", function( e, data ) {
    var toPage = data.toPage;
    if ( typeof toPage === "object" && !data.options.dataUrl ) {
        if ( toPage[ 0 ] === $.mobile.firstPage[ 0 ] ) {
            data.options.dataUrl = $.mobile.getDocumentUrl();
        }
    }
});

The code snippet above makes use of the dataUrl option to changePage() which is a mechanism that can be used to tell the framework what to actually use when setting the location. In the case above we're saying, if we're actually going to go to the first page, always make sure you use the original documentUrl.

@jblas

This comment has been minimized.

Show comment
Hide comment
@jblas

jblas Oct 10, 2011

Contributor

Ok, so after discussing with @johnbender out-of-band, I think I'll make the clean url case the default. Folks can use a similar workaround like the one above to add the hash if they really want one.

Contributor

jblas commented Oct 10, 2011

Ok, so after discussing with @johnbender out-of-band, I think I'll make the clean url case the default. Folks can use a similar workaround like the one above to add the hash if they really want one.

jblas added a commit that referenced this issue Oct 10, 2011

Fix for issue #2644 - Navigating to root "/" does not generate clean …
…URL if root page has id="*"

- Fixed a bug in the hashchange handler for the pushstate/replacestate plugin that was incorrectly resolving hashchanges for ids against the current location.href, which could be a different document. We now resolve id hashes against the document URL.

- Modified changePage() so that it sets the settings.dataUrl option to the documentUrl, when navigating to the first-page of the application document. This prevents any id on the first-page from being added to the location hash. This means that URLs that used to be produced like this:

	http://site.com/apps/#first-page-id

will now display as:

		http://site.com/apps/

Developers that wish to get the old behavior back can register a pagebeforechange handler and do something like this:

	$( document ).bind( "pagebeforechange", function( e, data ) {
		var toPage = data.toPage;
		if ( typeof toPage === "object" && !data.options.dataUrl && toPage[ 0 ] === $.mobile.firstPage[ 0 ] && toPage[ 0 ].id ) {
			data.options.dataUrl = "#" + toPage[ 0 ].id;
		}
	});

The handler above will make sure that any page changes to the first-page will always display as:

	http://site.com/apps/#first-page-id
@jblas

This comment has been minimized.

Show comment
Hide comment
@jblas

jblas Oct 10, 2011

Contributor

I just landed a fix for this on the head. Developers that want the old behavior of the first-page id in the hash can use a pagebeforechange handler like the following to get back the old behavior:

$( document ).bind( "pagebeforechange", function( e, data ) {
    var toPage = data.toPage;
    if ( typeof toPage === "object" && !data.options.dataUrl && toPage[ 0 ] === $.mobile.firstPage[ 0 ] && toPage[ 0 ].id ) {
        data.options.dataUrl = "#" + toPage[ 0 ].id;
    }
});

Note, that I still need to add a unit test case for this.

Contributor

jblas commented Oct 10, 2011

I just landed a fix for this on the head. Developers that want the old behavior of the first-page id in the hash can use a pagebeforechange handler like the following to get back the old behavior:

$( document ).bind( "pagebeforechange", function( e, data ) {
    var toPage = data.toPage;
    if ( typeof toPage === "object" && !data.options.dataUrl && toPage[ 0 ] === $.mobile.firstPage[ 0 ] && toPage[ 0 ].id ) {
        data.options.dataUrl = "#" + toPage[ 0 ].id;
    }
});

Note, that I still need to add a unit test case for this.

@jblas

This comment has been minimized.

Show comment
Hide comment
@jblas

jblas Oct 10, 2011

Contributor

@serandel

You mentioned in your post that you don't see pageinit fired ... for which page, and when? I ask because the first-page of the application document is only ever created once. It is never pruned from the DOM so when you go to the first-page, we are really just showing you what is in the DOM already, so no pageinit ever occurs for the first-page after the initial application document load.

Contributor

jblas commented Oct 10, 2011

@serandel

You mentioned in your post that you don't see pageinit fired ... for which page, and when? I ask because the first-page of the application document is only ever created once. It is never pruned from the DOM so when you go to the first-page, we are really just showing you what is in the DOM already, so no pageinit ever occurs for the first-page after the initial application document load.

@jblas

This comment has been minimized.

Show comment
Hide comment
@jblas

jblas Oct 10, 2011

Contributor

And for completeness, I just wanted to document that for someone to get the old id hash behavior back, they can do a "pagebeforechange" handler like this:

$( document ).bind( "pagebeforechange", function( e, data ) {
    var toPage = data.toPage;
    if ( typeof toPage === "object" && !data.options.dataUrl && toPage[ 0 ] === $.mobile.firstPage[ 0 ] && toPage[ 0 ].id ) {
        data.options.dataUrl = "#" + toPage[ 0 ].id;
    }
});

This will always make the location show the id for the first-page in the app document if an id attribute exists.

Contributor

jblas commented Oct 10, 2011

And for completeness, I just wanted to document that for someone to get the old id hash behavior back, they can do a "pagebeforechange" handler like this:

$( document ).bind( "pagebeforechange", function( e, data ) {
    var toPage = data.toPage;
    if ( typeof toPage === "object" && !data.options.dataUrl && toPage[ 0 ] === $.mobile.firstPage[ 0 ] && toPage[ 0 ].id ) {
        data.options.dataUrl = "#" + toPage[ 0 ].id;
    }
});

This will always make the location show the id for the first-page in the app document if an id attribute exists.

@ahutch

This comment has been minimized.

Show comment
Hide comment
@ahutch

ahutch Oct 10, 2011

With the latest I see that this issue has been fixed. Upon returning to the root the url is site.com/
Thanks.

ahutch commented Oct 10, 2011

With the latest I see that this issue has been fixed. Upon returning to the root the url is site.com/
Thanks.

@jblas

This comment has been minimized.

Show comment
Hide comment
@jblas

jblas Oct 10, 2011

Contributor

@ahutch

Thanks for confirming! I've tagged this issue with "Needs unit test case" so we don't forget about the use case. Closing this issue.

Contributor

jblas commented Oct 10, 2011

@ahutch

Thanks for confirming! I've tagged this issue with "Needs unit test case" so we don't forget about the use case. Closing this issue.

@jblas jblas closed this Oct 10, 2011

@serandel

This comment has been minimized.

Show comment
Hide comment
@serandel

serandel Oct 10, 2011

Disregard that part of my comment, I misunderstood the event I should be referencing. I'm hooking now my code in "pagebeforeshow" and it works beautifully.

Disregard that part of my comment, I misunderstood the event I should be referencing. I'm hooking now my code in "pagebeforeshow" and it works beautifully.

@jblas

This comment has been minimized.

Show comment
Hide comment
@jblas

jblas Oct 10, 2011

Contributor

@serandel

Thanks for the follow up!

Contributor

jblas commented Oct 10, 2011

@serandel

Thanks for the follow up!

@serandel

This comment has been minimized.

Show comment
Hide comment
@serandel

serandel Oct 10, 2011

Thx to you for the superb work! :)

Sergio Delgado -- Serandel
mailto:serandel@gmail.com

Quoth the raven, 'Nevermore.'

‎"Ever tried. Ever failed. No matter. Try again. Fail again. Fail better."
-- Samuel Beckett

2011/10/11 Kin Blas
reply@reply.github.com:

@serandel

Thanks for the follow up!

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

Thx to you for the superb work! :)

Sergio Delgado -- Serandel
mailto:serandel@gmail.com

Quoth the raven, 'Nevermore.'

‎"Ever tried. Ever failed. No matter. Try again. Fail again. Fail better."
-- Samuel Beckett

2011/10/11 Kin Blas
reply@reply.github.com:

@serandel

Thanks for the follow up!

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment