Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
westonruter opened this issue Mar 21, 2018 · 6 comments
Milestone

Comments

@westonruter
Copy link
Member

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
Copy link
Contributor

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?

@aghassemi aghassemi self-assigned this Mar 21, 2018
@aghassemi aghassemi added this to the New FRs milestone Mar 21, 2018
@ericlindley-g
Copy link
Contributor

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
Copy link

stale bot commented Dec 29, 2020

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.

@stale stale bot added the Stale Inactive for one year or more label Dec 29, 2020
@westonruter
Copy link
Member Author

bump

@stale stale bot removed the Stale Inactive for one year or more label Dec 29, 2020
@westonruter
Copy link
Member Author

Note that this would largely be a non-issue if native non-XHR POST forms were allowed in AMP. See #27638.

@stale
Copy link

stale bot commented Oct 1, 2022

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.

@stale stale bot added the Stale Inactive for one year or more label Oct 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants