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

setLayout before attaching to DOM #2605

Open
maartenbreddels opened this issue Nov 7, 2019 · 15 comments
Open

setLayout before attaching to DOM #2605

maartenbreddels opened this issue Nov 7, 2019 · 15 comments
Milestone

Comments

@maartenbreddels
Copy link
Member

Currently, we wait till the DOMWidgetView object is displayed (i.e. attached to DOM), before we set the layout:

this.displayed.then(() => {

This originally comes from this commit:
9c3de09

However, this leads to unneeded resize events. First the DOM elements get attached, and the element get a particular size, then the layout may influence the sizing, leaving several libraries (ipyvolume, bqplot, probably also ipyleaflet) to redraw itself.

I think we should set the layout, style and css _dom_classes before attaching.

@vidartf
Copy link
Member

vidartf commented Nov 7, 2019

I believe it can be worth exploring, but there is a high enough chance for this to be significant that we carefully look if there are any corner cases that will break with this.

@martinRenou
Copy link
Member

I think we can avoid this now. Since my fix on the display logic.

@martinRenou
Copy link
Member

Actually if I understand correctly, this is fixed

@martinRenou
Copy link
Member

We might want to remove the extra code that was executed after the displayed event

@maartenbreddels
Copy link
Member Author

I think we can avoid this now. Since my fix on the display logic.

which fix are you talking about?

@martinRenou
Copy link
Member

Oh I'm sorry I was using the phone UI, I thought this was on the bqplot repository. You can ignore my comments :)

@martinRenou
Copy link
Member

martinRenou commented Dec 18, 2019

After some in-person discussions on this, it seems like we could need an extra promise that would be resolved when the displayed promise is resolved (after attached to the DOM), AND the layout and style have been applied to the widget.

It would be important in bqplot because the Figure widget could look for the available space after the layout/style/CSS is applied.

The bqplot 0.12.0 behavior is that the Figure is first rendered with a default size, then attached to the DOM, and only on resize event (which is triggered after attached and after the layout and style are applied) will take the available space. All of this triggers lots of rendering of the Figure and slows the whole thing down.

Since this fix in bqplot: bqplot/bqplot#982, the Figure is rendered only after the displayed promise is resolved (after attached to the DOM). It reduces the number of renderings of the Figure. But I feel like we can still do better by waiting for the layout and CSS to be applied.

In DOMWidgetView there is this piece of code:

this.displayed.then(() => {
    this.update_classes([], this.model.get('_dom_classes'));
    this.setLayout(this.model.get('layout'));
    this.setStyle(this.model.get('style'));
});

Maybe this could be set to something like:

this.layoutApplied = this.displayed.then(() => {
    this.update_classes([], this.model.get('_dom_classes'));
    this.setLayout(this.model.get('layout'));
    this.setStyle(this.model.get('style'));
});

I'm sure we can find a better name than layoutApplied ;) But this is the idea.

@maartenbreddels
Copy link
Member Author

What about:

 this.layedOut  = this.displayed.then(async () => {
     this.update_classes([], this.model.get('_dom_classes'));
     this.setLayout(this.model.get('layout'));
     this.setStyle(this.model.get('style'));
     await this.layoutPromise;
     await this.stylePromise;
 });

@martinRenou
Copy link
Member

Related to this issue. We wait for the displayed promise to be resolved in bqplot before rendering the plot. Once the promise is resolved, we compute the available client size with getBoundingClientRect and render the plot.

This does not work well in the case of a tab widget. Because the displayed promise is resolved after attaching (after-attach event) the Lumino widget to the DOM, the available client size is 0 in the case of a Tab that is not expanded. For this reason, I wonder if the displayed promise should not be resolved after showing (after-show event) the widget for the first time.

@blois
Copy link
Contributor

blois commented Sep 15, 2021

Can ResizeObserver be relied on to resolve some ambiguity around layout events? The browser support matrix is looking pretty green now.

Decreasing the dependency on Lumino DOM events may make it easier to leverage widgets in other contexts/frameworks.

@jasongrout
Copy link
Member

Looking at the timeline at https://caniuse.com/resizeobserver, it looks like things basically became green at the start of 2020 or so.

@jasongrout
Copy link
Member

Decreasing the dependency on Lumino DOM events may make it easier to leverage widgets in other contexts/frameworks.

Yes, eventually. In the short term, using the ResizeObserver to trigger a resize event on the root Lumino widget provides a nice transition.

We could do this in the classic notebook now, for example, or in the html widget manager.

@vidartf
Copy link
Member

vidartf commented Oct 29, 2021

I agree with this issue in that I think there is an optimization to be had by applying styles and layout before the widget's root node is attached to the DOM. That said, for reasons similar to that stated in #3288 (review), the setLayout and setStyle calls should probably still happen after the render call has finished/resolved. To achieve both points, I think probably we would want the calls to happen in response to the first before-attach event, instead of the after-attach event. Thoughts?

@vidartf
Copy link
Member

vidartf commented Nov 2, 2021

I will do a PR to the effect of the above comment ^

@vidartf
Copy link
Member

vidartf commented Nov 16, 2021

I tried to implement the change outline above, but was unable to find a solution where the layout code runs prior to the element being attached. The main points to note are:

  • The layout code needs to run after the render method completes (might be async).
  • The before-attach message handling code in synchronous, i.e. the element will be attached soon after the method returns.
  • The code to create the Layout widget is asyncronous (load_class, etc).

In sum, I wasn't able to find a way to have the before-attach message handler block on the layout work completing before returning and having the element be attached.

I'll leave this open for now in case someone else can think of a clever solution of how to implement this, but I will move it off the 8..0 milestone.

@vidartf vidartf modified the milestones: 8.0, Future Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants