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

[FIX] menus are not closed when clicking inside webviews #8633

Merged
merged 1 commit into from
Oct 27, 2020

Conversation

shahar-h
Copy link
Contributor

Signed-off-by: Shahar Harari [email protected]

What it does

Menus are now closed when clicking inside webviews.
In order to achieve this mousedown event needs to be dispatched to main window.
Fixes #7752

How to test

See described steps in #7752.

Review checklist

Reminder for reviewers

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shahar-h do you mind updating the pull-request to not include unnecessary changes (formatting)?

@vince-fugnitto vince-fugnitto added the menus issues related to the menu label Oct 14, 2020
@vince-fugnitto
Copy link
Member

Please note that there is already an open pull-request which attempts to resolve the issue: #8576.

@shahar-h
Copy link
Contributor Author

@shahar-h do you mind updating the pull-request to not include unnecessary changes (formatting)?

Sorry for that, VS Code converted all tabs to spaces, reverted all unrelated changes.

@shahar-h
Copy link
Contributor Author

Please note that there is already an open pull-request which attempts to resolve the issue: #8576.

I know, I'm trying to address only #7752 with a different approach.

@shahar-h
Copy link
Contributor Author

@vince-fugnitto Windows build is failing with:

Downloading tarball...
Failed to download https://yarnpkg.com/downloads/1.13.0/yarn-v1.13.0.tar.gz.
The command "curl -o- -L https://yarnpkg.com/install.sh | bash -s -- --version 1.13.0" failed and exited with 1 during .

@shahar-h
Copy link
Contributor Author

@vince-fugnitto this PR attempts to resolve only #7752 without breaking other functionality.
Can we go forward with it?

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shahar-h the following pull-request introduces a flickering bug when navigating menus which I believe the previous pull-request attempted to address. For example, you can see how not only the tab flickers, but also the toolbar items which gives a bad user experience:

webview

@shahar-h
Copy link
Contributor Author

the following pull-request introduces a flickering bug when navigating menus which I believe the previous pull-request attempted to address.

This issue is not related to my PR, it is also reproduced on master.

@vince-fugnitto
Copy link
Member

the following pull-request introduces a flickering bug when navigating menus which I believe the previous pull-request attempted to address.

This issue is not related to my PR, it is also reproduced on master.

@shahar-h this is master, the toolbar items on master do not flicker (disappear and reappear completely):

master

As compared to the pull-request:

webview

@shahar-h
Copy link
Contributor Author

@vince-fugnitto I'm not sure how you checked, I've just reproduced the same behavior on master:
flickering

BTW, is there any reason there are two Open Preview to the Side buttons?
image

@vince-fugnitto
Copy link
Member

@vince-fugnitto I'm not sure how you checked, I've just reproduced the same behavior on master:

I was able to reproduce more consistently on master as well 👍

BTW, is there any reason there are two Open Preview to the Side buttons?
image

There are two items since we provide the functionality from @theia/preview and the builtin extension (plugin).
In an actual application, we'd only choose one extension to provide the functionality: #6984

@vince-fugnitto vince-fugnitto added the webviews issues related to webviews label Oct 26, 2020
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionally the changes work very well 👍
I'll open a follow-up issue regarding the webview stealing focus and how toolbar items flicker.

Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM otherwise.

packages/plugin-ext/src/main/browser/webview/webview.ts Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
menus issues related to the menu webviews issues related to webviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Menu not collapsed when clicking on webviews
3 participants