Skip to content

Commit b41a20d

Browse files
fcollonvalfcollonval
authored
Don't refresh if palette is within a hidden branch (#746)
* Don't refresh if palette is with in a hidden branch Fixes #744 * Update API * Improve isVisible instead of adding a new API * Comment a skipped test * Produce sourcemap for tests to ease debug * Fix english mistake --------- Co-authored-by: Frédéric Collonval <[email protected]>
1 parent 7251957 commit b41a20d

File tree

7 files changed

+166
-73
lines changed

7 files changed

+166
-73
lines changed

‎buildutils/src/rollup.tests.config.js

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,15 @@
55

66
import nodeResolve from '@rollup/plugin-node-resolve';
77
import commonjs from '@rollup/plugin-commonjs';
8+
import sourcemaps from 'rollup-plugin-sourcemaps';
89

910
export function createRollupTestConfig(options) {
1011
return {
1112
input: './lib/index.spec.js',
1213
output: {
13-
file: './lib/bundle.test.js'
14+
file: './lib/bundle.test.js',
15+
sourcemap: true
1416
},
15-
plugins: [commonjs(), nodeResolve()]
17+
plugins: [commonjs(), nodeResolve(), sourcemaps()]
1618
};
1719
}

‎packages/widgets/src/commandpalette.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -295,7 +295,9 @@ export class CommandPalette extends Widget {
295295
* A message handler invoked on an `'update-request'` message.
296296
*/
297297
protected onUpdateRequest(msg: Message): void {
298-
if (this.isHidden) {
298+
if (!this.isVisible) {
299+
// Ensure to clear the content if the widget is hidden
300+
VirtualDOM.render(null, this.contentNode);
299301
return;
300302
}
301303

‎packages/widgets/src/widget.ts

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,10 @@ export class Widget implements IMessageHandler, IObservableDisposable {
112112

113113
/**
114114
* Test whether the widget is explicitly hidden.
115+
*
116+
* #### Notes
117+
* You should prefer `!{@link isVisible}` over `{@link isHidden}` if you want to know if the
118+
* widget is hidden as this does not test if the widget is hidden because one of its ancestors is hidden.
115119
*/
116120
get isHidden(): boolean {
117121
return this.testFlag(Widget.Flag.IsHidden);
@@ -123,9 +127,20 @@ export class Widget implements IMessageHandler, IObservableDisposable {
123127
* #### Notes
124128
* A widget is visible when it is attached to the DOM, is not
125129
* explicitly hidden, and has no explicitly hidden ancestors.
130+
*
131+
* Since 2.7.0, this does not rely on the {@link Widget.Flag.IsVisible} flag.
132+
* It recursively checks the visibility of all parent widgets.
126133
*/
127134
get isVisible(): boolean {
128-
return this.testFlag(Widget.Flag.IsVisible);
135+
// eslint-disable-next-line @typescript-eslint/no-this-alias
136+
let parent: Widget | null = this;
137+
do {
138+
if (parent.isHidden || !parent.isAttached) {
139+
return false;
140+
}
141+
parent = parent.parent;
142+
} while (parent != null);
143+
return true;
129144
}
130145

131146
/**
@@ -482,6 +497,9 @@ export class Widget implements IMessageHandler, IObservableDisposable {
482497
*
483498
* #### Notes
484499
* This will not typically be called directly by user code.
500+
*
501+
* Since 2.7.0, {@link Widget.Flag.IsVisible} is deprecated.
502+
* It will be removed in a future version.
485503
*/
486504
testFlag(flag: Widget.Flag): boolean {
487505
return (this._flags & flag) !== 0;
@@ -492,6 +510,9 @@ export class Widget implements IMessageHandler, IObservableDisposable {
492510
*
493511
* #### Notes
494512
* This will not typically be called directly by user code.
513+
*
514+
* Since 2.7.0, {@link Widget.Flag.IsVisible} is deprecated.
515+
* It will be removed in a future version.
495516
*/
496517
setFlag(flag: Widget.Flag): void {
497518
this._flags |= flag;
@@ -502,6 +523,9 @@ export class Widget implements IMessageHandler, IObservableDisposable {
502523
*
503524
* #### Notes
504525
* This will not typically be called directly by user code.
526+
*
527+
* Since 2.7.0, {@link Widget.Flag.IsVisible} is deprecated.
528+
* It will be removed in a future version.
505529
*/
506530
clearFlag(flag: Widget.Flag): void {
507531
this._flags &= ~flag;
@@ -856,6 +880,9 @@ export namespace Widget {
856880

857881
/**
858882
* The widget is visible.
883+
*
884+
* @deprecated since 2.7.0, apply that flag consistently was not reliable
885+
* so it was dropped in favor of a recursive check of the visibility of all parents.
859886
*/
860887
IsVisible = 0x8,
861888

‎packages/widgets/tests/src/commandpalette.spec.ts

Lines changed: 119 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ describe('@lumino/widgets', () => {
4444
beforeEach(() => {
4545
commands = new CommandRegistry();
4646
palette = new CommandPalette({ commands });
47+
Widget.attach(palette, document.body);
4748
});
4849

4950
afterEach(() => {
@@ -493,77 +494,134 @@ describe('@lumino/widgets', () => {
493494
});
494495

495496
let palette = new CommandPalette({ commands });
496-
palette.addItem({ command: 'example:cut', category: 'Edit' });
497-
palette.addItem({ command: 'example:copy', category: 'Edit' });
498-
palette.addItem({ command: 'example:paste', category: 'Edit' });
499-
palette.addItem({ command: 'example:one', category: 'Number' });
500-
palette.addItem({ command: 'example:two', category: 'Number' });
501-
palette.addItem({ command: 'example:three', category: 'Number' });
502-
palette.addItem({ command: 'example:four', category: 'Number' });
503-
palette.addItem({ command: 'example:black', category: 'Number' });
504-
palette.addItem({ command: 'example:new-tab', category: 'File' });
505-
palette.addItem({ command: 'example:close-tab', category: 'File' });
506-
palette.addItem({ command: 'example:save-on-exit', category: 'File' });
507-
palette.addItem({
508-
command: 'example:open-task-manager',
509-
category: 'File'
510-
});
511-
palette.addItem({ command: 'example:close', category: 'File' });
512-
palette.addItem({
513-
command: 'example:clear-cell',
514-
category: 'Notebook Cell Operations'
515-
});
516-
palette.addItem({
517-
command: 'example:cut-cells',
518-
category: 'Notebook Cell Operations'
519-
});
520-
palette.addItem({
521-
command: 'example:run-cell',
522-
category: 'Notebook Cell Operations'
523-
});
524-
palette.addItem({ command: 'example:cell-test', category: 'Console' });
525-
palette.addItem({ command: 'notebook:new', category: 'Notebook' });
526-
palette.id = 'palette';
527-
528-
// Search the command palette: update the inputNode, then force a refresh
529-
palette.inputNode.value = 'clea';
497+
Widget.attach(palette, document.body);
498+
try {
499+
palette.addItem({ command: 'example:cut', category: 'Edit' });
500+
palette.addItem({ command: 'example:copy', category: 'Edit' });
501+
palette.addItem({ command: 'example:paste', category: 'Edit' });
502+
palette.addItem({ command: 'example:one', category: 'Number' });
503+
palette.addItem({ command: 'example:two', category: 'Number' });
504+
palette.addItem({ command: 'example:three', category: 'Number' });
505+
palette.addItem({ command: 'example:four', category: 'Number' });
506+
palette.addItem({ command: 'example:black', category: 'Number' });
507+
palette.addItem({ command: 'example:new-tab', category: 'File' });
508+
palette.addItem({ command: 'example:close-tab', category: 'File' });
509+
palette.addItem({
510+
command: 'example:save-on-exit',
511+
category: 'File'
512+
});
513+
palette.addItem({
514+
command: 'example:open-task-manager',
515+
category: 'File'
516+
});
517+
palette.addItem({ command: 'example:close', category: 'File' });
518+
palette.addItem({
519+
command: 'example:clear-cell',
520+
category: 'Notebook Cell Operations'
521+
});
522+
palette.addItem({
523+
command: 'example:cut-cells',
524+
category: 'Notebook Cell Operations'
525+
});
526+
palette.addItem({
527+
command: 'example:run-cell',
528+
category: 'Notebook Cell Operations'
529+
});
530+
palette.addItem({
531+
command: 'example:cell-test',
532+
category: 'Console'
533+
});
534+
palette.addItem({ command: 'notebook:new', category: 'Notebook' });
535+
palette.id = 'palette';
536+
537+
// Search the command palette: update the inputNode, then force a refresh
538+
palette.inputNode.value = 'clea';
539+
palette.refresh();
540+
MessageLoop.flush();
541+
542+
// Expect that headers and items appear in descending score order,
543+
// even if the same header occurs multiple times.
544+
const children = palette.contentNode.children;
545+
expect(children.length).to.equal(7);
546+
expect(children[0].textContent).to.equal('Notebook Cell Operations');
547+
expect(children[1].getAttribute('data-command')).to.equal(
548+
'example:clear-cell'
549+
);
550+
// The next match should be from a different category
551+
expect(children[2].textContent).to.equal('File');
552+
expect(children[3].getAttribute('data-command')).to.equal(
553+
'example:close-tab'
554+
);
555+
// The next match should be the same as in a previous category
556+
expect(children[4].textContent).to.equal('Notebook Cell Operations');
557+
expect(children[5].getAttribute('data-command')).to.equal(
558+
'example:cut-cells'
559+
);
560+
// The next match has the same category as the previous one did, so the header is not repeated
561+
expect(children[6].getAttribute('data-command')).to.equal(
562+
'example:run-cell'
563+
);
564+
} finally {
565+
palette.dispose();
566+
}
567+
});
568+
569+
it('should clear the widget content if hidden', () => {
570+
commands.addCommand('test', { execute: () => {}, label: 'test' });
571+
palette.addItem({ command: 'test', category: 'test' });
572+
const content = palette.contentNode;
573+
const itemClass = '.lm-CommandPalette-item';
574+
const items = () => content.querySelectorAll(itemClass);
575+
530576
palette.refresh();
531577
MessageLoop.flush();
532578

533-
// Expect that headers and items appear in descending score order,
534-
// even if the same header occurs multiple times.
535-
const children = palette.contentNode.children;
536-
expect(children.length).to.equal(7);
537-
expect(children[0].textContent).to.equal('Notebook Cell Operations');
538-
expect(children[1].getAttribute('data-command')).to.equal(
539-
'example:clear-cell'
540-
);
541-
// The next match should be from a different category
542-
expect(children[2].textContent).to.equal('File');
543-
expect(children[3].getAttribute('data-command')).to.equal(
544-
'example:close-tab'
545-
);
546-
// The next match should be the same as in a previous category
547-
expect(children[4].textContent).to.equal('Notebook Cell Operations');
548-
expect(children[5].getAttribute('data-command')).to.equal(
549-
'example:cut-cells'
550-
);
551-
// The next match has the same category as the previous one did, so the header is not repeated
552-
expect(children[6].getAttribute('data-command')).to.equal(
553-
'example:run-cell'
554-
);
579+
expect(items()).to.have.length(1);
580+
581+
palette.hide();
582+
palette.refresh();
583+
MessageLoop.flush();
584+
expect(items()).to.have.length(0);
585+
});
586+
587+
it('should clear the widget content if container is hidden', () => {
588+
const parent = new Widget();
589+
Widget.attach(parent, document.body);
590+
try {
591+
palette.parent = parent;
592+
commands.addCommand('test', { execute: () => {}, label: 'test' });
593+
palette.addItem({ command: 'test', category: 'test' });
594+
const content = palette.contentNode;
595+
const itemClass = '.lm-CommandPalette-item';
596+
const items = () => content.querySelectorAll(itemClass);
597+
598+
palette.refresh();
599+
MessageLoop.flush();
600+
601+
expect(items()).to.have.length(1);
602+
603+
parent.hide();
604+
palette.refresh();
605+
MessageLoop.flush();
606+
expect(items()).to.have.length(0);
607+
} finally {
608+
parent.dispose();
609+
}
555610
});
556611
});
557612

558613
describe('#handleEvent()', () => {
559614
it('should handle click, keydown, and input events', () => {
560615
let palette = new LogPalette({ commands });
561616
Widget.attach(palette, document.body);
562-
['click', 'keydown', 'input'].forEach(type => {
563-
palette.node.dispatchEvent(new Event(type, { bubbles }));
564-
expect(palette.events).to.contain(type);
565-
});
566-
palette.dispose();
617+
try {
618+
['click', 'keydown', 'input'].forEach(type => {
619+
palette.node.dispatchEvent(new Event(type, { bubbles }));
620+
expect(palette.events).to.contain(type);
621+
});
622+
} finally {
623+
palette.dispose();
624+
}
567625
});
568626

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

574632
palette.addItem(defaultOptions);
575-
Widget.attach(palette, document.body);
576633
MessageLoop.flush();
577634

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

589646
palette.addItem(defaultOptions);
590-
Widget.attach(palette, document.body);
591647
MessageLoop.flush();
592648

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

606662
palette.addItem(defaultOptions);
607-
Widget.attach(palette, document.body);
608663
MessageLoop.flush();
609664

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

627682
palette.addItem(defaultOptions);
628-
Widget.attach(palette, document.body);
629683
MessageLoop.flush();
630684

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

649703
palette.addItem(defaultOptions);
650-
Widget.attach(palette, document.body);
651704
MessageLoop.flush();
652705

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

679732
palette.addItem(defaultOptions);
680-
Widget.attach(palette, document.body);
681733
MessageLoop.flush();
682734

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

707-
Widget.attach(palette, document.body);
708759
MessageLoop.flush();
709760

710761
let content = palette.contentNode;
@@ -734,7 +785,6 @@ describe('@lumino/widgets', () => {
734785
});
735786
});
736787

737-
Widget.attach(palette, document.body);
738788
MessageLoop.flush();
739789

740790
let headers = () =>

‎packages/widgets/tests/src/tabbar.spec.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1815,6 +1815,7 @@ describe('@lumino/widgets', () => {
18151815
* TODO:
18161816
* Find a way to trigger the change of focus.
18171817
*/
1818+
/*
18181819
it.skip('should keep focus on the second tab on tabulation', () => {
18191820
const node = document.createElement('div');
18201821
node.setAttribute('tabindex', '0');
@@ -1844,6 +1845,7 @@ describe('@lumino/widgets', () => {
18441845
);
18451846
expect(document.activeElement).to.equal(secondTab);
18461847
});
1848+
*/
18471849
});
18481850

18491851
context('contextmenu', () => {

0 commit comments

Comments
 (0)