ampproject / ampproject/amphtml
Unable to reload (self-redirect) via AMP-Redirect-To when fragment added to current location
- Dominant language
- JavaScript
- Stars
- 14.9k
- Forks
- 4.1k
- PR merge metrics
- No merged PRs in 30d
Description
## What's the issue?
In WordPress comments, the normal flow is:
0. Given user located at a post with comments at https://example.com/foo/
1. User submits comment form which is posted to `/wp-comments-post.php`.
2. 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](https://stackoverflow.com/q/1589799/93579) 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](https://github.com/Automattic/amp-wp/pull/1029/files/1e32d6c0f4f3a42affef4080cd80bd8a991de476#diff-2585472f207548f626a43a7ff3cab922R419) in the AMP plugin for WordPress:
```php
$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.
Contributor guide
Research direction
Start by reproducing the action-xhr flow described, using an AMP-Redirect-To value that matches the Referer URL with a fragment identifier. Trace how the browser client handles AMP-Redirect-To when the target matches the current location, and determine the intended behavior for existing versus missing anchors. Done means the agreed redirect behavior is covered without breaking normal anchor navigation.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- javascript
- Domain
- frontend
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 30/100