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.

Sign up for GitHub

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

Jump to bottom

fix(TabBar): Event forwarding fails when hosted in a ShadowRoot #276

Merged
merged 1 commit into from
Dec 28, 2021
Merged

fix(TabBar): Event forwarding fails when hosted in a ShadowRoot #276

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions packages/widgets/src/docklayout.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ export class DockLayout extends Layout {
if (options.spacing !== undefined) {
this._spacing = Utils.clampDimension(options.spacing);
}
this._document = options.document || document;
this._hiddenMode =
options.hiddenMode !== undefined
? options.hiddenMode
Expand Down Expand Up @@ -1094,7 +1095,7 @@ export class DockLayout extends Layout {
*/
private _createTabBar(): TabBar<Widget> {
// Create the tab bar using the renderer.
let tabBar = this.renderer.createTabBar();
let tabBar = this.renderer.createTabBar(this._document);

// Enforce necessary tab bar behavior.
tabBar.orientation = 'horizontal';
Expand Down Expand Up @@ -1140,6 +1141,7 @@ export class DockLayout extends Layout {
private _dirty = false;
private _root: Private.LayoutNode | null = null;
private _box: ElementExt.IBoxSizing | null = null;
private _document: Document | ShadowRoot;
private _hiddenMode: Widget.HiddenMode;
private _items: Private.ItemMap = new Map<Widget, LayoutItem>();
}
Expand All @@ -1152,6 +1154,13 @@ export namespace DockLayout {
* An options object for creating a dock layout.
*/
export interface IOptions {
/**
* The document to use with the dock panel.
*
* The default is the global `document` instance.
*/
document?: Document | ShadowRoot;

/**
* The method for hiding widgets.
*
Expand Down Expand Up @@ -1181,7 +1190,7 @@ export namespace DockLayout {
*
* @returns A new tab bar for a dock layout.
*/
createTabBar(): TabBar<Widget>;
createTabBar(document?: Document | ShadowRoot): TabBar<Widget>;

/**
* Create a new handle node for use with a dock layout.
Expand Down Expand Up @@ -2184,7 +2193,7 @@ namespace Private {
renderer: DockLayout.IRenderer
): TabLayoutNode {
// Create the tab bar for the layout node.
let tabBar = renderer.createTabBar();
let tabBar = renderer.createTabBar(this._document);
Copy link
Member

@hbcarlos hbcarlos Jan 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this exist in this context?
This function is inside the Private namespace.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm updating lumino in JupyterLab, and there is an issue with this._document, when changing to a single-mode document.
jupyterlab/jupyterlab#11823

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have an example of the breaking code - I couldn't see anything related in the failing visual regression test?

Copy link
Contributor Author

@GordonSmith GordonSmith Jan 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix incoming.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: #281

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @GordonSmith, thanks for the quick reply.

Do you have an example of the breaking code

Yes, when in JupyterLab you change the view to a single-mode document, there is an error on this line:
https:/jupyterlab/jupyterlab/blob/3b21d568035f7383dfd88332e051277130aea7d8/packages/application/src/shell.ts#L557

Following the trace, you can see that the error comes from

lumino/packages/widgets/src/docklayout.ts

Line 2196 in 948eec9

let tabBar = renderer.createTabBar(this._document);

where this is null.


// Hide each widget and add it to the tab bar.
each(config.widgets, widget => {
Expand Down
31 changes: 16 additions & 15 deletions packages/widgets/src/dockpanel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ export class DockPanel extends Widget {

// Set up the dock layout for the panel.
this.layout = new DockLayout({
document: this._document,
renderer,
spacing: options.spacing,
hiddenMode: options.hiddenMode
Expand Down Expand Up @@ -723,12 +724,12 @@ export class DockPanel extends Widget {
event.stopPropagation();

// Add the extra document listeners.
document.addEventListener('keydown', this, true);
document.addEventListener('mouseup', this, true); // <DEPRECATED>
document.addEventListener('mousemove', this, true); // <DEPRECATED>
document.addEventListener('pointerup', this, true);
document.addEventListener('pointermove', this, true);
document.addEventListener('contextmenu', this, true);
this._document.addEventListener('keydown', this, true);
this._document.addEventListener('mouseup', this, true); // <DEPRECATED>
this._document.addEventListener('mousemove', this, true); // <DEPRECATED>
this._document.addEventListener('pointerup', this, true);
this._document.addEventListener('pointermove', this, true);
this._document.addEventListener('contextmenu', this, true);

// Compute the offset deltas for the handle press.
let rect = handle.getBoundingClientRect();
Expand Down Expand Up @@ -798,12 +799,12 @@ export class DockPanel extends Widget {
this._pressData = null;

// Remove the extra document listeners.
document.removeEventListener('keydown', this, true);
document.removeEventListener('mouseup', this, true); // <DEPRECATED>
document.removeEventListener('mousemove', this, true); // <DEPRECATED>
document.removeEventListener('pointerup', this, true);
document.removeEventListener('pointermove', this, true);
document.removeEventListener('contextmenu', this, true);
this._document.removeEventListener('keydown', this, true);
this._document.removeEventListener('mouseup', this, true); // <DEPRECATED>
this._document.removeEventListener('mousemove', this, true); // <DEPRECATED>
this._document.removeEventListener('pointerup', this, true);
this._document.removeEventListener('pointermove', this, true);
this._document.removeEventListener('contextmenu', this, true);
}

/**
Expand Down Expand Up @@ -922,7 +923,7 @@ export class DockPanel extends Widget {
*/
private _createTabBar(): TabBar<Widget> {
// Create the tab bar.
let tabBar = this._renderer.createTabBar();
let tabBar = this._renderer.createTabBar(this._document);

// Set the generated tab bar property for the tab bar.
Private.isGeneratedTabBarProperty.set(tabBar, true);
Expand Down Expand Up @@ -1399,8 +1400,8 @@ export namespace DockPanel {
*
* @returns A new tab bar for a dock panel.
*/
createTabBar(): TabBar<Widget> {
let bar = new TabBar<Widget>();
createTabBar(document?: Document | ShadowRoot): TabBar<Widget> {
let bar = new TabBar<Widget>({ document });
bar.addClass('lm-DockPanel-tabBar');
/* <DEPRECATED> */
bar.addClass('p-DockPanel-tabBar');
Expand Down
55 changes: 37 additions & 18 deletions packages/widgets/src/tabbar.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ export class TabBar<T> extends Widget {
/* </DEPRECATED> */
this.contentNode.setAttribute('role', 'tablist');
this.setFlag(Widget.Flag.DisallowLayout);
this._document = options.document || document;
this.tabsMovable = options.tabsMovable || false;
this.titlesEditable = options.titlesEditable || false;
this.allowDeselect = options.allowDeselect || false;
Expand Down Expand Up @@ -156,6 +157,15 @@ export class TabBar<T> extends Widget {
*/
readonly renderer: TabBar.IRenderer<T>;

/**
* The document to use with the tab bar.
*
* The default is the global `document` instance.
*/
get document(): Document | ShadowRoot {
return this._document;
}

/**
* Whether the tabs are movable by the user.
*
Expand Down Expand Up @@ -779,8 +789,8 @@ export class TabBar<T> extends Widget {
};

// Add the document mouse up listener.
document.addEventListener('mouseup', this, true); // <DEPRECATED>
document.addEventListener('pointerup', this, true);
this.document.addEventListener('mouseup', this, true); // <DEPRECATED>
this.document.addEventListener('pointerup', this, true);

// Do nothing else if the middle button or add button is clicked.
if (event.button === 1 || addButtonClicked) {
Expand All @@ -795,10 +805,10 @@ export class TabBar<T> extends Widget {

// Add the extra listeners if the tabs are movable.
if (this.tabsMovable) {
document.addEventListener('mousemove', this, true); // <DEPRECATED>
document.addEventListener('pointermove', this, true);
document.addEventListener('keydown', this, true);
document.addEventListener('contextmenu', this, true);
this.document.addEventListener('mousemove', this, true); // <DEPRECATED>
this.document.addEventListener('pointermove', this, true);
this.document.addEventListener('keydown', this, true);
this.document.addEventListener('contextmenu', this, true);
}

// Update the current index as appropriate.
Expand Down Expand Up @@ -916,12 +926,12 @@ export class TabBar<T> extends Widget {
event.stopPropagation();

// Remove the extra mouse event listeners.
document.removeEventListener('mousemove', this, true); // <DEPRECATED>
document.removeEventListener('mouseup', this, true); // <DEPRECATED>
document.removeEventListener('pointermove', this, true);
document.removeEventListener('pointerup', this, true);
document.removeEventListener('keydown', this, true);
document.removeEventListener('contextmenu', this, true);
this.document.removeEventListener('mousemove', this, true); // <DEPRECATED>
this.document.removeEventListener('mouseup', this, true); // <DEPRECATED>
this.document.removeEventListener('pointermove', this, true);
this.document.removeEventListener('pointerup', this, true);
this.document.removeEventListener('keydown', this, true);
this.document.removeEventListener('contextmenu', this, true);

// Handle a release when the drag is not active.
if (!data.dragActive) {
Expand Down Expand Up @@ -1051,12 +1061,12 @@ export class TabBar<T> extends Widget {
this._dragData = null;

// Remove the extra mouse listeners.
document.removeEventListener('mousemove', this, true); // <DEPRECATED>
document.removeEventListener('mouseup', this, true); // <DEPRECATED>
document.removeEventListener('pointermove', this, true);
document.removeEventListener('pointerup', this, true);
document.removeEventListener('keydown', this, true);
document.removeEventListener('contextmenu', this, true);
this.document.removeEventListener('mousemove', this, true); // <DEPRECATED>
this.document.removeEventListener('mouseup', this, true); // <DEPRECATED>
this.document.removeEventListener('pointermove', this, true);
this.document.removeEventListener('pointerup', this, true);
this.document.removeEventListener('keydown', this, true);
this.document.removeEventListener('contextmenu', this, true);

// Indicate the drag has been aborted. This allows the mouse
// event handlers to return early when the drag is canceled.
Expand Down Expand Up @@ -1227,6 +1237,7 @@ export class TabBar<T> extends Widget {
private _currentIndex = -1;
private _titles: Title<T>[] = [];
private _orientation: TabBar.Orientation;
private _document: Document | ShadowRoot;
private _titlesEditable: boolean = false;
private _previousTitle: Title<T> | null = null;
private _dragData: Private.IDragData | null = null;
Expand Down Expand Up @@ -1319,6 +1330,14 @@ export namespace TabBar {
* An options object for creating a tab bar.
*/
export interface IOptions<T> {
/**
* The document to use with the tab bar.
*
* The default is the global `document` instance.
*/

document?: Document | ShadowRoot;

/**
* Name of the tab bar.
*
Expand Down
7 changes: 7 additions & 0 deletions packages/widgets/src/tabpanel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,13 @@ export namespace TabPanel {
* An options object for initializing a tab panel.
*/
export interface IOptions {
/**
* The document to use with the tab panel.
*
* The default is the global `document` instance.
*/
document?: Document | ShadowRoot;

/**
* Whether the tabs are movable by the user.
*
Expand Down