Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 27 additions & 17 deletions src/static/js/ace2_inner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2855,22 +2855,39 @@ function Ace2Inner(editorInfo, cssManagers) {
// This is required, browsers will try to do normal default behavior on
// page up / down and the default behavior SUCKS
evt.preventDefault();
const oldVisibleLineRange = scroll.getVisibleLineRange(rep);
let topOffset = rep.selStart[0] - oldVisibleLineRange[0];
if (topOffset < 0) {
topOffset = 0;
}

const isPageDown = evt.which === 34;
const isPageUp = evt.which === 33;

const oldVisibleLineRange = scroll.getVisibleLineRange(rep);
let topOffset = rep.selStart[0] - oldVisibleLineRange[0];
if (topOffset < 0) topOffset = 0;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like more brackets but I can understand both styles. Makes it more visible what is inside the if.


scheduler.setTimeout(() => {
// the visible lines IE 1,10
const newVisibleLineRange = scroll.getVisibleLineRange(rep);
// total count of lines in pad IE 10
const linesCount = rep.lines.length();
// How many lines are in the viewport right now?
const numberOfLinesInViewport = newVisibleLineRange[1] - newVisibleLineRange[0];

// Calculate lines to skip based on viewport pixel height divided by
// the average rendered line height. This correctly handles long wrapped
// lines that consume multiple visual rows (fixes #4562).
const viewportHeight = getInnerHeight();
const visibleStart = newVisibleLineRange[0];
const visibleEnd = newVisibleLineRange[1];
let totalPixelHeight = 0;
for (let i = visibleStart; i <= Math.min(visibleEnd, linesCount - 1); i++) {
const entry = rep.lines.atIndex(i);
if (entry && entry.lineNode) {
totalPixelHeight += entry.lineNode.offsetHeight;
}
}
// scroll.getVisibleLineRange() returns an inclusive end index, so the
// number of visible logical lines is (end - start + 1), matching the
// iteration bounds used to sum totalPixelHeight above.
const visibleLogicalLines = visibleEnd - visibleStart + 1;
// Use pixel-based count: how many logical lines fit in one viewport
const numberOfLinesInViewport = visibleLogicalLines > 0 && totalPixelHeight > 0
? Math.max(1, Math.round(visibleLogicalLines * viewportHeight / totalPixelHeight))
: Math.max(1, visibleLogicalLines);
Comment thread
qodo-free-for-open-source-projects[bot] marked this conversation as resolved.

if (isPageUp && padShortcutEnabled.pageUp) {
rep.selStart[0] -= numberOfLinesInViewport;
Expand All @@ -2886,18 +2903,11 @@ function Ace2Inner(editorInfo, cssManagers) {
rep.selStart[0] = Math.max(0, Math.min(rep.selStart[0], linesCount - 1));
rep.selEnd[0] = Math.max(0, Math.min(rep.selEnd[0], linesCount - 1));
updateBrowserSelectionFromRep();
// get the current caret selection, can't use rep. here because that only gives
// us the start position not the current
// scroll to the caret position
const myselection = targetDoc.getSelection();
// get the carets selection offset in px IE 214
let caretOffsetTop = myselection.focusNode.parentNode.offsetTop ||
myselection.focusNode.offsetTop;

// sometimes the first selection is -1 which causes problems
// (Especially with ep_page_view)
// so use focusNode.offsetTop value.
if (caretOffsetTop === -1) caretOffsetTop = myselection.focusNode.offsetTop;
// set the scrollY offset of the viewport on the document
scroll.setScrollY(caretOffsetTop);
}, 200);
}
Expand Down
60 changes: 60 additions & 0 deletions src/tests/frontend-new/specs/page_up_down.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,66 @@ test.describe('Page Up / Page Down', function () {
expect(selection).toBeLessThan(50);
});

// Regression test for #4562: consecutive very long wrapped lines should not
// cause PageDown/PageUp to skip too many or too few logical lines. The
// pixel-based calculation must account for lines that occupy far more visual
// rows than the viewport height.
test('PageDown with consecutive long wrapped lines moves by correct amount (#4562)', async function ({page}) {
const padBody = await getPadBody(page);
await clearPadContent(page);

// Build a pad with long lines interspersed with short ones via the inner
// document directly to avoid slow keyboard.type on Firefox.
const longLine = 'word '.repeat(300);
const innerFrame = page.frame('ace_inner')!;
await innerFrame.evaluate((text: string) => {
const body = document.getElementById('innerdocbody')!;
body.innerHTML = '';
for (let i = 0; i < 6; i++) {
const longDiv = document.createElement('div');
longDiv.textContent = text;
body.appendChild(longDiv);
const shortDiv = document.createElement('div');
shortDiv.textContent = `short ${i}`;
body.appendChild(shortDiv);
}
}, longLine);
// Wait for Etherpad to process the DOM changes
await page.waitForTimeout(2000);

// Move caret to the very top
await page.keyboard.down('Control');
await page.keyboard.press('Home');
await page.keyboard.up('Control');
await page.waitForTimeout(200);

// Press PageDown twice and verify caret advances each time
const getCaretLine = async () => {
const innerFrame = page.frame('ace_inner')!;
return innerFrame.evaluate(() => {
const sel = document.getSelection();
if (!sel || !sel.focusNode) return -1;
let node = sel.focusNode as HTMLElement;
while (node && node.tagName !== 'DIV') node = node.parentElement!;
if (!node) return -1;
const divs = Array.from(document.getElementById('innerdocbody')!.children);
return divs.indexOf(node);
});
};

const lineBefore = await getCaretLine();

await page.keyboard.press('PageDown');
await page.waitForTimeout(1000);
const lineAfterFirst = await getCaretLine();
expect(lineAfterFirst).toBeGreaterThan(lineBefore);

await page.keyboard.press('PageDown');
await page.waitForTimeout(1000);
const lineAfterSecond = await getCaretLine();
expect(lineAfterSecond).toBeGreaterThan(lineAfterFirst);
});

test('PageDown then PageUp returns to approximately same position', async function ({page}) {
const padBody = await getPadBody(page);
await clearPadContent(page);
Expand Down
Loading