-
Notifications
You must be signed in to change notification settings - Fork 945
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
Do not wait for "displayed" before applying the layout #3288
Do not wait for "displayed" before applying the layout #3288
Conversation
And trigger a "layout-applied" event
8213f17
to
a728de5
Compare
Widget.ResizeMessage.UnknownSize | ||
); | ||
}); | ||
this.trigger('layout-applied'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a "layout-applied" event to the widget, we need to document this
@@ -920,23 +918,25 @@ export class DOMWidgetView extends WidgetView { | |||
oldLayoutView.remove(); | |||
} | |||
|
|||
return this.create_child_view(layout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might need to revert this
@@ -956,7 +956,7 @@ export class DOMWidgetView extends WidgetView { | |||
oldStyleView.remove(); | |||
} | |||
|
|||
return this.create_child_view(style) | |||
this.create_child_view(style) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are reverting the other one, we should also revert this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From reading the code in the core ipywidgets repo, I made these notes:
- The
layoutPromise
andstylePromise
are currently typed asPromise<any>
, but at least thestylePromise
is used under the assumption that it is aPromise<undefined | StyleView>
:ipywidgets/packages/controls/src/widget_selection.ts
Lines 561 to 565 in c7bd329
this.stylePromise.then(function (style) { if (style) { style.style(); } }); - By not waiting for the
displayed
promise before callingsetLayout
, there is an opportunity for theStyleView
to be initialized before therender
method of the owning widget is called. If this happens, the child elements of the widget might not be created yet, which means that any styles applied via query strings will fail to be applied (e.g. DescriptionWidget, IntSlider, etc.). This will not currently happen for the example widgets, since they create their child elements before anyawait
calls in therender
method, but there is no guarantee that this is true for all third-party widgets.
So in conclusion: If we decide to go ahead with this pattern, point 1. needs to be addressed. On the other hand, I think we should ensure that setStyle
(and probably also setLayout
for consistency) does not get called until the initial render
method of the view has resolved. The displayed
promise seems to be the best bet for this, so I would much prefer a solution that keeps this pattern.
To expand on the race condition mentioned above: class MyWidgetView extends DOMWidgetView {
async render() {
await someAsyncAction();
// Create DOM elements here
}
} AFAICT, the DOM element creation is likely to happen after the styles are applied. Ref:
ipywidgets/packages/base/src/widget_style.ts Lines 83 to 85 in 441135a
Example of a style that has a selector for a child element: ipywidgets/packages/controls/src/widget_int.ts Lines 37 to 50 in 441135a
I haven't been able to find any examples of a real-life widget that has both an async render method AND a style selector that targets a child element, but that doesn't mean it doesn't exist, and I believe we should support it if it does (as we currently do). |
Right. Let's close this PR then, and not touch this logic as it might break. I will open a separate PR that only adds a "layout-applied" and a "style-applied" event, this should be enough. |
See #2762
Also fix #2605
And trigger a "layout-applied" event