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.

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

JupyterLab part maximize places part in window top-left #27

Open
quigleyj-mavenomics opened this issue Oct 30, 2019 · 3 comments
Open
Labels
bug Something isn't working TBD Issue that isn't yet fleshed out enough to work on, but should still be percolating

Comments

@quigleyj-mavenomics
Copy link
Member

Pressing Ctrl+M places the part in the wrong spot. It should take up the full rendering area of the dashboard.

Instead, it takes the size of the dashboard but not the position:

image

@quigleyj-mavenomics quigleyj-mavenomics added the bug Something isn't working label Oct 30, 2019
@quigleyj-mavenomics
Copy link
Member Author

The m-Region-Maximized rule sets position: fixed, which in this case is wrong as that makes it stay in a single spot with no respect to scroll. The maximized region must scroll with it's container (in this case, the notebook panel), and must be positioned at the dashboard's position.

What's tricky is we need to measure our position from the scroll root so that we can get the correct offset w.r.t. scroll regions, since getBoundingClientRect returns position relative to the viewport root. So we need to A.) find the first scrolling parent, B.) calculate the delta between the dashboard's top/left and the scroll parent's top/left, C.) add that delta to the parent scrollX/Y to get the final top/left.

@quigleyj-mavenomics
Copy link
Member Author

It looks like we could actually simplify this. By making the no-stack rule add overflow: visible, we can do position:absolute within the dashboard and simply calculate the screen-space offset between the top/left of the part and the top/left of the dashboard. Seems to work even with tab panels (tab panels set z-indices on their children, making them a good litmus test to see if stacking contexts will bite us).

@quigleyj-mavenomics
Copy link
Member Author

Setting this aside, as it's proving rather intractable. The above approach doesn't work because the maximization is changing the layout underneath the maximization check- the values it calculates are only correct until the parent reflows after the child is pulled out of the CSS page layout. Which is to say, they aren't always correct and can't always be correct until at a minimum, the next rendered frame.

Further, the original position will have to be 'cached' somehow since on the next frame, the algorithm detailed above is no longer correct. If it executed correctly, the offset will become 0,0 instead of whatever negative 2-tuple it needs to be to correct for the difference between where it was originally positioned and where the upper left corner of the dashboard is. If it didn't execute correctly the first time around, it'll become even more incorrect!

So in order to appropriately layout the maximized region, we must first remove it from the layout flow, then calculate the correct offset between the resultant position post-hide and the upper-left-corner of the dashboard. then remember that offset and store it until maximization is disabled.

Shifting it in the DOM causes us problems because at that point the maximization operation is no longer 'safe' and atomic- the layout pruning, model dirtiness, etc. will all happen in the background. Worse yet, moving things in the DOM breaks user state and IFrames in a few specific places (child being moved, sibling of child being moved, grandchild/sibling-child, etc.). So most parts must at that point rerender (and most UDPs must fully reinit). We already pay for this in layout manipulations, but the user is less likely to notice since their mental context is probably focused on layout during layout manipulations. That is not true during maximization (eg, maximize a PivotPart to edit the pivot config)!

We could make a copy and move that. The PartManager doesn't allow for it right now but can be modified for it. However, user state won't sync between the copies and orchestrating bindings between the clones would be a major challenge.

We could also try having the child content (for WidgetLayoutRegions in particular) detach from the layout region, and reattach to a special portion of the LayoutManager. That could mostly work and institutes the least API headaches, but it doesn't solve the problem of parts re-rendering while a user is trying to interact with them.

@quigleyj-mavenomics quigleyj-mavenomics added the TBD Issue that isn't yet fleshed out enough to work on, but should still be percolating label Nov 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working TBD Issue that isn't yet fleshed out enough to work on, but should still be percolating
Projects
None yet
Development

No branches or pull requests

1 participant