Skip to content

Remove the scrollIntoView function#20953

Open
calixteman wants to merge 1 commit intomozilla:masterfrom
calixteman:rm_scrollintoview
Open

Remove the scrollIntoView function#20953
calixteman wants to merge 1 commit intomozilla:masterfrom
calixteman:rm_scrollintoview

Conversation

@calixteman
Copy link
Contributor

and rely on the HTMLElement.scrollIntoView function instead.

@codecov-commenter
Copy link

codecov-commenter commented Mar 22, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.55%. Comparing base (2643125) to head (e87b9c5).

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #20953      +/-   ##
==========================================
+ Coverage   62.51%   62.55%   +0.03%     
==========================================
  Files         172      172              
  Lines      121789   121789              
==========================================
+ Hits        76134    76180      +46     
+ Misses      45655    45609      -46     
Flag Coverage Δ
unittestcli 62.55% <ø> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to ask, but how much did you actually test this!?

This "breaks" even something as simple as the next/previous-buttons, since the page no longer aligns properly at the top and there's now an empty space left between the toolbar and the page.

Image

@calixteman
Copy link
Contributor Author

Sorry to ask, but did how much did you actually test this!?

Yep I tested with pdf.pdf which has outlines.
Anyway I was sure to overlook some cases ;)

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Mar 22, 2026

Yep I tested with pdf.pdf which has outlines.
Anyway I was sure to overlook some cases ;)

Testing that one now, it seems that it has the same problem with incorrect offsets even when clicking the destinations in the outline.

I believe that the old scrollIntoView helper takes the margin/border of the page-element into account, whereas a raw Element.scrollIntoView does not appear to do that.

and rely on the HTMLElement.scrollIntoView function instead.
scrollIntoView(div, pageSpot);

div.scrollIntoView({
behavior: "auto",

This comment was marked as resolved.

This comment was marked as resolved.

width: 816px;
height: 1056px;
margin: var(--page-margin);
margin-inline: auto;
Copy link
Collaborator

@Snuffleupagus Snuffleupagus Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this not have a value around 10px (via a new CSS variable), to mimic the effect of the old page-border rule?

Likely related to this is that pages in spreadMode, or when using the horizontal and wrapped scrollModes, no longer have any horizontal space between them.

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Mar 23, 2026

The pages are not aligned correctly vertically in spreadMode (it also shows the lack of horizontal space mentioned above).

spread

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants