Skip to content

Commit 26dec5b

Browse files
authored
Bug Fix: Prevent Card Fob from dispatching duplicate events when a fob is removed (#5976)
## Motivation for features / changes #5968 ## Technical description of changes The card fob would dispatch a `timeSelectionChanged` event on mouse up even if the time selection had not changed... ## Detailed steps to verify changes work correctly (as executed by you) 1) Start tensorboard 2) Go to http://localhost:6006 3) Enable step selection 4) Open the console and clear it 5) Dismiss the fob by clicking this button ![image](https://user-images.githubusercontent.com/78179109/195714210-5f1bee4e-fb1b-4c89-ae24-e6933fa004c0.png) ## Alternate designs / implementations considered Add a `mousedown` event listener to the dismiss button which prevents propagation. Unfortunately this would prevent clicks starting on the dismiss button from dragging the fob.
1 parent 5aa82d7 commit 26dec5b

File tree

2 files changed

+39
-18
lines changed

2 files changed

+39
-18
lines changed

tensorboard/webapp/widgets/card_fob/card_fob_controller_component.ts

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ export class CardFobControllerComponent {
6363
new EventEmitter<TimeSelectionWithAffordance>();
6464
@Output() onTimeSelectionToggled = new EventEmitter();
6565

66+
private hasFobMoved: boolean = false;
6667
private currentDraggingFob: Fob = Fob.NONE;
6768
private affordance: TimeSelectionAffordance = TimeSelectionAffordance.NONE;
6869

@@ -117,11 +118,14 @@ export class CardFobControllerComponent {
117118
document.removeEventListener('mousemove', this.mouseListener);
118119
document.removeEventListener('mouseup', this.stopListener);
119120
this.currentDraggingFob = Fob.NONE;
120-
this.onTimeSelectionChanged.emit({
121-
timeSelection: this.timeSelection,
122-
affordance: this.affordance,
123-
});
121+
if (this.hasFobMoved) {
122+
this.onTimeSelectionChanged.emit({
123+
timeSelection: this.timeSelection,
124+
affordance: this.affordance,
125+
});
126+
}
124127
this.affordance = TimeSelectionAffordance.NONE;
128+
this.hasFobMoved = false;
125129
}
126130

127131
isVertical() {
@@ -202,6 +206,7 @@ export class CardFobControllerComponent {
202206
this.onTimeSelectionChanged.emit({
203207
timeSelection: newTimeSelection,
204208
});
209+
this.hasFobMoved = true;
205210
}
206211

207212
isDraggingLower(position: number, movement: number): boolean {

tensorboard/webapp/widgets/card_fob/card_fob_controller_test.ts

Lines changed: 30 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -316,6 +316,34 @@ describe('card_fob_controller', () => {
316316
affordance: TimeSelectionAffordance.FOB,
317317
});
318318
});
319+
320+
it('does not fire event when time selection does not change', () => {
321+
const fixture = createComponent({
322+
timeSelection: {start: {step: 2}, end: {step: 3}},
323+
});
324+
fixture.detectChanges();
325+
const fobController = fixture.componentInstance.fobController;
326+
327+
fobController.startDrag(
328+
Fob.END,
329+
TimeSelectionAffordance.FOB,
330+
new MouseEvent('mouseDown')
331+
);
332+
expect((fobController as any).currentDraggingFob).toEqual(Fob.END);
333+
334+
const fakeEvent = new MouseEvent('mousemove', {
335+
clientY: 0,
336+
movementY: 0,
337+
});
338+
fobController.mouseMove(fakeEvent);
339+
expect((fobController as any).currentDraggingFob).toEqual(Fob.END);
340+
341+
fixture.detectChanges();
342+
fobController.stopDrag();
343+
fixture.detectChanges();
344+
345+
expect(onTimeSelectionChanged).not.toHaveBeenCalled();
346+
});
319347
});
320348

321349
describe('vertical dragging', () => {
@@ -723,13 +751,7 @@ describe('card_fob_controller', () => {
723751
expect(
724752
fobController.startFobWrapper.nativeElement.getBoundingClientRect().left
725753
).toEqual(2);
726-
expect(onTimeSelectionChanged).toHaveBeenCalledWith({
727-
timeSelection: {
728-
start: {step: 2},
729-
end: null,
730-
},
731-
affordance: TimeSelectionAffordance.FOB,
732-
});
754+
expect(onTimeSelectionChanged).not.toHaveBeenCalled();
733755
});
734756

735757
it('does not call getStepLowerThanMousePosition or getStepHigherThanMousePosition when mouse is dragging right but, is left of the fob', () => {
@@ -762,13 +784,7 @@ describe('card_fob_controller', () => {
762784
expect(
763785
fobController.startFobWrapper.nativeElement.getBoundingClientRect().left
764786
).toEqual(4);
765-
expect(onTimeSelectionChanged).toHaveBeenCalledWith({
766-
timeSelection: {
767-
start: {step: 4},
768-
end: null,
769-
},
770-
affordance: TimeSelectionAffordance.FOB,
771-
});
787+
expect(onTimeSelectionChanged).not.toHaveBeenCalled();
772788
});
773789

774790
it('does not move the start fob or call call getStepLowerThanMousePosition or getStepHigherThanMousePosition when mouse is dragging right but, the fob is already on the final step', () => {

0 commit comments

Comments
 (0)