Skip to content

Commit

Permalink
fix(popover-edit): hover content not showing up if content changes af…
Browse files Browse the repository at this point in the history
…ter init (#18937)

If the table's content changes after the first render, the popover edit wasn't showing up on hover, because some of the new subscribers weren't getting the previous value. These changes fix the issue by using `shareReplay` instead of `share`.

Fixes #18934.

(cherry picked from commit f70f49a)
  • Loading branch information
crisbeto authored and jelbourn committed Mar 31, 2020
1 parent f1c0c09 commit d7ff7cb
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 15 deletions.
16 changes: 8 additions & 8 deletions src/cdk-experimental/popover-edit/edit-event-dispatcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@ import {
distinctUntilChanged,
filter,
map,
share,
skip,
startWith,
shareReplay,
} from 'rxjs/operators';

import {CELL_SELECTOR, ROW_SELECTOR} from './constants';
Expand All @@ -33,7 +33,7 @@ const FOCUS_DELAY = 0;
/**
* The possible states for hover content:
* OFF - Not rendered.
* FOCUSABLE - Rendered in the dom and stylyed for its contents to be focusable but invisible.
* FOCUSABLE - Rendered in the dom and styled for its contents to be focusable but invisible.
* ON - Rendered and fully visible.
*/
export const enum HoverContentState {
Expand Down Expand Up @@ -81,7 +81,7 @@ export class EditEventDispatcher {
private readonly _startWithNull = startWith<Element|null>(null);
private readonly _distinctShare = pipe(
this._distinctUntilChanged as MonoTypeOperatorFunction<HoverContentState>,
share(),
shareReplay(1),
);
private readonly _startWithNullDistinct = pipe(
this._startWithNull,
Expand All @@ -90,7 +90,7 @@ export class EditEventDispatcher {

readonly editingAndEnabled = this.editing.pipe(
filter(cell => cell == null || !this.disabledCells.has(cell)),
share(),
shareReplay(1),
);

/** An observable that emits the row containing focus or an active edit. */
Expand All @@ -105,7 +105,7 @@ export class EditEventDispatcher {
this._distinctUntilChanged as MonoTypeOperatorFunction<Element|null>,
auditTime(FOCUS_DELAY), // Use audit to skip over blur events to the next focused element.
this._distinctUntilChanged as MonoTypeOperatorFunction<Element|null>,
share(),
shareReplay(1),
);

/** Tracks rows that contain hover content with a reference count. */
Expand All @@ -132,16 +132,16 @@ export class EditEventDispatcher {
skip(1), // Skip the initial emission of [null, null, null, null].
map(computeHoverContentState),
distinctUntilChanged(areMapEntriesEqual),
// Optimization: Enter the zone before share() so that we trigger a single
// Optimization: Enter the zone before shareReplay so that we trigger a single
// ApplicationRef.tick for all row updates.
this._enterZone(),
share(),
shareReplay(1),
);

private readonly _editingAndEnabledDistinct = this.editingAndEnabled.pipe(
distinctUntilChanged(),
this._enterZone(),
share(),
shareReplay(1),
);

// Optimization: Share row events observable with subsequent callers.
Expand Down
55 changes: 48 additions & 7 deletions src/cdk-experimental/popover-edit/popover-edit.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,13 +70,18 @@ abstract class BaseTestComponent {
@ViewChild('table') table: ElementRef;

preservedValues = new FormValueContainer<PeriodicElement, {'name': string}>();

nameEditDisabled = false;
ignoreSubmitUnlessValid = true;
clickOutBehavior: PopoverEditClickOutBehavior = 'close';
colspan: CdkPopoverEditColspan = {};
direction: Direction = 'ltr';

constructor() {
this.renderData();
}

abstract renderData(): void;

onSubmit(element: PeriodicElement, form: NgForm) {
if (!form.valid) { return; }

Expand Down Expand Up @@ -202,7 +207,11 @@ abstract class BaseTestComponent {
`
})
class VanillaTableOutOfCell extends BaseTestComponent {
elements = createElementData();
elements: ChemicalElement[];

renderData() {
this.elements = createElementData();
}
}

@Component({
Expand Down Expand Up @@ -231,7 +240,11 @@ class VanillaTableOutOfCell extends BaseTestComponent {
`
})
class VanillaTableInCell extends BaseTestComponent {
elements = createElementData();
elements: ChemicalElement[];

renderData() {
this.elements = createElementData();
}
}

class ElementDataSource extends DataSource<PeriodicElement> {
Expand Down Expand Up @@ -289,7 +302,11 @@ class ElementDataSource extends DataSource<PeriodicElement> {
})
class CdkFlexTableInCell extends BaseTestComponent {
displayedColumns = ['before', 'name', 'weight'];
dataSource = new ElementDataSource();
dataSource: ElementDataSource;

renderData() {
this.dataSource = new ElementDataSource();
}
}

@Component({
Expand Down Expand Up @@ -335,7 +352,11 @@ class CdkFlexTableInCell extends BaseTestComponent {
})
class CdkTableInCell extends BaseTestComponent {
displayedColumns = ['before', 'name', 'weight'];
dataSource = new ElementDataSource();
dataSource: ElementDataSource;

renderData() {
this.dataSource = new ElementDataSource();
}
}

const testCases: ReadonlyArray<[Type<BaseTestComponent>, string]> = [
Expand Down Expand Up @@ -369,7 +390,7 @@ describe('CDK Popover Edit', () => {
afterEach(() => {
// The overlay container's `ngOnDestroy` won't be called between test runs so we need
// to call it ourselves, in order to avoid leaking containers between tests and potentially
// throwing `querySelector` calls.
// throwing off `querySelector` calls.
overlayContainer.ngOnDestroy();
});

Expand Down Expand Up @@ -411,6 +432,21 @@ describe('CDK Popover Edit', () => {
expect(component.hoverContentStateForRow(1)).toBe(HoverContentState.ON);
}));

it('shows the hover content if the data changes after initialization', fakeAsync(() => {
fixture.componentInstance.renderData();
fixture.detectChanges();

const row = component.getRows()[0];
row.dispatchEvent(new Event('mouseover', {bubbles: true}));
row.dispatchEvent(new Event('mousemove', {bubbles: true}));

tick(20);
row.dispatchEvent(new Event('mousemove', {bubbles: true}));
tick(50);

expect(component.hoverContentStateForRow(0)).toBe(HoverContentState.ON);
}));

it('shows hover content for the focused row and makes the rows above and below focusable',
fakeAsync(() => {
expect(component.hoverContentStateForRow(1)).toBe(HoverContentState.OFF);
Expand Down Expand Up @@ -930,7 +966,12 @@ cdkPopoverEditTabOut`, fakeAsync(() => {
}
});

function createElementData() {
interface ChemicalElement {
name: string;
weight: number;
}

function createElementData(): ChemicalElement[] {
return [
{name: 'Hydrogen', weight: 1.007},
{name: 'Helium', weight: 4.0026},
Expand Down

0 comments on commit d7ff7cb

Please sign in to comment.