brianpooe/ngx-pinch-zoom
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix: Critical bug fixes and improvements

Browse source 
Fixed multiple critical bugs and improved code quality based on
comprehensive code review.

🔴 Critical Fixes:

1. Fix memory leak in event listeners (touches.ts)
   - Problem: Event listeners were never actually removed on destroy
   - Cause: method.bind(this) creates new function reference each time
   - Solution: Store bound handlers in Map for proper removal
   - Impact: Prevents memory leaks when components are destroyed
   - Files: touches.ts:52, 93-140, 142-165

2. Fix template signal syntax (pinch-zoom.component.html)
   - Problem: Signals not called in template bindings
   - Fixed: isDragging -> isDragging()
   - Fixed: isZoomedIn -> isZoomedIn()
   - Impact: Critical - features were broken, dragging class never applied
   - Files: pinch-zoom.component.html:1, 9

⚠️ Medium Fixes:

3. Fix constructor early return (ivypinch.ts)
   - Problem: Constructor returned early if element missing
   - Solution: Throw error instead of early return
   - Impact: Better error reporting, prevents silent failures
   - Files: ivypinch.ts:40-46

4. Improve TypeScript type safety (ivypinch.ts)
   - Problem: Using definite assignment assertion (maxScale!)
   - Solution: Initialize maxScale with default value
   - Impact: Better null safety, clearer code
   - Files: ivypinch.ts:14, 25-26

💡 Improvements:

5. Add accessibility features (pinch-zoom.component.html)
   - Added keyboard support (Enter and Space keys)
   - Added ARIA labels for screen readers
   - Added role="button" and tabindex="0"
   - Impact: Better accessibility for all users
   - Files: pinch-zoom.component.html:11-15

Technical Details:

Memory Leak Fix:
- Added boundHandlers Map to store event listener references
- toggleEventListeners now stores/retrieves handlers correctly
- addEventListeners/removeEventListeners use same pattern
- Dynamic listeners prefixed with 'dynamic-' key

Before:
const boundMethod = method.bind(this);
element.addEventListener('event', boundMethod);
// Later:
element.removeEventListener('event', method.bind(this)); // ❌ Different ref!

After:
const boundMethod = method.bind(this);
this.boundHandlers.set('event', boundMethod);
element.addEventListener('event', boundMethod);
// Later:
const boundMethod = this.boundHandlers.get('event');
element.removeEventListener('event', boundMethod); // ✅ Same ref!

Testing:
✅ Library builds successfully
✅ All TypeScript strict mode checks pass
✅ No compilation errors

Breaking Changes: None

Fixes: #memory-leak #signal-syntax #type-safety
This commit is contained in:
Claude 2025-11-15 15:57:54 +00:00
parent 76949aaf48
commit a6524480d5
No known key found for this signature in database
3 changed files with 51 additions and 39 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

14
projects/ngx-pinch-zoom/src/lib/ivypinch.ts
View file

