-
Notifications
You must be signed in to change notification settings - Fork 30
feat: Add Google One Tap #14335
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
feat: Add Google One Tap #14335
Conversation
Hello 👋! When you're ready to run Chromatic, please apply the You will need to reapply the label each time you want to run Chromatic. |
Co-authored-by: Mahesh Makani <[email protected]>
…ipt islands are loaded
…re displaying FedCM prompt
797216e
to
ec2df7d
Compare
<Island priority="enhancement" defer={{ until: 'idle' }}> | ||
<GoogleOneTap /> | ||
</Island> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is a server side test, in which case could we wrap the island with a check to see if the test is active? It would mean the island wouldn't get async imported or hydrated unless actually required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good point! At the moment its a client side test, but I could make it a server side one. I'd have to use one of the limited 0% experiment slots though, is that alright?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats fine there's still a couple available. First come, first served!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, I've switched it to a served side experiment in guardian/frontend#28160 and pushed a commit in this PR to use it. Thanks for the suggestion!
…unecessary bundles
export const isInGoogleOneTapTest = (tests: ServerSideTests): boolean => | ||
tests['googleOneTapVariant'] === 'variant'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies if I'm pointing out the obvious here but this is fine for the 0% test as we only want to initiate if the user is in the variant. However, if this does become a sampled test > 0% you'd want to initialise if the user is in the control or variant of the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay, so would this mean that if we roll this out to users we'd basically have 3 scenarios?
A. googleOneTapVariant === 'variant'
-> User is in test and we should load and initialize Google One Tap
B. googleOneTapVariant === 'control'
-> User is in test and we should load but not initialize Google One Tap
C. googleOneTapVariant === undefined
-> User isn't in test, don't load or hydrate the island at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good q, actually if I can revise my comment, I think it depends on what you want from your test.
If we want D&I to analyse any statistical difference between control and variant wrt core web vitals, click-through rates, bounce-rate etc, then we need to decide on the control and variant behaviour we want to compare.
If we want to compare the complete absence of GOT (i.e. not loading GOT at all including the island) vs loading and running GOT then the control shouldn't initialise GOT.
If we want to measure a closer simulation to what a production release might look like, where I assume the decision to initialise GOT is made client-side then the island will be included by default and the test would measure the difference between loading the island but not running GOT (control) vs running GOT.
Those could be variants of the same test or two separate tests depending on how far this work goes.
Just thought I would raise now in case we get to a sampled test.
For a 0% test the current setup is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is a 0% test and we only load the island if in the server side test variant this is okay for now.
If we do move to a sampled bucket it would be good to have a wider conversation about adding this to the site.
writable: true, | ||
}); | ||
|
||
describe('GoogleOneTap', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding tests!
What does this change?
Implements Google One Tap by relying on the built-in FedCM API instead of the Google GSI library. This allows us having to download and execute a ~80KB (compressed) third party bundle from Google to initialize Google One Tap, it also avoids some data privacy concerns about loading an additional google library.
In theory directly using FedCM and bypassing the Google GSI library limits browser support as Firefox and Safari don't support FedCM, Mozilla (firefox) in particular have some genuinely good concerns about the current FedCM specification. In practice, we were already planning to limit Google One Tap to just Chrome users, and support for Firefox was already deprecated by Google GSI due to its reliance on third-party cookies. Googles recommendation for new Google One Tap applications is to enable the switch that uses FedCM under the hood anyway, thus breaking Firefox support.
For the time being this will still remain behind a 0% test whilst we work on the server side implementation of Google One Tap in Gateway, but by having the One Tap functionality behind the 0% test we can start to test the server side implementation without having to take DCR CODE.
Planned Flow
email
field from it.login_hint
parameter.If the
login_hint
parameter matches the email of the users current session Google automatically redirects the user back to us with the credentials we need to create their Okta account without prompting the user for any input.Steps 6 might change as its not an ideal experience for the user to get sent to google and then back to us, they would see a white flash whilst Google verifies they have a valid session. What we might try instead is to extract the users email address from the token and directly create the users Okta account, but this is going to have to be something we experiment once we get to the server side implementation
Potential Issues
I don't know how happy Google are with us using Google One Tap without using their GSI library. I couldn't find any details about how to directly implement FedCM without using GSI so some amount of this PR has been reverse engineered by observing network traffic made by initializing GSI, for example I couldn't find any public reference to the FedCM
configUrl
and had to find it from the network traffic.The GSI library is also not versioned, meaning Google could update their FedCM endpoints and rollout a change to their library at the same time and break our implementation, hopefully this would simply cause Google One Tap to temporarily stop appearing on the site.
We're also unable to change the positioning of Google One Tap which may be an issue when it comes to displaying it over top nav ads, or the Guardian logo. We're pending decisions from stakeholders about which scenarios the banner should be showed in.
Screenshots