search

ampproject / amphtml

The AMP web component framework.
https://amp.dev
Apache License 2.0
14.89k stars 3.89k forks source link

Unable to reload (self-redirect) via AMP-Redirect-To when fragment added to current location #14170

Open westonruter opened 6 years ago

westonruter commented 6 years ago

What's the issue?

In WordPress comments, the normal flow is:

  1. Given user located at a post with comments at https://example.com/foo/
  2. User submits comment form which is posted to /wp-comments-post.php.
  3. WordPress posts the comment and redirects to the comments's permalink at URL like https://example.com/foo/#comment-123

This behavior cannot be currently be implemented in AMP because when AMP-Redirect-To returns with https://example.com/foo/#comment-123 then the browser client just tries jump to #comment-123 on the existing page instead of reloading and then navigating to that page where the newly-approved comment can then be seen.

How do we reproduce the issue?

To reproduce the issue just have a form that does an action-xhr endpoint that does a AMP-Redirect-To value which consists of the Referer URL appended with a random fragment identifier.

As noted in SO#1589799 there are two possible fixes to this problem:

  1. Add a query param to force the URL to be different aside from the fragment identifer.
  2. Update the URL fragment and then do location.reload()

We are currently taking the first approach in the AMP plugin for WordPress:

$url = add_query_arg( 'comment', $comment->comment_ID, $url );

Since naturally the second option wouldn't be possible in AMP.

This being said, it may be a current desired effect for AMP-Redirect-To to just jump the user to a given anchor in the page. A possible hybrid solution would be to force the reload if the returned target does not exist in the document as an ID'ed element. If this is not non-reloading behavior is not intended, however, then I think any time that an AMP-Redirect-To header is returned it should always cause the page to be unloaded and the browser to navigate to the page returned from the server.

What browsers are affected?

All browsers.

Which AMP version is affected?

Not a new issue as far as I know. v1521593671635.

aghassemi commented 6 years ago

I really like the hybrid approach you mentioned:

A possible hybrid solution would be to force the reload if the returned target does not exist in the document as an ID'ed element.

/cc @choumx @ericlindley-g WDYT?

ericlindley-g commented 6 years ago

This sounds like a good solution — how would it work in the context of an AMP viewer? Would it reload with the AMP URL on origin, the canonical URL, or the full viewer URL (which in the case of the viewer used by Google, I think would land the user on a single-doc viewer)?

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

westonruter commented 3 years ago

bump

westonruter commented 3 years ago

Note that this would largely be a non-issue if native non-XHR POST forms were allowed in AMP. See https://github.com/ampproject/amphtml/issues/27638.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.