Skip to content

fix: Use layout index as fallback criterion for zIndex sorting #333

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

Open
veracioux wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: Use layout index as fallback criterion for zIndex sorting #333

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 7 additions & 1 deletion packages/core/src/Renderable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -582,7 +582,13 @@ export abstract class Renderable extends BaseRenderable {

private ensureZIndexSorted(): void {
if (this.needsZIndexSort) {
this._childrenInZIndexOrder.sort((a, b) => (a.zIndex > b.zIndex ? 1 : a.zIndex < b.zIndex ? -1 : 0))
const layoutIndexes = new Map<Renderable, number>()
for (let i = 0; i < this._childrenInLayoutOrder.length; i++) {
Copy link
Collaborator

@kommander kommander Nov 21, 2025

Choose a reason for hiding this comment

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

I know this only runs when something changes, but in a long list for example that can be frequent. It seems like the opencode issue is that the "copied to clipboard" notification should just have a higher zIndex?

Copy link
Contributor Author

@veracioux veracioux Nov 21, 2025

Choose a reason for hiding this comment

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

This is a fix for a general issue, not just for the opencode issue. Opencode is just the first place I discovered it. For example if you look at this code:
https://github.com/veracioux/opentui-332-repro/blob/main/index.tsx#L7

If you remove the Show element around the inner box, the issue isn't reproducible. I think this is confusing behavior and should be fixed.

If you are concerned about performance, would you be ok with updating this map inside Renderable.add function and only reading it in ensureZIndexSorted?

Copy link
Contributor Author

@veracioux veracioux Nov 23, 2025

Choose a reason for hiding this comment

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

@kommander Do you think it would be ok to implement it that way?

Copy link
Collaborator

@kommander kommander Nov 23, 2025

Choose a reason for hiding this comment

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

would you be ok with updating this map inside Renderable.add function and only reading it in ensureZIndexSorted?

I would prefer something like that, yeah.

I think I am failing to understand what the issue is actually. Isn't the "copied to clipboard" toast positioned absolutely? I wonder why the layout order should influence that then.

Could we maybe have that issue reproduced in a test? Can be a snapshot test so ensure it is rendered in expected order.

Copy link
Contributor Author

@veracioux veracioux Nov 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

In browsers the expectation is that the stacking order is determined by zIndex, but if two elements have the same zindex (or undefined), then the element that comes later in the layout should be displayed over the element that comes before. Absoluteness has nothing to do with it.

Open this minimalistic index.html in a browser to see what I mean. Btw, the behavior is the same with position: static instead of relative.

I think opentui should do the same.

I'll try to create a test reproducing the issue like https://github.com/veracioux/opentui-332-repro does.

layoutIndexes.set(this._childrenInLayoutOrder[i], i)
}
this._childrenInZIndexOrder.sort((a, b) =>
a.zIndex > b.zIndex ? 1 : a.zIndex < b.zIndex ? -1 : (layoutIndexes.get(a) ?? 0) - (layoutIndexes.get(b) ?? 0),
)
this.needsZIndexSort = false
}
}
Expand Down
Loading