From 3b75d3271e77b6eee4be6ef028e58e01cf8d52f9 Mon Sep 17 00:00:00 2001 From: shamoon <4887959+shamoon@users.noreply.github.com> Date: Thu, 30 Oct 2025 14:54:28 -0700 Subject: [PATCH 1/6] Fix: delay iframe DOM removal for print in FF --- .../document-detail.component.spec.ts | 4 ++++ .../document-detail/document-detail.component.ts | 16 +++++++++++----- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src-ui/src/app/components/document-detail/document-detail.component.spec.ts b/src-ui/src/app/components/document-detail/document-detail.component.spec.ts index 752df3b21..db537e1b2 100644 --- a/src-ui/src/app/components/document-detail/document-detail.component.spec.ts +++ b/src-ui/src/app/components/document-detail/document-detail.component.spec.ts @@ -1489,6 +1489,8 @@ describe('DocumentDetailComponent', () => { mockContentWindow.onafterprint(new Event('afterprint')) } + tick(500) + expect(removeChildSpy).toHaveBeenCalledWith(mockIframe) expect(revokeObjectURLSpy).toHaveBeenCalledWith('blob:mock-url') @@ -1563,6 +1565,8 @@ describe('DocumentDetailComponent', () => { mockIframe.onload(new Event('load')) } + tick(500) + expect(toastSpy).toHaveBeenCalled() expect(removeChildSpy).toHaveBeenCalledWith(mockIframe) expect(revokeObjectURLSpy).toHaveBeenCalledWith('blob:mock-url') diff --git a/src-ui/src/app/components/document-detail/document-detail.component.ts b/src-ui/src/app/components/document-detail/document-detail.component.ts index 48ddf61e5..4e0d3259f 100644 --- a/src-ui/src/app/components/document-detail/document-detail.component.ts +++ b/src-ui/src/app/components/document-detail/document-detail.component.ts @@ -21,7 +21,7 @@ import { dirtyCheck, DirtyComponent } from '@ngneat/dirty-check-forms' import { PDFDocumentProxy, PdfViewerModule } from 'ng2-pdf-viewer' import { NgxBootstrapIconsModule } from 'ngx-bootstrap-icons' import { DeviceDetectorService } from 'ngx-device-detector' -import { BehaviorSubject, Observable, of, Subject } from 'rxjs' +import { BehaviorSubject, Observable, of, Subject, timer } from 'rxjs' import { catchError, debounceTime, @@ -1448,13 +1448,19 @@ export class DocumentDetailComponent iframe.contentWindow.focus() iframe.contentWindow.print() iframe.contentWindow.onafterprint = () => { - document.body.removeChild(iframe) - URL.revokeObjectURL(blobUrl) + timer(500).subscribe(() => { + // delay to avoid print failure + document.body.removeChild(iframe) + URL.revokeObjectURL(blobUrl) + }) } } catch (err) { this.toastService.showError($localize`Print failed.`, err) - document.body.removeChild(iframe) - URL.revokeObjectURL(blobUrl) + timer(500).subscribe(() => { + // delay to avoid print failure + document.body.removeChild(iframe) + URL.revokeObjectURL(blobUrl) + }) } } }, From d6e2456bafe95629b97834ac2cc5330d21b7b7de Mon Sep 17 00:00:00 2001 From: shamoon <4887959+shamoon@users.noreply.github.com> Date: Thu, 30 Oct 2025 15:14:44 -0700 Subject: [PATCH 2/6] Update document-detail.component.ts --- .../document-detail.component.ts | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src-ui/src/app/components/document-detail/document-detail.component.ts b/src-ui/src/app/components/document-detail/document-detail.component.ts index 4e0d3259f..e7197b9b9 100644 --- a/src-ui/src/app/components/document-detail/document-detail.component.ts +++ b/src-ui/src/app/components/document-detail/document-detail.component.ts @@ -1448,16 +1448,23 @@ export class DocumentDetailComponent iframe.contentWindow.focus() iframe.contentWindow.print() iframe.contentWindow.onafterprint = () => { - timer(500).subscribe(() => { - // delay to avoid print failure + timer(100).subscribe(() => { + // delay to avoid FF print failure document.body.removeChild(iframe) URL.revokeObjectURL(blobUrl) }) } } catch (err) { - this.toastService.showError($localize`Print failed.`, err) - timer(500).subscribe(() => { - // delay to avoid print failure + const isCrossOriginAfterPrintError = + err instanceof DOMException && + (err.name === 'SecurityError' || + err.message.includes('onafterprint')) && + err.message.includes('cross-origin') + if (!isCrossOriginAfterPrintError) { + this.toastService.showError($localize`Print failed.`, err) + } + timer(100).subscribe(() => { + // delay to avoid FF print failure document.body.removeChild(iframe) URL.revokeObjectURL(blobUrl) }) From a8c75d95d8314dd8a5bb2ec2ea37a8d738a2a91c Mon Sep 17 00:00:00 2001 From: shamoon <4887959+shamoon@users.noreply.github.com> Date: Thu, 30 Oct 2025 15:29:56 -0700 Subject: [PATCH 3/6] Update document-detail.component.ts --- .../document-detail/document-detail.component.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src-ui/src/app/components/document-detail/document-detail.component.ts b/src-ui/src/app/components/document-detail/document-detail.component.ts index e7197b9b9..3c692e496 100644 --- a/src-ui/src/app/components/document-detail/document-detail.component.ts +++ b/src-ui/src/app/components/document-detail/document-detail.component.ts @@ -1448,13 +1448,11 @@ export class DocumentDetailComponent iframe.contentWindow.focus() iframe.contentWindow.print() iframe.contentWindow.onafterprint = () => { - timer(100).subscribe(() => { - // delay to avoid FF print failure - document.body.removeChild(iframe) - URL.revokeObjectURL(blobUrl) - }) + document.body.removeChild(iframe) + URL.revokeObjectURL(blobUrl) } } catch (err) { + // FF throws cross-origin error on onafterprint const isCrossOriginAfterPrintError = err instanceof DOMException && (err.name === 'SecurityError' || From 245e52a4eb3732fffc2a040412d967a24969227d Mon Sep 17 00:00:00 2001 From: shamoon <4887959+shamoon@users.noreply.github.com> Date: Thu, 30 Oct 2025 16:59:49 -0700 Subject: [PATCH 4/6] Coverage --- .../document-detail.component.spec.ts | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/src-ui/src/app/components/document-detail/document-detail.component.spec.ts b/src-ui/src/app/components/document-detail/document-detail.component.spec.ts index db537e1b2..e1bc0bf2b 100644 --- a/src-ui/src/app/components/document-detail/document-detail.component.spec.ts +++ b/src-ui/src/app/components/document-detail/document-detail.component.spec.ts @@ -1577,4 +1577,71 @@ describe('DocumentDetailComponent', () => { createObjectURLSpy.mockRestore() revokeObjectURLSpy.mockRestore() })) + + it('should suppress toast if cross-origin afterprint error occurs', fakeAsync(() => { + initNormally() + + const appendChildSpy = jest + .spyOn(document.body, 'appendChild') + .mockImplementation((node: Node) => node) + const removeChildSpy = jest + .spyOn(document.body, 'removeChild') + .mockImplementation((node: Node) => node) + const createObjectURLSpy = jest + .spyOn(URL, 'createObjectURL') + .mockReturnValue('blob:mock-url') + const revokeObjectURLSpy = jest + .spyOn(URL, 'revokeObjectURL') + .mockImplementation(() => {}) + + const toastSpy = jest.spyOn(toastService, 'showError') + + const mockContentWindow = { + focus: jest.fn().mockImplementation(() => { + throw new DOMException( + 'Accessing onafterprint triggered a cross-origin violation', + 'SecurityError' + ) + }), + print: jest.fn(), + onafterprint: null, + } + + const mockIframe: any = { + style: {}, + src: '', + onload: null, + contentWindow: mockContentWindow, + } + + const createElementSpy = jest + .spyOn(document, 'createElement') + .mockReturnValue(mockIframe as any) + + const blob = new Blob(['test'], { type: 'application/pdf' }) + component.printDocument() + + const req = httpTestingController.expectOne( + `${environment.apiBaseUrl}documents/${doc.id}/download/` + ) + req.flush(blob) + + tick() + + if (mockIframe.onload) { + mockIframe.onload(new Event('load')) + } + + tick(200) + + expect(toastSpy).not.toHaveBeenCalled() + expect(removeChildSpy).toHaveBeenCalledWith(mockIframe) + expect(revokeObjectURLSpy).toHaveBeenCalledWith('blob:mock-url') + + createElementSpy.mockRestore() + appendChildSpy.mockRestore() + removeChildSpy.mockRestore() + createObjectURLSpy.mockRestore() + revokeObjectURLSpy.mockRestore() + })) }) From 8f969ecab54f70df408fd07e12a661d488c8622e Mon Sep 17 00:00:00 2001 From: shamoon <4887959+shamoon@users.noreply.github.com> Date: Thu, 30 Oct 2025 17:24:44 -0700 Subject: [PATCH 5/6] Fix: delay iframe DOM removal for print in FF --- .../components/document-detail/document-detail.component.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src-ui/src/app/components/document-detail/document-detail.component.ts b/src-ui/src/app/components/document-detail/document-detail.component.ts index 3c692e496..9c0c84592 100644 --- a/src-ui/src/app/components/document-detail/document-detail.component.ts +++ b/src-ui/src/app/components/document-detail/document-detail.component.ts @@ -1455,9 +1455,7 @@ export class DocumentDetailComponent // FF throws cross-origin error on onafterprint const isCrossOriginAfterPrintError = err instanceof DOMException && - (err.name === 'SecurityError' || - err.message.includes('onafterprint')) && - err.message.includes('cross-origin') + err.message.includes('onafterprint') if (!isCrossOriginAfterPrintError) { this.toastService.showError($localize`Print failed.`, err) } From 9f0a4ac19d0c45ccb4f431679ac81239c349e3eb Mon Sep 17 00:00:00 2001 From: shamoon <4887959+shamoon@users.noreply.github.com> Date: Thu, 30 Oct 2025 18:00:19 -0700 Subject: [PATCH 6/6] Sure sonar, consolidate --- .../document-detail.component.spec.ts | 195 +++++++----------- 1 file changed, 79 insertions(+), 116 deletions(-) diff --git a/src-ui/src/app/components/document-detail/document-detail.component.spec.ts b/src-ui/src/app/components/document-detail/document-detail.component.spec.ts index e1bc0bf2b..dada60074 100644 --- a/src-ui/src/app/components/document-detail/document-detail.component.spec.ts +++ b/src-ui/src/app/components/document-detail/document-detail.component.spec.ts @@ -1514,134 +1514,97 @@ describe('DocumentDetailComponent', () => { ) }) - it('should show error toast if printing throws inside iframe', fakeAsync(() => { - initNormally() + const iframePrintErrorCases: Array<{ + description: string + thrownError: Error + expectToast: boolean + }> = [ + { + description: 'should show error toast if printing throws inside iframe', + thrownError: new Error('focus failed'), + expectToast: true, + }, + { + description: + 'should suppress toast if cross-origin afterprint error occurs', + thrownError: new DOMException( + 'Accessing onafterprint triggered a cross-origin violation', + 'SecurityError' + ), + expectToast: false, + }, + ] - const appendChildSpy = jest - .spyOn(document.body, 'appendChild') - .mockImplementation((node: Node) => node) - const removeChildSpy = jest - .spyOn(document.body, 'removeChild') - .mockImplementation((node: Node) => node) - const createObjectURLSpy = jest - .spyOn(URL, 'createObjectURL') - .mockReturnValue('blob:mock-url') - const revokeObjectURLSpy = jest - .spyOn(URL, 'revokeObjectURL') - .mockImplementation(() => {}) + iframePrintErrorCases.forEach(({ description, thrownError, expectToast }) => { + it( + description, + fakeAsync(() => { + initNormally() - const toastSpy = jest.spyOn(toastService, 'showError') + const appendChildSpy = jest + .spyOn(document.body, 'appendChild') + .mockImplementation((node: Node) => node) + const removeChildSpy = jest + .spyOn(document.body, 'removeChild') + .mockImplementation((node: Node) => node) + const createObjectURLSpy = jest + .spyOn(URL, 'createObjectURL') + .mockReturnValue('blob:mock-url') + const revokeObjectURLSpy = jest + .spyOn(URL, 'revokeObjectURL') + .mockImplementation(() => {}) - const mockContentWindow = { - focus: jest.fn().mockImplementation(() => { - throw new Error('focus failed') - }), - print: jest.fn(), - onafterprint: null, - } + const toastSpy = jest.spyOn(toastService, 'showError') - const mockIframe: any = { - style: {}, - src: '', - onload: null, - contentWindow: mockContentWindow, - } + const mockContentWindow = { + focus: jest.fn().mockImplementation(() => { + throw thrownError + }), + print: jest.fn(), + onafterprint: null, + } - const createElementSpy = jest - .spyOn(document, 'createElement') - .mockReturnValue(mockIframe as any) + const mockIframe: any = { + style: {}, + src: '', + onload: null, + contentWindow: mockContentWindow, + } - const blob = new Blob(['test'], { type: 'application/pdf' }) - component.printDocument() + const createElementSpy = jest + .spyOn(document, 'createElement') + .mockReturnValue(mockIframe as any) - const req = httpTestingController.expectOne( - `${environment.apiBaseUrl}documents/${doc.id}/download/` - ) - req.flush(blob) + const blob = new Blob(['test'], { type: 'application/pdf' }) + component.printDocument() - tick() - - if (mockIframe.onload) { - mockIframe.onload(new Event('load')) - } - - tick(500) - - expect(toastSpy).toHaveBeenCalled() - expect(removeChildSpy).toHaveBeenCalledWith(mockIframe) - expect(revokeObjectURLSpy).toHaveBeenCalledWith('blob:mock-url') - - createElementSpy.mockRestore() - appendChildSpy.mockRestore() - removeChildSpy.mockRestore() - createObjectURLSpy.mockRestore() - revokeObjectURLSpy.mockRestore() - })) - - it('should suppress toast if cross-origin afterprint error occurs', fakeAsync(() => { - initNormally() - - const appendChildSpy = jest - .spyOn(document.body, 'appendChild') - .mockImplementation((node: Node) => node) - const removeChildSpy = jest - .spyOn(document.body, 'removeChild') - .mockImplementation((node: Node) => node) - const createObjectURLSpy = jest - .spyOn(URL, 'createObjectURL') - .mockReturnValue('blob:mock-url') - const revokeObjectURLSpy = jest - .spyOn(URL, 'revokeObjectURL') - .mockImplementation(() => {}) - - const toastSpy = jest.spyOn(toastService, 'showError') - - const mockContentWindow = { - focus: jest.fn().mockImplementation(() => { - throw new DOMException( - 'Accessing onafterprint triggered a cross-origin violation', - 'SecurityError' + const req = httpTestingController.expectOne( + `${environment.apiBaseUrl}documents/${doc.id}/download/` ) - }), - print: jest.fn(), - onafterprint: null, - } + req.flush(blob) - const mockIframe: any = { - style: {}, - src: '', - onload: null, - contentWindow: mockContentWindow, - } + tick() - const createElementSpy = jest - .spyOn(document, 'createElement') - .mockReturnValue(mockIframe as any) + if (mockIframe.onload) { + mockIframe.onload(new Event('load')) + } - const blob = new Blob(['test'], { type: 'application/pdf' }) - component.printDocument() + tick(200) - const req = httpTestingController.expectOne( - `${environment.apiBaseUrl}documents/${doc.id}/download/` + if (expectToast) { + expect(toastSpy).toHaveBeenCalled() + } else { + expect(toastSpy).not.toHaveBeenCalled() + } + expect(removeChildSpy).toHaveBeenCalledWith(mockIframe) + expect(revokeObjectURLSpy).toHaveBeenCalledWith('blob:mock-url') + + createElementSpy.mockRestore() + appendChildSpy.mockRestore() + removeChildSpy.mockRestore() + createObjectURLSpy.mockRestore() + revokeObjectURLSpy.mockRestore() + }) ) - req.flush(blob) - - tick() - - if (mockIframe.onload) { - mockIframe.onload(new Event('load')) - } - - tick(200) - - expect(toastSpy).not.toHaveBeenCalled() - expect(removeChildSpy).toHaveBeenCalledWith(mockIframe) - expect(revokeObjectURLSpy).toHaveBeenCalledWith('blob:mock-url') - - createElementSpy.mockRestore() - appendChildSpy.mockRestore() - removeChildSpy.mockRestore() - createObjectURLSpy.mockRestore() - revokeObjectURLSpy.mockRestore() - })) + }) })