mirror of
https://github.com/ether/etherpad-lite.git
synced 2026-07-17 16:47:05 +00:00
* fix(pad): show saved-revision markers in in-pad history mode (#7946) When #7659 moved the timeslider into the pad as an embedded iframe, the user-facing control became the outer #history-slider-input range input. The saved-revision stars are still drawn by broadcast_slider.ts into the iframe's #ui-slider-bar, but that DOM is hidden in embed mode, and pad_mode.ts bridged rev/max/value/timer/authors to the outer slider but never the saved revisions. Result: clicking "Save Revision" appeared to work but no markers showed in the timeslider (regression in 3.3.x). Bridge the embedded timeslider's clientVars.savedRevisions onto the outer slider as percentage-positioned star markers, rendered on first sync and re-rendered when the slider max changes. Markers are a purely visual, aria-hidden overlay (keyboard/SR users already reach any revision via the slider + step buttons) with a click-to-seek convenience for mouse users. Adds a frontend-new Playwright spec exercising the real user flow (save a revision in the pad, enter in-pad history mode, assert a visible marker on the outer slider). The only prior coverage lived in the legacy mocha suite (src/tests/frontend/specs/timeslider_revisions.js) which no CI workflow runs, and the modern timeslider specs drive the ?embed=1 iframe directly and never exercise the outer history UI — so this regression was invisible to CI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(pad): live saved-revision markers + Qodo review fixes (#7946) Address Qodo review on #7948: - Live updates (bug 1): the server's SAVE_REVISION handler never broadcast NEW_SAVEDREV, so the client handler that adds a star was dead and no open timeslider ever updated live. Wire it up: Pad.addSavedRevision returns the new revision (undefined on duplicate); handleSaveRevisionMessage broadcasts NEW_SAVEDREV to the pad room. pad_mode.ts now sources outer markers from the embedded slider's live #ui-slider-bar .star DOM (labels from clientVars) and observes that bar so a revision saved by a collaborator appears on an already-open history slider. Live editors ignore the unknown message type. - Single-revision pad (bug 2): allow max === 0 so a revision saved at rev 0 still renders a marker instead of being cleared by the old max <= 0 guard. - Test rigor (bug 3): assert the marker's inline left percentage directly instead of falling back to a layout coordinate, which let left:0% pass. Adds a two-client Playwright test for the live path (verified it fails with the server broadcast removed). Backend Pad + pad API specs (88) still pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
John McLear
GitHub
parent
8b6a0a2f3a
commit
a65e3b75a4
6 changed files with 257 additions and 5 deletions
|
|
@ -868,6 +868,8 @@ class Pad {
|
|||
await this.saveToDatabase();
|
||||
}
|
||||
|
||||
// Returns the newly created saved revision, or undefined if this revision
|
||||
// was already saved (so callers can broadcast only genuine additions).
|
||||
async addSavedRevision(revNum: string, savedById: string, label: string) {
|
||||
// if this revision is already saved, return silently
|
||||
for (const i in this.savedRevisions) {
|
||||
|
|
@ -887,6 +889,7 @@ class Pad {
|
|||
// save this new saved revision
|
||||
this.savedRevisions.push(savedRevision);
|
||||
await this.saveToDatabase();
|
||||
return savedRevision;
|
||||
}
|
||||
|
||||
getSavedRevisions() {
|
||||
|
|
|
|||
|
|
@ -622,7 +622,18 @@ exports.handleMessage = async (socket:any, message: ClientVarMessage) => {
|
|||
const handleSaveRevisionMessage = async (socket:any, message: ClientSaveRevisionMessage) => {
|
||||
const {padId, author: authorId} = sessioninfos[socket.id];
|
||||
const pad = await padManager.getPad(padId, null, authorId);
|
||||
await pad.addSavedRevision(pad.head, authorId);
|
||||
const savedRevision = await pad.addSavedRevision(pad.head, authorId);
|
||||
// Notify every client in the pad room — including any open timeslider —
|
||||
// so saved-revision markers appear live instead of only on the next
|
||||
// timeslider load (#7946). The client's NEW_SAVEDREV handler existed but
|
||||
// was never reached because this broadcast was missing; live editors that
|
||||
// don't handle the type ignore it. Skip the emit for duplicate saves.
|
||||
if (savedRevision) {
|
||||
socketio.sockets.in(padId).emit('message', {
|
||||
type: 'COLLABROOM',
|
||||
data: {type: 'NEW_SAVEDREV', savedRev: savedRevision},
|
||||
});
|
||||
}
|
||||
};
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -214,12 +214,52 @@ body.history-mode #history-controls { display: flex; }
|
|||
.history-controls button.buttonicon.buttonicon-play.pause::before {
|
||||
content: "\e829";
|
||||
}
|
||||
.history-slider-input {
|
||||
.history-slider-wrap {
|
||||
position: relative;
|
||||
flex: 1 1 auto;
|
||||
min-width: 80px;
|
||||
margin: 0 6px;
|
||||
display: flex;
|
||||
align-items: center;
|
||||
}
|
||||
.history-slider-input {
|
||||
flex: 1 1 auto;
|
||||
min-width: 0;
|
||||
width: 100%;
|
||||
cursor: pointer;
|
||||
}
|
||||
/* Saved-revision markers overlaid on the slider track (issue #7946). Inset
|
||||
* left/right by ~half the native range thumb so a marker lines up with the
|
||||
* thumb centre at the track extremes. pointer-events:none on the layer keeps
|
||||
* slider dragging unobstructed; the stars themselves re-enable clicks to seek. */
|
||||
.history-slider-stars {
|
||||
position: absolute;
|
||||
left: 8px;
|
||||
right: 8px;
|
||||
top: 0;
|
||||
bottom: 0;
|
||||
pointer-events: none;
|
||||
}
|
||||
.history-slider-stars .history-star {
|
||||
position: absolute;
|
||||
top: 50%;
|
||||
transform: translate(-50%, -50%);
|
||||
width: 16px;
|
||||
height: 16px;
|
||||
padding: 0;
|
||||
margin: 0;
|
||||
border: 0;
|
||||
background: none;
|
||||
line-height: 1;
|
||||
cursor: pointer;
|
||||
pointer-events: auto;
|
||||
}
|
||||
.history-slider-stars .history-star::before {
|
||||
font-family: fontawesome-etherpad;
|
||||
content: "\e856";
|
||||
color: #da9700;
|
||||
font-size: 14px;
|
||||
}
|
||||
.history-timer {
|
||||
flex: 0 0 auto;
|
||||
font-size: 12px;
|
||||
|
|
@ -282,7 +322,7 @@ body.history-mode #history-controls { display: flex; }
|
|||
@media (max-width: 800px) {
|
||||
.history-controls { padding: 0 6px; gap: 4px; min-height: 36px; }
|
||||
.history-controls button.buttonicon { padding: 4px 6px; min-width: 32px; }
|
||||
.history-slider-input { min-width: 60px; margin: 0 2px; }
|
||||
.history-slider-wrap { min-width: 60px; margin: 0 2px; }
|
||||
}
|
||||
@media (max-width: 480px) {
|
||||
.history-controls #history-leftstep,
|
||||
|
|
|
|||
|
|
@ -55,6 +55,10 @@ class PadModeController {
|
|||
private padId: string;
|
||||
private innerHashChangeHandler: (() => void) | null = null;
|
||||
private revObserver: MutationObserver | null = null;
|
||||
// Watches the embedded slider's #ui-slider-bar so saved revisions added live
|
||||
// (NEW_SAVEDREV from a collaborator while we're in history mode) get mirrored
|
||||
// onto the outer slider — clientVars only carries the entry-time snapshot.
|
||||
private savedRevObserver: MutationObserver | null = null;
|
||||
private syncingHash = false;
|
||||
|
||||
// History-mode bridges — populated on enter, torn down on exit.
|
||||
|
|
@ -194,6 +198,11 @@ class PadModeController {
|
|||
}
|
||||
this.revLabel.textContent = '';
|
||||
this.dateLabel.textContent = '';
|
||||
const stars = document.getElementById('history-slider-stars');
|
||||
if (stars) {
|
||||
stars.replaceChildren();
|
||||
stars.dataset.sig = '';
|
||||
}
|
||||
}
|
||||
|
||||
// Restore everything entry-time we stashed: chat message visibility, the
|
||||
|
|
@ -248,6 +257,10 @@ class PadModeController {
|
|||
this.revObserver.disconnect();
|
||||
this.revObserver = null;
|
||||
}
|
||||
if (this.savedRevObserver) {
|
||||
this.savedRevObserver.disconnect();
|
||||
this.savedRevObserver = null;
|
||||
}
|
||||
if (this.iframe) {
|
||||
try {
|
||||
if (this.innerHashChangeHandler && this.iframe.contentWindow) {
|
||||
|
|
@ -374,6 +387,10 @@ class PadModeController {
|
|||
playBtn.classList.toggle('pause', playing);
|
||||
playBtn.setAttribute('aria-pressed', playing ? 'true' : 'false');
|
||||
}
|
||||
// Saved-revision markers depend on the slider max, which is only known
|
||||
// once the inner slider has reported its length — render them here so we
|
||||
// pick up the correct positions on first sync and on any max change.
|
||||
this.renderSavedRevisionStars(innerWin);
|
||||
};
|
||||
// The hook registered earlier in attachInnerBridges already calls
|
||||
// onRevChange — piggyback on it for slider input/timer updates by
|
||||
|
|
@ -386,10 +403,87 @@ class PadModeController {
|
|||
}
|
||||
BS.onSlider(sync);
|
||||
sync(BS.getSliderPosition?.() ?? 0);
|
||||
// Now that the inner slider exists, watch it for live NEW_SAVEDREV stars.
|
||||
this.observeInnerSavedRevisions(innerWin);
|
||||
};
|
||||
registerSync();
|
||||
}
|
||||
|
||||
// Mirror the embedded timeslider's saved revisions onto the outer slider as
|
||||
// clickable star markers (issue #7946). The inner slider draws its own stars
|
||||
// on #ui-slider-bar, but that DOM is hidden in embed mode, so users only see
|
||||
// the outer #history-slider-input — which had no markers.
|
||||
//
|
||||
// The inner #ui-slider-bar .star elements are the live source of truth: the
|
||||
// timeslider keeps them current as NEW_SAVEDREV messages arrive (each carries
|
||||
// a `pos` attribute = revNum), whereas clientVars.savedRevisions is only the
|
||||
// entry-time snapshot. We read positions from those stars and pull labels
|
||||
// from the snapshot where available. A signature guard keeps this cheap when
|
||||
// sync() fires on every scrub; positions are percentage-based so they reflow
|
||||
// on resize for free.
|
||||
private renderSavedRevisionStars(innerWin: Window): void {
|
||||
const inner: any = innerWin as any;
|
||||
const layer = document.getElementById('history-slider-stars');
|
||||
const sliderInput = document.getElementById('history-slider-input') as HTMLInputElement | null;
|
||||
if (!layer || !sliderInput || !innerWin.document) return;
|
||||
|
||||
const max = Number(sliderInput.max) || 0;
|
||||
const revNums = Array.from(innerWin.document.querySelectorAll('#ui-slider-bar .star'))
|
||||
.map((el) => Number(el.getAttribute('pos')))
|
||||
// max === 0 is a valid single-revision pad: only rev 0 belongs there.
|
||||
.filter((n) => Number.isFinite(n) && n >= 0 && (max === 0 ? n === 0 : n <= max));
|
||||
|
||||
if (revNums.length === 0 || max < 0) {
|
||||
if (layer.childElementCount) layer.replaceChildren();
|
||||
layer.dataset.sig = '';
|
||||
return;
|
||||
}
|
||||
|
||||
// Labels live in the clientVars snapshot, keyed by revNum.
|
||||
const labels = new Map<number, string>();
|
||||
const snapshot = inner.clientVars?.savedRevisions;
|
||||
if (Array.isArray(snapshot)) {
|
||||
for (const r of snapshot) {
|
||||
const n = Number(r && r.revNum);
|
||||
if (Number.isFinite(n) && r && typeof r.label === 'string' && r.label) labels.set(n, r.label);
|
||||
}
|
||||
}
|
||||
|
||||
const sig = `${max}:${[...revNums].sort((a, b) => a - b).join(',')}`;
|
||||
if (layer.dataset.sig === sig) return;
|
||||
layer.dataset.sig = sig;
|
||||
layer.replaceChildren();
|
||||
|
||||
for (const revNum of revNums) {
|
||||
const frac = max === 0 ? 0 : revNum / max;
|
||||
// A purely visual marker (the layer is aria-hidden): keyboard/screen
|
||||
// reader users already reach any revision via the slider and step
|
||||
// buttons, so we mirror the legacy timeslider's mouse-only stars rather
|
||||
// than inject extra tab stops. The hover title aids mouse users; the
|
||||
// click is a convenience to jump straight to the saved point.
|
||||
const star = document.createElement('span');
|
||||
star.className = 'history-star';
|
||||
star.style.left = `${(frac * 100).toFixed(4)}%`;
|
||||
star.title = labels.get(revNum) || `Revision ${revNum}`;
|
||||
star.addEventListener('click', () => {
|
||||
try { inner.BroadcastSlider?.setSliderPosition?.(revNum); } catch (_e) { /* inner gone */ }
|
||||
});
|
||||
layer.appendChild(star);
|
||||
}
|
||||
}
|
||||
|
||||
// Re-render the outer markers whenever the embedded slider adds a star
|
||||
// (NEW_SAVEDREV). Observing the inner #ui-slider-bar covers saved revisions
|
||||
// created live while history mode is open, which sync()'s scrub-driven
|
||||
// callback would otherwise miss until the next slider move.
|
||||
private observeInnerSavedRevisions(innerWin: Window): void {
|
||||
if (this.savedRevObserver) return;
|
||||
const bar = innerWin.document && innerWin.document.getElementById('ui-slider-bar');
|
||||
if (!bar) return;
|
||||
this.savedRevObserver = new MutationObserver(() => { this.renderSavedRevisionStars(innerWin); });
|
||||
this.savedRevObserver.observe(bar, {childList: true});
|
||||
}
|
||||
|
||||
// Capture the live state we'll restore on exit: live chat message
|
||||
// visibility (just the timestamps — actual messages stay), live users
|
||||
// panel HTML, and current Export hrefs.
|
||||
|
|
|
|||
|
|
@ -159,8 +159,15 @@
|
|||
</button>
|
||||
<button type="button" id="history-rightstep" class="buttonicon buttonicon-step-forward">
|
||||
</button>
|
||||
<input type="range" id="history-slider-input" class="history-slider-input"
|
||||
min="0" max="0" value="0" step="1">
|
||||
<!-- Slider + saved-revision markers. The stars overlay sits above the
|
||||
range track; pad_mode.ts fills it from the embedded timeslider's
|
||||
savedRevisions so explicit "Save Revision" points stay visible in
|
||||
in-pad history mode (issue #7946). -->
|
||||
<span class="history-slider-wrap">
|
||||
<input type="range" id="history-slider-input" class="history-slider-input"
|
||||
min="0" max="0" value="0" step="1">
|
||||
<span id="history-slider-stars" class="history-slider-stars" aria-hidden="true"></span>
|
||||
</span>
|
||||
<span id="history-timer" class="history-timer hide-for-mobile" aria-live="polite"></span>
|
||||
<!-- Follow toggle: an icon-only button. The eye is always rendered;
|
||||
the diagonal slash overlay is shown only when aria-pressed is
|
||||
|
|
|
|||
|
|
@ -0,0 +1,97 @@
|
|||
import {expect, Page, test} from "@playwright/test";
|
||||
import {clearPadContent, goToNewPad, goToPad, writeToPad} from "../helper/padHelper";
|
||||
|
||||
// Regression coverage for #7946: after #7659 moved the timeslider into the pad
|
||||
// as an embedded iframe, the user-facing control became the outer
|
||||
// #history-slider-input range input. Saved revisions ("Save Revision" button)
|
||||
// are still drawn as stars inside the now-hidden iframe slider, so they
|
||||
// stopped appearing for users. pad_mode.ts must bridge the saved revisions out
|
||||
// onto the outer slider as markers.
|
||||
test.describe('timeslider saved-revision markers', function () {
|
||||
test.describe.configure({mode: 'serial'});
|
||||
|
||||
test.beforeEach(async ({context}) => {
|
||||
await context.clearCookies();
|
||||
});
|
||||
|
||||
const enterHistoryMode = async (page: Page) => {
|
||||
await page.click('.buttonicon-history');
|
||||
await page.waitForSelector('#history-controls:not([hidden])', {state: 'visible'});
|
||||
await page.waitForSelector('#history-frame');
|
||||
};
|
||||
|
||||
test('a saved revision shows a marker on the outer history slider', async function ({page}) {
|
||||
await goToNewPad(page);
|
||||
await clearPadContent(page);
|
||||
|
||||
// Build a few revisions, save one partway through, then add more so the
|
||||
// saved marker lands in the middle of the slider (not under the thumb).
|
||||
await writeToPad(page, 'One ');
|
||||
await page.waitForTimeout(400);
|
||||
await writeToPad(page, 'Two ');
|
||||
await page.waitForTimeout(600);
|
||||
|
||||
// Save a revision at the current head.
|
||||
await page.click('.buttonicon-savedRevision');
|
||||
// The save confirmation gritter carries class `saved-revision`.
|
||||
await page.waitForSelector('.saved-revision', {state: 'visible'});
|
||||
|
||||
// Add more edits so head advances past the saved revision.
|
||||
await writeToPad(page, 'Three ');
|
||||
await page.waitForTimeout(400);
|
||||
await writeToPad(page, 'Four ');
|
||||
await page.waitForTimeout(800);
|
||||
|
||||
await enterHistoryMode(page);
|
||||
|
||||
// The outer slider must show at least one visible saved-revision marker.
|
||||
const marker = page.locator('.history-star');
|
||||
await expect(marker.first()).toBeVisible({timeout: 15000});
|
||||
expect(await marker.count()).toBeGreaterThanOrEqual(1);
|
||||
|
||||
// The marker must be positioned within the slider track, not collapsed to
|
||||
// the origin. Assert on the inline left percentage directly (markers are
|
||||
// always positioned via style.left = "N%"); a fallback to a layout-derived
|
||||
// coordinate would let a degenerate left:0% render still pass.
|
||||
const leftPct = await marker.first().evaluate(
|
||||
(el) => parseFloat((el as HTMLElement).style.left));
|
||||
expect(leftPct).toBeGreaterThan(0);
|
||||
expect(leftPct).toBeLessThan(100);
|
||||
});
|
||||
|
||||
test('a revision saved live appears on an already-open history slider', async function ({browser}) {
|
||||
// Reviewer (history viewer).
|
||||
const ctxA = await browser.newContext();
|
||||
const pageA = await ctxA.newPage();
|
||||
const padId = await goToNewPad(pageA);
|
||||
await clearPadContent(pageA);
|
||||
await writeToPad(pageA, 'Alpha Beta ');
|
||||
await pageA.waitForTimeout(600);
|
||||
await pageA.click('.buttonicon-savedRevision');
|
||||
await pageA.waitForSelector('.saved-revision', {state: 'visible'});
|
||||
await writeToPad(pageA, 'Gamma Delta ');
|
||||
await pageA.waitForTimeout(800);
|
||||
await enterHistoryMode(pageA);
|
||||
await expect(pageA.locator('.history-star').first()).toBeVisible({timeout: 15000});
|
||||
const before = await pageA.locator('.history-star').count();
|
||||
|
||||
// A second collaborator keeps editing the live pad and saves a revision
|
||||
// while pageA is sitting in history mode. The server must broadcast
|
||||
// NEW_SAVEDREV so pageA's open timeslider gains a marker without reloading.
|
||||
const ctxB = await browser.newContext();
|
||||
const pageB = await ctxB.newPage();
|
||||
await goToPad(pageB, padId);
|
||||
await writeToPad(pageB, 'Epsilon Zeta Eta ');
|
||||
await pageB.waitForTimeout(800);
|
||||
await pageB.click('.buttonicon-savedRevision');
|
||||
await pageB.waitForSelector('.saved-revision', {state: 'visible'});
|
||||
|
||||
await expect.poll(
|
||||
async () => await pageA.locator('.history-star').count(),
|
||||
{timeout: 20000})
|
||||
.toBeGreaterThan(before);
|
||||
|
||||
await ctxA.close();
|
||||
await ctxB.close();
|
||||
});
|
||||
});
|
||||
Loading…
Add table
Add a link
Reference in a new issue