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

Don't refresh if palette is within a hidden branch #746

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Don't refresh if palette is within a hidden branch #746

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0ad4841
Don't refresh if palette is with in a hidden branch
fcollonval Mar 7, 2025
2cd2fe1
Update API
fcollonval Mar 7, 2025
e40bd13
Improve isVisible instead of adding a new API
fcollonval Mar 21, 2025
a55068a
Comment a skipped test
fcollonval Mar 21, 2025
eb5d361
Produce sourcemap for tests to ease debug
fcollonval Mar 21, 2025
cc1b9c4
Fix english mistake
fcollonval Mar 21, 2025
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
6 changes: 4 additions & 2 deletions buildutils/src/rollup.tests.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,15 @@

import nodeResolve from '@rollup/plugin-node-resolve';
import commonjs from '@rollup/plugin-commonjs';
import sourcemaps from 'rollup-plugin-sourcemaps';

export function createRollupTestConfig(options) {
return {
input: './lib/index.spec.js',
output: {
file: './lib/bundle.test.js'
file: './lib/bundle.test.js',
sourcemap: true
},
plugins: [commonjs(), nodeResolve()]
plugins: [commonjs(), nodeResolve(), sourcemaps()]
};
}
4 changes: 3 additions & 1 deletion packages/widgets/src/commandpalette.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,9 @@ export class CommandPalette extends Widget {
* A message handler invoked on an `'update-request'` message.
*/
protected onUpdateRequest(msg: Message): void {
if (this.isHidden) {
if (!this.isVisible) {
// Ensure to clear the content if the widget is hidden
VirtualDOM.render(null, this.contentNode);
return;
}

Expand Down
29 changes: 28 additions & 1 deletion packages/widgets/src/widget.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,10 @@ export class Widget implements IMessageHandler, IObservableDisposable {

/**
* Test whether the widget is explicitly hidden.
*
* #### Notes
* You should prefer `!{@link isVisible}` over `{@link isHidden}` if you want to know if the
* widget is hidden as this does not test if the widget is hidden because one of its ancestors is hidden.
*/
get isHidden(): boolean {
return this.testFlag(Widget.Flag.IsHidden);
Expand All @@ -123,9 +127,20 @@ export class Widget implements IMessageHandler, IObservableDisposable {
* #### Notes
* A widget is visible when it is attached to the DOM, is not
* explicitly hidden, and has no explicitly hidden ancestors.
*
* Since 2.7.0, this does not rely on the {@link Widget.Flag.IsVisible} flag.
* It recursively checks the visibility of all parent widgets.
*/
get isVisible(): boolean {
return this.testFlag(Widget.Flag.IsVisible);
// eslint-disable-next-line @typescript-eslint/no-this-alias
let parent: Widget | null = this;
do {
if (parent.isHidden || !parent.isAttached) {
return false;
}
parent = parent.parent;
} while (parent != null);
return true;
}

/**
Expand Down Expand Up @@ -482,6 +497,9 @@ export class Widget implements IMessageHandler, IObservableDisposable {
*
* #### Notes
* This will not typically be called directly by user code.
*
* Since 2.7.0, {@link Widget.Flag.IsVisible} is deprecated.
* It will be removed in a future version.
*/
testFlag(flag: Widget.Flag): boolean {
return (this._flags & flag) !== 0;
Expand All @@ -492,6 +510,9 @@ export class Widget implements IMessageHandler, IObservableDisposable {
*
* #### Notes
* This will not typically be called directly by user code.
*
* Since 2.7.0, {@link Widget.Flag.IsVisible} is deprecated.
* It will be removed in a future version.
*/
setFlag(flag: Widget.Flag): void {
this._flags |= flag;
Expand All @@ -502,6 +523,9 @@ export class Widget implements IMessageHandler, IObservableDisposable {
*
* #### Notes
* This will not typically be called directly by user code.
*
* Since 2.7.0, {@link Widget.Flag.IsVisible} is deprecated.
* It will be removed in a future version.
*/
clearFlag(flag: Widget.Flag): void {
this._flags &= ~flag;
Expand Down Expand Up @@ -856,6 +880,9 @@ export namespace Widget {

/**
* The widget is visible.
*
* @deprecated since 2.7.0, apply that flag consistently was not reliable
* so it was drop in favor of a recursive check of the visibility of all parents.
*/
IsVisible = 0x8,

Expand Down
188 changes: 119 additions & 69 deletions packages/widgets/tests/src/commandpalette.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ describe('@lumino/widgets', () => {
beforeEach(() => {
commands = new CommandRegistry();
palette = new CommandPalette({ commands });
Widget.attach(palette, document.body);
});

afterEach(() => {
Expand Down Expand Up @@ -493,77 +494,134 @@ describe('@lumino/widgets', () => {
});

let palette = new CommandPalette({ commands });
palette.addItem({ command: 'example:cut', category: 'Edit' });
palette.addItem({ command: 'example:copy', category: 'Edit' });
palette.addItem({ command: 'example:paste', category: 'Edit' });
palette.addItem({ command: 'example:one', category: 'Number' });
palette.addItem({ command: 'example:two', category: 'Number' });
palette.addItem({ command: 'example:three', category: 'Number' });
palette.addItem({ command: 'example:four', category: 'Number' });
palette.addItem({ command: 'example:black', category: 'Number' });
palette.addItem({ command: 'example:new-tab', category: 'File' });
palette.addItem({ command: 'example:close-tab', category: 'File' });
palette.addItem({ command: 'example:save-on-exit', category: 'File' });
palette.addItem({
command: 'example:open-task-manager',
category: 'File'
});
palette.addItem({ command: 'example:close', category: 'File' });
palette.addItem({
command: 'example:clear-cell',
category: 'Notebook Cell Operations'
});
palette.addItem({
command: 'example:cut-cells',
category: 'Notebook Cell Operations'
});
palette.addItem({
command: 'example:run-cell',
category: 'Notebook Cell Operations'
});
palette.addItem({ command: 'example:cell-test', category: 'Console' });
palette.addItem({ command: 'notebook:new', category: 'Notebook' });
palette.id = 'palette';

// Search the command palette: update the inputNode, then force a refresh
palette.inputNode.value = 'clea';
Widget.attach(palette, document.body);
try {
palette.addItem({ command: 'example:cut', category: 'Edit' });
palette.addItem({ command: 'example:copy', category: 'Edit' });
palette.addItem({ command: 'example:paste', category: 'Edit' });
palette.addItem({ command: 'example:one', category: 'Number' });
palette.addItem({ command: 'example:two', category: 'Number' });
palette.addItem({ command: 'example:three', category: 'Number' });
palette.addItem({ command: 'example:four', category: 'Number' });
palette.addItem({ command: 'example:black', category: 'Number' });
palette.addItem({ command: 'example:new-tab', category: 'File' });
palette.addItem({ command: 'example:close-tab', category: 'File' });
palette.addItem({
command: 'example:save-on-exit',
category: 'File'
});
palette.addItem({
command: 'example:open-task-manager',
category: 'File'
});
palette.addItem({ command: 'example:close', category: 'File' });
palette.addItem({
command: 'example:clear-cell',
category: 'Notebook Cell Operations'
});
palette.addItem({
command: 'example:cut-cells',
category: 'Notebook Cell Operations'
});
palette.addItem({
command: 'example:run-cell',
category: 'Notebook Cell Operations'
});
palette.addItem({
command: 'example:cell-test',
category: 'Console'
});
palette.addItem({ command: 'notebook:new', category: 'Notebook' });
palette.id = 'palette';

// Search the command palette: update the inputNode, then force a refresh
palette.inputNode.value = 'clea';
palette.refresh();
MessageLoop.flush();

// Expect that headers and items appear in descending score order,
// even if the same header occurs multiple times.
const children = palette.contentNode.children;
expect(children.length).to.equal(7);
expect(children[0].textContent).to.equal('Notebook Cell Operations');
expect(children[1].getAttribute('data-command')).to.equal(
'example:clear-cell'
);
// The next match should be from a different category
expect(children[2].textContent).to.equal('File');
expect(children[3].getAttribute('data-command')).to.equal(
'example:close-tab'
);
// The next match should be the same as in a previous category
expect(children[4].textContent).to.equal('Notebook Cell Operations');
expect(children[5].getAttribute('data-command')).to.equal(
'example:cut-cells'
);
// The next match has the same category as the previous one did, so the header is not repeated
expect(children[6].getAttribute('data-command')).to.equal(
'example:run-cell'
);
} finally {
palette.dispose();
}
});

it('should clear the widget content if hidden', () => {
commands.addCommand('test', { execute: () => {}, label: 'test' });
palette.addItem({ command: 'test', category: 'test' });
const content = palette.contentNode;
const itemClass = '.lm-CommandPalette-item';
const items = () => content.querySelectorAll(itemClass);

palette.refresh();
MessageLoop.flush();

// Expect that headers and items appear in descending score order,
// even if the same header occurs multiple times.
const children = palette.contentNode.children;
expect(children.length).to.equal(7);
expect(children[0].textContent).to.equal('Notebook Cell Operations');
expect(children[1].getAttribute('data-command')).to.equal(
'example:clear-cell'
);
// The next match should be from a different category
expect(children[2].textContent).to.equal('File');
expect(children[3].getAttribute('data-command')).to.equal(
'example:close-tab'
);
// The next match should be the same as in a previous category
expect(children[4].textContent).to.equal('Notebook Cell Operations');
expect(children[5].getAttribute('data-command')).to.equal(
'example:cut-cells'
);
// The next match has the same category as the previous one did, so the header is not repeated
expect(children[6].getAttribute('data-command')).to.equal(
'example:run-cell'
);
expect(items()).to.have.length(1);

palette.hide();
palette.refresh();
MessageLoop.flush();
expect(items()).to.have.length(0);
});

it('should clear the widget content if container is hidden', () => {
const parent = new Widget();
Widget.attach(parent, document.body);
try {
palette.parent = parent;
commands.addCommand('test', { execute: () => {}, label: 'test' });
palette.addItem({ command: 'test', category: 'test' });
const content = palette.contentNode;
const itemClass = '.lm-CommandPalette-item';
const items = () => content.querySelectorAll(itemClass);

palette.refresh();
MessageLoop.flush();

expect(items()).to.have.length(1);

parent.hide();
palette.refresh();
MessageLoop.flush();
expect(items()).to.have.length(0);
} finally {
parent.dispose();
}
});
});

describe('#handleEvent()', () => {
it('should handle click, keydown, and input events', () => {
let palette = new LogPalette({ commands });
Widget.attach(palette, document.body);
['click', 'keydown', 'input'].forEach(type => {
palette.node.dispatchEvent(new Event(type, { bubbles }));
expect(palette.events).to.contain(type);
});
palette.dispose();
try {
['click', 'keydown', 'input'].forEach(type => {
palette.node.dispatchEvent(new Event(type, { bubbles }));
expect(palette.events).to.contain(type);
});
} finally {
palette.dispose();
}
});

context('click', () => {
Expand All @@ -572,7 +630,6 @@ describe('@lumino/widgets', () => {
commands.addCommand('test', { execute: () => (called = true) });

palette.addItem(defaultOptions);
Widget.attach(palette, document.body);
MessageLoop.flush();

let node = palette.contentNode.querySelector(
Expand All @@ -587,7 +644,6 @@ describe('@lumino/widgets', () => {
commands.addCommand('test', { execute: () => (called = true) });

palette.addItem(defaultOptions);
Widget.attach(palette, document.body);
MessageLoop.flush();

let node = palette.contentNode.querySelector(
Expand All @@ -604,7 +660,6 @@ describe('@lumino/widgets', () => {
let content = palette.contentNode;

palette.addItem(defaultOptions);
Widget.attach(palette, document.body);
MessageLoop.flush();

let node = content.querySelector('.lm-mod-active');
Expand All @@ -625,7 +680,6 @@ describe('@lumino/widgets', () => {
let content = palette.contentNode;

palette.addItem(defaultOptions);
Widget.attach(palette, document.body);
MessageLoop.flush();

let node = content.querySelector('.lm-mod-active');
Expand All @@ -647,7 +701,6 @@ describe('@lumino/widgets', () => {
let content = palette.contentNode;

palette.addItem(defaultOptions);
Widget.attach(palette, document.body);
MessageLoop.flush();

let node = content.querySelector('.lm-mod-active');
Expand Down Expand Up @@ -677,7 +730,6 @@ describe('@lumino/widgets', () => {
let content = palette.contentNode;

palette.addItem(defaultOptions);
Widget.attach(palette, document.body);
MessageLoop.flush();

expect(content.querySelector('.lm-mod-active')).to.equal(null);
Expand All @@ -704,7 +756,6 @@ describe('@lumino/widgets', () => {
palette.addItem({ command: name, category: 'test' });
});

Widget.attach(palette, document.body);
MessageLoop.flush();

let content = palette.contentNode;
Expand Down Expand Up @@ -734,7 +785,6 @@ describe('@lumino/widgets', () => {
});
});

Widget.attach(palette, document.body);
MessageLoop.flush();

let headers = () =>
Expand Down
2 changes: 2 additions & 0 deletions packages/widgets/tests/src/tabbar.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1815,6 +1815,7 @@ describe('@lumino/widgets', () => {
* TODO:
* Find a way to trigger the change of focus.
*/
/*
it.skip('should keep focus on the second tab on tabulation', () => {
const node = document.createElement('div');
node.setAttribute('tabindex', '0');
Expand Down Expand Up @@ -1844,6 +1845,7 @@ describe('@lumino/widgets', () => {
);
expect(document.activeElement).to.equal(secondTab);
});
*/
});

context('contextmenu', () => {
Expand Down
Loading
Loading