electron / electron

:electron: Build cross-platform desktop apps with JavaScript, HTML, and CSS
https://electronjs.org
MIT License
113.68k stars 15.3k forks source link

`zoomin`, `zoomout`, `resetzoom` roles target last created <webview>, not focused <webview> #28068

Open andersk opened 3 years ago

andersk commented 3 years ago

Preflight Checklist

Issue Details

Expected Behavior

A menu item with the zoomin, zoomout, or resetzoom role should affect the currently focused web contents.

Actual Behavior

A menu item with the zoomin, zoomout, or resetzoom role affects the most recently created <webview>, whether or not it is focused. This is due to a problem with WebContents#isFocused (see below).

To Reproduce

Run this Electron Fiddle: https://gist.github.com/andersk/86ab57716125046320b566f928bc3563. Focus one of the three text boxes, and use the Ctrl+Shift+Plus, Ctrl+Minus, Ctrl+0 accelerators for zoom in, zoom out, reset zoom. These should affect the focused <webview> (or the outer web contents if neither are focused), but instead they all affect the second <webview> regardless of focus.

Screenshots

https://user-images.githubusercontent.com/26471/110422296-f3736900-8053-11eb-8bdd-944e89617c76.mp4

Additional Information

This is the same as #20667, which was auto-closed since the original Electron version in that report has become unsupported. To quote my analysis there:

https://github.com/electron/electron/blob/3ae63c9a06a4b5c9074572ee8b7137520fe4ba4e/lib/browser/api/web-contents.ts#L731-L741

getFocusedWebContents uses WebContents#isFocused to check whether each webview is focused, but empirically, WebContents#isFocused returns true as long as the window is focused regardless of how many webviews are in the window. So getFocusedWebContents will always return the first webview it finds, not the focused webview.

andersk commented 2 years ago

Although this is labeled 12-x-y, it still reproduces in all supported versions. Can someone label this 13-x-y, 14-x-y, 15-x-y, and 16-x-y so this doesn’t get auto-closed by the triage bot again?

brrd commented 2 years ago

I can reproduce in Electron versions 17 and 19. webContents.getFocusedWebContents() is broken when the browser window contains multiple webviews.

andersk commented 1 year ago

Still a bug in 21.0.1, 22.0.0-alpha.1, and 23.0.0-nightly.20221004.

github-actions[bot] commented 1 year ago

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. If you have any new additional information—in particular, if this is still reproducible in the latest version of Electron or in the beta—please include it with your comment!

brrd commented 1 year ago

webContents.getFocusedWebContents() is still broken in electron 22.0.2. It returns a random webview, not the focused one.

andersk commented 1 year ago

Still an issue in 22.0.3, 23.0.0-beta.4, and 24.0.0-nightly.20230120.

github-actions[bot] commented 1 year ago

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. If you have any new additional information—in particular, if this is still reproducible in the latest version of Electron or in the beta—please include it with your comment!

andersk commented 1 year ago

Still broken in 24.1.2, 25.0.0-alpha.4, and 26.0.0-nightly.20230421. As a reminder, my report above includes a direct pointer to the buggy code in getFocusedWebContents, which is still the same today.

(Do the developers actually find this stale issue bot useful? I sure don’t.)

github-actions[bot] commented 1 year ago

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. If you have any new additional information—in particular, if this is still reproducible in the latest version of Electron or in the beta—please include it with your comment!

andersk commented 1 year ago

Still broken in 25.0.1, 26.0.0-beta.6, and 27.0.0-nightly.20230721.

electron-issue-triage[bot] commented 10 months ago

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. If you have any new additional information—in particular, if this is still reproducible in the latest version of Electron or in the beta—please include it with your comment!

andersk commented 10 months ago

Still broken in 27.0.2, 28.0.0-alpha.4, and 29.0.0-nightly.20231020.

electron-issue-triage[bot] commented 7 months ago

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. If you have any new additional information—in particular, if this is still reproducible in the latest version of Electron or in the beta—please include it with your comment!

andersk commented 7 months ago

This is still an issue, and I’ve helped to diagnose it above, but I’m getting tired of having this useless conversation with a bot. If a human has a better reason than “another 90 days have passed” to suspect this might have been fixed, I’ll happily retest. Otherwise I’m just going to keep mindlessly bumping this.

electron-issue-triage[bot] commented 4 months ago

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. If you have any new additional information—in particular, if this is still reproducible in the latest version of Electron or in the beta—please include it with your comment!

andersk commented 4 months ago

bump

electron-issue-triage[bot] commented 1 month ago

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. If you have any new additional information—in particular, if this is still reproducible in the latest version of Electron or in the beta—please include it with your comment!

andersk commented 1 month ago

bump