fix: Correct scrollbar position/size for "Just Text" pages (BL-14112) #348
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
src/bloom-player-core.tsxOops! Something went wrong! :( ESLint: 9.13.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by WalkthroughThe changes in this pull request involve modifications to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/bloom-player-core.tsx (1)
2313-2329: Remove unused variable 'scale' in 'addingScrollbarsToPage'The variable
scaleis declared but not used within theaddingScrollbarsToPagemethod. Removing it will clean up the code and eliminate unnecessary declarations.Apply this diff:
private addingScrollbarsToPage(index: number): void { if (this.state.isLoading || this.startingUpSwiper) { //console.log("aborting showingPage because still loading"); return; } const bloomPage = this.getPageAtSwiperIndex(index); if (!bloomPage) { // It MIGHT be a blank initial or final page placeholder, but more likely, we did a long // scroll using the slider, so we're switching to a page that is, for the moment, // empty due to laziness. A later render will fill it in. We want to try again then. Not sure how // else to make sure that happens. window.setTimeout(() => this.addingScrollbarsToPage(index), 50); return; // nothing more we can do until the page we want really exists. } - let scale = 1; addScrollbarsToPage(bloomPage); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/bloom-player-core.tsx(2 hunks)src/scrolling.ts(4 hunks)
🔇 Additional comments (5)
src/scrolling.ts (4)
71-72: Include 'cursorWidth' in scrollbar adjustments
The addition of 'cursorWidth' in the destructured assignment from ComputeNiceScrollOffsets ensures that the scrollbar's cursor width is correctly adjusted based on the page scale.
189-192: Use dynamic 'cursorWidth' in 'niceScroll' options
By setting cursorwidth and cursorborderradius to cursorWidth, the scrollbar cursor now scales proportionally with the page, enhancing the visual consistency when the page is zoomed or scaled.
219-245: Enhance 'ComputeNiceScrollOffsets' to adjust for scaling
The updates in ComputeNiceScrollOffsets method correctly calculate topAdjust, leftAdjust, and cursorWidth based on the page scaling factor. This improvement ensures that scrollbars are properly positioned and scaled when the page size changes.
275-279: Update 'fixNiceScrollOffsets' to use dynamic 'cursorWidth'
The function now retrieves cursorWidth from ComputeNiceScrollOffsets and correctly applies it to groupNiceScroll[0].opt.cursorwidth, ensuring the scrollbar cursor width stays consistent with the page scaling.
src/bloom-player-core.tsx (1)
1828-1833: Invoke 'addingScrollbarsToPage' on slide transition end
Adding the call to addingScrollbarsToPage in slideChangeTransitionEnd ensures that scrollbars are added to the current page after slide transitions, enhancing scrollbar functionality during navigation.
| const topPadding = | ||
| compStyles.getPropertyValue("padding-top") ?? "0"; | ||
| const leftPadding = | ||
| compStyles.getPropertyValue("padding-left") ?? "0"; | ||
| topAdjust = parseFloat(topPadding) * (scale - 1); | ||
| leftAdjust = parseFloat(leftPadding) * (scale - 1); | ||
| } |
There was a problem hiding this comment.
Fix potential 'NaN' issue when computing padding values
When getPropertyValue returns an empty string, parseFloat results in NaN, causing topAdjust and leftAdjust to be NaN. Replace the nullish coalescing operator ?? with logical OR || to handle empty strings effectively.
Apply this diff to fix the issue:
const topPadding =
- compStyles.getPropertyValue("padding-top") ?? "0";
+ compStyles.getPropertyValue("padding-top") || "0";
const leftPadding =
- compStyles.getPropertyValue("padding-left") ?? "0";
+ compStyles.getPropertyValue("padding-left") || "0";📝 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.
| const topPadding = | |
| compStyles.getPropertyValue("padding-top") ?? "0"; | |
| const leftPadding = | |
| compStyles.getPropertyValue("padding-left") ?? "0"; | |
| topAdjust = parseFloat(topPadding) * (scale - 1); | |
| leftAdjust = parseFloat(leftPadding) * (scale - 1); | |
| } | |
| const topPadding = | |
| compStyles.getPropertyValue("padding-top") || "0"; | |
| const leftPadding = | |
| compStyles.getPropertyValue("padding-left") || "0"; | |
| topAdjust = parseFloat(topPadding) * (scale - 1); | |
| leftAdjust = parseFloat(leftPadding) * (scale - 1); | |
| } |
There was a problem hiding this comment.
If niceScroll is confused by the combination of padding on TG and outer scaling, it surprises me a bit that it doesn't make mistakes with the overall size of the scroll bar if there is top (or bottom) padding and scaling. Worth testing?
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @StephenMcConnel)
src/scrolling.ts line 219 at r1 (raw file):
let topAdjust = 0; let leftAdjust = 0; let cursorWidth = "12px";
Actually the width (and thumb radius) for the nice scroll bar. Deserves a comment, or even a rename where we can, since I don't think anyone but nicescroll calls the thumb a cursor.
src/scrolling.ts line 222 at r1 (raw file):
if (scale !== 1) { const parent2 = elt.parentElement?.parentElement; const parent3 = parent2?.parentElement;
How about we call these splitPaneComponentInner and marginBox, since we don't make any use of them unless that's what they turn out to be?
src/scrolling.ts line 228 at r1 (raw file):
) { // The nicescroll elements are added outside the marginBox, and also // outside the #page-scaling-container. So we need to adjust for the
As far as I can tell, #page-scaling-container is specific to bloom-desktop in edit mode. If there is something analogous that happens in BP, this comment should be adjusted. (Scaling in BP typically happens to the .bloomPlayer element, many layers above the marginBox.)
It would also be helpful if you could clarify why only these exact elements (two layers down from a split-pane-container-inner that is a child of marginBox) result in the nicescroll elements being outside the element that is scaled and so needing a different correction. For example, nicescroll can get applied to branding text that has .scrolling; these might well be only one or two levels down from the marginBox. Cover title TG is a direct child of marginBox. In our sample book with overflowing pages, the overflowing bloom-editables are two layers down from split-pane-component-inner but that is several layers below the marginBox. Just-text page seems to be the only case where the thing that is scrolling matches this pattern, and it is what this issue is about, but why does that result in the nicescroll elements being outside the scaling container when title and .scrolling don't?
(Is there any chance of a more meaningful algorithm, such as being a certain number of levels below the element whose computed style has a transform?)
src/scrolling.ts line 234 at r1 (raw file):
const right = parent2.offsetLeft + parent2.offsetWidth; topAdjust = -(top * (scale - 1)); leftAdjust = -(right * (scale - 1));
I wish it was possible to explain why the above is the right correction to apply, but since what we're computing is essentially an experimentally-determined value of "the correction niceScroll needs to put the scroll bar in the right place when the whole page is scaled" it may not be.
src/bloom-player-core.tsx line 1827 at r1 (raw file):
// with a good internet. 30s allows for it to be a LOT slower on a phone with a poor // connection.. We want SOME limit so // we don't keep using power for hours if the device is left on this page.
This comment specifically applies to the value for msToContinueFF60RepairChecks so should be moved to that line if we keep it...better yet, get rid of it altogether now we no longer need to support ff60. JohnH recently merged a change that I think removes the only call to the only reader of this value, so it would be ideal to remove it and all its setters and all comments referring to it.
src/bloom-player-core.tsx line 1831 at r1 (raw file):
this.addingScrollbarsToPage( this.swiperInstance.activeIndex, );
It used to happen unconditionally. Probably nothing on the front cover needs scroll bars, but JohnH's recent changes allow a book to be opened for the first time on a different page. Will we now fail to add scroll bars to such pages if needed? (Or maybe this gets called even for the first page shown...)
src/bloom-player-core.tsx line 2313 at r1 (raw file):
} private addingScrollbarsToPage(index: number): void {
This name sounds like a test whether we are in the middle of adding them. How about addScrollBarsToPageWhenReady?
This change is
Summary by CodeRabbit
New Features
Bug Fixes
Documentation