Calendly: Change buttons to a standard link for AMP requests

[pablinos]

AMP strips out onClick handlers, so when the button version of
the block is rendered we should change it to a standard link to the Calendly
appointment page.

We could probably get around this by attaching the click handler in
another way, but it would still be good to treat an AMP request
differently.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

This a bug fix to a new feature

Testing instructions:

• Install the AMP for WordPress plugin.
• Create a Calendly block with the button style
• Publish the page and view it with /amp on the end of the URL
• Check that the link works.

Proposed changelog entry for your changes:

No changelog is necessary.

[matticbot]

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

[jetpackbot]

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: April 7, 2020.
Scheduled code freeze: March 31, 2020

Generated by 🚫 dangerJS against 30a52b9

[glendaviesnz]

This tested well for me.

before:

after:

And non AMP rendering still works as before.

[creativecoder]

We might consider adding and target="_blank + rel="noopener noreferrer" when the embed functions as a regular link.

Also, the way the Eventbrite block approaches this might be interesting, since it naturally functions as a regular link when the event handler doesn't work.

[creativecoder]

Tests well for me and makes the link functional within the AMP page.

[matticbot]

pablinos, Your synced wpcom patch D38684-code has been updated.

[jeherve]

It would be good to handle the inline style as well as we're working on this.

[matticbot]

pablinos, Your synced wpcom patch D38684-code has been updated.

[pablinos]

I've changed the approach to be similar to the way we've implemented the Eventbrite block. I'm thinking that we can update the SubmitButton component to be closer to the core button component and have the other blocks benefit from the extra settings, like border-radius and (possibly) gradients.

This inlines the styles for the when custom colours are used. Is that what you were thinking @jeherve?

[jeherve]

When using the button style, this seems to break an existing button until I edit the page, make a change to the block, and re-save. Is there a way we can avoid that?

[pablinos]

I seem to have issues changing colors of existing blocks right now.

I've been testing this and had a problem with setting a custom text colour on the button. The problem meant the colour wouldn't be displayed in the editor, but would be used in the frontend. I've fixed that in 3c15158 Could you let me know if you're still seeing any issues please @jeherve?

[Copons]

@jeherve @pablinos Apparently there's something going on with SubmitButton on Gutenberg 7.6 (updated to current master, but happened with a version from a couple of days ago too).
Note that this is only relevant for the editor, not just for Calendly (happens on Contact Form as well), the front end works just fine.

If I deactivate the Guten plugin, I can change the button colors just fine, in both this branch and master.

But with the Guten plugin enabled, the color controls work in kind of unexpected ways.

• Set a named color for either or both text and background.
• The button does not update.
• Switch to code editor.
• The colors attributes show up correctly.
• Switch back to visual editor.
• The button is updated with the correct colors.
• Change colors again: the button does not update.
• Try setting custom colors.
• The button updates correctly.

EDIT: just to clarify, whatever is happening it's not related to this PR.

EDIT 2:

I think there's something more complicated going on here, because at some point I got to make it work on any branch and on both Calendly and Contact Form, but now I can't again.
Apologize for not having proper test steps, but I'll keep investigating tomorrow.

[pablinos]

If I deactivate the Guten plugin, I can change the button colors just fine, in both this branch and master.

This is weird as the button text colour is definitely broken in master at the moment. I've been playing with different combinations, trying to replicate the problems you saw @Copons, but I'm struggling to get it to break with and without Gutenberg activated.

Is there any chance you could test it again @Copons and see if you can get it to consistently fail?

[matticbot]

pablinos, Your synced wpcom patch D38684-code has been updated.

[Copons]

So, this is step by step everything I've tested:

MASTER, NO GUTENBERG

• Insert Calendly, switch to link mode.
• Can change colors.
• Insert Contact Form.
• Can change colors.
• Refresh
• Calendly appears with the correct colors; can change them.
• Contact Form appears with the DEFAULT colors; can change them but they won't be saved.

Apparently, Contact Form only saves named colors as CSS classes, but not as attributes? (Custom colors are saved correctly, e.g. "customBackgroundButtonColor":"#225ccf")

Comparison:

<!-- wp:jetpack/calendly {"submitButtonClasses":"wp-block-button__link has-text-color has-secondary-color has-background has-primary-background-color","style":"link","url":"https://calendly.com/foo-bar","backgroundButtonColor":"primary","textButtonColor":"secondary"} -->

<!-- wp:jetpack/contact-form {"submitButtonClasses":"wp-block-button__link has-text-color has-secondary-color has-background has-primary-background-color","hasFormSettingsSet":"yes"} -->

MASTER, GUTENBERG ACTIVE

• Reload the same post with Calendly and Contact Form.
• Calendly appears with the correct colors; can change them.
• Contact Form appears with the DEFAULT colors; can change them but they won't be saved.

Same behaviour with or without Gutenberg.

PR 14630, NO GUTENBERG

