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

[$500] Web - Login - Sign in with Google banner in ../sign-in-with-google page jumps #28080

Closed
1 of 6 tasks
lanitochka17 opened this issue Sep 23, 2023 · 49 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review

Comments

@lanitochka17
Copy link

lanitochka17 commented Sep 23, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Go to https://staging.new.expensify.com/sign-in-with-google
    on Safari
  2. Refresh the page

Expected Result:

The Sign in with Google banner will not jump

Actual Result:

The Sign in with Google banner jumps

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.73-0

Reproducible in staging?: Yes

Reproducible in production?: Yes

If this was caught during regression testing, add the test name, ID and link from TestRail:

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug6211713_20230924_000637.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01d1eaacbfa4ad6b5d
  • Upwork Job ID: 1707431368733585408
  • Last Price Increase: 2023-10-19
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 23, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 23, 2023

Triggered auto assignment to @maddylewis (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot
Copy link

melvin-bot bot commented Sep 23, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@melvin-bot melvin-bot bot added the Overdue label Sep 26, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 27, 2023

@maddylewis Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot melvin-bot bot removed the Overdue label Sep 28, 2023
@maddylewis
Copy link
Contributor

i can reproduce this on Safari:

2023-09-28_12-23-14.mp4

@maddylewis maddylewis added the External Added to denote the issue can be worked on by a contributor label Sep 28, 2023
@melvin-bot melvin-bot bot changed the title Web - Login - Sign in with Google banner in ../sign-in-with-google page jumps [$500] Web - Login - Sign in with Google banner in ../sign-in-with-google page jumps Sep 28, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 28, 2023

Job added to Upwork: https://www.upwork.com/jobs/~01d1eaacbfa4ad6b5d

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 28, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 28, 2023

Triggered auto assignment to Contributor-plus team member for initial proposal review - @situchan (External)

@ghost
Copy link

ghost commented Sep 28, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

Sign-in with Google banner keeps jumping up when page is refreshed

What is the root cause of that problem?

GoogleSigninButton component is not rendered properly

What changes do you think we should make in order to solve the problem?

Use SocialIcon component that has a built-in type for Google.

What alternative solutions did you explore? (Optional)

N/A

@solomon-ochepa
Copy link

Hi everyone, I'm Solomon Ochepa by name. I'm a full-stack developer (General PHP, Laravel, CodeIgniter, etc.) with more than ten (10) years experience.

I saw your post on the UpWork platform. Before proceeding, I'd like to know the stack.

Thanks.

@melvin-bot
Copy link

melvin-bot bot commented Sep 28, 2023

📣 @solomon-ochepa! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@solomon-ochepa
Copy link

Contributor details
Expensify account email: solomonochepa@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01dfbdd135c64803fa

@studentofcoding
Copy link
Contributor

studentofcoding commented Sep 28, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue

Web - Login - Sign in with Google banner in ../sign-in-with-google page jumps

What is the root cause of that problem?

We call an external script on src/components/SignInButtons/GoogleSignIn/index.website.js, but the component is rendered first before all the styling finishes loading. This makes the layout shift, when the styling is loaded, and makes the button jump

function GoogleSignIn({translate, isDesktopFlow}) {
const loadScript = useCallback(() => {
const google = window.google;
if (google) {
google.accounts.id.initialize({
client_id: CONFIG.GOOGLE_SIGN_IN.WEB_CLIENT_ID,
callback: signIn,
});
// Apply styles for each button
google.accounts.id.renderButton(document.getElementById(mainId), {
theme: 'outline',
size: 'large',
type: 'icon',
shape: 'circle',
});
google.accounts.id.renderButton(document.getElementById(desktopId), {
theme: 'outline',
size: 'large',
type: 'standard',
shape: 'pill',
width: '300px',
});
}
}, []);
React.useEffect(() => {
const script = document.createElement('script');
script.src = 'https://accounts.google.com/gsi/client';
script.addEventListener('load', loadScript);
script.async = true;
document.body.appendChild(script);
return () => {
script.removeEventListener('load', loadScript);
document.body.removeChild(script);
};
}, [loadScript]);

If we go to the network tab and reload the page, we can verify that the Button showed first and the layout shift (button jump) is happening when font 4UabrENHsxJlGDuGo1OIlLU94YtzCwM.ttf is loaded (after https://accounts.google.com/gsi/button/* is called)

Screenshot 2023-10-18 at 16 27 49 Screenshot 2023-09-29 at 01 58 45

What changes do you think we should make in order to solve the problem?

We need to wait for all the script and styling inside the GoogleSignIn button to be finished first and then render the component afterward.

This way when GoogleSignIn button shows, it won't jump. As we are using Google 3rd-party button thus the optimal way to achieve this is to:
Add network listener to waiting for last font load style (from https://fonts.gstatic.com/s/*) but as the last logged activity is from https://accounts.google.com/gsi/button/* as listed below (or one activity before font load)

Screenshot 2023-10-18 at 16 32 27

Then the only way to capture all the activity is adding setTimeout of 250ms** (as from the test it's ranging from > 80ms to 250ms, with the example below & on video result)

Screenshot 2023-10-18 at 15 47 41

Here are the complete code solution of it

const loadScript = useCallback(() => {
        const google = window.google;
        if (google) {
            google.accounts.id.initialize({
                client_id: CONFIG.GOOGLE_SIGN_IN.WEB_CLIENT_ID,
                callback: signIn,
            });
            // Apply styles for each button
            google.accounts.id.renderButton(document.getElementById(mainId), {
                theme: 'outline',
                size: 'large',
                type: 'icon',
                shape: 'circle',
            });
            google.accounts.id.renderButton(document.getElementById(desktopId), {
                theme: 'outline',
                size: 'large',
                type: 'standard',
                shape: 'pill',
                width: '300px',
            });
            if (!isDesktopFlow) {
                document.getElementById(mainId).style.opacity = 0;
            }
            document.getElementById(desktopId).style.opacity = 0;

            // Add network listener to waiting for the last style (https://accounts.google.com/gsi/button) from Google Sign Button to be loaded
            const checkAllStyleLoaded = setInterval(() => {
                const entries = window.performance.getEntriesByType('resource');

                const buttonStyleLoaded = _.some(entries, entry => entry.name.includes('https://accounts.google.com/gsi/button') && entry.duration > 0);
                if (buttonStyleLoaded) {
                    clearInterval(checkAllStyleLoaded);
                    // As styling implemented a little late on Safari we add timeout of 250ms to ensure all the style is implemented with no layout shift
                    setTimeout(() => {
                        if (!isDesktopFlow) {
                            document.getElementById(mainId).style.opacity = 1;
                        }
                        document.getElementById(desktopId).style.opacity = 1;
                    }, 250);
                }
            }, 50);
        }
}, [isDesktopFlow]);

React.useEffect(() => {
        const script = document.createElement('script');
        script.src = 'https://accounts.google.com/gsi/client';
        script.addEventListener('load', loadScript);
        script.async = true;
        script.onload = () => {
            loadScript();
        };
    
        document.body.appendChild(script);

        return () => {
            script.removeEventListener('load', loadScript);
            document.body.removeChild(script);
        };
}, [loadScript]);

What alternative solutions did you explore? (Optional)

We can directly waiting for all script & style to be loaded and implemented with 750ms, but it's not desirable

Result

Safari
https://github.com/Expensify/App/assets/20473526/8eab98d9-0c2c-467c-8134-98adc5e563c9

Chrome
https://github.com/Expensify/App/assets/20473526/d503c890-e97b-449d-8aa3-a2cd3715f482

mWeb Safari
https://github.com/Expensify/App/assets/20473526/a8cc722f-9765-4d8a-b35d-c01c1febe3e2

mWeb Chrome
https://github.com/Expensify/App/assets/20473526/e580fc19-dc00-440a-b782-df68518f2d55

@maddylewis @situchan

@melvin-bot melvin-bot bot added the Overdue label Oct 2, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 2, 2023

@maddylewis, @situchan Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@maddylewis
Copy link
Contributor

@situchan - bump on proposal review - thanks!!

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Oct 2, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 5, 2023

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@situchan
Copy link
Contributor

situchan commented Oct 5, 2023

I don't see solid proposal yet

@melvin-bot melvin-bot bot removed the Overdue label Oct 5, 2023
@studentofcoding
Copy link
Contributor

I don't see solid proposal yet

Do you have any feedback on my proposal?

We can see from the Network activity that the Button showed first and the layout shift (button jump) is happening when font 4UabrENHsxJlGDuGo1OIlLU94YtzCwM.ttf is downloaded & loaded as my proposal, thus I believe this is the Root Cause of it

@melvin-bot
Copy link

melvin-bot bot commented Oct 7, 2023

@maddylewis @situchan this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

@studentofcoding
Copy link
Contributor

studentofcoding commented Oct 25, 2023

If the alternative is fixing it via changing the width and height for googlePillButtonContainer, like the one shared here then it's only fixing the problem on Chromium-based Browser (Chrome, Brave, etc) and not Safari.

Here is the demo via fixing style, below

  • Chromium Browser
button_chrome.mov
  • Safari
button_safari.mp4

Based on my last update, the RCA is not on the button styling (that we can control like above), but rather on how some Google Sign-In styles are loaded late. Thus, if there's a simpler way to fix it, then I'm very open to learning more, but I believe the only way to fix this is by showing the button after all the late styles are loaded.

cc: @situchan @maddylewis

@maddylewis
Copy link
Contributor

i'll let @situchan confirm whether or not @studentofcoding's proposal makes sense to move forward with.

@mkhutornyi
Copy link
Contributor

mkhutornyi commented Oct 29, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

"Sign in with Google" button jumps on Safari

What is the root cause of that problem?

script.src = 'https://accounts.google.com/gsi/client';

This button uses external script and this view displays instantly before style is loaded

What changes do you think we should make in order to solve the problem?

We already had similar issue in the past - #25768
The solution was to hide overflow. We can apply same solution here.

App/src/styles/styles.ts

Lines 3574 to 3578 in e99c297

googlePillButtonContainer: {
colorScheme: 'light',
height: 40,
width: 219,
},

Add overflow: 'hidden'

And width is wrongly set. 300 is the correct width - width: 300

google.accounts.id.renderButton(document.getElementById(desktopId), {
theme: 'outline',
size: 'large',
type: 'standard',
shape: 'pill',
width: '300px',

@melvin-bot melvin-bot bot added the Overdue label Oct 29, 2023
@situchan
Copy link
Contributor

I am fine with @mkhutornyi's proposal as using same solution applied in circle button.
🎀 👀 🎀 C+ reviewed

@melvin-bot
Copy link

melvin-bot bot commented Oct 30, 2023

Triggered auto assignment to @marcaaron, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Nov 1, 2023
@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Nov 27, 2023
Copy link

melvin-bot bot commented Nov 27, 2023

This issue has not been updated in over 15 days. @marcaaron, @maddylewis, @mkhutornyi, @situchan eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@marcaaron
Copy link
Contributor

This one landed on production a while ago. But seems like the issue got stuck. Did everyone who was supposed to get paid get paid here?

@marcaaron marcaaron added Daily KSv2 External Added to denote the issue can be worked on by a contributor and removed Monthly KSv2 labels Nov 29, 2023
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 29, 2023
@marcaaron marcaaron removed Internal Requires API changes or must be handled by Expensify staff Help Wanted Apply this label when an issue is open to proposals by contributors labels Nov 29, 2023
Copy link

melvin-bot bot commented Nov 29, 2023

Current assignee @situchan is eligible for the External assigner, not assigning anyone new.

@situchan
Copy link
Contributor

No payment yet

@maddylewis
Copy link
Contributor

maddylewis commented Dec 1, 2023

Payment Summary

  • Reporting bonus - N/A
  • C+ - @situchan ($500 via Upwork)
  • Contributor - @mkhutornyi ($500 via Upwork)

the job is closed so creating a new one and sending offers.

@maddylewis
Copy link
Contributor

@mkhutornyi - remind me, are you a contractor or do you require payment through Upwork?

@mkhutornyi
Copy link
Contributor

mkhutornyi commented Dec 4, 2023

@maddylewis upwork please, similar to #30616 (comment)

@maddylewis
Copy link
Contributor

thanks! paid 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

7 participants