Skip to content

Commit

Permalink
aux window - hopefully fix contextview issue
Browse files Browse the repository at this point in the history
  • Loading branch information
@bpasero
bpasero committed Nov 17, 2023
1 parent 634df63 commit af63c41
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 28 deletions.
22 changes: 12 additions & 10 deletions src/vs/base/browser/ui/contextview/contextview.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,29 +136,33 @@ export class ContextView extends Disposable {

private container: HTMLElement | null = null;
private view: HTMLElement;
private useFixedPosition: boolean;
private useShadowDOM: boolean;
private useFixedPosition = false;
private useShadowDOM = false;
private delegate: IDelegate | null = null;
private toDisposeOnClean: IDisposable = Disposable.None;
private toDisposeOnSetContainer: IDisposable = Disposable.None;
private shadowRoot: ShadowRoot | null = null;
private shadowRootHostElement: HTMLElement | null = null;

constructor(container: HTMLElement | null, domPosition: ContextViewDOMPosition) {
constructor(container: HTMLElement, domPosition: ContextViewDOMPosition) {
super();

this.view = DOM.$('.context-view');
this.useFixedPosition = false;
this.useShadowDOM = false;

DOM.hide(this.view);

this.setContainer(container, domPosition);

this._register(toDisposable(() => this.setContainer(null, ContextViewDOMPosition.ABSOLUTE)));
}

setContainer(container: HTMLElement | null, domPosition: ContextViewDOMPosition): void {
this.useFixedPosition = domPosition !== ContextViewDOMPosition.ABSOLUTE;
const usedShadowDOM = this.useShadowDOM;
this.useShadowDOM = domPosition === ContextViewDOMPosition.FIXED_SHADOW;

if (container === this.container && usedShadowDOM !== this.useShadowDOM) {

This comment has been minimized.

Sign in to view

Copy link
@justschen

justschen Dec 12, 2023 •
edited
Loading

Contributor

this seems to force some cases to return early since useShadowDom changed, so the action widget does not get removed correctly from shadow-root (ref. #198881)

This happens in the following steps:

  1. Reload OSS/VS Code
  2. Open context menu with right click
  3. Hit refactor from the context menu
  4. observe action list in shadow root

(note: this flow is not happening on MacOS because when right clicking, it uses the native context menu, vs. built in context menu from VS Code)

Does not happen when refactoring using CtrlCmd + Shift + R. I suspect the above error happens because useShadowRoot is set to true via line 160 when the context menu is opened, is observed as true when the action widget is opened, sees the change (this.useShadowDom is changed to false), and returns early.

This comment has been minimized.

Sign in to view

Copy link
@bpasero

bpasero Dec 12, 2023

Author Member

Sorry for regression, if you have a fix in mind please go ahead, otherwise I will check tomorrow.
return; // container is the same and now shadow DOM usage has changed
}

if (this.container) {
this.toDisposeOnSetContainer.dispose();

Expand All @@ -173,12 +177,10 @@ export class ContextView extends Disposable {

this.container = null;
}

if (container) {
this.container = container;

this.useFixedPosition = domPosition !== ContextViewDOMPosition.ABSOLUTE;
this.useShadowDOM = domPosition === ContextViewDOMPosition.FIXED_SHADOW;

if (this.useShadowDOM) {
this.shadowRootHostElement = DOM.$('.shadow-root-host');
this.container.appendChild(this.shadowRootHostElement);
Expand Down
31 changes: 13 additions & 18 deletions src/vs/platform/contextview/browser/contextViewService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,44 +9,32 @@ import { ILayoutService } from 'vs/platform/layout/browser/layoutService';
import { IContextViewDelegate, IContextViewService } from './contextView';

export class ContextViewService extends Disposable implements IContextViewService {

declare readonly _serviceBrand: undefined;

private currentViewDisposable: IDisposable = Disposable.None;
private contextView: ContextView;
private container: HTMLElement | null;
private shadowRoot: boolean | undefined;
private readonly contextView = this._register(new ContextView(this.layoutService.mainContainer, ContextViewDOMPosition.ABSOLUTE));

constructor(
@ILayoutService private readonly layoutService: ILayoutService
) {
super();

this.container = layoutService.mainContainer;
this.contextView = this._register(new ContextView(this.container, ContextViewDOMPosition.ABSOLUTE));
this.layout();

this._register(layoutService.onDidLayoutContainer(() => this.layout()));
}

// ContextView

private setContainer(container: HTMLElement, domPosition?: ContextViewDOMPosition): void {
this.container = container;
this.contextView.setContainer(container, domPosition || ContextViewDOMPosition.ABSOLUTE);
}

showContextView(delegate: IContextViewDelegate, container?: HTMLElement, shadowRoot?: boolean): IDisposable {
let domPosition: ContextViewDOMPosition;
if (container) {
if (container !== this.container || this.shadowRoot !== shadowRoot) {
this.setContainer(container, shadowRoot ? ContextViewDOMPosition.FIXED_SHADOW : ContextViewDOMPosition.FIXED);
}
domPosition = shadowRoot ? ContextViewDOMPosition.FIXED_SHADOW : ContextViewDOMPosition.FIXED;
} else {
if (this.container !== this.layoutService.activeContainer) {
this.setContainer(this.layoutService.activeContainer, ContextViewDOMPosition.ABSOLUTE);
}
domPosition = ContextViewDOMPosition.ABSOLUTE;
}

this.shadowRoot = shadowRoot;
this.contextView.setContainer(container ?? this.layoutService.activeContainer, domPosition);

this.contextView.show(delegate);

Expand All @@ -71,4 +59,11 @@ export class ContextViewService extends Disposable implements IContextViewServic
hideContextView(data?: any): void {
this.contextView.hide(data);
}

override dispose(): void {
super.dispose();

this.currentViewDisposable.dispose();
this.currentViewDisposable = Disposable.None;
}
}

0 comments on commit af63c41

Please sign in to comment.