[PerfSampleError] `getPropertyValue`

[vscodenpa]

[PerfSampleError] by <<renderer>> in getPropertyValue
at getPropertyValue
at f in src/vs/base/browser/dom.ts:326:17
at getBorderTopWidth in out/vs/workbench/workbench.desktop.main.js:90:29318
at getContentHeight in src/vs/base/browser/dom.ts:548:17
at layout in out/vs/workbench/workbench.desktop.main.js:135:13910
at layout in out/vs/workbench/workbench.desktop.main.js:168:8747
at layout in out/vs/workbench/workbench.desktop.main.js:747:140980
at <anonymous> in src/vs/workbench/browser/parts/notifications/notificationsToasts.ts:558:39
at U in src/vs/workbench/browser/parts/notifications/notificationsToasts.ts:557:10
at M in src/vs/workbench/browser/parts/notifications/notificationsToasts.ts:156:10
at <anonymous> in src/vs/workbench/browser/parts/notifications/notificationsToasts.ts:153:52
at execute in out/vs/workbench/workbench.desktop.main.js:90:27087
at At in src/vs/base/browser/dom.ts:193:31

Go to Errors Site

[bpasero]

The origin is actually in the list widget:

vscode/src/vs/base/browser/ui/list/listView.ts

Line 779 in 815295b

height: typeof height === 'number' ? height : getContentHeight(this.domNode)

[joaomoreno]

@bpasero The list is a virtualized rendering widget, it needs to know its container size. Ideally this is always known and height would be non-optional. I'm nice and allow people to pass undefined, but that means I'll invoke the DOM to measure the container height.

If you want to fix this specific issue, you need to provide a fixed height from notificationsToasts.ts. Otherwise we're always bound to relayout in order to measure the list container height.

[bpasero]

Notifications with buttons are of dynamic height because their text is dynamic given from outside, so at one point I need to compute the height and cannot rely on a static value. That is why I rely on the list widget to compute for me.

That said, I might not need to compute it in all cases, for example when no button is shown, the height is fixed (single line of message).

[joaomoreno]

Notifications with buttons are of dynamic height because their text is dynamic given from outside, so at one point I need to compute the height and cannot rely on a static value. That is why I rely on the list widget to compute for me.

Yeah no surprises there 😄

That said, I might not need to compute it in all cases, for example when no button is shown, the height is fixed (single line of message).

Sure, we can optimize for this. We can also just ignore this error, unsure if it's really an issue.

[bpasero]

@joaomoreno so to clarify, am I expected to pass in a height to this method:

vscode/src/vs/workbench/browser/parts/notifications/notificationsList.ts

Lines 251 to 261 in 515971a

	layout(width: number, maxHeight?: number): void {
	if (this.listContainer && this.list) {
	this.listContainer.style.width = `${width}px`;
	if (typeof maxHeight === 'number') {
	this.list.getHTMLElement().style.maxHeight = `${maxHeight}px`;
	}
	this.list.layout();
	}
	}

Or is it sufficient to ensure the container has a height associated?

I am basically not seeing a way for me to configure the list in a way that I can even pass in width and height, maybe its missing to be forwarded here:

vscode/src/vs/workbench/browser/parts/notifications/notificationsList.ts

Line 259 in 515971a

this.list.layout();

[joaomoreno]

You need to pass a height to the list, if you want to avoid that it calls the expensive getContentHeight.

[bpasero]

@joaomoreno hmm, so I am already implementing getHeight method of IListVirtualDelegate:

vscode/src/vs/workbench/browser/parts/notifications/notificationsViewer.ts

Line 53 in ce00cd7

getHeight(notification: INotificationViewItem): number {

And in there I am clever to only compute the height when I know the notification is complex and not a simple one liner.

Given each item returns a height, why do you still need to compute the height in the list itself? Can you not just take the heights and multiply by the items?