• Reload the same post with Calendly and Contact Form.
• Calendly appears with correct colors. Cannot change them as named colors, but custom colors work fine.
• Contact Form appears with the DEFAULT colors. Cannot change them as named colors, but custom colors work fine.
• Try setting named colors for both blocks, even if they don't seem to work.
• Switch to code editor.
• The colors are in fact saved in the post content (although, Contact Form still doesn't save named colors into attributes).
• Switch back to visual.
• The blocks use the correct colors I've blindly set before.

PR 14630, GUTENBERG ACTIVE
PR 14630 REBASED, NO GUTENBERG
PR 14630 REBASED, GUTENBERG ACTIVE

• Exactly same behaviour as before.

---

In Calendly's case, I think it's caused by our re-implementation of the Core Button block (and 6eb1b38 is my proposed fix).
In Contact Form case it's because it's missing two attributes (PR open at #14898).

[matticbot]

pablinos, Your synced wpcom patch D38684-code has been updated.

[pablinos]

I've changed the SubmitButton code back so it's more in line with master now. That was essentially the same as your commit @Copons, so I think there was something that went wrong with a rebase. Thanks for your help in tracking that down, it was extremely confusing!

I've done a load of testing with and without the Gutenberg plugin, as well as creating a button on the master branch, and swapping to this one. The only difference is this branch has the fix for the customTextButtonColor, which is currently a bug in master.

So, I'm hoping this is ready for a final review.

[Copons]

I think there's yet another thing going on here... 😭

• Add Calendly, turn it into a button.
• Set the button colors to named colors. Pick something clearly different than the defaults to make.
• Save and reload.
• Try clearing the colors.
• The colors are indeed cleared (you can check by switching to code editor and back, or by saving and reloading), but the button in the editor does not visually update.

After more investigation: this happens on master as well.

[Copons]

Ok @pablinos I think I know what's happening here with the clear, but before committing I need more context with SubmitButton and its quirks, or I might create regressions all over the place.

Basically the culprit lies in these 4 lines:

jetpack/extensions/shared/submit-button.js

Lines 89 to 92 in 557a85a

	const backgroundColor = backgroundButtonColor.color || fallbackBackgroundColor;
	const color = textButtonColor.color || fallbackTextColor;
	const buttonStyle = { border: 'none', backgroundColor, color };
	const buttonClasses = this.getButtonClasses();

When this SubmitButton is created, withFallbackStyles sets the fallback colors to the actual colors of the "real" button element. See for example:

jetpack/extensions/shared/submit-button.js

Lines 42 to 44 in 557a85a

	if ( node ) {
	fallbackBackgroundColor = getComputedStyle( button ).backgroundColor;
	}

This means that these two down here will always have a value:

const backgroundColor = backgroundButtonColor.color || fallbackBackgroundColor;
const color = textButtonColor.color || fallbackTextColor;

Now, this is what happens (I'll only talk about the background for clarity):

• Create a block with a button.
  • The button looks red because of the theme styles.
  • fallbackBackgroundColor = red
• Set the button background as black.
  • The background attribute is now black.
  • backgrondColor = backgroundButtonColor.color || fallbackBackgroundColor
    • backgroundColor = black || red
    • backgroundColor = black
  • buttonClasses = has-black-background
  • buttonStyles = background:black
  • The button looks black.
• Clear the background color.
  • backgrondColor = backgroundButtonColor.color || fallbackBackgroundColor
    • backgroundColor = undefined || red
    • backgroundColor = red
  • buttonClasses = undefined
  • buttonStyles = background:red
  • The button looks red.

So far so good.
But let's set the button background as black one more time, then save and reload.

• The button is loaded.
  • The button looks black because of the background attribute.
  • fallbackBackgroundColor = black ⚠️
• Clear the background color.
  • backgrondColor = backgroundButtonColor.color || fallbackBackgroundColor
    • backgroundColor = undefined || black ⚠️
    • backgroundColor = black
  • buttonClasses = undefined
  • buttonStyles = background:black ⚠️
  • The button looks black. ⚠️
• Set the button background as white.
  • The background attribute is now white.
  • backgrondColor = backgroundButtonColor.color || fallbackBackgroundColor
    • backgroundColor = white || black ⚠️
    • backgroundColor = white
  • buttonClasses = has-white-background
  • buttonStyles = background:white
  • The button looks white.
• Clear the background color.
  • backgrondColor = backgroundButtonColor.color || fallbackBackgroundColor
    • backgroundColor = undefined || black ⚠️
    • backgroundColor = black
  • buttonClasses = undefined
  • buttonStyles = background:black ⚠️
  • The button looks black. ⚠️

What does this very verbose example means?

It means that falling back to the fallback colors here is messing up the clear button:

const backgroundColor = backgroundButtonColor.color || fallbackBackgroundColor;
const color = textButtonColor.color || fallbackTextColor;

The first time we add a block we don't realize it, because the fallback color is the original button color. So, clearing would revert the button to the color we expect to see.

But, when we load a button with a different color, this becomes the new fallback.
When we clear the color, instead of having a cool undefined color, we fall back to the fallback color, which is not the original color anymore.

I'm not entirely sure of the purpose of the Core's fallback colors, but in our case they are getting in the way.
If backgroundButtonColor.color is falsy, it actually means that the button doesn't have any set background colors, and we should treat it as such, but instead we give it a background color that may or may not be the correct/expected one.

I hope I've been clear enough, but the comment is very long and verbose so I might not have been clear at all. 😅
Feel free to bother me any time! 👍

[pablinos]

Nice sleuthing @Copons! I didn't test the clearing of the colour, so I'm glad you did.

At first glance, I'm not sure if the new useColors hook will solve this problem. It seems to be searching for background colours through the DOM, but is still using the same computed style technique.

I have seen other resets working this way, that it resets the value to the last saved value, so maybe that is the logic here?

Maybe some testing with the hook is in order, and if that works we can backport the fallback stuff to SubmitButton

[creativecoder]

Per our team discussion today, I'm assigning this to @Copons to re-evaluate for the next steps (whether to review and get this merged or drop it in favor of more holistic updates to block buttons.

[Copons]

Closing this as it will be fixed with the new Button inner block.
I've opened an issue to track the work needed here: #15498