Alt fix #288518
Alt fix #288518
Conversation
Introduces a new LayoutAnchorMode.OVERLAP_OKAY that allows views to overlap the anchor when they don't fit on the preferred side, rather than flipping to the opposite side far away from the anchor. This fixes large hovers appearing far from their target element when there isn't enough space above/below.
There was a problem hiding this comment.
Pull request overview
This PR introduces a fix for issue #288500 by adding a new OVERLAP_OKAY layout mode for context views. The mode allows large hovers to shift and overlap with anchors rather than being flipped to the opposite side when they don't fit perfectly on the preferred side.
Changes:
- Adds
OVERLAP_OKAYenum value toLayoutAnchorModeto enable overlap-tolerant positioning - Modifies the
layoutfunction to skip the "flip to other side" logic whenOVERLAP_OKAYmode is active - Applies
OVERLAP_OKAYmode to vertical anchors in context view positioning - Adds test case for large hover positioning behavior
- Includes debugging code that adds repetitive 'more' text to agent session tooltips (should be removed)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/vs/base/browser/ui/contextview/contextview.ts | Adds OVERLAP_OKAY mode and updates layout logic to prevent flipping when this mode is set |
| src/vs/base/test/browser/ui/contextview/contextview.test.ts | Adds test case for large hover layout behavior |
| src/vs/workbench/contrib/chat/browser/agentSessions/agentSessionsViewer.ts | Contains debugging code adding 'more' text to tooltips (should be removed) |
| lines.push('more'); | ||
| lines.push('more'); | ||
| lines.push('more'); | ||
| lines.push('more'); | ||
| lines.push('more'); | ||
| lines.push('more'); | ||
| lines.push('more'); | ||
| lines.push('more'); | ||
| lines.push('more'); | ||
| lines.push('more'); | ||
| lines.push('more'); | ||
| lines.push('more'); | ||
|
|
There was a problem hiding this comment.
This appears to be debugging/test code that should not be committed. These 12 lines adding 'more' to the tooltip will result in unexpected UI behavior where tooltips display repeated "more" text. This should be removed before merging.
| lines.push('more'); | |
| lines.push('more'); | |
| lines.push('more'); | |
| lines.push('more'); | |
| lines.push('more'); | |
| lines.push('more'); | |
| lines.push('more'); | |
| lines.push('more'); | |
| lines.push('more'); | |
| lines.push('more'); | |
| lines.push('more'); | |
| lines.push('more'); |
| AVOID, | ||
| ALIGN | ||
| ALIGN, | ||
| OVERLAP_OKAY |
There was a problem hiding this comment.
The new OVERLAP_OKAY enum value lacks documentation explaining its purpose and when it should be used, unlike the other layout modes. Consider adding a comment to describe when this mode is appropriate, especially given that it changes the layout behavior to prefer overlapping the anchor rather than flipping to the other side.
| test('layout large hover', () => { | ||
| // When the view is large and almost fits on the preferred side, we should prefer to shift it | ||
| // rather than flip it to the other side which might be far away. | ||
| assert.strictEqual(layout(1084, 306, { offset: 296, size: 2, position: LayoutAnchorPosition.After }), 0); |
There was a problem hiding this comment.
This test doesn't include the mode parameter in the anchor object, so it's testing the default AVOID behavior rather than the new OVERLAP_OKAY behavior that was introduced. To properly test the "shift rather than flip" behavior described in the comment, the test should specify mode: LayoutAnchorMode.OVERLAP_OKAY in the anchor object. Otherwise, the existing logic would flip to the other side rather than overlap.
Alternate fix for #288500