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.

Sign up for GitHub

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

Jump to bottom

[BUG] browser.close does not resolve after right-click on Webkit headful #2700

Closed
arjunattam opened this issue Jun 25, 2020 · 3 comments · Fixed by #2802 or #2879
Closed

[BUG] browser.close does not resolve after right-click on Webkit headful #2700

arjunattam opened this issue Jun 25, 2020 · 3 comments · Fixed by #2802 or #2879
Assignees
@aslushnikov @JoelEinbinder

Comments

@arjunattam
Copy link
Contributor

arjunattam commented Jun 25, 2020 •
edited
Loading

Context:

  • Playwright Version: 1.1.1
  • Operating System: Tested on Windows and Mac
  • Browser: Webkit

Code Snippet

Similar to #2699. When running Webkit in headful mode, the browser.close does not resolve because it leaves a dangling context menu. Pressing escape or clicking on a menu item resolves browser.close.

const {webkit} = require('playwright');

(async () => {
    const browser = await webkit.launch({headless: false});
    const context = await browser.newContext();
    const page = await context.newPage();
    await page.goto("https://www.bing.com");
    await page.click("//*[@id='b_logo']", {button: 'right'});
    await browser.close();
})();
aslushnikov added a commit to aslushnikov/playwright that referenced this issue Jul 2, 2020
@aslushnikov
browser(webkit): let web page close when it has open context menu
cdeb53d 
Currently, if web page has an open context menu, then it won't close.
This prevents browser from quitting.

In stock safari, this behavior can also be observed in a way that
context menu will stay opened even if related page got closed.

While investigating this behavior on Mac, a crucial observation was
that `[NSMenu popUpContextMenu]` is spawning a nested event loop,
keeping reference to `WebContextMenuProxyMac` instance, which in turn
keeps references on associated `NSView` with `WKWebView`.

To exit the loop, we need to explicitly cancel context menu. For this,
this patch adds a method `hide` on `WebContextMenuProxy` that uses
port-specific code to cancel context menu.

Windows part of this patch is somewhat speculative: I didn't check
it, but given the same symptomps, I applied the same solution.

Fixes microsoft#2700
@aslushnikov aslushnikov mentioned this issue Jul 2, 2020
Merged
aslushnikov added a commit that referenced this issue Jul 2, 2020
@aslushnikov
browser(webkit): let web page close when it has open context menu (#2802
14162f8 
)

Review URL: aslushnikov/WebKit@42f86e9

Currently, if web page has an open context menu, then it won't close.
This prevents browser from quitting.

In stock safari, this behavior can also be observed in a way that
context menu will stay opened even if related page got closed.

While investigating this behavior on Mac, a crucial observation was
that `[NSMenu popUpContextMenu]` is spawning a nested event loop,
keeping reference to `WebContextMenuProxyMac` instance, which in turn
keeps references on associated `NSView` with `WKWebView`.

To exit the loop, we need to explicitly cancel context menu. For this,
this patch adds a method `hide` on `WebContextMenuProxy` that uses
port-specific code to cancel context menu.

Windows part of this patch is somewhat speculative: I didn't check
it, but given the same symptomps, I applied the same solution.

Fixes #2700
@aslushnikov
Copy link
Collaborator

Oops too early to close! This requires a test and a roll.

@aslushnikov aslushnikov reopened this Jul 2, 2020
aslushnikov added a commit to aslushnikov/playwright that referenced this issue Jul 2, 2020
@aslushnikov
test: add failing test for context menus that prevent browser close
5bcee4a 
References microsoft#2700
@aslushnikov aslushnikov mentioned this issue Jul 2, 2020
Merged
@aslushnikov
Copy link
Collaborator

Turns out Chromium is also susceptible to this!

aslushnikov added a commit that referenced this issue Jul 2, 2020
@aslushnikov
test: add failing test for context menus that prevent browser close (#…
f9f3aeb 
…2811)

References #2700
aslushnikov added a commit to aslushnikov/playwright that referenced this issue Jul 2, 2020
@aslushnikov
feat(webkit): roll WebKit to r1298
5eee35f 
References microsoft#2700
This was referenced Jul 2, 2020
Merged
Merged
aslushnikov added a commit to aslushnikov/playwright that referenced this issue Jul 3, 2020
@aslushnikov
feat(webkit): roll webkit to r1301
fc86dbe 
References microsoft#2700
@aslushnikov aslushnikov mentioned this issue Jul 3, 2020
Merged
aslushnikov added a commit that referenced this issue Jul 3, 2020
@aslushnikov
feat(webkit): roll webkit to r1301 (#2827)
7f60c4d 
References #2700
@aslushnikov aslushnikov added v1.3 and removed v1.2 labels Jul 7, 2020
@aslushnikov
Copy link
Collaborator

Upstream chromium patch: https://chromium-review.googlesource.com/c/chromium/src/+/2285520

aslushnikov added a commit to aslushnikov/playwright that referenced this issue Jul 8, 2020
@aslushnikov
feat(chromium): roll Chromium to r786218
3b11878 
This roll includes:
- https://crrev.com/786119 - mac: close context menu when destroying RenderViewContextMenuMacCocoa

Fixes microsoft#2700
@aslushnikov aslushnikov mentioned this issue Jul 8, 2020
Merged
aslushnikov added a commit that referenced this issue Jul 9, 2020
@aslushnikov
feat(chromium): roll Chromium to r786218 (#2879)
458aaa5 
This roll includes:
- https://crrev.com/786119 - mac: close context menu when destroying RenderViewContextMenuMacCocoa

Fixes #2700
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@aslushnikov aslushnikov

@JoelEinbinder JoelEinbinder

Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@arjunattam @aslushnikov @pavelfeldman @JoelEinbinder