Skip to content

Commit

Permalink
Bug Fix: Prevent Card Fob from dispatching duplicate events when a fo…
Browse files Browse the repository at this point in the history
…b is removed (tensorflow#5976)

## Motivation for features / changes
tensorflow#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.
  • Loading branch information
rileyajones authored and dna2github committed May 1, 2023
1 parent cf829b0 commit 03a249e
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 18 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ export class CardFobControllerComponent {
new EventEmitter<TimeSelectionWithAffordance>();
@Output() onTimeSelectionToggled = new EventEmitter();

private hasFobMoved: boolean = false;
private currentDraggingFob: Fob = Fob.NONE;
private affordance: TimeSelectionAffordance = TimeSelectionAffordance.NONE;

Expand Down Expand Up @@ -117,11 +118,14 @@ export class CardFobControllerComponent {
document.removeEventListener('mousemove', this.mouseListener);
document.removeEventListener('mouseup', this.stopListener);
this.currentDraggingFob = Fob.NONE;
this.onTimeSelectionChanged.emit({
timeSelection: this.timeSelection,
affordance: this.affordance,
});
if (this.hasFobMoved) {
this.onTimeSelectionChanged.emit({
timeSelection: this.timeSelection,
affordance: this.affordance,
});
}
this.affordance = TimeSelectionAffordance.NONE;
this.hasFobMoved = false;
}

isVertical() {
Expand Down Expand Up @@ -202,6 +206,7 @@ export class CardFobControllerComponent {
this.onTimeSelectionChanged.emit({
timeSelection: newTimeSelection,
});
this.hasFobMoved = true;
}

isDraggingLower(position: number, movement: number): boolean {
Expand Down
44 changes: 30 additions & 14 deletions tensorboard/webapp/widgets/card_fob/card_fob_controller_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,34 @@ describe('card_fob_controller', () => {
affordance: TimeSelectionAffordance.FOB,
});
});

it('does not fire event when time selection does not change', () => {
const fixture = createComponent({
timeSelection: {start: {step: 2}, end: {step: 3}},
});
fixture.detectChanges();
const fobController = fixture.componentInstance.fobController;

fobController.startDrag(
Fob.END,
TimeSelectionAffordance.FOB,
new MouseEvent('mouseDown')
);
expect((fobController as any).currentDraggingFob).toEqual(Fob.END);

const fakeEvent = new MouseEvent('mousemove', {
clientY: 0,
movementY: 0,
});
fobController.mouseMove(fakeEvent);
expect((fobController as any).currentDraggingFob).toEqual(Fob.END);

fixture.detectChanges();
fobController.stopDrag();
fixture.detectChanges();

expect(onTimeSelectionChanged).not.toHaveBeenCalled();
});
});

describe('vertical dragging', () => {
Expand Down Expand Up @@ -723,13 +751,7 @@ describe('card_fob_controller', () => {
expect(
fobController.startFobWrapper.nativeElement.getBoundingClientRect().left
).toEqual(2);
expect(onTimeSelectionChanged).toHaveBeenCalledWith({
timeSelection: {
start: {step: 2},
end: null,
},
affordance: TimeSelectionAffordance.FOB,
});
expect(onTimeSelectionChanged).not.toHaveBeenCalled();
});

it('does not call getStepLowerThanMousePosition or getStepHigherThanMousePosition when mouse is dragging right but, is left of the fob', () => {
Expand Down Expand Up @@ -762,13 +784,7 @@ describe('card_fob_controller', () => {
expect(
fobController.startFobWrapper.nativeElement.getBoundingClientRect().left
).toEqual(4);
expect(onTimeSelectionChanged).toHaveBeenCalledWith({
timeSelection: {
start: {step: 4},
end: null,
},
affordance: TimeSelectionAffordance.FOB,
});
expect(onTimeSelectionChanged).not.toHaveBeenCalled();
});

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', () => {
Expand Down

0 comments on commit 03a249e

Please sign in to comment.