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.

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

Fix tree view flickering from async eventemitter #3227

Merged
merged 1 commit into from Mar 21, 2019

Conversation

Projects
None yet
4 participants
@gnomeontherun
Copy link
Contributor

gnomeontherun commented Mar 21, 2019

fix(tree) set tree view node event emitter to synchronous and don't output selected changes when setting input on node

A tree view with a number of nodes could be tricked into a state of uncertainty (flickering) by double clicking fast to trigger too many async event loops in the bindings. This stops the event propagation from firing in this specific case, but keeps it in the remainder where it is needed to update other nodes.

fixes #3073

Release notes

Bugfix for tree view where it could trigger a flickering state by double clicking fast, or in any case where the tree tried to update before the tree had stabilized.

Show resolved Hide resolved src/clr-angular/data/tree-view/tree-node.ts Outdated
expect(child.componentInstance.selected).toBe(ClrSelectedState.SELECTED);
this.clarityDirective.selected = ClrSelectedState.UNSELECTED;
this.detectChanges();
expect(child.componentInstance.selected).toBe(ClrSelectedState.UNSELECTED);

This comment has been minimized.

Copy link
@youdz

youdz Mar 21, 2019

Contributor

Testing the child here seems to have nothing to do with the current unit test. I get that the line

this.clarityDirective.selected = ClrSelectedState.UNSELECTED;

doesn't actually trigger the output for the parent, but this test relies on the entire logic of downward propagation of selection for eager trees, which feels wrong for a pure two-way binding test.

I think the next unit test you added covers the two-way binding, so you can remove this one or change it to describe more specifically what you're testing (presumably chocolate errors?).

This comment has been minimized.

Copy link
@gnomeontherun

gnomeontherun Mar 21, 2019

Author Contributor

Since the selection will propigate to the child, I'm testing that as the two way binding to get around the input change limitation I'm testing in the other test. If you're happy with the second test then I'll just remove this one.

multiRootSelected: { [key: string]: ClrSelectedState } = {};
private buildDefaultSelected(rootMap, selectedMap: SelectedMap = {}) {
if (!Array.isArray(rootMap)) {
rootMap = [rootMap];

This comment has been minimized.

Copy link
@Shijir

Shijir Mar 21, 2019

Contributor

In this case of !Array.isArray(rootMap), rootMap is nodelist, no? Shouldn't we do Array.from(rootMap) instead of [rootMap]?

This comment has been minimized.

Copy link
@gnomeontherun

gnomeontherun Mar 21, 2019

Author Contributor

rootMap is either an array or an object, see the singleRoot and multiRoot properties on the class.

This comment has been minimized.

Copy link
@Shijir

Shijir Mar 21, 2019

Contributor

But on line 51, we have this.buildDefaultSelected(node.children, selectedMap);
So we are basically putting node.children, which is NodeList in the array.

@@ -41,12 +44,25 @@ export class EagerRecursiveTreeDemo {
},
];

singleRootSelected: { [key: string]: ClrSelectedState } = {};
multiRootSelected: { [key: string]: ClrSelectedState } = {};
private buildDefaultSelected(rootMap, selectedMap: SelectedMap = {}) {

This comment has been minimized.

Copy link
@Shijir

Shijir Mar 21, 2019

Contributor

Can we have a type on rootMap parameter?

This comment has been minimized.

Copy link
@gnomeontherun

gnomeontherun Mar 21, 2019

Author Contributor

Added.

@Jinnie

Jinnie approved these changes Mar 21, 2019

Copy link
Contributor

Jinnie left a comment

LGTM
with minor comments

this._model.setSelected(value, this.featuresService.eager, this.featuresService.eager);
this.skipEmitChange = false;
}

// We need an async EventEmitter or we will trigger chocolate errors like it's 2016.

This comment has been minimized.

Copy link
@Jinnie

Jinnie Mar 21, 2019

Contributor

So this comment is not relevant anymore.

}

// We need an async EventEmitter or we will trigger chocolate errors like it's 2016.
@Output('clrSelectedChange') selectedChange = new EventEmitter<ClrSelectedState>(true);
@Output('clrSelectedChange') selectedChange = new EventEmitter<ClrSelectedState>(false);

This comment has been minimized.

Copy link
@Jinnie

Jinnie Mar 21, 2019

Contributor

Do we need explicit false param? It's false by default.

This comment has been minimized.

Copy link
@gnomeontherun

gnomeontherun Mar 21, 2019

Author Contributor

I know, but decided to leave it to be clear that we want it false.

<h4>Eager tree, inconsistent pre-selection (Christmas Edition)</h4>
<!-- THIS IS AN UNSUPPORTED CASE, AND WILL THROW CHOCOLATE ERRORS. RETAINING ONLY FOR TEST CASES. -->

<!-- <h4>Eager tree, inconsistent pre-selection (Christmas Edition)</h4>

This comment has been minimized.

Copy link
@Jinnie

Jinnie Mar 21, 2019

Contributor

Btw, as a developer, I found this section useful when I first saw it.

// Here we are expecting that the output has not emitted.
// See https://github.com/vmware/clarity/issues/3073
expect<ClrSelectedState>(this.hostComponent.selected).toBe(ClrSelectedState.SELECTED);
// By setting the model here, we will emit the binding since its not through the input

This comment has been minimized.

Copy link
@Jinnie

Jinnie Mar 21, 2019

Contributor

nit: its -> it's

@gnomeontherun gnomeontherun self-assigned this Mar 21, 2019

@gnomeontherun gnomeontherun added this to the 1.1.2 milestone Mar 21, 2019

@youdz

youdz approved these changes Mar 21, 2019

fix(tree) set tree view node event emitter to synchronous and don't o…
…utput selected changes when setting input on node

A tree view with a number of nodes could be tricked into a state of uncertainty (flickering) by double clicking fast to trigger too many async event loops in the bindings. This stops the event propagation from firing in this specific case, but keeps it in the remainder where it is needed to update other nodes.

fixes #3073

Signed-off-by: Jeremy Wilken <gnomation@gnomeontherun.com>

@gnomeontherun gnomeontherun force-pushed the gnomeontherun:tree-flicker branch from 2b56a76 to 6082b48 Mar 21, 2019

@gnomeontherun gnomeontherun merged commit d5ea70d into vmware:master Mar 21, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@gnomeontherun gnomeontherun deleted the gnomeontherun:tree-flicker branch Mar 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.