Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: improve selection of webview #1727

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

pollend
Copy link
Contributor

@pollend pollend commented Jan 22, 2025

I played with a couple of solutions; sometimes there is an id that links the view back to the containing webview but that is not consistent across vscode. The main issue is the webviews are elevated to the top of the dom so you can't easily tell if that webview is associated with a given element. Testing the overlap seems the most consistent way to approach this?

ref: #1492

@pollend pollend force-pushed the bugfix/improve-selection-webview branch from f4fc8a7 to 0b2cc9e Compare January 22, 2025 19:27
@@ -627,7 +627,7 @@ export async function findBestContainingElement(container: IRectangle, testEleme
const ax = Math.max(container.x, rect.x);
const ay = Math.max(container.y, rect.y);
const bx = Math.min(container.x + container.width, rect.x + rect.width);
const by = Math.min(container.y + container.width, rect.y + rect.height);
const by = Math.min(container.y + container.height, rect.y + rect.height);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made a small mistake here.

@djelinek djelinek force-pushed the bugfix/improve-selection-webview branch from 6c88fcb to 6ce70bb Compare February 3, 2025 15:00
@djelinek djelinek self-requested a review February 3, 2025 15:00
@djelinek
Copy link
Collaborator

djelinek commented Feb 3, 2025

Hello,
thank you for your PR and time you have invested here, really appreciated! I will take a look closer ASAP this week.

From top of my head, it would be nice to also have test cases which are handling multiple WebView types opened at same time.

Basically it can happen state when, Editor WebView / BottomBar WebView and SideBar WebView are open/visible at same time. This is a edge case which was reported in a past also

@pollend
Copy link
Contributor Author

pollend commented Feb 3, 2025

Hello, thank you for your PR and time you have invested here, really appreciated! I will take a look closer ASAP this week.

From top of my head, it would be nice to also have test cases which are handling multiple WebView types opened at same time.

Basically it can happen state when, Editor WebView / BottomBar WebView and SideBar WebView are open/visible at same time. This is a edge case which was reported in a past also

I have one test where I open multiple grouped webview but are you saying I should add another test for BottomBar/Editor Webview and Editor Webview/sidebar

@djelinek
Copy link
Collaborator

djelinek commented Feb 3, 2025

Hello, thank you for your PR and time you have invested here, really appreciated! I will take a look closer ASAP this week.
From top of my head, it would be nice to also have test cases which are handling multiple WebView types opened at same time.
Basically it can happen state when, Editor WebView / BottomBar WebView and SideBar WebView are open/visible at same time. This is a edge case which was reported in a past also

I have one test where I open multiple grouped webview but are you saying I should add another test for BottomBar/Editor Webview and Editor Webview/sidebar

That is awesome what you have added, it brings a value. I am more expressing my first thoughts here about complexity of problem.

What I am saying is that we need to cover also case, mentioned above, with multiple webviews of different kind opened in a same time, probably in a new standalone test. It can be also checked manually if this is working and we can provide other test in another PR. I am just raising it to not forget about that 🙂

Michael Pollind added 5 commits February 12, 2025 14:07
ref: redhat-developer#1492

Signed-off-by: Michael Pollind <quic_mpollind@quicinc.com>
Signed-off-by: Michael Pollind <quic_mpollind@quicinc.com>
Signed-off-by: Michael Pollind <quic_mpollind@quicinc.com>
Signed-off-by: Michael Pollind <<quic_mpollind@quicinc.com>
Signed-off-by: Michael Pollind <quic_mpollind@quicinc.com>
@djelinek djelinek force-pushed the bugfix/improve-selection-webview branch from 6ce70bb to ac99444 Compare February 12, 2025 13:07
@djelinek djelinek changed the title bugfix: improve selection of webview fix: improve selection of webview Feb 13, 2025
@djelinek djelinek added ready-for-review Pull Request is Ready for Review prio bug Something isn't working labels Feb 13, 2025
@pollend
Copy link
Contributor Author

pollend commented Feb 18, 2025

sorry, for the lack of response I've been on a bit of a trip. I'll see if I can address your comments by the end of this week.

Signed-off-by: Michael Pollind <quic_mpollind@quicinc.com>
@rajwcg
Copy link

rajwcg commented Feb 26, 2025

I am eagerly waiting this issue to be fixed. I am facing an issue with multiple web views on the page—one in the sidebar and another in the editor. After the sidebar view is launched, I am unable to locate the editor web view. Could you please help me resolve this? I have already tried using the openEditor method from the EditorView but it didn’t work.

@pollend
Copy link
Contributor Author

pollend commented Mar 11, 2025

Hello, @djelinek where have we left off, is there anything else that needs to be address?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working prio ready-for-review Pull Request is Ready for Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants