[$250] BUG: Desktop App - Black screen on window close in full-screen mode reported by @fedirjh

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Open Desktop App
2. Set full screen mode
3. Click close button

Expected Result:

Window is hidden

Actual Result:

Full screen gets black

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number: 1.2.18-3 Reproducible in staging?: y Reproducible in production?: y Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos:

https://user-images.githubusercontent.com/43996225/197005671-cd6952ec-748b-4fc9-93bf-a52c7cd22928.mp4

https://user-images.githubusercontent.com/43996225/197005267-69572945-9bcd-4c1e-a7c2-e17ee007d6c5.mov

Expensify/Expensify Issue URL: Issue reported by: (http://github.com/fedirjh) Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1666181224266319

View all open jobs on GitHub

[melvin-bot[bot]]

Triggered auto assignment to @CortneyOfstad (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[fedirjh]

this issue happens when we change the default close logic https://github.com/electron/electron/issues/20263 https://github.com/Expensify/App/blob/299f07bfa29c0dd81a6126691aae29e161097baa/desktop/main.js#L262-L269

We have to exit full screen mode then we hide window

browserWindow.on('close', (evt) => { 
    if (quitting || hasUpdate ) { 
        return; 
    } 
 
    evt.preventDefault(); 

    if (browserWindow.isFullScreen()) { 
        browserWindow.once('leave-full-screen', () => browserWindow.hide()) 
        browserWindow.setFullScreen(false) 
    } else { 
        browserWindow.hide() 
    } 
 
});

Or we just quit

browserWindow.on('close', (evt) => { 
    if (quitting || hasUpdate || browserWindow.isFullScreen() ) { 
        return; 
    } 
 
    evt.preventDefault(); 
    browserWindow.hide() 
 
});

[CortneyOfstad]

Was able to recreate this on my side and had to completely quit the app in order to get out of the black screen

[melvin-bot[bot]]

Triggered auto assignment to @PauloGasparSv (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[PauloGasparSv]

Marking this as External! Found some other fix suggestions on the web other than the one commented here so it's not an uncommon issue with electron

[melvin-bot[bot]]

Triggered auto assignment to @dylanexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Triggered auto assignment to @aldo-expensify (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[dylanexpensify]

Reviewing today!

[thesahindia]

I am not sure if we should go with the workaround suggested here. What's your opinion about it @aldo-expensify ?

[aldo-expensify]

From a short research I agree with it being a bug on the electron side (https://github.com/electron/electron/issues/20263). I think the workaround proposed is fine while the bug is there.

Besides implementing the solution, we could create a new issue like this one https://github.com/electron/electron/issues/20263, but with the electro version we are using. I understand that the existing issue was closed because it mentions an unmaintained electron version.

I tested the proposed solution and the results is good enough for me:

https://user-images.githubusercontent.com/87341702/197594263-4a6f23cc-72bd-44d3-ba92-2869ec61485c.mov

[thesahindia]

What are the next steps here?

[aldo-expensify]

What are the next steps here?

I would go with this proposal: https://github.com/Expensify/App/issues/12042#issuecomment-1285900417 (if it is a proposal 😆 )

[thesahindia]

Cool, let's move forward with @fedirjh's proposal (first approach).

C+ reviewed 🎀👀🎀

[dylanexpensify]

reviewing today!

[melvin-bot[bot]]

📣 @fedirjh You have been assigned to this job by @aldo-expensify! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[melvin-bot[bot]]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist will need to be completed before the issue can be closed:

• [ ] A regression test has been added or updated so that the same bug will not reach production again. Link to the updated test here:
• [ ] The PR that introduced the bug has been identified. Link to the PR:
• [ ] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [ ] A discussion in #contributor-plus has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [ ] Payment has been made to the issue reporter (if applicable)
• [ ] Payment has been made to the contributor that fixed the issue (if applicable)
• [ ] Payment has been made to the contributor+ that helped on the issue (if applicable)

[michaelhaxhiu]

@dylanexpensify For this checklist step, I think you can best handle it by posting in #contributor-plus slack channel and using this type of approach (since this checklist is actively evolving):

Hey @user1, @user2, @user3, @user4 - this PR (link) caused this bug (link). What do you think we could have done differently in the original PR to prevent this bug from being introduced?

[dylanexpensify]

ah nice one!

[dylanexpensify]

@thesahindia and @fedirjh any idea on the offending PR that led to this bug?

[thesahindia]

@dylanexpensify, as mentioned at https://github.com/Expensify/App/issues/12042#issuecomment-1285900417, It is an electron bug. Not a regression.

[dylanexpensify]

reviewing today!

[dylanexpensify]

@fedirjh can you let us know when your PR will be ready for review?

[thesahindia]

It's already in production.

[fedirjh]

@fedirjh can you let us know when your PR will be ready for review?

https://github.com/Expensify/App/pull/12368 : Deployed to production

[dylanexpensify]

Working through steps on checklist

[melvin-bot[bot]]

@fedirjh, @dylanexpensify, @thesahindia, @aldo-expensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

[dylanexpensify]

reviewing today!

[dylanexpensify]

reviewing today mel!

[thesahindia]

@dylanexpensify, it's ready to be settled. The PR was deployed to production 11 days ago