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

Automatically append ?amp to URLs in Paired mode (AMP-to-AMP navigation) #1389

Closed
jukefr opened this issue Aug 31, 2018 · 13 comments · Fixed by #3627
Closed

Automatically append ?amp to URLs in Paired mode (AMP-to-AMP navigation) #1389

jukefr opened this issue Aug 31, 2018 · 13 comments · Fixed by #3627
Labels
P0 High priority
Milestone

Comments

@jukefr
Copy link

jukefr commented Aug 31, 2018

Hey again,

Wanted to say thanks for the awesome work you put into this project and that the v1 is looking better and better every day 👏

Now for my issue :
I want the entirety of my Wordpress to be available as AMP.
I have activated the plugin in Paired mode and started transitioning my templates with is_amp_endpoint()
The problem I am having is that any links that get rendered on an AMP page will point to the non-amp version of the page.

I have beta 2 and activated automatic stripping of non valid markup.
I have tested accessing said pages (the first one and the one that is linked) directly with ?amp and all went well.

I was planning on maybe checking the query parameters inside a hook and append ?amp to every link that gets rendered for that call.
I am pretty sure however that there might be a better way to do this.

I might also have a problem on my end or a bad understanding of how things work as I'd expect that this would be a feature that would be enabled by default for an AMP plugin. I tried searing in the Wiki and the Issues but could not find anything relating to this apart from trying to manually enable paired mode and setting a template folder in my functions.php but that did not help.

If there is a more appropriate place to ask question like those so that I don't end up spamming the issues I'd be glad to hear about it !
Cheers

@amedina
Copy link
Member

amedina commented Sep 1, 2018

@jukefr Could you share a link where we can observe the behavior?

@westonruter
Copy link
Member

The problem I am having is that any links that get rendered on an AMP page will point to the non-amp version of the page.

This is normal behavior. Links in the page continue to point to their canonical versions. There has been some work to define an official way for AMP documents to link to other AMP documents instead of non-AMP documents. In the AMP project this is called “AMP-to-AMP navigation” and you can read about it here: ampproject/amphtml#12496. Nevertheless, it appears that this feature is only being developed in the context of AMP-to-AMP navigation within the AMP viewer and not for navigation on your own origin (see ampproject/amphtml#12981 (comment)).

All this to say: at the moment if you want to retain navigation in AMP when linking to other pages on your site, the only solution at present is you add ?amp to all internal links when is_amp_endpoint(). This would be best handled at the sanitizer level with a query to the DOM for all a[href] elements.

Nevertheless, I am not sure at present if this would have any negative impact on the SEO for the canonical URLs on your site. We should get confirmation from Google Search on whether this is a good suggestion. I expect it would be fine since the AMP documents still have the canonical links, but I wouldn't want to lead you astray. If it is fine to do, then this capability could be an option that we make available in the plugin itself, at least at the programatic level. @amedina I recall you mentioning this feature suggestion a few months ago.

@westonruter westonruter changed the title automatically append ?amp to URLs in Paired mode Automatically append ?amp to URLs in Paired mode Sep 4, 2018
@westonruter westonruter changed the title Automatically append ?amp to URLs in Paired mode Automatically append ?amp to URLs in Paired mode (AMP-to-AMP navigation) Sep 4, 2018
@westonruter
Copy link
Member

So far we've only been considering AMP-to-AMP navigation on the origin. What about an opt-in to AMP-to-AMP navigation on the AMP Cache?

@pbakaus If AMP-to-AMP navigation is enabled, can links on pages served from the AMP Cache be rewritten to link to the AMP Cache URLs as opposed to the origin URLs? For example if I go to the XWP website on the AMP Cache at https://xwp-co.cdn.ampproject.org/c/s/xwp.co/ could the link to “Work” be rewritten from https://xwp.co/work/ (as it is served from the origin) to https://xwp-co.cdn.ampproject.org/c/s/xwp.co/work/ ? This would be vey cool as it would mean a site could more fully take advantage of the AMP Cache as a CDN.

@pbakaus
Copy link

pbakaus commented Sep 5, 2018

@cramforce to add thoughts on A2A (if there's anything to share). This would indeed be more a feature for the AMP Caches or AMP itself, vs. the WordPress plugin.

@cramforce
Copy link
Member

I think we could keep the 2 discussions separate. Should AMP articles link to other AMP articles: Yes. Maybe add an option to turn it off if folks don't want it in some circumstance.

AMP-to-AMP-navigation on the viewer side is something I continue to find interesting, but it'll only be for specific use cases long term. When web packaging ships URLs will be on the origin all the time. This just makes the first point (actually link to AMP) more important.

@amedina
Copy link
Member

amedina commented Sep 10, 2018

I like the idea of supporting A2A linking in WordPress enabled by the plugin, with a flexible option to opt-in/out of it.

@westonruter
Copy link
Member

Should AMP articles link to other AMP articles: Yes. Maybe add an option to turn it off if folks don't want it in some circumstance.

I'd say that we should default to linking to the non-AMP version when serving classic templates, since this is the legacy behavior and because the classic templates are so limited.

But going forward for sites that use the new paired mode we can then default to linking to other AMP pages from an AMP page since now there is much more parity.

@westonruter
Copy link
Member

I've worked out a little AMP extension plugin to implement AMP-to-AMP linking: https://gist.github.com/westonruter/f9ee9ea717d52471bae092879e3d52b0

This plugin is mostly relevant for sites that are using paired AMP (paired/classic mode). It's mostly irrelevant for AMP-first sites (native mode), except that it implements “A2A linking” features as outlined on ampproject/amphtml#12496, namely the amphtml link relations and the opt-in meta tag.

Also, per the plugin description:

Make all inter-site URLs on AMP pages link to other AMP pages. In paired/classic modes, the ?amp query var is added to all links (in classic, only in the content). Additionally, initial support for AMP-to-AMP (A2A) linking is implemented: in all modes, the amphtml relation is added to all frontend links (though just for the content in classic mode), and native/paired mode includes the A2A meta tag is added. Nevertheless, do note that A2A is not known to be implemented yet in any AMP viewer.

It will be nice when/if the AMP viewers implement A2A linking as that would I hope mean that you could browse an entire site in AMP without ever leaving the AMP cache. Though again, this may not be relevant when when web packaging is widely deployed.

@amedina
Copy link
Member

amedina commented May 7, 2019

Thinking more about this. I am not certain that we should provide AMP-to-AMP linking automatically in Transitional (paired) mode, because we cannot be certain the differences between the AMP and non-AMP versions would not have a negative effect on SEO.

On the other hand, if we offer this as an opt-in feature, we could give the control to the user for making that decision. When opted in, the plugin would automate the outbound linking to AMP pages from AMP pages.

@westonruter
Copy link
Member

westonruter commented Sep 27, 2019

Let's do this:

  1. Enable AMP-to-AMP linking by default in Transitional mode, but disable by default in Reader mode.
  2. Add a filter (e.g. amp_to_amp_linking_enabled) which allows this to be programmatically overridden (but it should be forcibly-enabled if in a paired-browsing UI: Add paired browsing interface for Transitional mode to aid with checking parity #3365). Do not add any checkbox UI for this option (Decisions, not Options)

@westonruter
Copy link
Member

westonruter commented Sep 27, 2019

The format of the paired AMP URLs will be bound up with #2204 (and #1383), but work on this can start beforehand. Just be aware that the logic for computing the paired AMP URLs will eventually need to use the same logic.

@westonruter westonruter added this to the v.1.4.1 milestone Oct 24, 2019
@westonruter westonruter removed their assignment Oct 24, 2019
@kmyram kmyram assigned kmyram and schlessera and unassigned kmyram Oct 25, 2019
@westonruter westonruter modified the milestones: v.1.4.1, v1.4 Oct 27, 2019
@westonruter westonruter modified the milestones: v1.4, v.1.4.1 Oct 28, 2019
@schlessera
Copy link
Collaborator

Testing instructions:

  1. Use a paired mode (Transitional or Reader mode).
  2. Go to a non-AMP URL.
  3. Verify: Internal URLs (checking by hovering over them) should not contain the AMP slug as an argument: ?amp.
  4. Go to an AMP URL.
  5. Verify: Internal URLs (checking by hovering over them) should contain the AMP slug as an argument: ?amp.

@csossi
Copy link

csossi commented Oct 30, 2019

Verified in QA

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 High priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants