Social Signup: Handle the case where users block third-party cookies/data #13983

pento · 2017-05-12T06:15:40Z

This pull request addresses #13553 by disabling the Google signup button, and showing an error message, when 3rd party cookies are disabled:

Testing instructions

Run git checkout fix/13553-signup-3rd-party-cookies and start your server, or open a live branch
Open the Social Signup page
Check that the "Continue with Google" button is enabled, and there is no error message.
In your browser settings, enabled the "Block third-party cookies" setting.
Refresh the Social Signup page.
Check that the "Continue with Google" button is disabled, and there is an error message.

Reviews

Code
Design
Product

matticbot · 2017-05-12T06:15:41Z

Test live: https://calypso.live/?branch=fix/13553-signup-3rd-party-cookies

pento · 2017-05-12T06:16:28Z

I just hacked in some CSS for the error message, I would appreciate a pointer as to what CSS rules I should actually be using. :-)

gziolo · 2017-05-12T10:04:17Z

client/components/social-buttons/google.js

@@ -19,6 +19,8 @@ class GoogleLoginButton extends Component {
 		fetchBasicProfile: true,
 	};

+	state = { error: false };


It should be initialised to either null or an empty string ``.

gziolo · 2017-05-12T10:05:46Z

client/components/social-buttons/google.js

@@ -45,6 +47,10 @@ class GoogleLoginButton extends Component {
 			return this.initialized;
 		}

+		this.setState( { error: '' } );


Can it be initialized more than once? If not then we can remove this state change.

Yep, initialize() can be called multiple times - this.initialized won't be set until the gapi instance is properly initialised.

gziolo · 2017-05-12T10:06:34Z

client/components/social-buttons/google.js

@@ -73,26 +91,33 @@ class GoogleLoginButton extends Component {
 	}

 	render() {
+		let buttonDisabled = false;


It can be simplified to:

const buttonDisabled = Boolean( this.state.error );

gziolo · 2017-05-12T10:13:37Z

client/components/social-buttons/google.js

+					this.setState( { error: translate( 'Your privacy settings are blocking us from connecting to your Google account. ' +
+						'Please enable "third-party cookies" in your browser or operating system, or log in with an email address instead.'
+					) } );
+					return null;


@Tug should we return rejected Promise in this case, too?

Yes, I believe we can simply remove that return statement.

If i removed the return null, the console shows a JS error:

Object {error: "idpiframe_initialization_failed", details: "Failed to read the 'localStorage' property from 'Window': Access is denied for this document."}

I think it's fine to show an error in the console here. if we want to hide it in a debug() call for instance, that should be done by calling catch on the promise returned by this.initialize().

gziolo · 2017-05-12T10:23:15Z

I just hacked in some CSS for the error message, I would appreciate a pointer as to what CSS rules I should actually be using. :-)

I guess it needs some tweaks :)

I left my comments, those are only nitpicks. It works as advertised. Let's hear from designers before we merge this one.

fabianapsimoes · 2017-05-12T10:48:54Z

I wonder if we could be more proactive about this, and display the error message without a click? It's not a big issue if it's going to mean a lot more work, but it'd be nicer to disable the social buttons from start if they can't be used.

Also, since we're on signup, the copy should probably say "or sign up with an email address" instead of "or log in with an email address".

cc @breezyskies for design input. I'm not sure I've seen errors associated with buttons displayed like we're doing in this PR. I suppose we usually go with a notice in cases like this?

fabianapsimoes · 2017-05-12T10:52:44Z

Also, since we're on signup, the copy should probably say "or sign up with an email address" instead of "or log in with an email address".

Updated this with 61efbdd.

breezyskies · 2017-05-12T10:53:41Z

I'm not sure I've seen errors associated with buttons displayed like we're doing in this PR. I suppose we usually go with a notice in cases like this?

Correct, this would need to be a notice. Given the length of the copy, I don't think we can use global notices in this case, so would need to be the standard notice component.

Tug · 2017-05-12T11:00:03Z

I wonder if we could be more proactive about this, and display the error message without a click?

It's already the case, the lib is loaded when the component mounts. I tested it and it seems to work.

gziolo · 2017-05-12T11:17:37Z

Yes, there is no need to click anything.

breezyskies · 2017-05-12T11:23:33Z

The downside of that is we're showing an error message to someone who might not even choose to use social signup.

fabianapsimoes · 2017-05-12T11:28:24Z

Yes, there is no need to click anything.

Hm... Weird, I must have missed something while testing 🤔

Anyway, now that we're talking about using a notice, I have the same concern as Brie. The notice is quite prominent with its size and color, and might distract users from their actual goals (or just generally cause confusion since third-party cookies are not familiar to all users).

Maybe the best would be to indeed only display the error on click.

gziolo · 2017-05-12T11:57:10Z

What do you think about disable state with tooltip? We could add a small red icon indicating some error inside the button.

davewhitley · 2017-05-12T14:46:41Z

client/components/signup-form/style.scss

@@ -50,3 +50,9 @@
 .signup-form__notice.notice {
 	margin-bottom: 10px;
 }
+
+.social-buttons__service-error {
+	color: red;


Color can only use one of our color variables, in this case $alert-red

davewhitley · 2017-05-12T14:50:09Z

client/components/signup-form/style.scss

+.social-buttons__service-error {
+	color: red;
+	font-size: 11px;
+	margin: 0 10px 10px 10px;


Can be shortened to margin: 0 10px 10px; I believe

davewhitley · 2017-05-12T15:08:25Z

A lot of this text is superfluous.

But, focusing on just the error text:

Shouldn't be red
Google logo should also looked disabled
Probably shouldn't even be there unless they click on the disabled button. Reason is that we don't want to distract users who don't even want to sign up with Google.

I would clean up all of the text I pointed to in the screenshot by doing this:

Or create an account using a social profile. We'll never post without your permission.

[google button disabled state]

Please enable "third-party cookies" to connect your Google account.

[facebook button]

cc @ranh for the shortened error text I wrote

ranh · 2017-05-14T10:50:48Z

Nice improvement on that cookies notice @drw158, 👍

Your suggestion also removes "Connect to your existing social profile to get started faster". This sentence is meant to provide additional context for users who will not immediately understand the function or benefit of the "social" buttons. I don't think it's superfluous, so I think we should keep it.

a8ci18n · 2017-05-15T02:40:46Z

client/components/signup-form/social.jsx

-					{ this.props.translate(
-						"Connect to your existing social profile to get started faster. We'll never post without your permission."
-					) }
+					{ this.props.translate( 'Connect to your existing social profile to get started faster.' ) }


😞 17 existing translations will be lost with this change.

The popover will only show when the button is hovered or clicked.

…vering

drewblaisdell · 2017-05-24T18:41:41Z

LGTM. 👍

drewblaisdell · 2017-05-24T19:10:02Z

Actually, we need to fix the tests first. :) I rebased and they are failing due to the import of Popover in social-buttons/google.js.

…ecause reasons.

pento · 2017-05-25T05:47:32Z

That this can even happen is... less than optimal. 🙃

yurynix · 2017-05-25T08:56:41Z

client/components/social-buttons/google.js

 	render() {
+		let classes = 'social-buttons__button button';


Another option is:

import classNames from 'classnames'; classNames( 'social-buttons__button', 'button', { disabled: this.state.error } );

So many magic functions. 😁

yurynix · 2017-05-25T09:05:49Z

Code looks 👍 It works for me in Chrome when I disable "third party cookies" 👍 All tests pass 👍
IMO lets 🚢 it

pento self-assigned this May 12, 2017

matticbot added the [Size] M Medium sized issue label May 12, 2017

pento added this to the Social Signup: M1 milestone May 12, 2017

gziolo reviewed May 12, 2017

View reviewed changes

fabianapsimoes added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels May 12, 2017

davewhitley reviewed May 12, 2017

View reviewed changes

a8ci18n reviewed May 15, 2017

View reviewed changes

matticbot removed the [Size] M Medium sized issue label May 15, 2017

pento and others added 10 commits May 24, 2017 11:37

Tweak the copy.

a25780c

Add a disabled-style Google logo for when the Google button is disabled.

8f37861

Reject the promise in case of a idpiframe_initialization_failed error

a5f8d5b

Sign up: Show Google service error block only when there is an error

acafb76

Updated info text for social buttons.

e258338

Change the error message to a popover

b8c4049

The popover will only show when the button is hovered or clicked.

Remove an unnecessary newline

e34bad4

Always associate the Popover with the social button

46d8aa6

Switch from current() to currentTarget to find the button when ho…

111f090

…vering

Revert the form text changes from fc3acc4 and 4cf9438

8084b81

drewblaisdell force-pushed the fix/13553-signup-3rd-party-cookies branch from fd5e5cf to 8084b81 Compare May 24, 2017 18:38

When testing, the Login form must be loaded after useFakeDom(), b…

8f0d8ba

…ecause reasons.

pento added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels May 25, 2017

yurynix reviewed May 25, 2017

View reviewed changes

gziolo added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels May 26, 2017

pento merged commit 9fcffd2 into master May 29, 2017

pento deleted the fix/13553-signup-3rd-party-cookies branch May 29, 2017 00:25

matticbot removed the [Status] Ready to Merge label May 29, 2017

ranh mentioned this pull request Jul 20, 2017

Sign Up: Polish form spacing #16391

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Social Signup: Handle the case where users block third-party cookies/data #13983

Social Signup: Handle the case where users block third-party cookies/data #13983

pento commented May 12, 2017 •

edited by Tug

Loading

matticbot commented May 12, 2017

pento commented May 12, 2017

gziolo May 12, 2017

gziolo May 12, 2017

pento May 15, 2017

gziolo May 12, 2017

gziolo May 12, 2017

Tug May 12, 2017

pento May 15, 2017

Tug May 15, 2017

gziolo commented May 12, 2017

fabianapsimoes commented May 12, 2017

fabianapsimoes commented May 12, 2017

breezyskies commented May 12, 2017

Tug commented May 12, 2017 •

edited

Loading

gziolo commented May 12, 2017

breezyskies commented May 12, 2017

fabianapsimoes commented May 12, 2017

gziolo commented May 12, 2017 •

edited

Loading

davewhitley May 12, 2017

davewhitley May 12, 2017

davewhitley commented May 12, 2017 •

edited

Loading

ranh commented May 14, 2017

a8ci18n May 15, 2017

drewblaisdell commented May 24, 2017

drewblaisdell commented May 24, 2017

pento commented May 25, 2017

yurynix May 25, 2017

pento May 25, 2017

yurynix commented May 25, 2017

Social Signup: Handle the case where users block third-party cookies/data #13983

Social Signup: Handle the case where users block third-party cookies/data #13983

Conversation

pento commented May 12, 2017 • edited by Tug Loading

Testing instructions

Reviews

matticbot commented May 12, 2017

pento commented May 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented May 12, 2017

fabianapsimoes commented May 12, 2017

fabianapsimoes commented May 12, 2017

breezyskies commented May 12, 2017

Tug commented May 12, 2017 • edited Loading

gziolo commented May 12, 2017

breezyskies commented May 12, 2017

fabianapsimoes commented May 12, 2017

gziolo commented May 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davewhitley commented May 12, 2017 • edited Loading

ranh commented May 14, 2017

Choose a reason for hiding this comment

drewblaisdell commented May 24, 2017

drewblaisdell commented May 24, 2017

pento commented May 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurynix commented May 25, 2017

pento commented May 12, 2017 •

edited by Tug

Loading

Tug commented May 12, 2017 •

edited

Loading

gziolo commented May 12, 2017 •

edited

Loading

davewhitley commented May 12, 2017 •

edited

Loading