Intersection Observers for getting "Current Page(s)" #3845

Gazook89 · 2024-10-21T06:00:10Z

This PR is mostly motivated by the work on the #3799 PR which allows users to view many pages at once, but was causing issues with what is the "current page" and also issues of viewing "page 10 out of 9 total pages"-like issues in the Toolbar.

Description

This sets up two Intersection Observers: the first captures every page that is at least 30% visible inside the .pages container, and the second captures every page that has at least one pixel on the horizontal center line of .pages. Both can be arrays of integers (page index).

The visiblePages array is duplicated and formatted into a formattedPages state variable as strings, which gets displayed in the toolbar. visiblePages is kept so that it can be used in the "previous/next page" buttons of the toolbar later.

The toolbar displays the formattedPages strings, unless the user clicks into the page input and enters their own integer (only a single integer, no range), which can then jump the preview to that page on Enter or blur().

The Arrow 'change page' buttons jump the preview back and forth by a 'full set'. Meaning that if you are viewing one page a time it will move through the previous/next pages one at a time, but if you are viewing 10 pages at once it will move you through 10 pages at a time.

Related Issues or Discussions

Closes Rethink how current page number is figured in Preview Pane #3824
Fix toolbar issues #3694 -- ultimately scuttled for another PR but raises issue of bad page count calculations

QA Instructions, Screenshots, Recordings

First, sorry for the bad single commit on this one, it doesn't really walk you through the development.

An example document (500 pages) with some additional styling to make it easy to see page numbers:

HB-NecromanticArmorofSaladDressing.txt

or, just put this CSS in any long brew:

.page:has(.insideCover, .frontCover, .backCover)::after {
  display: grid;
}

.page::after {
  all:unset;
  content: counter(page-numbers);
  position: absolute;
  top: 0;
  left: 0;
  width: 100%;
  height: 100%;
  background: #AAAA;
  display: grid;
  place-items: center;
  font-size: 200px;
  }
  
.page:nth-child(even):after {
  transform: unset;
 }

To Test:

Load the page and it should load up with 1 / [total pages] in the toolbar.
You should see it change when scrolling in the Preview pane, stopping at 1 or the total page count without going over.
Then, change the zoom so you see multiple pages and you should see ranges of pages in that input, like 10 - 12. You can also change the current "Spread" to facing or flow.
Try the "previous/next" arrows. It should not overrun past the limits.
Try typing a single integer in the 'current page' input and hitting enter or clicking outside of it-- it should take you to that page. Typing non-integer characters should be ignored, as well as ranges, and integers outside the total page count will bring you to the last page.
After entering your own page number, scrolling again continues to work.

Single page with >30% of area inside view:

Book spread, shows that only pages with >30% of area inside view are shown in the 'current pages' input:

Many pages, shows the "range":

Reviewer Checklist

No checklist yet. (I have to go to bed)

*Reviewers, refer to this list when testing features, or suggest new items *

Copy this list

- [ ] Verify new features are functional
  - [ ] Page ranges are shown when multiple pages in view (more than a sliver).
  - [ ] Prev/Next Page buttons in toolbar move through based on how many pages are in view.
  - [ ] Accurately describes pages in view when pages are in `flex` format (multiple pages in row)
- [ ] Verify old features have not broken
  - [ ] Brew/Source Jump buttons still work
  - [ ] Prev/Next buttons in toolbar still work
- [ ] Test for edge cases / try to break things
  - [ ] Does not show current page higher than total number of pages, or lower than "1". 
  - [ ] Functions at any zoom level
  - [ ] Doesn't cause performance issues
- [ ] Identify opportunities for simplification and refactoring
- [ ] Check for code legibility and appropriate comments

