feat(PdfReader): add OnPrintingAsync parameter#731
Merged
Conversation
Reviewer's GuideAdds a new printing callback to the PdfReader component and wires it up from the JS viewer layer, including a hidden iframe-based print implementation and associated event cleanup. Sequence diagram for PdfReader printing callback flowsequenceDiagram
actor User
participant Browser as Browser_UI
participant PdfJs as PdfReader_js
participant DotNet as PdfReader_dotnet
User->>Browser: Click print button
Browser->>PdfJs: click event on .bb-view-print
activate PdfJs
PdfJs->>PdfJs: printPdf(url)
PdfJs->>Browser: Create hidden iframe
Browser-->>PdfJs: iframe onload
PdfJs->>Browser: iframe.contentWindow.print()
PdfJs-->>DotNet: invokeMethodAsync Printing
activate DotNet
DotNet->>DotNet: Printing()
DotNet->>DotNet: OnPrintingAsync?
alt OnPrintingAsync is not null
DotNet-->>DotNet: await OnPrintingAsync()
end
deactivate DotNet
Browser-->>Browser: afterprint event
Browser->>Browser: Remove iframe
deactivate PdfJs
Updated class diagram for PdfReader printing callbackclassDiagram
class PdfReader {
+string? MoreButtonIcon
+Func~Task~? OnDownloadAsync
+Func~Task~? OnPrintingAsync
+Task PageChanged(uint pageIndex)
+Task Printing()
}
class OnDownloadAsyncCallback {
<<delegate>>
+Invoke() Task
}
class OnPrintingAsyncCallback {
<<delegate>>
+Invoke() Task
}
PdfReader --> OnDownloadAsyncCallback : uses
PdfReader --> OnPrintingAsyncCallback : uses
Flow diagram for PdfReader printPdf and disposal lifecycleflowchart TD
A[User clicks .bb-view-print button] --> B[Call printPdf with url]
B --> C{Existing .bb-view-print-iframe?}
C -->|Yes| D[Remove existing iframe]
C -->|No| E[Create new iframe element]
D --> E
E --> F[Set class bb-view-print-iframe and hidden fixed position]
F --> G[Set iframe.src to url]
G --> H[Append iframe to document.body]
H --> I[iframe onload handler]
I --> J[Add afterprint listener on iframe.contentWindow]
J --> K[Call iframe.contentWindow.focus and print]
K --> L[User completes browser print]
L --> M[afterprint event fires]
M --> N[Remove iframe from document.body]
subgraph Disposal_cleanup
O[dispose called with id] --> P[Locate PdfReader element]
P --> Q[Remove thumbnails click handler]
Q --> R[Remove .bb-view-controls click handler]
R --> S{Find .bb-view-print-iframe?}
S -->|Yes| T[Remove iframe]
S -->|No| U[No iframe to remove]
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- In
PdfReader.razor.js, the click handler passed toEventHandler.on(controls, "click", ".bb-view-print", e => { ... })usesawaitinside a non-async arrow function, which will cause a syntax error; make the handlerasync e => { ... }or remove theawait. - When using
iframe.contentWindowinsideprintPdf, consider guarding against it beingnull(e.g., in some sandboxed or unusual browser contexts) before callingaddEventListener,focus, andprintto avoid runtime errors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `PdfReader.razor.js`, the click handler passed to `EventHandler.on(controls, "click", ".bb-view-print", e => { ... })` uses `await` inside a non-async arrow function, which will cause a syntax error; make the handler `async e => { ... }` or remove the `await`.
- When using `iframe.contentWindow` inside `printPdf`, consider guarding against it being `null` (e.g., in some sandboxed or unusual browser contexts) before calling `addEventListener`, `focus`, and `print` to avoid runtime errors.
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:144-145` </location>
<code_context>
}
+
+ const controls = el.querySelector(".bb-view-controls");
+ EventHandler.on(controls, "click", ".bb-view-print", e => {
+ printPdf(options.url);
+ await invoke.invokeMethodAsync("Printing");
+ });
</code_context>
<issue_to_address>
**issue (bug_risk):** The event handler uses `await` but is not marked `async`, which will cause a syntax error.
In the `pagesloaded` handler, the `EventHandler.on` callback is defined as `e => { ... }` but contains `await invoke.invokeMethodAsync("Printing");`, which is invalid in non-`async` functions and will cause a syntax error. Make the callback `async e => { ... }` or remove `await` and handle the returned promise explicitly.
</issue_to_address>
### Comment 2
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:139-148` </location>
<code_context>
await invoke.invokeMethodAsync("PagesLoaded", e.pagesCount);
}
+
+ const controls = el.querySelector(".bb-view-controls");
+ EventHandler.on(controls, "click", ".bb-view-print", e => {
+ printPdf(options.url);
+ await invoke.invokeMethodAsync("Printing");
</code_context>
<issue_to_address>
**suggestion:** Null handling for `.bb-view-controls` should be added before wiring event listeners.
If `.bb-view-controls` is missing, `el.querySelector(".bb-view-controls")` returns `null`, and passing that into `EventHandler.on` will likely throw. Please guard this with a null check (e.g. `if (controls) { EventHandler.on(...); }`) so layouts without controls don’t cause runtime errors.
```suggestion
if (options.triggerPagesLoaded === true) {
await invoke.invokeMethodAsync("PagesLoaded", e.pagesCount);
}
const controls = el.querySelector(".bb-view-controls");
if (controls) {
EventHandler.on(controls, "click", ".bb-view-print", async e => {
printPdf(options.url);
await invoke.invokeMethodAsync("Printing");
});
}
})
```
</issue_to_address>
### Comment 3
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:291-292` </location>
<code_context>
+ iframe.remove();
+ }
+
+ iframe = document.createElement("iframe");
+ iframe.classList = "bb-view-print-iframe";
+ iframe.style.position = "fixed";
+ iframe.style.right = "100%";
</code_context>
<issue_to_address>
**issue (bug_risk):** Assigning a string directly to `classList` is incorrect; use `className` or `classList.add` instead.
Because `classList` is a `DOMTokenList`, assigning a string (`iframe.classList = "bb-view-print-iframe";`) won’t reliably apply the class and may be ignored by some browsers. Use `iframe.className = "bb-view-print-iframe";` or `iframe.classList.add("bb-view-print-iframe");` so the iframe actually receives the CSS class.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+144
to
+145
| EventHandler.on(controls, "click", ".bb-view-print", e => { | ||
| printPdf(options.url); |
There was a problem hiding this comment.
issue (bug_risk): The event handler uses await but is not marked async, which will cause a syntax error.
In the pagesloaded handler, the EventHandler.on callback is defined as e => { ... } but contains await invoke.invokeMethodAsync("Printing");, which is invalid in non-async functions and will cause a syntax error. Make the callback async e => { ... } or remove await and handle the returned promise explicitly.
Comment on lines
139
to
148
| if (options.triggerPagesLoaded === true) { | ||
| await invoke.invokeMethodAsync("PagesLoaded", e.pagesCount); | ||
| } | ||
|
|
||
| const controls = el.querySelector(".bb-view-controls"); | ||
| EventHandler.on(controls, "click", ".bb-view-print", e => { | ||
| printPdf(options.url); | ||
| await invoke.invokeMethodAsync("Printing"); | ||
| }); | ||
| }) |
There was a problem hiding this comment.
suggestion: Null handling for .bb-view-controls should be added before wiring event listeners.
If .bb-view-controls is missing, el.querySelector(".bb-view-controls") returns null, and passing that into EventHandler.on will likely throw. Please guard this with a null check (e.g. if (controls) { EventHandler.on(...); }) so layouts without controls don’t cause runtime errors.
Suggested change
| if (options.triggerPagesLoaded === true) { | |
| await invoke.invokeMethodAsync("PagesLoaded", e.pagesCount); | |
| } | |
| const controls = el.querySelector(".bb-view-controls"); | |
| EventHandler.on(controls, "click", ".bb-view-print", e => { | |
| printPdf(options.url); | |
| await invoke.invokeMethodAsync("Printing"); | |
| }); | |
| }) | |
| if (options.triggerPagesLoaded === true) { | |
| await invoke.invokeMethodAsync("PagesLoaded", e.pagesCount); | |
| } | |
| const controls = el.querySelector(".bb-view-controls"); | |
| if (controls) { | |
| EventHandler.on(controls, "click", ".bb-view-print", async e => { | |
| printPdf(options.url); | |
| await invoke.invokeMethodAsync("Printing"); | |
| }); | |
| } | |
| }) |
There was a problem hiding this comment.
Pull request overview
This PR adds a new OnPrintingAsync callback parameter to the PdfReader component, allowing consumers to receive notifications when the print function is triggered. The PR also includes a naming consistency fix, changing the JavaScript method invocation from "pagesInit" to "PagesInit" to match the C# PascalCase convention.
Key changes:
- Added
OnPrintingAsyncparameter and correspondingPrinting()JSInvokable method - Implemented
printPdf()function that creates a hidden iframe to handle PDF printing - Fixed method name from "pagesInit" to "PagesInit" for naming consistency
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| PdfReader.razor.js | Fixed "pagesInit" to "PagesInit" for naming consistency; added print event handler and printPdf() function; updated dispose to clean up print-related resources |
| PdfReader.razor.cs | Added OnPrintingAsync parameter and Printing() JSInvokable callback method; updated download callback comment |
| BootstrapBlazor.PdfReader.csproj | Version bump from 10.0.1-beta07 to 10.0.1-beta08 |
Comments suppressed due to low confidence (1)
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:146
- Error: Unexpected token
await invoke.invokeMethodAsync("Printing");
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| const controls = el.querySelector(".bb-view-controls"); | ||
| EventHandler.on(controls, "click", ".bb-view-print", e => { |
There was a problem hiding this comment.
The event handler callback function is missing the async keyword but uses await on line 146. This will cause a syntax error. The arrow function on line 144 should be declared as async (e) => { instead of e => {.
Suggested change
| EventHandler.on(controls, "click", ".bb-view-print", e => { | |
| EventHandler.on(controls, "click", ".bb-view-print", async e => { |
Comment on lines
+144
to
+147
| EventHandler.on(controls, "click", ".bb-view-print", e => { | ||
| printPdf(options.url); | ||
| await invoke.invokeMethodAsync("Printing"); | ||
| }); |
There was a problem hiding this comment.
The print event handler should follow the same pattern as other callbacks in this file by checking if the feature is enabled via an options flag. Add a TriggerPrinting option check similar to how triggerPagesInit, triggerPagesLoaded, and triggerPageChanged are used (see lines 129, 139, 169). This prevents unnecessary event handler registration and callback invocations when OnPrintingAsync is null.
Suggested change
| EventHandler.on(controls, "click", ".bb-view-print", e => { | |
| printPdf(options.url); | |
| await invoke.invokeMethodAsync("Printing"); | |
| }); | |
| if (options.triggerPrinting === true) { | |
| EventHandler.on(controls, "click", ".bb-view-print", async e => { | |
| printPdf(options.url); | |
| await invoke.invokeMethodAsync("Printing"); | |
| }); | |
| } |
Comment on lines
+142
to
148
|
|
||
| const controls = el.querySelector(".bb-view-controls"); | ||
| EventHandler.on(controls, "click", ".bb-view-print", e => { | ||
| printPdf(options.url); | ||
| await invoke.invokeMethodAsync("Printing"); | ||
| }); | ||
| }) |
There was a problem hiding this comment.
The print event handler is registered inside the 'pagesloaded' event callback. This means the handler will be registered multiple times if the PDF is reloaded, causing the print function and callback to be invoked multiple times on each print click. Consider registering this handler once in the main addEventListener function body, similar to how the minus/plus button handlers are registered on lines 197-198.
Suggested change
| const controls = el.querySelector(".bb-view-controls"); | |
| EventHandler.on(controls, "click", ".bb-view-print", e => { | |
| printPdf(options.url); | |
| await invoke.invokeMethodAsync("Printing"); | |
| }); | |
| }) | |
| }); | |
| // Register print button click handler once | |
| const controls = el.querySelector(".bb-view-controls"); | |
| if (controls) { | |
| EventHandler.on(controls, "click", ".bb-view-print", async e => { | |
| printPdf(options.url); | |
| await invoke.invokeMethodAsync("Printing"); | |
| }); | |
| } |
| } | ||
|
|
||
| iframe = document.createElement("iframe"); | ||
| iframe.classList = "bb-view-print-iframe"; |
There was a problem hiding this comment.
Setting classList directly to a string is incorrect. The classList property is a read-only DOMTokenList. Use iframe.classList.add("bb-view-print-iframe") instead, or use iframe.className = "bb-view-print-iframe" to set the class attribute.
Suggested change
| iframe.classList = "bb-view-print-iframe"; | |
| iframe.className = "bb-view-print-iframe"; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link issues
fixes #730
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add print support and a printing callback to PdfReader and align JS interop method names.
New Features:
Bug Fixes:
Enhancements: