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

Add workaround for subscriptions widget form submission failure in AMP pages #11637

Conversation

westonruter
Copy link
Contributor

@westonruter westonruter commented Mar 21, 2019

It was discovered that the subscription widget form submission fails on AMP pages. At issue is AMP form submissions appear to not include a field for the named button that sent the form. This field is used to initiate the subscription flow:

if ( isset( $_REQUEST['jetpack_subscriptions_widget'] ) )

Because this field is not included in the submission, the result is what appears to be a reload of the page.

For more information about the issue, see ampproject/amphtml#21498 which I've reported as an apparent bug in the amp-form component.

See also this workaround plugin: https://gist.github.com/westonruter/3b61ffcfbc1b528b1135ac7cc065eaa2

Note: This workaround plugin also shows how the subscription widget UX could be greatly improved on AMP pages by not requiring a full page reload to submit the form. The form can instead be submitted via Ajax and the success/error message can instead be shown right in the existing form. The AMP_Jetpack_Subscriptions_Form_Response_Container_Injector in this plugin is a hack to inject the div[submit-error] and div[submit-success] elements into the form; this wouldn't be needed if the widget itself output the required elements. Also, the success/error messages could be refactored into a common helper method for use in Ajax responses as well as full page responses.

See #9730.
Fixes #11595.

Changes proposed in this Pull Request:

  • Add a hidden jetpack_subscriptions_widget field to the subscriptions form in AMP pages to ensure the subscription flow is initiated.

Testing instructions:

  • Active the AMP plugin and enable Paired mode.
  • Add a Subscriptions widget to the sidebar.
  • Navigate to an AMP page that has the subscriptions widget on it.
  • Attempt to subscribe.

Proposed changelog entry for your changes:

  • Fix subscriptions widget form submissions on AMP pages.

@jetpackbot
Copy link

Warnings
⚠️

The PR is missing at least one [Status] label. Suggestions: [Status] In Progress, [Status] Needs Review

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 6f9ba98

@jeherve jeherve added [Type] Bug When a feature is broken and / or not performing as intended [Feature] Subscriptions All subscription-related things such as paid and unpaid, user management, and newsletter settings. AMP [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Mar 21, 2019
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello westonruter! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D25863-code before merging this PR. Thank you!

@jeherve
Copy link
Member

jeherve commented Mar 21, 2019

I can't seem to get this to appear for some reason.

Should this work on any page of the site in Paired mode, as long as I am on the AMP view?

@westonruter
Copy link
Contributor Author

I can't seem to get this to appear for some reason.

You can't get the widget to appear? It doesn't render in dev mode.

@jeherve
Copy link
Member

jeherve commented Mar 22, 2019

The widget does appear, but I can’t seem to get the invisible additional field to appear

@westonruter
Copy link
Contributor Author

And you're looking at an AMP page?

In any case, the issue in AMP is actively being fixed: ampproject/amphtml#21518

So I'm sure it will be rolled out before this workaround in Jetpack, so I don't think this PR is necessary.

A separate PR to improve the UX of submitting the form would be nice.

@westonruter
Copy link
Contributor Author

And fix has been merged into AMP: ampproject/amphtml#21518

Should be rolled out to every AMP page in 1-2 weeks.

@kraftbj kraftbj removed the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Apr 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AMP [Feature] Subscriptions All subscription-related things such as paid and unpaid, user management, and newsletter settings. Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Subscriptions Widget Broken Under AMP with Native Template Mode
5 participants