Bad commit here with too much stuff. I apologize. This sets up two Intersection Observers: the first captures every page that is at least 30% visible inside the `.pages` container, and the second captures every page that has at least one pixel on the horizontal center line of `.pages`. Both can be arrays of integers (page index). The "visiblePages" array is duplicated and formatted into a "formattedPages" state, which gets displayed in the toolbar. The toolbar displays that, unless the user clicks into the page input and enters their own integer (only a single integer, no range), which can then jump the preview to that page on Enter or blur(). The Arrow 'change page' buttons jump the preview back and forth by a 'full set'. If one page is viewed at a time, this is moved on page a time, and if 10 pages are viewed at a time it jumps the pages by 10. Left to do: adapt the "jump editor to match preview" divider button to work with new "centerPage".

This fixes the "jump editor to preview position" button.

Reduce the visual prominence of the page input by using a darker background and a text color that matches the rest of the toolbar icons. Darker background still indicates this is an interactive item (is an input), hopefully.

Removes currentPage as a variable since it's been replaced.

Little changes like removing console.logs and adding comments.

Doesn't need to be set in brewRenderer state and passed as a prop, when it can just do it's work directly in the toolbar.

G-Ambatte · 2024-10-22T03:32:31Z

When the toolbar showing 100% zoom and page displaying 7 - 8, the following occurs:

clicking Next Page results in the view moving to page 9 - Expectation: either end of visible range (8) or page immediately after visible range (9)
clicking Previous Page results in the view moving to page 5 - Expectation: either start of visible range (7) or page immediately before visible range (6)

Presumably this issue will also appear at other zoom levels.

Gazook89 · 2024-10-22T03:39:45Z

clicking Next Page results in the view moving to page 9 - Expectation: either end of visible range (8) or page immediately after visible range (9)

To be clear, are you saying that this expectation is met? Because when I do it, 7 - 8 and "next", it goes to 9. And then you are saying that the actual problem is this:

clicking Previous Page results in the view moving to page 5 - Expectation: either start of visible range (7) or page immediately before visible range (6)

Just want to be sure you are listing one problem, not two.

G-Ambatte · 2024-10-22T03:40:55Z

When the toolbar showing 40% zoom and page displaying 7 - 9, the following occurs:

clicking Next Page results in the view moving to pages 10 - 11 - Expectation: either page range starting at the end of visible range (9 - 10) or page range starting immediately after visible range (10 - 11)
clicking Previous Page results in the view moving to pages 4 - 5 - Expectation: either page range ending at the start of visible range (6 - 7) or page range ending immediately before the start of the visible range (5 - 6)

So it appears that the Previous Page option is consistently selecting one page earlier than expected.

G-Ambatte · 2024-10-22T03:44:13Z

Just want to be sure you are listing one problem, not two.

Correct, Next Page is working within my intuitive expectations, while Previous Page is not.

Gazook89 · 2024-10-22T03:47:17Z

So it's taking the number of visible pages (3), walking back that number of pages from the "first in range" (or, subtracting 3 from 7), and then using that result to plug into scrollToPage.

Because you are only viewing partials of the 'top' and 'bottom' pages, you have 3 visible pages. But when scrollToPage is called, it places the first page right at the top of the page, and only 2 pages are visible in the pane. So you are jumping back to page 4, but placing it right at the top so only it and page 5 are visible.

I'll have to think about how this should be handled.

edit: every subsequent click of the previous button would be fine as is currently setup

Gazook89 · 2024-10-22T03:52:39Z

Perhaps before calculating the next range to scroll to, I can call scrollToPage with the first page in the original array to move it fully into view which would minimize the length of the visiblePages array, and then go forward with the rest of the "previous" button logic.

i hope

Gazook89 · 2024-10-23T03:50:37Z

@G-Ambatte i pushed a change, see if it's better. The behavior still isn't entirely consistent between the "Previous" and the "Next" buttons, but it should no longer skip a whole page entirely.

When you are viewing pages 6 - 7 at 100% zoom, and

you do "Previous": it will jump to 5 with page five aligned at top of pane.
you do "Next": it will jump to 7

When you are viewing pages 50 - 53 at 39% zoom, and

you do "Previous": it will jump to 48 - 49 with page 48 aligned at top of pane.
you do "Next": it will jump to 53 - 54 with page 53 aligned at top of pane.

So basically when you are advancing a "page", it will show you the last page of your previous state plus whatever else registers as 'visible'.

And when you are going back a "page", it will show you a completely new set of pages that immediately precede your previous set.

When only one page is registered as "visible" it just works as expected.

Is this acceptable?

G-Ambatte · 2024-10-29T19:33:13Z

@G-Ambatte i pushed a change, see if it's better. The behavior still isn't entirely consistent between the "Previous" and the "Next" buttons, but it should no longer skip a whole page entirely.

When you are viewing pages 6 - 7 at 100% zoom, and
* you do "Previous": it will jump to `5 ` with page five aligned at top of pane.

* you do "Next": it will jump to `7`
When you are viewing pages 50 - 53 at 39% zoom, and
* you do "Previous": it will jump to `48 - 49 ` with page 48 aligned at top of pane.

* you do "Next": it will jump to `53 - 54` with page 53 aligned at top of pane.
So basically when you are advancing a "page", it will show you the last page of your previous state plus whatever else registers as 'visible'.

And when you are going back a "page", it will show you a completely new set of pages that immediately precede your previous set.

When only one page is registered as "visible" it just works as expected.

Is this acceptable?

Sorry, just realized I didn't respond to this. From testing, it appears to operate as expected. Looks good!

Prior to fix, the "next page" button in the toolbar wouldn't work well if there were multiple pages in view that were in a single 'row'. This is because the logic is to take the pages that are "visible", take the max of those pages, and then scroll to that page. But the issue is that if the 'max' page is in the same row as other pages, the range of visible pages doesn't change....the max will always be the same. So the change here basically runs the scroll function twice-- if the first run results in the same 'max' page as before the scroll, it runs it again but with the target page being "max + 1", which will bump the target to the next row.

Gazook89 · 2024-11-07T05:20:12Z

Some changes made and things fixed. I think it's outlined well enough in the commit messages, but quick notes:

The 'fit to view' zoom button, which normally sets the zoom so that a single full page fits just within the preview pane, now sets the zoom to fit two pages side-by-side if the current spread is 'facing'.
This logic is changed:

When you are viewing pages 50 - 53 at 39% zoom, and you do "Next": it will jump to 53 - 54 with page 53 aligned at top of pane.

Now, it will jump you to page 54 at the start of the range and any other pages after it.
did a small css change to justify-content properties that I didn't know existed until now: they are now safe center in both facing and flow spreads. Basically, they are centered unless there isn't space enough for one page, in which case it becomes basically flex-start (aligned to the left).

Oh, and, master has been merged in.

When the page is zoomed in very close, such that <30% of the page is in view, it doesn't register changes to the 'current page'. This fixes that, passing in the 'centerPage' if 'visiblePages' is empty. I don't love this fix, i think the visiblePages should always have *something* in it, but I can't quite figure out how to set that (since the normal update to visiblePages is happening in an observer that doesn't fire if nothing is in view).

client/homebrew/brewRenderer/toolBar/toolBar.jsx

5e-Cleric · 2024-11-07T21:52:02Z

The following is more like a personal take on this, but i think it can be useful, in the jsx for the page numebr input add a manual style attribute like so:

style={{width:`${pageNum.toString().length + 2}ch`}}

and in the CSS changing the width to set a max-width:10ch or whatever looks reasonable.

This should keep a small input when a small value is set, and grow with it to a maximum we deem appropiate. It accounts for the number of characters in the pageNum string, plus 2 for good measure, acting like a faux padding.

Gazook89 · 2024-11-08T01:40:28Z

The following is more like a personal take on this, but i think it can be useful, in the jsx for the page numebr input add a manual style attribute like so:

style={{width:`${pageNum.toString().length + 2}ch`}}

and in the CSS changing the width to set a max-width:10ch or whatever looks reasonable.

This should keep a small input when a small value is set, and grow with it to a maximum we deem appropiate. It accounts for the number of characters in the pageNum string, plus 2 for good measure, acting like a faux padding.

The idea is fine, but I'm going to push through a slightly different implementation. I'm not going to do +2ch just to get some padding-- i can just use padding for that. Also, I'd rather have min-width setup than max-width, so that it starts with ~5 character width and then can grow to accommodate any length.

