ipywidgets icon indicating copy to clipboard operation
ipywidgets copied to clipboard

setLayout before attaching to DOM

Open maartenbreddels opened this issue 6 years ago • 15 comments
trafficstars

Currently, we wait till the DOMWidgetView object is displayed (i.e. attached to DOM), before we set the layout: https://github.com/jupyter-widgets/ipywidgets/blob/92d7d42c00a1b0d9ce921533acb08beefdea3eb2/packages/base/src/widget.ts#L810

This originally comes from this commit: https://github.com/jupyter-widgets/ipywidgets/commit/9c3de0980968f42ce92feddc6c00dbeb01dd41a5

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.

maartenbreddels avatar Nov 07 '19 14:11 maartenbreddels

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.

vidartf avatar Nov 07 '19 17:11 vidartf

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

martinRenou avatar Dec 18 '19 12:12 martinRenou

Actually if I understand correctly, this is fixed

martinRenou avatar Dec 18 '19 12:12 martinRenou

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

martinRenou avatar Dec 18 '19 12:12 martinRenou

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

which fix are you talking about?

maartenbreddels avatar Dec 18 '19 12:12 maartenbreddels

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

martinRenou avatar Dec 18 '19 12:12 martinRenou

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: https://github.com/bloomberg/bqplot/pull/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.

martinRenou avatar Dec 18 '19 14:12 martinRenou

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;
 });

maartenbreddels avatar Dec 18 '19 14:12 maartenbreddels

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.

martinRenou avatar Jan 20 '20 15:01 martinRenou

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.

blois avatar Sep 15 '21 15:09 blois

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

jasongrout avatar Sep 17 '21 21:09 jasongrout

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.

jasongrout avatar Sep 17 '21 21:09 jasongrout

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 https://github.com/jupyter-widgets/ipywidgets/pull/3288#pullrequestreview-780860627, 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 avatar Oct 29 '21 15:10 vidartf

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

vidartf avatar Nov 02 '21 17:11 vidartf

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 avatar Nov 16 '21 16:11 vidartf