Icon Dynamic Loading Logic Might Cause Incorrect SVG Element Rendering

[Roman-Lo]

Issue Story

Recently I use a way that listen the mouse enter/leave event to control the nzType property for the [nz-icon] directive (ng-zorro). Let's say the initial icon type is A:A, and the hover state icon type is A:B (mouse entered). I found that while I move my mouse super fast, which make the mouseenter and mouseleave event triggered so closely, the icon style will remain on the hover state (A:B), but not the initial state A:A.

This might be a rare case but it surely would occured under some circumstances.

Why?

The reason that causing this issue is the mishandle ajax promise mechanism, which, the icon directive does not check if the resolved svg data is the same type/name with the current type. Because the A:A is the initial state and was loaded the time the application ready, but A:B was load dynamically at the very first mouse enter event. If the A:B svg data resolved time is later than the mouse leave event trigger time (which now the icon type in the icon directive is A:A), the resolved A:B svg will be rendered on the page:

  // file: ant-design-icons/packages/icons-angular/src/component/icon.directive.ts (line: #32)
  protected _changeIcon(): Promise<SVGElement | null> {
    return new Promise<SVGElement | null>(resolve => {
      if (!this.type) {
        this._clearSVGElement();
        resolve(null);
      } else {
        this._iconService.getRenderedContent(
          this._parseIconType(this.type, this.theme),
          this.twoToneColor
        ).subscribe(svg => {
          this._setSVGElement(svg);
          resolve(svg);
        });
      }
    });
  }

How to Fix?

As the Why? section mentioned, the key to solve this issue is to stop rendering svg to the page which does not match the current type (theme or twoToneColor). So, we can:

Option 1

Firstly: Change the return type of _loadSVGFromCacheOrCreateNew (https://github.com/ant-design/ant-design-icons/blob/14912a18fac76d25ce983ac11047670fd8b4b072/packages/icons-angular/src/component/icon.service.ts#L286) to a type that contains the icon key {key: string, svg: SVGElement}, as well as the getRenderedContent function, to make its subscribers know what more information of the resolved svg data;

Then: In the _changeIcon() function of icon.directive.ts (https://github.com/ant-design/ant-design-icons/blob/14912a18fac76d25ce983ac11047670fd8b4b072/packages/icons-angular/src/component/icon.directive.ts#L32), add a validation for the svg information before execute the this._setSVGElement(svg);. If the validation passed, continue execution; otherwise, do not call this._setSVGElement(svg); and resolve a null value. This can prevent the incorrect svg element being rendered on the page.

Option 2

While Option 1 got lot of things to do, we can also simplify the process, by recording the icon information at the beginning of the _changeIcon() method and ensure them before rendering the svg data on the page. The updated _changeIcon() function may look like:

protected _changeIcon(): Promise<SVGElement | null> {
  // record the icon information
  const iconState = `${this.type}_${this.theme}_${this.twoToneColor}`;
  return new Promise<SVGElement | null>(resolve => {
    if (!this.type) {
      this._clearSVGElement();
      resolve(null);
    } else {
      this._iconService.getRenderedContent(
        this._parseIconType(this.type, this.theme),
        this.twoToneColor
      ).subscribe(svg => {
        // generate the current icon information and see if it is the same as before.
        const curIconState = `${this.type}_${this.theme}_${this.twoToneColor}`;
        if (curIconState === iconState) {
          this._setSVGElement(svg);
          resolve(svg);
        } else {
          resolve(null);
        }

      });
    }
  });
}

Ending

I'm happy to listen for your feedback, and ready for a PR anytime you needed.

[vthinkxie]

cc @wendellhu95

[wzhudev]

I get your point here @Roman-Lo. We were running into a race condition. IMHO, the second fix is more localized and hence reasonable. I will fix this. Thank you!

[Roman-Lo]

I get your point here @Roman-Lo. We were running into a race condition. IMHO, the second fix is more localized and hence reasonable. I will fix this. Thank you!

Thanks!