you might think this can get out of control if someone types in a super long number...but the input width won't expand until the toolbar is rerendered, which doesn't happen until the number is 'submitted', at which point it'll be knocked down to the 'max' page which will be a more reasonable character length, hopefully

What about this? Need more padding or is it fine?

pretty much completely unchanged, was originally moved just to help with merging master in (ie it was erroneously removed)

5e-Cleric

Overall looks great!

client/homebrew/brewRenderer/toolBar/toolBar.jsx

5e-Cleric · 2024-11-08T22:50:10Z

client/homebrew/brewRenderer/toolBar/toolBar.jsx

+		for (let i = 1; i <= sortedPages.length; i++) {
+			// If the current page is not consecutive or it's the end of the list
+			if(i === sortedPages.length || sortedPages[i] !== sortedPages[i - 1] + 1) {
+				// Push the range to the list


Same as before, L91 comment doesn't add anything. Is this from the toaster?

It probably is a remainder from cGPT, though I'm not sure it should be removed. It's part of a series of comments on each step of the method, so it'd be odd to leave one step uncommented. Someone else can remove it or do whatever to resolve this.

yes, but a comment that says push the range to the list, then the next line is range.push( is pretty redundant IMO

Gazook89 added 2 commits October 21, 2024 00:30

Merge branch 'master' into Intersection-Observer

119755e

Gazook89 marked this pull request as draft October 21, 2024 06:04

Gazook89 added 4 commits October 21, 2024 20:43

modify effect to enable Jump Editor button

0afc2ab

This fixes the "jump editor to preview position" button.

style change on page text input

183dd63

Reduce the visual prominence of the page input by using a darker background and a text color that matches the rest of the toolbar icons. Darker background still indicates this is an interactive item (is an input), hopefully.

Fix NaN/undefined showing on first load

822d0c7

Removes currentPage as a variable since it's been replaced.

Merge branch 'master' into Intersection-Observer

b6bd7cc

Gazook89 marked this pull request as ready for review October 22, 2024 02:33

Gazook89 added UI/UX User Interface, user experience P2 - minor feature or not urgent Minor bugs or less-popular features 🔍 R0 - Needs first review 👀 PR ready but has not been reviewed labels Oct 22, 2024

Gazook89 added 4 commits October 21, 2024 22:13

Add some comments and cleanup

de7b13b

Little changes like removing console.logs and adding comments.

move formatting of visible pages to toolbar

5c0d6e6

Doesn't need to be set in brewRenderer state and passed as a prop, when it can just do it's work directly in the toolbar.

add comment

26050e2

linting

4126188

adjust prev/next page buttons to meet expectations

5ab867f

i hope

calculuschild temporarily deployed to homebrewery-pr-3845 October 23, 2024 18:22 Inactive

G-Ambatte mentioned this pull request Oct 29, 2024

Use IntersectionObserver to detect current page #3682

Draft

Gazook89 mentioned this pull request Nov 4, 2024

View Modes - Single / Facing Pages / "Flow" arrangements #3799

Merged

12 tasks

Gazook89 added 3 commits November 5, 2024 14:03

Merge branch 'Intersection-Observer' into Observer-Master-merge

93b9f1d

Merge branch 'master' into Observer-Master-merge

2222550

calculuschild temporarily deployed to homebrewery-pr-3845 November 7, 2024 05:08 Inactive

Gazook89 requested review from calculuschild, 5e-Cleric and G-Ambatte November 7, 2024 05:20

calculuschild temporarily deployed to homebrewery-pr-3845 November 7, 2024 16:18 Inactive

Gazook89 requested a review from dbolack-ab November 7, 2024 16:18

lint and refactor

9ef11bc

calculuschild temporarily deployed to homebrewery-pr-3845 November 7, 2024 16:41 Inactive

5e-Cleric reviewed Nov 7, 2024

View reviewed changes

client/homebrew/brewRenderer/toolBar/toolBar.jsx Outdated Show resolved Hide resolved

5e-Cleric reviewed Nov 7, 2024

View reviewed changes

client/homebrew/brewRenderer/toolBar/toolBar.jsx Outdated Show resolved Hide resolved

5e-Cleric added 🔍 R3 - Reviewed - Awaiting Fixes 🔧 PR is okayed but needs fixes before merging and removed 🔍 R0 - Needs first review 👀 PR ready but has not been reviewed labels Nov 7, 2024

fix 'disabled' attribute on min/max of page range

650ec04

calculuschild temporarily deployed to homebrewery-pr-3845 November 8, 2024 00:57 Inactive

dynamic text input width to match characters

28855d0

calculuschild temporarily deployed to homebrewery-pr-3845 November 8, 2024 01:46 Inactive

add scrollToHash method back in

28a7f24

pretty much completely unchanged, was originally moved just to help with merging master in (ie it was erroneously removed)

calculuschild temporarily deployed to homebrewery-pr-3845 November 8, 2024 02:32 Inactive

5e-Cleric approved these changes Nov 8, 2024

View reviewed changes

5e-Cleric added 🔍 R4 - Reviewed - Fixed! Awaiting final review⭐ PR review comments addressed and removed 🔍 R3 - Reviewed - Awaiting Fixes 🔧 PR is okayed but needs fixes before merging labels Nov 8, 2024

Gazook89 added 4 commits November 8, 2024 21:56

give dismisskeys a default

2e8368d

lint

b7b1981

Merge remote-tracking branch 'upstream/master'

234d484

Merge branch 'master' into Intersection-Observer

4bad047

calculuschild deployed to homebrewery-pr-3845 November 10, 2024 21:56 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Intersection Observers for getting "Current Page(s)" #3845

Intersection Observers for getting "Current Page(s)" #3845

Gazook89 commented Oct 21, 2024 •

edited

Loading

G-Ambatte commented Oct 22, 2024

Gazook89 commented Oct 22, 2024

G-Ambatte commented Oct 22, 2024

G-Ambatte commented Oct 22, 2024 •

edited

Loading

Gazook89 commented Oct 22, 2024 •

edited

Loading

Gazook89 commented Oct 22, 2024

Gazook89 commented Oct 23, 2024

G-Ambatte commented Oct 29, 2024

Gazook89 commented Nov 7, 2024

5e-Cleric commented Nov 7, 2024 •

edited

Loading

Gazook89 commented Nov 8, 2024

5e-Cleric left a comment

5e-Cleric Nov 8, 2024

Gazook89 Nov 9, 2024

5e-Cleric Nov 9, 2024

Intersection Observers for getting "Current Page(s)" #3845

Are you sure you want to change the base?

Intersection Observers for getting "Current Page(s)" #3845

Conversation

Gazook89 commented Oct 21, 2024 • edited Loading

Description

Related Issues or Discussions

QA Instructions, Screenshots, Recordings

Reviewer Checklist

G-Ambatte commented Oct 22, 2024

Gazook89 commented Oct 22, 2024

G-Ambatte commented Oct 22, 2024

G-Ambatte commented Oct 22, 2024 • edited Loading

Gazook89 commented Oct 22, 2024 • edited Loading

Gazook89 commented Oct 22, 2024

Gazook89 commented Oct 23, 2024

G-Ambatte commented Oct 29, 2024

Gazook89 commented Nov 7, 2024

5e-Cleric commented Nov 7, 2024 • edited Loading

Gazook89 commented Nov 8, 2024

5e-Cleric left a comment

Choose a reason for hiding this comment

5e-Cleric Nov 8, 2024

Choose a reason for hiding this comment

Gazook89 Nov 9, 2024

Choose a reason for hiding this comment

5e-Cleric Nov 9, 2024

Choose a reason for hiding this comment

Gazook89 commented Oct 21, 2024 •

edited

Loading

G-Ambatte commented Oct 22, 2024 •

edited

Loading

Gazook89 commented Oct 22, 2024 •

edited

Loading

5e-Cleric commented Nov 7, 2024 •

edited

Loading