From d1fbc56d5e0cf7da3f213c2be7e7ebfd8add26e6 Mon Sep 17 00:00:00 2001 From: Karen Lee Date: Wed, 20 Jul 2022 11:15:10 -0700 Subject: [PATCH 1/5] fix(highlight): Ignore empty rects when rendering highlight canvas rects --- src/highlight/HighlightCanvas.tsx | 6 ++++++ src/highlight/__mocks__/data.ts | 16 ++++++++++++++++ src/highlight/__tests__/HighlightCanvas-test.tsx | 8 +++++++- 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/src/highlight/HighlightCanvas.tsx b/src/highlight/HighlightCanvas.tsx index 9e6ba4dea..dcdaf35a7 100644 --- a/src/highlight/HighlightCanvas.tsx +++ b/src/highlight/HighlightCanvas.tsx @@ -67,6 +67,12 @@ export default class HighlightCanvas extends React.PureComponent { shapesArray.forEach(rect => { const { height, isActive, isHover, width, x, y } = rect; + + // Ignore empty rects with a width or height of 0 + if (width === 0 || height === 0) { + return; + } + const rectHeight = (height / 100) * canvasHeight; const rectWidth = (width / 100) * canvasWidth; const x1 = (x / 100) * canvasWidth; diff --git a/src/highlight/__mocks__/data.ts b/src/highlight/__mocks__/data.ts index 96068240e..f9b13ddd8 100644 --- a/src/highlight/__mocks__/data.ts +++ b/src/highlight/__mocks__/data.ts @@ -8,6 +8,22 @@ export const rect: Rect = { y: 5, }; +export const noWidthRect: Rect = { + type: 'rect' as const, + height: 10, + width: 0, + x: 5, + y: 5, +}; + +export const noHeightRect: Rect = { + type: 'rect' as const, + height: 0, + width: 20, + x: 5, + y: 5, +}; + export const target: TargetHighlight = { location: { type: 'page' as const, diff --git a/src/highlight/__tests__/HighlightCanvas-test.tsx b/src/highlight/__tests__/HighlightCanvas-test.tsx index cf050834c..48d6092ac 100644 --- a/src/highlight/__tests__/HighlightCanvas-test.tsx +++ b/src/highlight/__tests__/HighlightCanvas-test.tsx @@ -1,7 +1,7 @@ import React from 'react'; import { shallow, ShallowWrapper } from 'enzyme'; import HighlightCanvas, { Props } from '../HighlightCanvas'; -import { rect as mockRect } from '../__mocks__/data'; +import { rect as mockRect, noWidthRect, noHeightRect } from '../__mocks__/data'; describe('HighlightCanvas', () => { const defaults: Props = { @@ -56,5 +56,11 @@ describe('HighlightCanvas', () => { const wrapper = getWrapper(); expect(wrapper.find('canvas').hasClass('ba-HighlightCanvas')).toBe(true); }); + + test('should render, excluding 0 width and height values', () => { + const wrapper = getWrapper(); + wrapper.setProps({ shapes: [mockRect, noWidthRect, noHeightRect] }); + expect(wrapper.find('canvas').hasClass('ba-HighlightCanvas')).toBe(true); + }); }); }); From df7a95c9086baf945ae6e0316d76f4d21d1c3802 Mon Sep 17 00:00:00 2001 From: Karen Lee Date: Fri, 29 Jul 2022 12:26:39 -0700 Subject: [PATCH 2/5] fix(highlight): Prevent empty rects from being sent to the backend --- src/highlight/__tests__/highlightUtil-test.ts | 39 +++++++++++++++++++ src/highlight/highlightUtil.ts | 6 +++ 2 files changed, 45 insertions(+) diff --git a/src/highlight/__tests__/highlightUtil-test.ts b/src/highlight/__tests__/highlightUtil-test.ts index 575065514..63e12e8cc 100644 --- a/src/highlight/__tests__/highlightUtil-test.ts +++ b/src/highlight/__tests__/highlightUtil-test.ts @@ -166,6 +166,45 @@ describe('highlightUtil', () => { expect(result[1]).toEqual(rects[3]); expect(result[2]).toEqual(rects[4]); }); + + test('should combine rects by row, excluding 0 width and height values', () => { + const rects = [ + { + height: 20, + width: 100, + x: 100, + y: 200, + }, + { + height: 0, + width: 100, + x: 200, + y: 200, + }, + { + height: 21, + width: 100, + x: 500, + y: 200, + }, + { + height: 23, + width: 0, + x: 400, + y: 300, + }, + ]; + + const result = combineRects(rects); + expect(result).toHaveLength(2); + expect(result[0]).toEqual({ + height: 20, + width: 100, + x: 100, + y: 200, + }); + expect(result[1]).toEqual(rects[2]); + }); }); describe('getShapeRelativeToContainer', () => { diff --git a/src/highlight/highlightUtil.ts b/src/highlight/highlightUtil.ts index 11f92aea2..6fbc13a41 100644 --- a/src/highlight/highlightUtil.ts +++ b/src/highlight/highlightUtil.ts @@ -103,6 +103,12 @@ export const combineRects = (rects: Shape[]): Shape[] => { dedupeRects(rects).forEach(rect => { const { height, width, x, y } = rect; + + // Ignore empty rects with a width or height of 0 + if (width === 0 || height === 0) { + return; + } + const lastRect = result.pop(); if (!lastRect) { From 4ffdb6929594178f377d616cc5c7963032831b40 Mon Sep 17 00:00:00 2001 From: Karen Lee Date: Fri, 29 Jul 2022 14:22:31 -0700 Subject: [PATCH 3/5] fix(highlight): Update highlight util unit test --- src/highlight/__tests__/highlightUtil-test.ts | 57 ++++++++++--------- 1 file changed, 29 insertions(+), 28 deletions(-) diff --git a/src/highlight/__tests__/highlightUtil-test.ts b/src/highlight/__tests__/highlightUtil-test.ts index 63e12e8cc..ce3bd1f46 100644 --- a/src/highlight/__tests__/highlightUtil-test.ts +++ b/src/highlight/__tests__/highlightUtil-test.ts @@ -168,34 +168,35 @@ describe('highlightUtil', () => { }); test('should combine rects by row, excluding 0 width and height values', () => { - const rects = [ - { - height: 20, - width: 100, - x: 100, - y: 200, - }, - { - height: 0, - width: 100, - x: 200, - y: 200, - }, - { - height: 21, - width: 100, - x: 500, - y: 200, - }, - { - height: 23, - width: 0, - x: 400, - y: 300, - }, - ]; + const validRect1 = { + height: 20, + width: 100, + x: 100, + y: 200, + }; - const result = combineRects(rects); + const validRect2 = { + height: 21, + width: 100, + x: 500, + y: 200, + }; + + const noHeightRect = { + height: 0, + width: 100, + x: 200, + y: 200, + }; + + const noWidthRect = { + height: 23, + width: 0, + x: 400, + y: 300, + }; + + const result = combineRects([validRect1, noHeightRect, validRect2, noWidthRect]); expect(result).toHaveLength(2); expect(result[0]).toEqual({ height: 20, @@ -203,7 +204,7 @@ describe('highlightUtil', () => { x: 100, y: 200, }); - expect(result[1]).toEqual(rects[2]); + expect(result[1]).toEqual(validRect2); }); }); From 13228699e82455b129da199b1be61690952bc939 Mon Sep 17 00:00:00 2001 From: Karen Lee Date: Mon, 1 Aug 2022 18:00:21 -0700 Subject: [PATCH 4/5] fix(highlight): Update highlight canvas unit test --- src/highlight/__mocks__/data.ts | 7 +++++++ .../__tests__/HighlightCanvas-test.tsx | 21 ++++++++++++++----- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/src/highlight/__mocks__/data.ts b/src/highlight/__mocks__/data.ts index f9b13ddd8..1c123bd57 100644 --- a/src/highlight/__mocks__/data.ts +++ b/src/highlight/__mocks__/data.ts @@ -71,3 +71,10 @@ export const selection = { }, ], }; + +export const canvasContext = { + fillRect: jest.fn(), + restore: jest.fn(), + save: jest.fn(), + strokeRect: jest.fn(), +}; diff --git a/src/highlight/__tests__/HighlightCanvas-test.tsx b/src/highlight/__tests__/HighlightCanvas-test.tsx index 48d6092ac..d43cb8d6c 100644 --- a/src/highlight/__tests__/HighlightCanvas-test.tsx +++ b/src/highlight/__tests__/HighlightCanvas-test.tsx @@ -1,7 +1,7 @@ import React from 'react'; import { shallow, ShallowWrapper } from 'enzyme'; import HighlightCanvas, { Props } from '../HighlightCanvas'; -import { rect as mockRect, noWidthRect, noHeightRect } from '../__mocks__/data'; +import { canvasContext, rect as mockRect, noWidthRect, noHeightRect } from '../__mocks__/data'; describe('HighlightCanvas', () => { const defaults: Props = { @@ -39,7 +39,6 @@ describe('HighlightCanvas', () => { describe('getContext()', () => { test.each` canvasRef | expectedResult - ${null} | ${null} ${{ getContext: () => 'ctx' }} | ${'ctx'} `('should return $expectedResult as context if canvasRef is $canvasRef', ({ canvasRef, expectedResult }) => { const wrapper = getWrapper(); @@ -58,9 +57,21 @@ describe('HighlightCanvas', () => { }); test('should render, excluding 0 width and height values', () => { - const wrapper = getWrapper(); - wrapper.setProps({ shapes: [mockRect, noWidthRect, noHeightRect] }); - expect(wrapper.find('canvas').hasClass('ba-HighlightCanvas')).toBe(true); + const wrapper = getWrapper({ shapes: [mockRect, noWidthRect, noHeightRect] }); + const instance = wrapper.instance() as HighlightCanvas; + + instance.canvasRef = { + current: { + // eslint-disable-next-line @typescript-eslint/ban-ts-ignore + // @ts-ignore + getContext: () => canvasContext, + }, + }; + + instance.renderRects(); + + expect(canvasContext.fillRect).toHaveBeenCalledTimes(1); + expect(canvasContext.strokeRect).toHaveBeenCalledTimes(1); }); }); }); From ec4ac659c2f65a1cc8f46c899d73b6159be1e20d Mon Sep 17 00:00:00 2001 From: Karen Lee Date: Tue, 2 Aug 2022 10:46:29 -0700 Subject: [PATCH 5/5] fix(highlight): Update highlight canvas unit test per feedback --- src/highlight/__tests__/HighlightCanvas-test.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/src/highlight/__tests__/HighlightCanvas-test.tsx b/src/highlight/__tests__/HighlightCanvas-test.tsx index d43cb8d6c..201cd8372 100644 --- a/src/highlight/__tests__/HighlightCanvas-test.tsx +++ b/src/highlight/__tests__/HighlightCanvas-test.tsx @@ -39,6 +39,7 @@ describe('HighlightCanvas', () => { describe('getContext()', () => { test.each` canvasRef | expectedResult + ${null} | ${null} ${{ getContext: () => 'ctx' }} | ${'ctx'} `('should return $expectedResult as context if canvasRef is $canvasRef', ({ canvasRef, expectedResult }) => { const wrapper = getWrapper();