fix: Use layout index as fallback criterion for zIndex sorting #333
Conversation
| 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++) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@kommander Do you think it would be ok to implement it that way?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
2 participants
Fixes #332.