ClrPopoverContent rootNodes of undefined when opening/closing popup

[BramMeerten]

Describe the bug

When opening and closing a popover synchronously, we get the following error since clarity 3.0.0:
TypeError: Cannot read property 'rootNodes' of undefined

This is caused by the code change on this line:

// Here we collect subsequent synchronously received content-check events and only take action
// at the end of the cycle. See below for details on the check-collector pattern.
this.checkCollector.pipe(debounceTime(0)).subscribe(() => {
    this.alignContent();
    this.shouldRealign = false;
    this.renderer.setStyle(this.view.rootNodes[0], 'opacity', '1');
})

By the time the code is triggered, the popup is already closed (because of debounceTime) and this.view is undefined, causing an error.

How to reproduce

Can be reproduced with the following test:

import {ComponentFixture, TestBed} from '@angular/core/testing';
import {Component} from '@angular/core';
import {By} from '@angular/platform-browser';
import {ClarityModule} from '@clr/angular';

describe('Open/close filter', () => {

  beforeEach(() => {
    TestBed.configureTestingModule({
      imports: [ClarityModule],
      declarations: [TestComponent]
    });
  });

  it('Open and close filter', () => {
    const fixture = TestBed.createComponent(TestComponent);
    fixture.detectChanges();

    // Open and close filter
    toggleFilter(fixture);
    toggleFilter(fixture);

    expect(false).toBeFalsy();
  });

  function toggleFilter(fixture: ComponentFixture<any>) {
    const toggle = fixture.debugElement.query(By.css('clr-dg-column .datagrid-filter-toggle'));
    toggle.nativeElement.click();
    fixture.detectChanges();
  }
});

@Component({
  template: `
    <clr-datagrid>

      <clr-dg-column>
        Field
        <clr-dg-filter>Filter</clr-dg-filter>
      </clr-dg-column>

      <clr-dg-row><clr-dg-cell>Row</clr-dg-cell></clr-dg-row>
    </clr-datagrid>
  `
})
class TestComponent {
}

Expected behavior

The code should check if view is still defined. alignContent already has this check, but should also be added for renderer.setStyle:

// Here we collect subsequent synchronously received content-check events and only take action
// at the end of the cycle. See below for details on the check-collector pattern.
this.checkCollector.pipe(debounceTime(0)).subscribe(() => {
    this.alignContent();
    this.shouldRealign = false;
    if (this.view) { 
        this.renderer.setStyle(this.view.rootNodes[0], 'opacity', '1');
    }
})

Versions

App

• Angular: 9.1.0
• Node: 13.10.1
• Clarity: 3.0.1

[BramMeerten]

In case you're wondering what our use case is:
We use this in tests where we open a grid filter, get the content from the filter, and close the filter.
Right now a lot of our tests are broken after upgrading.

[gnomeontherun]

Thanks for the report, we'll investigate further.

[Shijir]

Hi @BramMeerten, do you have Ivy enabled in your app? If yes, that's related to Angular's regression error: angular/angular#35967

[BramMeerten]

Hey @Shijir , thanks for your response, I'm using Ivy, but I'm not convinced this is the same problem.
The angular bugreport describes an error being raised when calling view.rootnodes:

ERROR Error: ASSERTION ERROR: Should be one of Element, Container, Projection, ElementContainer, IcuContainer but got [Expected=> true == false <=Actual]

But in this case view is just undefined.
This is because everything happens in this order:

1. Filter is opened
2. checkCollector event is emitted
3. Filter is closed
4. code inside subscribe on checkCollector is executed, but filter is already closed.

Step 4 is after step 3 because of the debounceTime(0).

I added some logging to verify this:

// Here we collect subsequent synchronously received content-check events and only take action
// at the end of the cycle. See below for details on the check-collector pattern.
this.checkCollector.pipe(
    map(() => console.log('Received content-check event. Is view truthy?', !!this.view)),
    debounceTime(0)).subscribe(() => {
          console.log('Inside subscribe. Is view truthy?', !!this.view);
          this.alignContent();
          this.shouldRealign = false;
          this.renderer.setStyle(this.view.rootNodes[0], 'opacity', '1');
}));

This is the output:

toggle filter // console.log in toggleFilter function in my test
Received content-check event. Is view truthy? true
toggle filter // filter is closed
Inside subscribe. Is view truthy? false

[BramMeerten]

@Shijir , did you get a chance to look at my comment? Do you agree this is a different bug?

@gnomeontherun I'd like to fix this and create a pull request, here is the proposal:

Summary

When handling the content-check events in the ClrPopoverContent, make sure view is still defined before updating the renderer style.
This way there won't be any errors when the popover is closed before the subscriber receives an event. Right now this error can occur in unit tests.

This change does not introduce a new behavior.

Examples

It would look something like this:

// Here we collect subsequent synchronously received content-check events and only take action
// at the end of the cycle. See below for details on the check-collector pattern.
this.checkCollector.pipe(debounceTime(0)).subscribe(() => {
    this.alignContent();
    this.shouldRealign = false;
    if (this.view) {  // new code
        this.renderer.setStyle(this.view.rootNodes[0], 'opacity', '1');
    }
})

API

/

Implementation Plan

Only one file needs to be updated + a tests needs to be written. This is a low impact change.

Conclusion

I don't expect this to take very long to implement, the code change is very simple. The most work will be the test, but I already have an example in my original post.

[gnomeontherun]

@BramMeerten We'd be happy to see a PR for this, I agree the check is sensible but writing a good test is the hard part.

[BramMeerten]

Hey @gnomeontherun, I created a test + fix.
Do you have topic branch for me?

[gnomeontherun]

A topic branch isn't needed to make a PR, its only for long running collaborative contributions with a lots of iteration. You can make a PR from your own branch against master as the fastest solution here.

[github-actions]

Hi there 👋, this is an automated message. To help Clarity keep track of discussions, we automatically lock closed issues after 14 days. Please look for another open issue or open a new issue with updated details and reference this one as necessary.