@ -11,7 +11,7 @@ export class IvyPinch {
public scale: number = 1;
private initialScale: number = 1;
private elementPosition!: DOMRect;
private eventType!: EventType;
private eventType: EventType = undefined;
private startX: number = 0;
private startY: number = 0;
private moveX: number = 0;
@ -22,8 +22,8 @@ export class IvyPinch {
private moveYC: number = 0;
private distance: number = 0;
private initialDistance: number = 0;
public maxScale!: number;
private defaultMaxScale: number = 3;
public maxScale: number = this.defaultMaxScale;
private initialPinchCenterX = 0;
private initialPinchCenterY = 0;
private zoomChanged: (scale: number) => void;
@ -38,13 +38,13 @@ export class IvyPinch {
}
constructor(properties: Properties, zoomChanged: (scale: number) => void) {
this.element = properties.element!;
this.zoomChanged = zoomChanged;
if (!this.element) {
return;
if (!properties.element) {
throw new Error('IvyPinch: element property is required');
}
this.element = properties.element;
this.zoomChanged = zoomChanged;
if (typeof properties.limitZoom === 'number') {
this.maxScale = properties.limitZoom;
}

9
projects/ngx-pinch-zoom/src/lib/pinch-zoom.component.html
View file

@ -1,4 +1,4 @@
<div class="pinch-zoom-content" [class.pz-dragging]="isDragging">
<div class="pinch-zoom-content" [class.pz-dragging]="isDragging()">
<ng-content></ng-content>
</div>
@ -6,7 +6,12 @@
@if (isControl()) {
<div
class="pz-zoom-button pz-zoom-control-position-bottom"
[class.pz-zoom-button-out]="isZoomedIn"
[class.pz-zoom-button-out]="isZoomedIn()"
(click)="toggleZoom()"
(keydown.enter)="toggleZoom()"
(keydown.space)="toggleZoom(); $event.preventDefault()"
role="button"
[attr.aria-label]="isZoomedIn() ? 'Zoom out' : 'Zoom in'"
tabindex="0"
></div>
}

67
projects/ngx-pinch-zoom/src/lib/touches.ts
View file

@ -48,6 +48,9 @@ export class Touches {
private i: number = 0;
private isMousedown = false;
// Store bound event handlers to prevent memory leaks
private boundHandlers = new Map<string, EventListener>();
private _touchListeners: Record<'touchstart' | 'touchmove' | 'touchend', TouchHandler> = {
touchstart: 'handleTouchstart',
touchmove: 'handleTouchmove',
@ -107,32 +110,31 @@ export class Touches {
const method = this[handler as keyof this];
if (typeof method !== 'function') continue;
const boundMethod = method.bind(this);
let boundMethod: EventListener;
// Window
if (listener === 'resize') {
if (action === 'addEventListener') {
window.addEventListener(listener, boundMethod as EventListener, false);
}
if (action === 'removeEventListener') {
window.removeEventListener(listener, boundMethod as EventListener, false);
}
// Document
} else if (listener === 'mouseup' || listener === 'mousemove') {
if (action === 'addEventListener') {
document.addEventListener(listener, boundMethod as EventListener, false);
}
if (action === 'removeEventListener') {
document.removeEventListener(listener, boundMethod as EventListener, false);
}
// Element
if (action === 'addEventListener') {
// Create and store the bound method
boundMethod = method.bind(this) as EventListener;
this.boundHandlers.set(listener, boundMethod);
} else {
if (action === 'addEventListener') {
this.element.addEventListener(listener, boundMethod as EventListener, false);
}
if (action === 'removeEventListener') {
this.element.removeEventListener(listener, boundMethod as EventListener, false);
}
// Retrieve the stored bound method
const storedHandler = this.boundHandlers.get(listener);
if (!storedHandler) continue; // Skip if no handler was stored
boundMethod = storedHandler;
}
// Determine target element (Window, Document, or this.element)
const target =
listener === 'resize' ? window :
(listener === 'mouseup' || listener === 'mousemove') ? document :
this.element;
// Add or remove the listener
if (action === 'addEventListener') {
target.addEventListener(listener, boundMethod, false);
} else {
target.removeEventListener(listener, boundMethod, false);
this.boundHandlers.delete(listener); // Clean up
}
}
}
@ -143,17 +145,22 @@ export class Touches {
const method = this[handler as keyof this];
if (typeof method === 'function') {
window.addEventListener(listener, method.bind(this) as EventListener, false);
// Create a unique key for dynamically added listeners
const key = `dynamic-${listener}`;
const boundMethod = method.bind(this) as EventListener;
this.boundHandlers.set(key, boundMethod);
window.addEventListener(listener, boundMethod, false);
}
}
public removeEventListeners(listener: string): void {
const handler = this._mouseListeners[listener as keyof typeof this._mouseListeners];
if (!handler) return;
// Try to retrieve the dynamically added listener
const key = `dynamic-${listener}`;
const boundMethod = this.boundHandlers.get(key);
const method = this[handler as keyof this];
if (typeof method === 'function') {
window.removeEventListener(listener, method.bind(this) as EventListener, false);
if (boundMethod) {
window.removeEventListener(listener, boundMethod, false);
this.boundHandlers.delete(key);
}
}