fix(core): render hovers in the document of their target element - #17934
fix(core): render hovers in the document of their target element#17934safisa wants to merge 2 commits into
Conversation
ndoschek
left a comment
There was a problem hiding this comment.
Thanks for working on this! I tested the change with different views and editors and the intermediate issue you describe seems to be fixed, the tooltip appear now on the secondary window where it is expected.
Although I found two blocking problems:
-
Flickering: with a view or editor open in a secondary window, hovering the editor tab now shows the tooltip in the secondary window, but it flickers heavily regardless of the enhanced preview setting. This should be fixed.
-
Application Crash:
Steps to reproduce (using an editor, but it is not editor-specific, it also happens with views such as the AI chat view):- Open an editor in a secondary window.
- Hover the editor tab.
- Without leaving the tab, close the editor via its "x" (which brings you back to the main application).
- Now move a bit in the application, either opening another editor switching to another view. Just moving the mouse a little around the app is enough to crash the whole application after a few moments.
The only thing I can observe in the backend log is:
root INFO socket closed
Error sending from webFrameMain: Error: Render frame was disposed before WebFrameMain could be accessed
at WebFrameMain.send (node:electron/js2c/browser_init:2:109572)
at WebContents.send (node:electron/js2c/browser_init:2:93779)
at Object.sendWindowEvent (<theia>/examples/electron/lib/backend/electron-main.js:151668:12)
at BrowserWindow.<anonymous> (<theia>/examples/electron/lib/backend/electron-main.js:152288:48)
at BrowserWindow.emit (node:events:521:24)
Error sending from webFrameMain: Error: Render frame was disposed before WebFrameMain could be accessed
at WebFrameMain.send (node:electron/js2c/browser_init:2:109572)
at WebContents.send (node:electron/js2c/browser_init:2:93779)
at <theia>/examples/electron/lib/backend/electron-main.js:151707:14
at new Promise (<anonymous>)
at Object.requestClose (<theia>/examples/electron/lib/backend/electron-main.js:151700:16)
at TheiaElectronWindow.checkSafeToStop (<theia>/examples/electron/lib/backend/electron-main.js:151877:53)
at TheiaElectronWindow.handleStopRequest (<theia>/examples/electron/lib/backend/electron-main.js:151865:77)
at BrowserWindow.<anonymous> (<theia>/examples/electron/lib/backend/electron-main.js:151840:16)
at BrowserWindow.emit (node:events:521:24)
If you close the secondary window via the secondary windows' native close button, the editor is not closed but moves back to the main application and I cannot observe the crash in this case.
Also, I tested the same flow on current master and the crash does not occur there. Could you have another look please?
|
Both issues should be fixed now (fb530fd):
Unit tests added for all of the above. |
Hovers were always appended to the main window's document and positioned with viewport metrics of the main window, so tooltips for widgets moved to a secondary window appeared detached in the main window. Render and position the hover in the target's ownerDocument instead, listen for dismissing mousedown there, and guard hidePopover() against documents that are no longer fully active (secondary window closed while a hover is open). Also fall back to the perpendicular direction when a hover fits on neither side of its target (e.g. full-width items in a narrow secondary window), and clamp left/right hovers into the viewport as a last resort.
fb530fd to
fdbae0e
Compare
|
Thanks for the update @safisa! I cannot reproduce the crash anymore 👍 Also, the |
- create the hover host in the document it is shown in and never adopt it across documents; cancel the hover on pagehide of the hosting window and guard against closed windows, so hovers no longer break (or crash the Electron renderer) after closing a secondary window with an open hover - resolve the dismissal listeners and unRenderHover against the host of the current hover instead of recreating a host for the main document, so hovers in a secondary window are dismissed on mouse-out and mousedown instead of piling up - do not let a superseded render reposition, reveal, or leak css classes into the hover that replaced it
fdbae0e to
b99a079
Compare
|
The remaining issues should be addressed now (b99a079):
|
What it does
Fixes #17933: render hovers in the
ownerDocumentof their target element, so tooltips for widgets moved to a secondary window appear next to the hovered element instead of detached in the main window.target.ownerDocument.bodyand compute the viewport metrics from that document; register the dismissingmousedownlistener there and wait for an animation frame in the target's window.hidePopover()against documents that are no longer fully active — closing the secondary window while a hover is open used to throwInvalidStateErroron the next hover and break all hovers until reload.HoverPosition.fitsand fall back to the perpendicular direction when a hover fits on neither side of its target (e.g. full-width items in a narrow secondary window), so target and hover both stay visible; clamp left/right hovers into the viewport as a last resort.Behavior in the main window is unchanged. Adds unit tests for the new behavior (
hover-service.spec.ts).How to test
npx lerna run test --scope @theia/core(ornpx mocha packages/core/lib/browser/hover-service.spec.jsafter compiling).Follow-ups
None.
Breaking changes
Attribution
Review checklist
nlsservice (for details, please see the Internationalization/Localization section in the Coding Guidelines)Reminder for reviewers