ampproject / ampproject/amphtml

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

Open
#14,170 6 comments 0 reactions 0 assignees View on GitHub
Component: amp-form Customer: CMS P2: Soon Stale Type: DevX Type: Feature Request WG: components
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.