Add fullscreen toggle #110
Conversation
|
I just noticed that my version is not based on the latest upstream one and is missing one conflicting commit. Let me fix that |
csala
force-pushed
the
fullscreen-toggle
branch
from
6bac75c to
469edb8
Compare
|
@lfoppiano I restored the I believe that now everything is working as expected here, but if this PR is not going to be merged maybe the |
|
Perfect, no pressure. I just had done a dirtier version of this for my own use case and figured I could just clean it up and contribute it. |
|
I fixed the issue with the zoom-button class missing, I'm fine to keep the class name you use in this branch. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a fullscreen toggle feature to the PDF viewer component, allowing users to view PDFs in browser-level fullscreen mode instead of modal-level maximization.
- Introduces a new
show_fullscreen_toggleparameter to control visibility of the fullscreen button - Adds fullscreen toggle functionality with appropriate UI controls and event handling
- Updates component styling to accommodate the new control button layout
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| streamlit_pdf_viewer/frontend/src/PdfViewer.vue | Implements fullscreen toggle UI, event handlers, and updates control button layout |
| streamlit_pdf_viewer/init.py | Adds show_fullscreen_toggle parameter to the pdf_viewer function with documentation |
| README.md | Documents the new show_fullscreen_toggle parameter in the parameters table |
Comments suppressed due to low confidence (1)
streamlit_pdf_viewer/frontend/src/PdfViewer.vue:1
- [nitpick] The inline SVG icons are quite verbose and duplicated. Consider extracting them into separate components or using a more maintainable icon system to improve code readability.
<template>
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const enterFullscreen = () => { | ||
| const el = pdfContainer.value; | ||
| if (el && el.requestFullscreen) { | ||
| el.requestFullscreen(); |
There was a problem hiding this comment.
The fullscreen API requires error handling as requestFullscreen() can fail due to user gesture requirements or browser policies. Consider adding a try-catch block and graceful fallback handling.
| const enterFullscreen = () => { | |
| const el = pdfContainer.value; | |
| if (el && el.requestFullscreen) { | |
| el.requestFullscreen(); | |
| const enterFullscreen = async () => { | |
| const el = pdfContainer.value; | |
| if (el && el.requestFullscreen) { | |
| try { | |
| await el.requestFullscreen(); | |
| } catch (err) { | |
| console.error("Failed to enter fullscreen mode:", err); | |
| // Optionally, show a user-friendly message here | |
| } |
| const exitFullscreen = () => { | ||
| if (document.exitFullscreen) { | ||
| document.exitFullscreen(); |
There was a problem hiding this comment.
Similar to enterFullscreen, document.exitFullscreen() can fail and should include error handling with a try-catch block for robust implementation.
| const exitFullscreen = () => { | |
| if (document.exitFullscreen) { | |
| document.exitFullscreen(); | |
| const exitFullscreen = async () => { | |
| if (document.exitFullscreen) { | |
| try { | |
| await document.exitFullscreen(); | |
| } catch (error) { | |
| console.error("Failed to exit fullscreen:", error); | |
| } |
|
@csala I think I "fixed" the tests, or at least I removed the one that weren't running consistently. Could you update from main and check whether all the tests are running? |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
streamlit_pdf_viewer/frontend/src/PdfViewer.vue (1)
416-436: Harden Fullscreen API calls (errors, promises, prefixes).Handle rejections, await promises, and support WebKit-prefixed APIs for broader compatibility.
- const enterFullscreen = () => { - const el = pdfContainer.value; - if (el && el.requestFullscreen) { - el.requestFullscreen(); - } - }; - - const exitFullscreen = () => { - if (document.exitFullscreen) { - document.exitFullscreen(); - } - }; + const enterFullscreen = async () => { + const el = pdfContainer.value; + if (!el) return; + try { + const req = el.requestFullscreen || el.webkitRequestFullscreen; + if (req) { + // navigationUI hint is ignored where unsupported + await req.call(el, { navigationUI: "hide" }); + } else { + console.warn("Fullscreen API not supported on this element."); + } + } catch (err) { + console.error("Failed to enter fullscreen:", err); + } + }; + + const exitFullscreen = async () => { + try { + const exit = document.exitFullscreen || document.webkitExitFullscreen; + if (exit) { + await exit.call(document); + } + } catch (err) { + console.error("Failed to exit fullscreen:", err); + } + }; const toggleFullscreen = () => { if (!isFullscreen.value) { enterFullscreen(); } else { exitFullscreen(); } };
🧹 Nitpick comments (6)
README.md (1)
105-105: Document iframe/browser constraints for Fullscreen.Fullscreen may be blocked inside iframes unless the parent grants allow="fullscreen". Add a short caveat so users know why the button might do nothing in some deployments (e.g., Streamlit-hosted iframes).
Proposed wording to add under “Caveats”:
- Browser fullscreen requires permission in embedded contexts (iframes). If the host does not allow fullscreen, the toggle will have no effect. Check your container/host settings (e.g., iframe allow="fullscreen").
streamlit_pdf_viewer/frontend/src/PdfViewer.vue (5)
6-15: Improve button a11y and semantics.Add type="button" to avoid implicit submit and expose accessible names/state for the fullscreen toggle.
Apply:
- <button v-if="showFullscreen" class="control-button" @click.stop="toggleFullscreen" title="Toggle Fullscreen"> + <button + v-if="showFullscreen" + type="button" + class="control-button" + @click.stop="toggleFullscreen" + :aria-pressed="isFullscreen" + aria-label="Toggle fullscreen" + title="Toggle Fullscreen" + > - <button class="control-button" @click.stop="toggleZoomPanel"> + <button + type="button" + class="control-button" + @click.stop="toggleZoomPanel" + aria-label="Zoom options" + title="Zoom options" + >
19-23: Constrain manual zoom input at the element level.Reflect the 10–1000% clamp in the input attributes for immediate validation.
- <input + <input type="number" class="zoom-input" v-model="manualZoomInput" @keyup.enter="applyManualZoom" @blur="applyManualZoom" + min="10" + max="1000" + inputmode="numeric" />
85-87: Make showFullscreen reactive to prop updates.Use a computed instead of capturing the initial value only.
- const isFullscreen = ref(false); - const showFullscreen = props.args.show_fullscreen_toggle === true; + const isFullscreen = ref(false); + const showFullscreen = computed(() => props.args.show_fullscreen_toggle === true);
491-494: Track the correct fullscreen element and listen to prefixed events.Scope isFullscreen to this component’s container and react on Safari’s webkitfullscreenchange as well.
- const onFullscreenChange = () => { - isFullscreen.value = !!document.fullscreenElement; - }; + const getFullscreenElement = () => + document.fullscreenElement || document.webkitFullscreenElement; + const onFullscreenChange = () => { + isFullscreen.value = getFullscreenElement() === pdfContainer.value; + // Ensure layout recalculates promptly when entering/exiting FS + debouncedHandleResize(); + }; @@ - document.addEventListener('fullscreenchange', onFullscreenChange); + document.addEventListener('fullscreenchange', onFullscreenChange); + document.addEventListener('webkitfullscreenchange', onFullscreenChange); @@ - document.removeEventListener('fullscreenchange', onFullscreenChange); + document.removeEventListener('fullscreenchange', onFullscreenChange); + document.removeEventListener('webkitfullscreenchange', onFullscreenChange);Also applies to: 501-502, 507-508
543-551: Minor a11y polish for controls.Increase hit area to ~40×40px and ensure contrast meets WCAG AA.
.control-button { - padding: 8px 16px; - font-size: 14px; + min-width: 40px; + min-height: 40px; + padding: 8px 12px; + font-size: 14px; } .control-button:hover { - background: rgba(50, 50, 50, 0.9); + background: rgba(60, 60, 60, 0.95); }Also applies to: 553-557, 559-571, 572-575
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)streamlit_pdf_viewer/__init__.py(3 hunks)streamlit_pdf_viewer/frontend/src/PdfViewer.vue(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build (ubuntu-latest, 3.13, 20)
- GitHub Check: build (ubuntu-latest, 3.10, 18)
- GitHub Check: build (ubuntu-latest, 3.12, 18)
- GitHub Check: build (ubuntu-latest, 3.10, 20)
- GitHub Check: build (ubuntu-latest, 3.11, 18)
- GitHub Check: build (ubuntu-latest, 3.13, 18)
- GitHub Check: build (ubuntu-latest, 3.12, 20)
- GitHub Check: build (ubuntu-latest, 3.11, 20)
🔇 Additional comments (5)
README.md (1)
99-101: Reformatting looks good.The updated descriptions for zoom_level and viewer_align are accurate and consistent with the code.
streamlit_pdf_viewer/__init__.py (3)
42-43: Public API: new flag placement and default look good.Adding show_fullscreen_toggle with a True default is backwards compatible and well placed after the callback.
63-64: Docstring clarity LGTM.Description and default documented correctly.
137-139: Prop forwarding to frontend verified.Argument is passed through to the component. No further changes needed here.
streamlit_pdf_viewer/frontend/src/PdfViewer.vue (1)
486-489: Outside-click containment LGTM.Switching to .control-buttons correctly keeps the zoom panel open when interacting inside the controls.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
tests/test_zoom_numeric.py (1)
44-45: Replace fixed timeout with element-based wait.Same concern as in
test_zoom_auto_height.py: the hard-coded 2-second timeout is duplicated across multiple test files and should be replaced with a more reliable wait strategy.Consider creating a shared test helper function:
# In tests/conftest.py or tests/e2e_utils.py def wait_for_pdf_content(page: Page, iframe_index: int = 0): """Wait for PDF content to be loaded in the iframe.""" iframe_frame = page.frame_locator('iframe[title="streamlit_pdf_viewer.streamlit_pdf_viewer"]').nth(iframe_index) iframe_frame.locator('div[id="pdfViewer"] canvas').first.wait_for(timeout=5000, state='visible')Then use it in tests:
wait_for_pdf_content(page)tests/test_zoom_auto.py (2)
53-54: Same fixed timeout pattern across all test files.This is the third test file with the identical hard-coded 2-second wait. The concern and suggested refactor mentioned in
test_zoom_auto_height.pyapplies here as well.
61-61: Same button selector brittleness.The selector concern raised in
test_zoom_auto_height.pyapplies here. Consider the shared helper function approach suggested intest_zoom_numeric.pyto reduce duplication.
🧹 Nitpick comments (4)
tests/test_zoom_auto_height.py (1)
54-55: Consider replacing fixed timeout with element-based wait.The hard-coded 2-second timeout is fragile and may cause flakiness (too short) or slowness (too long). Consider waiting for a specific element that indicates content is loaded instead.
For example:
- # Wait for iframe to be properly loaded and have content - page.wait_for_timeout(2000) + # Wait for PDF content to load + iframe_frame = page.frame_locator('iframe[title="streamlit_pdf_viewer.streamlit_pdf_viewer"]').nth(0) + iframe_frame.locator('div[id="pdfViewer"] canvas').first.wait_for(timeout=5000, state='visible')tests/test_zoom_numeric.py (1)
56-56: Same button order assumption as other tests.The selector
.top-buttons button.control-button.nth(1) is duplicated across multiple test files with the same brittleness concern noted intest_zoom_auto_height.py.Consider extracting this into a helper function in
conftest.pyore2e_utils.py:def get_zoom_button(iframe_frame): """Get the zoom control button, accounting for optional fullscreen toggle.""" return iframe_frame.locator('.top-buttons button.control-button').nth(1)This centralizes the selector logic and makes it easier to update if the UI changes.
Also applies to: 78-78
streamlit_pdf_viewer/frontend/src/PdfViewer.vue (2)
476-497: Scroll position restoration could fail on undefined dataset values.Lines 484 and 488 access
scrollingContainer.dataset.savedScrollTopandsavedScrollLeft, but these may beundefinedbefore being set, potentially causing issues.The current code uses
|| 0and!== undefinedchecks, which should handle this safely. However, for clarity and consistency, consider using a more explicit pattern:const toggleMaximized = () => { isMaximized.value = !isMaximized.value; const scrollingContainer = document.querySelector('.scrolling-container'); if (scrollingContainer) { if (isMaximized.value) { - scrollingContainer.dataset.savedScrollTop = scrollingContainer.scrollTop; - scrollingContainer.dataset.savedScrollLeft = scrollingContainer.scrollLeft; + scrollingContainer.dataset.savedScrollTop = String(scrollingContainer.scrollTop); + scrollingContainer.dataset.savedScrollLeft = String(scrollingContainer.scrollLeft); } else { - if (scrollingContainer.dataset.savedScrollTop !== undefined) { - scrollingContainer.scrollTop = parseFloat(scrollingContainer.dataset.savedScrollTop); - scrollingContainer.scrollLeft = parseFloat(scrollingContainer.dataset.savedScrollLeft); - } + scrollingContainer.scrollTop = parseFloat(scrollingContainer.dataset.savedScrollTop || '0'); + scrollingContainer.scrollLeft = parseFloat(scrollingContainer.dataset.savedScrollLeft || '0'); } } handleMaximizedResize(); };
553-557: Add event.stopPropagation() and preventDefault() for defensive event handling.While codebase analysis reveals no conflicting Escape key handlers from other Streamlit components (Streamlit itself doesn't provide built-in Escape handling), adding these calls follows best practice for global keyboard listeners to prevent unintended event bubbling:
const handleKeyDown = (event) => { if (event.key === 'Escape' && isMaximized.value) { + event.stopPropagation(); + event.preventDefault(); toggleMaximized(); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(2 hunks)streamlit_pdf_viewer/__init__.py(3 hunks)streamlit_pdf_viewer/frontend/src/PdfViewer.vue(9 hunks)tests/test_zoom_auto.py(1 hunks)tests/test_zoom_auto_height.py(1 hunks)tests/test_zoom_numeric.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- streamlit_pdf_viewer/init.py
🧰 Additional context used
🧬 Code graph analysis (3)
tests/test_zoom_auto_height.py (1)
tests/conftest.py (1)
page(46-68)
tests/test_zoom_numeric.py (1)
tests/conftest.py (1)
page(46-68)
tests/test_zoom_auto.py (1)
tests/conftest.py (1)
page(46-68)
🪛 LanguageTool
README.md
[style] ~100-~100: To form a complete sentence, be sure to include a subject or ‘there’.
Context: ... | The zoom level of the PDF viewer. Can be a float (0.1-10.0), "auto" for fit...
(MISSING_IT_THERE)
[style] ~101-~101: To form a complete sentence, be sure to include a subject.
Context: ...of the PDF viewer within its container. Can be "center" (default), "left", or `...
(MISSING_IT_THERE)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build (ubuntu-latest, 3.11, 18)
- GitHub Check: build (ubuntu-latest, 3.10, 20)
- GitHub Check: build (ubuntu-latest, 3.13, 20)
- GitHub Check: build (ubuntu-latest, 3.12, 20)
- GitHub Check: build (ubuntu-latest, 3.11, 20)
- GitHub Check: build (ubuntu-latest, 3.12, 18)
- GitHub Check: build (ubuntu-latest, 3.13, 18)
- GitHub Check: build (ubuntu-latest, 3.10, 18)
🔇 Additional comments (5)
README.md (2)
28-28: LGTM! Clear feature addition.The new feature bullet accurately describes the maximization toggle. The phrase "within the component boundaries" effectively distinguishes this from browser-level fullscreen.
106-106: Documentation is clear and accurate.The parameter documentation correctly describes the
show_fullscreen_togglebehavior and clarifies that it provides "expanded viewing within the component boundaries rather than browser fullscreen."streamlit_pdf_viewer/frontend/src/PdfViewer.vue (3)
85-86: LGTM! Clean state management setup.The
isMaximizedreactive state andshowFullscreencomputed property provide clear separation between UI state and prop-driven visibility control.
110-149: Well-structured computed property with clear mode handling.The conditional logic cleanly separates maximized and normal modes, with appropriate styling for each state. The transition property adds polish to the mode switching.
2-15: UI structure aligns well with updated test selectors.The new
.top-buttonsand.control-buttonclass structure matches the test selectors (.top-buttons button.control-button), ensuring test reliability. The conditional rendering of the fullscreen button viav-if="showFullscreen"correctly implements the feature toggle.
| // Handle maximized state changes without full re-render | ||
| const handleMaximizedResize = () => { | ||
| if (isMaximized.value) { | ||
| // When maximized, adjust container dimensions without re-rendering PDF | ||
| const scrollingContainer = document.querySelector('.scrolling-container'); | ||
| if (scrollingContainer) { | ||
| scrollingContainer.style.height = 'calc(100vh - 120px)'; // Account for control buttons | ||
| } | ||
| // Adjust PDF viewer width to fit the maximized container | ||
| const pdfViewer = document.getElementById("pdfViewer"); | ||
| if (pdfViewer && maxWidth.value) { | ||
| pdfViewer.style.width = '100%'; | ||
| pdfViewer.style.maxWidth = 'none'; | ||
| } | ||
| } else { | ||
| // Restore original dimensions when exiting maximized mode | ||
| const scrollingContainer = document.querySelector('.scrolling-container'); | ||
| if (scrollingContainer) { | ||
| scrollingContainer.style.height = ''; | ||
| } | ||
| // Trigger a single re-render to restore original layout | ||
| handleResize(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Full re-render on exit may be expensive for large PDFs.
Line 443 calls handleResize() which re-renders the entire PDF when exiting maximized mode. For large documents, this could cause a noticeable delay or freeze.
Consider alternatives:
- Restore the original layout without re-rendering by reverting CSS changes only
- If re-render is necessary, show a loading indicator during the operation
- Cache the rendered pages to avoid full re-computation
Additionally, line 426 uses a magic number 120px for control button height. Consider extracting this to a constant or computing it dynamically:
+const CONTROL_BUTTONS_HEIGHT = 120; // Height of control buttons area
+
const handleMaximizedResize = () => {
if (isMaximized.value) {
const scrollingContainer = document.querySelector('.scrolling-container');
if (scrollingContainer) {
- scrollingContainer.style.height = 'calc(100vh - 120px)';
+ scrollingContainer.style.height = `calc(100vh - ${CONTROL_BUTTONS_HEIGHT}px)`;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Handle maximized state changes without full re-render | |
| const handleMaximizedResize = () => { | |
| if (isMaximized.value) { | |
| // When maximized, adjust container dimensions without re-rendering PDF | |
| const scrollingContainer = document.querySelector('.scrolling-container'); | |
| if (scrollingContainer) { | |
| scrollingContainer.style.height = 'calc(100vh - 120px)'; // Account for control buttons | |
| } | |
| // Adjust PDF viewer width to fit the maximized container | |
| const pdfViewer = document.getElementById("pdfViewer"); | |
| if (pdfViewer && maxWidth.value) { | |
| pdfViewer.style.width = '100%'; | |
| pdfViewer.style.maxWidth = 'none'; | |
| } | |
| } else { | |
| // Restore original dimensions when exiting maximized mode | |
| const scrollingContainer = document.querySelector('.scrolling-container'); | |
| if (scrollingContainer) { | |
| scrollingContainer.style.height = ''; | |
| } | |
| // Trigger a single re-render to restore original layout | |
| handleResize(); | |
| } | |
| }; | |
| const CONTROL_BUTTONS_HEIGHT = 120; // Height of control buttons area | |
| // Handle maximized state changes without full re-render | |
| const handleMaximizedResize = () => { | |
| if (isMaximized.value) { | |
| // When maximized, adjust container dimensions without re-rendering PDF | |
| const scrollingContainer = document.querySelector('.scrolling-container'); | |
| if (scrollingContainer) { | |
| scrollingContainer.style.height = `calc(100vh - ${CONTROL_BUTTONS_HEIGHT}px)`; // Account for control buttons | |
| } | |
| // Adjust PDF viewer width to fit the maximized container | |
| const pdfViewer = document.getElementById("pdfViewer"); | |
| if (pdfViewer && maxWidth.value) { | |
| pdfViewer.style.width = '100%'; | |
| pdfViewer.style.maxWidth = 'none'; | |
| } | |
| } else { | |
| // Restore original dimensions when exiting maximized mode | |
| const scrollingContainer = document.querySelector('.scrolling-container'); | |
| if (scrollingContainer) { | |
| scrollingContainer.style.height = ''; | |
| } | |
| // Trigger a single re-render to restore original layout | |
| handleResize(); | |
| } | |
| }; |
| watch(isMaximized, (newVal) => { | ||
| if (newVal) { | ||
| // When entering maximized mode, ensure zoom controls work properly | ||
| setTimeout(() => { | ||
| const scrollingContainer = document.querySelector('.scrolling-container'); | ||
| if (scrollingContainer) { | ||
| scrollingContainer.scrollTop = parseFloat(scrollingContainer.dataset.savedScrollTop || 0); | ||
| scrollingContainer.scrollLeft = parseFloat(scrollingContainer.dataset.savedScrollLeft || 0); | ||
| } | ||
| }, 100); // Small delay to ensure DOM has updated | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid setTimeout for DOM synchronization.
Lines 466-472 use setTimeout with a 100ms delay to wait for the DOM to update. This is fragile and could cause race conditions.
Use Vue's nextTick instead for proper DOM update synchronization:
watch(isMaximized, (newVal) => {
if (newVal) {
- setTimeout(() => {
+ nextTick(() => {
const scrollingContainer = document.querySelector('.scrolling-container');
if (scrollingContainer) {
scrollingContainer.scrollTop = parseFloat(scrollingContainer.dataset.savedScrollTop || 0);
scrollingContainer.scrollLeft = parseFloat(scrollingContainer.dataset.savedScrollLeft || 0);
}
- }, 100);
+ });
}
});Don't forget to import nextTick:
-import {onMounted, computed, ref, onUnmounted, watch} from "vue";
+import {onMounted, computed, ref, onUnmounted, watch, nextTick} from "vue";Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In streamlit_pdf_viewer/frontend/src/PdfViewer.vue around lines 463 to 474, the
watch callback uses setTimeout(…,100) to wait for DOM updates; replace this with
Vue's nextTick: import nextTick from 'vue' (or add it to the existing import),
make the watch callback async, await nextTick() instead of setTimeout, then run
the scrollingContainer.dataset restore logic immediately after the await so the
DOM synchronization is deterministic and the arbitrary delay is removed.
|
|
||
| # Check that zoom controls are present | ||
| zoom_button = iframe_frame.locator('button.zoom-button') | ||
| zoom_button = iframe_frame.locator('.top-buttons button.control-button').nth(1) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify button order assumption with nth(1).
The selector .top-buttons button.control-button.nth(1) assumes the zoom button is always the second button (index 1). If show_fullscreen_toggle is false or the button order changes, this will break.
Run this script to verify the button order across different configurations:
🏁 Script executed:
#!/bin/bash
# Check if show_fullscreen_toggle is configurable in test apps
rg -n "show_fullscreen_toggle" tests/streamlit_apps/
# Check the button rendering order in the Vue component
ast-grep --pattern $'<div class="top-buttons">
$$$
</div>'Length of output: 0
I need to explore the codebase structure to understand the button rendering logic and verify the concern about button order assumptions.
🏁 Script executed:
#!/bin/bash
# Find the test file and examine its contents
find . -name "test_zoom_auto_height.py" -type f 2>/dev/null
# Find streamlit apps directory structure
find . -path "*streamlit_apps*" -type d 2>/dev/null | head -10
# Search for button.control-button selector in test files
rg -n "button\.control-button" tests/ 2>/dev/null | head -20
# Look for show_fullscreen_toggle in the entire codebase
rg -n "show_fullscreen_toggle" . 2>/dev/null | head -20Length of output: 1782
🏁 Script executed:
#!/bin/bash
# Search for top-buttons div and button rendering in any component files
rg -n "top-buttons" . 2>/dev/null | head -30
# Look for nth(1) or similar index-based selectors in tests
rg -n "\.nth\(" tests/ 2>/dev/null | head -20Length of output: 3167
🏁 Script executed:
#!/bin/bash
# Find Vue/JavaScript/TypeScript component files that might render buttons
fd -e vue -e jsx -e tsx -e js . 2>/dev/null | head -30
# Look for fullscreen or zoom button related code
rg -n "fullscreen\|zoom" . --type vue --type js --type jsx --type tsx 2>/dev/null | head -30Length of output: 251
I need to examine the Vue component to understand the button rendering order and whether it depends on show_fullscreen_toggle.
🏁 Script executed:
#!/bin/bash
# Extract the top-buttons div and surrounding context from PdfViewer.vue
sed -n '7,50p' ./streamlit_pdf_viewer/frontend/src/PdfViewer.vue
# Also check for any conditional rendering of buttons
rg -A 15 "top-buttons" ./streamlit_pdf_viewer/frontend/src/PdfViewer.vueLength of output: 3827
🏁 Script executed:
#!/bin/bash
# Look for button rendering logic and fullscreen/zoom button definitions
rg -n "control-button\|zoom\|fullscreen" ./streamlit_pdf_viewer/frontend/src/PdfViewer.vue | head -40Length of output: 0
Now I need to verify what configurations the test apps use for show_fullscreen_toggle:
🏁 Script executed:
#!/bin/bash
# Check what test apps exist and their configurations
ls -la tests/streamlit_apps/
# Search for show_fullscreen_toggle in test app files
rg -n "show_fullscreen_toggle" tests/streamlit_apps/ -A 2 -B 2Length of output: 1399
🏁 Script executed:
#!/bin/bash
# Check if any test explicitly sets show_fullscreen_toggle to False or passes it as a parameter
rg -n "show_fullscreen_toggle\s*=\s*False\|show_fullscreen_toggle.*false" tests/ Length of output: 0
🏁 Script executed:
#!/bin/bash
# Read the test app files to see how they call pdf_viewer
cat tests/streamlit_apps/example_zoom_auto_height.py
echo "---"
cat tests/streamlit_apps/example_zoom_auto.py
echo "---"
cat tests/streamlit_apps/example_zoom_numeric.pyLength of output: 828
🏁 Script executed:
#!/bin/bash
# Check what the corresponding test files do - look at their app parameter
grep -n "st_pdf_viewer\|pdf_viewer\|example_" tests/test_zoom_auto_height.py | head -15
echo "---"
grep -n "st_pdf_viewer\|pdf_viewer\|example_" tests/test_zoom_auto.py | head -15
echo "---"
grep -n "st_pdf_viewer\|pdf_viewer\|example_" tests/test_zoom_numeric.py | head -15Length of output: 1790
Improve selector robustness—nth(1) depends on implicit fullscreen toggle configuration.
The selector .nth(1) assumes the zoom button is always the second button. However, the first button (fullscreen toggle) is conditionally rendered based on show_fullscreen_toggle, which defaults to True. While current test apps don't override this default, the selector is fragile and will break if:
- A test app sets
show_fullscreen_toggle=False - Component configuration changes in the future
Recommendation: Replace index-based selection with a more resilient selector that targets the zoom button directly, such as by its text content (contains %) or by its click handler. Alternatively, explicitly pass show_fullscreen_toggle=True in test apps and document this assumption.
🤖 Prompt for AI Agents
In tests/test_zoom_auto_height.py around line 62, the test uses .nth(1) to pick
the zoom button which is fragile because the fullscreen toggle may or may not be
present; replace the index-based locator with a resilient selector that targets
the zoom control explicitly (for example, select the button by its visible text
containing "%" or by a dedicated test attribute/aria-label), or alternatively
ensure test apps explicitly set show_fullscreen_toggle=True and document that
dependency; update the locator accordingly so the test no longer relies on
button order.
2 participants
Attempt at #109 via browser-level fullscreen toggling instead of in-app maximization.
IMO this is not the best solution yet, because fullscreen may be "too much" compared to modal-level maximization.
Also the current implementation forces re-rendering and suffers the same problem described in #108 about zoom changes.
On the other hand, I suppose that this is better than nothing, so it can be considered as an interim feature until proper maximization can be implemented. And it can also be a reference for future implementations.
Up to the maintainers to decide what to do with this.
Summary by CodeRabbit
New Features
Documentation
Style/Refactor
Tests