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

Allow hover target override for resource labels #230101

Merged
merged 7 commits into from
Oct 7, 2024
Merged

Allow hover target override for resource labels #230101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ffa30b8
Allow resource label hover target to be changed
benibenj Sep 30, 2024
f59bb66
adopt hover target override for attachments
benibenj Sep 30, 2024
f3e07c6
remove default hover delegate which does nothing special
benibenj Sep 30, 2024
d0a7707
Merge branch 'main' into benibenj/plastic-otter
benibenj Oct 5, 2024
7c08f26
Merge branch 'main' into benibenj/plastic-otter
benibenj Oct 6, 2024
3072114
Remove unused tabsHoverDelegate from EditorTabsControl
benibenj Oct 6, 2024
302f3ed
cleanup
benibenj Oct 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions src/vs/base/browser/ui/iconLabel/iconLabel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export interface IIconLabelCreationOptions {
readonly supportDescriptionHighlights?: boolean;
readonly supportIcons?: boolean;
readonly hoverDelegate?: IHoverDelegate;
readonly hoverTargetOverrride?: HTMLElement;
}

export interface IIconLabelValueOptions {
Expand Down Expand Up @@ -209,6 +210,14 @@ export class IconLabel extends Disposable {
return;
}

let hoverTarget = htmlElement;
if (this.creationOptions?.hoverTargetOverrride) {
if (!dom.isAncestor(htmlElement, this.creationOptions.hoverTargetOverrride)) {
throw new Error('hoverTargetOverrride must be an ancestor of the htmlElement');
}
hoverTarget = this.creationOptions.hoverTargetOverrride;
}

if (this.hoverDelegate.showNativeHover) {
function setupNativeHover(htmlElement: HTMLElement, tooltip: string | IManagedHoverTooltipMarkdownString | undefined): void {
if (isString(tooltip)) {
Expand All @@ -220,9 +229,9 @@ export class IconLabel extends Disposable {
htmlElement.removeAttribute('title');
}
}
setupNativeHover(htmlElement, tooltip);
setupNativeHover(hoverTarget, tooltip);
} else {
const hoverDisposable = getBaseLayerHoverDelegate().setupManagedHover(this.hoverDelegate, htmlElement, tooltip);
const hoverDisposable = getBaseLayerHoverDelegate().setupManagedHover(this.hoverDelegate, hoverTarget, tooltip);
if (hoverDisposable) {
this.customHovers.set(htmlElement, hoverDisposable);
}
Expand Down
10 changes: 0 additions & 10 deletions src/vs/workbench/browser/parts/editor/editorTabsControl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,6 @@ import { IAuxiliaryEditorPart, MergeGroupMode } from '../../../services/editor/c
import { isMacintosh } from '../../../../base/common/platform.js';
import { IHostService } from '../../../services/host/browser/host.js';
import { ServiceCollection } from '../../../../platform/instantiation/common/serviceCollection.js';
import { IHoverDelegate } from '../../../../base/browser/ui/hover/hoverDelegate.js';
import { getDefaultHoverDelegate } from '../../../../base/browser/ui/hover/hoverDelegateFactory.js';
import { IBaseActionViewItemOptions } from '../../../../base/browser/ui/actionbar/actionViewItems.js';
import { MarkdownString } from '../../../../base/common/htmlContent.js';
import { IManagedHoverTooltipMarkdownString } from '../../../../base/browser/ui/hover/hover.js';
Expand Down Expand Up @@ -127,8 +125,6 @@ export abstract class EditorTabsControl extends Themable implements IEditorTabsC

private renderDropdownAsChildElement: boolean;

private readonly tabsHoverDelegate: IHoverDelegate;

constructor(
protected readonly parent: HTMLElement,
protected readonly editorPartsView: IEditorPartsView,
Expand All @@ -149,8 +145,6 @@ export abstract class EditorTabsControl extends Themable implements IEditorTabsC

this.renderDropdownAsChildElement = false;

this.tabsHoverDelegate = getDefaultHoverDelegate('mouse');

const container = this.create(parent);

// Context Keys
Expand Down Expand Up @@ -469,10 +463,6 @@ export abstract class EditorTabsControl extends Themable implements IEditorTabsC
return title;
}

protected getHoverDelegate(): IHoverDelegate {
return this.tabsHoverDelegate;
}

protected updateTabHeight(): void {
this.parent.style.setProperty('--editor-group-tab-height', `${this.tabHeight}px`);
}
Expand Down
2 changes: 1 addition & 1 deletion src/vs/workbench/browser/parts/editor/multiEditorTabsControl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -817,7 +817,7 @@ export class MultiEditorTabsControl extends EditorTabsControl {
tabContainer.appendChild(tabBorderTopContainer);

// Tab Editor Label
const editorLabel = this.tabResourceLabels.create(tabContainer, { hoverDelegate: this.getHoverDelegate() });
const editorLabel = this.tabResourceLabels.create(tabContainer, { hoverTargetOverrride: tabContainer });
Copy link
Member

Choose a reason for hiding this comment

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

Don't we still need to pass delegate here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it is not needed because the label already provides the default hover behaviour and tabs don't need any special hover behaviour


// Tab Actions
const tabActionsContainer = document.createElement('div');
Expand Down
2 changes: 1 addition & 1 deletion src/vs/workbench/browser/parts/editor/singleEditorTabsControl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ export class SingleEditorTabsControl extends EditorTabsControl {
titleContainer.appendChild(labelContainer);

// Editor Label
this.editorLabel = this._register(this.instantiationService.createInstance(ResourceLabel, labelContainer, { hoverDelegate: this.getHoverDelegate() })).element;
this.editorLabel = this._register(this.instantiationService.createInstance(ResourceLabel, labelContainer, {})).element;
this._register(addDisposableListener(this.editorLabel.element, EventType.CLICK, e => this.onTitleLabelClick(e)));

// Breadcrumbs
Expand Down
16 changes: 5 additions & 11 deletions src/vs/workbench/contrib/chat/browser/chatContentParts/chatAttachmentsContentPart.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import { ChatResponseReferencePartStatusKind, IChatContentReference } from '../.
import { IOpenerService } from '../../../../../platform/opener/common/opener.js';
import { IHoverService } from '../../../../../platform/hover/browser/hover.js';
import { createInstantHoverDelegate } from '../../../../../base/browser/ui/hover/hoverDelegateFactory.js';
import { IManagedHoverContentOrFactory } from '../../../../../base/browser/ui/hover/hover.js';

export class ChatAttachmentsContentPart extends Disposable {
private readonly attachedContextDisposables = this._register(new DisposableStore());
Expand Down Expand Up @@ -48,15 +47,14 @@ export class ChatAttachmentsContentPart extends Disposable {

this.variables.forEach(async (attachment) => {
const widget = dom.append(container, dom.$('.chat-attached-context-attachment.show-file-icons'));
const label = this._contextResourceLabels.create(widget, { supportIcons: true, hoverDelegate });
const label = this._contextResourceLabels.create(widget, { supportIcons: true, hoverDelegate, hoverTargetOverrride: widget });
const file = URI.isUri(attachment.value) ? attachment.value : attachment.value && typeof attachment.value === 'object' && 'uri' in attachment.value && URI.isUri(attachment.value.uri) ? attachment.value.uri : undefined;
const range = attachment.value && typeof attachment.value === 'object' && 'range' in attachment.value && Range.isIRange(attachment.value.range) ? attachment.value.range : undefined;

const correspondingContentReference = this.contentReferences.find((ref) => typeof ref.reference === 'object' && 'variableName' in ref.reference && ref.reference.variableName === attachment.name);
const isAttachmentOmitted = correspondingContentReference?.options?.status?.kind === ChatResponseReferencePartStatusKind.Omitted;
const isAttachmentPartialOrOmitted = isAttachmentOmitted || correspondingContentReference?.options?.status?.kind === ChatResponseReferencePartStatusKind.Partial;

let hoverElement: IManagedHoverContentOrFactory;
let ariaLabel: string | undefined;

if (file && attachment.isFile) {
Expand All @@ -72,8 +70,6 @@ export class ChatAttachmentsContentPart extends Disposable {
ariaLabel = range ? localize('chat.fileAttachmentWithRange3', "Attached: {0}, line {1} to line {2}.", friendlyName, range.startLineNumber, range.endLineNumber) : localize('chat.fileAttachment3', "Attached: {0}.", friendlyName);
}

hoverElement = file.fsPath;

label.setFile(file, {
fileKind: FileKind.FILE,
hidePath: true,
Expand All @@ -83,7 +79,7 @@ export class ChatAttachmentsContentPart extends Disposable {
} else if (attachment.isImage) {
ariaLabel = localize('chat.imageAttachment', "Attached image, {0}", attachment.name);

hoverElement = dom.$('div.chat-attached-context-hover');
const hoverElement = dom.$('div.chat-attached-context-hover');
hoverElement.setAttribute('aria-label', ariaLabel);

// Custom label
Expand All @@ -107,12 +103,14 @@ export class ChatAttachmentsContentPart extends Disposable {
}

widget.style.position = 'relative';
if (!this.attachedContextDisposables.isDisposed) {
this.attachedContextDisposables.add(this.hoverService.setupManagedHover(hoverDelegate, widget, hoverElement));
}
} else {
const attachmentLabel = attachment.fullName ?? attachment.name;
const withIcon = attachment.icon?.id ? `$(${attachment.icon.id}) ${attachmentLabel}` : attachmentLabel;
label.setLabel(withIcon, correspondingContentReference?.options?.status?.description);

hoverElement = attachmentLabel;
ariaLabel = localize('chat.attachment3', "Attached context: {0}.", attachment.name);
}

Expand All @@ -122,7 +120,6 @@ export class ChatAttachmentsContentPart extends Disposable {
const description = correspondingContentReference?.options?.status?.description;
if (isAttachmentPartialOrOmitted) {
ariaLabel = `${ariaLabel}${description ? ` ${description}` : ''}`;
hoverElement = description;
for (const selector of ['.monaco-icon-suffix-container', '.monaco-icon-name-container']) {
const element = label.element.querySelector(selector);
if (element) {
Expand Down Expand Up @@ -150,9 +147,6 @@ export class ChatAttachmentsContentPart extends Disposable {

widget.ariaLabel = ariaLabel;
widget.tabIndex = 0;
if (!this.attachedContextDisposables.isDisposed) {
this.attachedContextDisposables.add(this.hoverService.setupManagedHover(hoverDelegate, widget, hoverElement));
}
});
}

Expand Down
14 changes: 5 additions & 9 deletions src/vs/workbench/contrib/chat/browser/chatInputPart.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import { IHistoryNavigationWidget } from '../../../../base/browser/history.js';
import { StandardKeyboardEvent } from '../../../../base/browser/keyboardEvent.js';
import * as aria from '../../../../base/browser/ui/aria/aria.js';
import { Button } from '../../../../base/browser/ui/button/button.js';
import { IManagedHoverContentOrFactory } from '../../../../base/browser/ui/hover/hover.js';
import { IHoverDelegate } from '../../../../base/browser/ui/hover/hoverDelegate.js';
import { createInstantHoverDelegate } from '../../../../base/browser/ui/hover/hoverDelegateFactory.js';
import { renderLabelWithIcons } from '../../../../base/browser/ui/iconLabel/iconLabels.js';
Expand Down Expand Up @@ -672,9 +671,8 @@ export class ChatInputPart extends Disposable implements IHistoryNavigationWidge
}

const widget = dom.append(container, $('.chat-attached-context-attachment.show-file-icons'));
const label = this._contextResourceLabels.create(widget, { supportIcons: true, hoverDelegate });
const label = this._contextResourceLabels.create(widget, { supportIcons: true, hoverDelegate, hoverTargetOverrride: widget });

let hoverElement: IManagedHoverContentOrFactory;
let ariaLabel: string | undefined;

const file = URI.isUri(attachment.value) ? attachment.value : attachment.value && typeof attachment.value === 'object' && 'uri' in attachment.value && URI.isUri(attachment.value.uri) ? attachment.value.uri : undefined;
Expand All @@ -685,7 +683,6 @@ export class ChatInputPart extends Disposable implements IHistoryNavigationWidge
const friendlyName = `${fileBasename} ${fileDirname}`;

ariaLabel = range ? localize('chat.fileAttachmentWithRange', "Attached file, {0}, line {1} to line {2}", friendlyName, range.startLineNumber, range.endLineNumber) : localize('chat.fileAttachment', "Attached file, {0}", friendlyName);
hoverElement = file.fsPath;

label.setFile(file, {
fileKind: FileKind.FILE,
Expand All @@ -696,7 +693,7 @@ export class ChatInputPart extends Disposable implements IHistoryNavigationWidge
} else if (attachment.isImage) {
ariaLabel = localize('chat.imageAttachment', "Attached image, {0}", attachment.name);

hoverElement = dom.$('div.chat-attached-context-hover');
const hoverElement = dom.$('div.chat-attached-context-hover');
hoverElement.setAttribute('aria-label', ariaLabel);

// Custom label
Expand All @@ -722,22 +719,21 @@ export class ChatInputPart extends Disposable implements IHistoryNavigationWidge
}

widget.style.position = 'relative';
if (!this.attachedContextDisposables.isDisposed) {
this.attachedContextDisposables.add(this.hoverService.setupManagedHover(hoverDelegate, widget, hoverElement, { trapFocus: false }));
}
} else {
const attachmentLabel = attachment.fullName ?? attachment.name;
const withIcon = attachment.icon?.id ? `$(${attachment.icon.id}) ${attachmentLabel}` : attachmentLabel;
label.setLabel(withIcon, undefined);

ariaLabel = localize('chat.attachment', "Attached context, {0}", attachment.name);
hoverElement = attachmentLabel;

this.attachButtonAndDisposables(widget, index, attachment, hoverDelegate);
}

widget.tabIndex = 0;
widget.ariaLabel = ariaLabel;
if (!this.attachedContextDisposables.isDisposed) {
this.attachedContextDisposables.add(this.hoverService.setupManagedHover(hoverDelegate, widget, hoverElement, { trapFocus: false }));
}
Comment on lines -738 to -740
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean attachments won't get hovers anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They do, as it is provided by the resource label. I had to add this initially to make sure that focusing an attachment would cause the hover to show because the focus index is on the parent element of the resource label which also has a hover. This is annoying, because even though there is already a hover on the child element, I had to create a hover on the parent element to make this work. This is why I would like to fix this with this PR or a different approach

});

if (oldHeight !== container.offsetHeight && !isLayout) {
Expand Down
Loading