From 97868f0486e3cf0c075a9cb51207c672de14c1bd Mon Sep 17 00:00:00 2001 From: Huang Xin Date: Fri, 26 Jun 2026 21:25:21 +0800 Subject: [PATCH] fix(reader): keep negative table margins from clipping wrapped layout tables (#4439) (#4808) A decorative table-of-contents page lays out as nested tables where the inner table pulls itself up with a negative top margin and the CONTENTS heading uses line-height:1em. Since #4400 wraps every table in a `.scroll-wrapper` (overflow:auto), that negative margin bled the heading above the wrapper's clip box and overflow cut off the top half of its glyphs. Hoist any negative margins from the wrapped element onto the wrapper and zero them on the element: the box stays in place, the element sits flush inside it so overflow cannot clip it, and scrollWidth is no longer inflated by the margin so a table that actually fits still gets marked fit. Positive and auto margins are left alone, so an over-wide table still scrolls and a centered table stays centered. Co-authored-by: Claude Opus 4.8 (1M context) --- .../paginator-table-toc-clip.browser.test.ts | 142 ++++++++++++++++++ .../__tests__/fixtures/data/repro-4439.epub | Bin 0 -> 2856 bytes .../src/__tests__/utils/scrollable.test.ts | 36 +++++ apps/readest-app/src/utils/scrollable.ts | 31 ++++ 4 files changed, 209 insertions(+) create mode 100644 apps/readest-app/src/__tests__/document/paginator-table-toc-clip.browser.test.ts create mode 100644 apps/readest-app/src/__tests__/fixtures/data/repro-4439.epub diff --git a/apps/readest-app/src/__tests__/document/paginator-table-toc-clip.browser.test.ts b/apps/readest-app/src/__tests__/document/paginator-table-toc-clip.browser.test.ts new file mode 100644 index 00000000..e5a8bf22 --- /dev/null +++ b/apps/readest-app/src/__tests__/document/paginator-table-toc-clip.browser.test.ts @@ -0,0 +1,142 @@ +import { describe, it, expect, beforeAll, afterEach } from 'vitest'; +import { DocumentLoader } from '@/libs/document'; +import type { BookDoc } from '@/libs/document'; +import type { ViewSettings } from '@/types/book'; +import type { Renderer } from '@/types/view'; +import { getStyles } from '@/utils/style'; +import { applyScrollableStyle, SCROLL_WRAPPER_CLASS } from '@/utils/scrollable'; +import { + DEFAULT_BOOK_FONT, + DEFAULT_BOOK_LAYOUT, + DEFAULT_BOOK_STYLE, + DEFAULT_BOOK_LANGUAGE, + DEFAULT_VIEW_CONFIG, + DEFAULT_TTS_CONFIG, + DEFAULT_TRANSLATOR_CONFIG, + DEFAULT_ANNOTATOR_CONFIG, + DEFAULT_SCREEN_CONFIG, +} from '@/services/constants'; + +// repro-4439.epub: a decorative table-of-contents page laid out as nested +// tables. The inner contents table has a negative top margin and the CONTENTS +// heading uses line-height:1em. Wrapping the table in a `.scroll-wrapper` +// (#4400) turned it into an overflow:auto clip box, and the negative margin bled +// the top of the heading above that box, cutting the glyphs in half (#4439). +const TOC_EPUB_URL = new URL('../fixtures/data/repro-4439.epub', import.meta.url).href; +// The contents page is the second spine item (cover, contents, ch1). +const TOC_SECTION_INDEX = 1; +const TOLERANCE_PX = 2; + +const loadEPUB = async (url: string, name: string) => { + const resp = await fetch(url); + const buffer = await resp.arrayBuffer(); + const file = new File([buffer], name, { type: 'application/epub+zip' }); + const { book } = await new DocumentLoader(file).open(); + return book; +}; + +const makeViewSettings = (): ViewSettings => + ({ + ...DEFAULT_BOOK_FONT, + ...DEFAULT_BOOK_LAYOUT, + ...DEFAULT_BOOK_STYLE, + ...DEFAULT_BOOK_LANGUAGE, + ...DEFAULT_VIEW_CONFIG, + ...DEFAULT_TTS_CONFIG, + ...DEFAULT_TRANSLATOR_CONFIG, + ...DEFAULT_ANNOTATOR_CONFIG, + ...DEFAULT_SCREEN_CONFIG, + }) as ViewSettings; + +const waitForStabilized = (el: HTMLElement, timeout = 10000) => + new Promise((resolve, reject) => { + const timer = setTimeout(() => reject(new Error('stabilized timeout')), timeout); + el.addEventListener( + 'stabilized', + () => { + clearTimeout(timer); + resolve(); + }, + { once: true }, + ); + }); + +const getSectionDoc = (paginator: Renderer, index: number): Document | null => + paginator.getContents().find((c) => c.index === index)?.doc ?? null; + +// Nearest ancestor that clips its overflow (the scroll-wrapper, when scrollable). +const nearestClipAncestor = (el: HTMLElement): HTMLElement | null => { + let cur: HTMLElement | null = el.parentElement; + const win = el.ownerDocument.defaultView!; + while (cur) { + if (win.getComputedStyle(cur).overflow !== 'visible') return cur; + cur = cur.parentElement; + } + return null; +}; + +let tocBook: BookDoc; + +describe('Paginator decorative TOC table', () => { + let paginator: Renderer; + + beforeAll(async () => { + tocBook = await loadEPUB(TOC_EPUB_URL, 'repro-4439.epub'); + await import('foliate-js/paginator.js'); + }, 30000); + + afterEach(() => { + if (paginator) { + try { + paginator.destroy(); + } catch { + /* iframe body may already be torn down */ + } + paginator.remove(); + } + }); + + it('does not clip the top of a heading inside a wrapped layout table', async () => { + const el = document.createElement('foliate-paginator') as Renderer; + Object.assign(el.style, { + width: '320px', + height: '800px', + position: 'absolute', + left: '0', + top: '0', + }); + document.body.appendChild(el); + paginator = el; + + paginator.open(tocBook); + paginator.setStyles?.(getStyles(makeViewSettings(), undefined, [])); + + const stabilized = waitForStabilized(paginator); + await paginator.goTo({ index: TOC_SECTION_INDEX }); + await stabilized; + + const doc = getSectionDoc(paginator, TOC_SECTION_INDEX); + expect(doc, 'contents section doc').toBeTruthy(); + paginator.setStyles?.(getStyles(makeViewSettings(), undefined, [])); + applyScrollableStyle(doc!); + + const heading = [...doc!.querySelectorAll('p.title')].find((p) => + /CONTENTS/.test(p.textContent ?? ''), + ); + expect(heading, 'CONTENTS heading').toBeTruthy(); + + // It must sit inside a scroll-wrapper (otherwise the test would not be + // exercising the clip path that #4400 introduced). + expect(heading!.closest(`.${SCROLL_WRAPPER_CLASS}`), 'heading is wrapped').toBeTruthy(); + + const clip = nearestClipAncestor(heading!); + if (clip) { + const headTop = heading!.getBoundingClientRect().top; + const clipTop = clip.getBoundingClientRect().top; + expect( + headTop, + `CONTENTS top (${headTop.toFixed(1)}) is clipped above its overflow box top (${clipTop.toFixed(1)})`, + ).toBeGreaterThanOrEqual(clipTop - TOLERANCE_PX); + } + }, 60000); +}); diff --git a/apps/readest-app/src/__tests__/fixtures/data/repro-4439.epub b/apps/readest-app/src/__tests__/fixtures/data/repro-4439.epub new file mode 100644 index 0000000000000000000000000000000000000000..8bbd4efaa702e0c0a18a92b3d9e480016076ec8f GIT binary patch literal 2856 zcmZuz2T+sg77d96j0BKkp(BDwNdW0h(1cECvY;5Gg_?wjks_cJ86QX!LNC%==wOhd z5>Zi5Kha?#t7Dzp2v zTdY74JP0^)014+vBH=xp9myWvL?n)M)k*rA2g&?zB90Doadb3VOJ&pff5a7wPjQ_;GMMJnI#;mJd3iJm8<9qb94nS&?=`jWY2%QE3mdIqSO_v0N38> z{!ef5$?HtNwq)B7e!UN9g(lDz_&+TndpjeD&i?2)-7AkpfOhP_To+vvJvBy{H2n$1 zTI(#5IgU_K2sLth6&~3?`uSmlw{Vb3rdIO^1oIE>prE+cyKhYC~&7 zKZ|X=SdF&DTxfMH@VD_%swaO9sS8$D;iZ3g4(Cq>w%T{svE0c7yizQk z9oTBrVi3B{ubB(!M0+px4SX|6IXSt(9HHhnY8jnfKwC&K-mR z&q6|iyP+H!b{sd1T?J{O)?P7%Wf$vIT3FHq+A$U^@O=j9Q!OfSal$^0W&30rl?xN( zqry0uQc63F(%MUER;JXN3LuuYHnT>Bkrs{wE&6@JXihM{WY&zkrqE1Ml`e%R6Gs_( z1H)K}%ccx)wnVa^C_YRPvnxlKnX*UjO+szTDY~9);8*BqlN4Fi2G;qeS^3%^Tn)07 zGFeFvIWA?_%LJ;CIVfOBkLFG(U{w!_mMU7Y#E4Au-rQ-_=}AtLbyC#65u-VfADC@g zuysk;Me9@T8IAyzgrv^vl2=z|HcL)=m46mobF`bJ)h15&Wksxh1jFoSBeGhXEe=b_ zVBcL(QpjkP?rIb}HWVsc>Yqsxa_ilF)|#} zzht~Wj|9#?=)TBK&iG(hb3AwPG!s#4k~-1@Lrwy^&jdIo(sj)Ot4kyq{mL)gZN4 z|4SWLjF@0nw16OM#)ZN``;L^NP=1Nwum_>iqI}K|B*nLf^mDWEOEy~<7uM(R7o3he zF%IRZ4|LIh`Pm0`j8Td2{}ldpgW@I6Aj^Rvn+0k&GyHpl+~t4HnRu5m*JBYj=wz@N z-33`Q^Gu!Tun?WQPZ=g=3C$yTBm1#J6Yn^(Qf$nHyPlf8)}gNDsIvEdm}zUZTWA)` zB%H-aQ56qCFvqhv=K=;+bj>zG34>Sf;{gv_&^MkIuNJ&$ypH6DX=^8S2C3vI+2i0B zm-@F#ZdvN!*r6TXA5)nr<&nGl!>?n!-aOVr;2Y%Y8@c@{k@SaBp)X~gCTfzj11Fjd zBh8JOz&wy$CBM%@KmGu|LIBgtM{=ewK2xq() z_u$olH~fVEeN8e#884MRXcZ8AenJ_p7)A<4W8I@z*!MpQMw?~iVDzy)jy7fnj(@uv jBMsw(?$O8r0rz+FYHrL7Sm$p3Wd-d64vT&N?$dt(xoLP5 literal 0 HcmV?d00001 diff --git a/apps/readest-app/src/__tests__/utils/scrollable.test.ts b/apps/readest-app/src/__tests__/utils/scrollable.test.ts index 30f8664f..3a92ea3a 100644 --- a/apps/readest-app/src/__tests__/utils/scrollable.test.ts +++ b/apps/readest-app/src/__tests__/utils/scrollable.test.ts @@ -67,6 +67,42 @@ describe('applyScrollableStyle', () => { expect(table.parentElement?.classList.contains(SCROLL_WRAPPER_CLASS)).toBe(true); }); + it('hoists a negative table margin onto the wrapper so it is not clipped (#4439)', () => { + // A decorative TOC lays out as nested tables; the inner table pulls itself + // up with a negative top margin. Inside an overflow:auto wrapper that margin + // would bleed the table (and its heading) past the box edge and get clipped. + document.body.innerHTML = ` +
+
CONTENTS
+
+ `; + applyScrollableStyle(document); + const table = document.querySelector('table')!; + const wrapper = table.parentElement!; + expect(wrapper.classList.contains(SCROLL_WRAPPER_CLASS)).toBe(true); + // The negative top margin moves to the wrapper (box stays in place)... + expect(wrapper.style.marginTop).toBe('-1em'); + // ...and is zeroed on the table so it sits flush inside the clip box. + expect(table.style.marginTop).toBe('0px'); + // The positive left margin is left alone (still counts toward overflow). + expect(wrapper.style.marginLeft).toBe(''); + expect(table.style.marginLeft).toBe('1em'); + }); + + it('leaves a table with only non-negative margins untouched', () => { + document.body.innerHTML = ` +
+
Cell
+
+ `; + applyScrollableStyle(document); + const table = document.querySelector('table')!; + const wrapper = table.parentElement!; + expect(wrapper.style.marginTop).toBe(''); + expect(table.style.marginTop).toBe('1em'); + expect(table.style.marginLeft).toBe('auto'); + }); + it('wraps a display equation (math that is the sole content of its container)', () => { document.body.innerHTML = `
diff --git a/apps/readest-app/src/utils/scrollable.ts b/apps/readest-app/src/utils/scrollable.ts index 010a445a..181c16be 100644 --- a/apps/readest-app/src/utils/scrollable.ts +++ b/apps/readest-app/src/utils/scrollable.ts @@ -169,6 +169,7 @@ export const applyScrollableStyle = (document: Document) => { wrapper.setAttribute('cfi-skip', ''); parent.insertBefore(wrapper, el); wrapper.appendChild(el); + hoistNegativeMargins(el, wrapper, win); decideTableFit(wrapper, win); }; document.querySelectorAll('table').forEach(wrap); @@ -177,6 +178,36 @@ export const applyScrollableStyle = (document: Document) => { }); }; +/** + * Move any NEGATIVE margins from the wrapped element onto the wrapper, zeroing + * them on the element. The wrapper is an overflow:auto clip box; a negative + * margin on the element pulls its content past the box edge, where overflow + * clips it — a decorative TOC laid out with nested tables (`margin: -1em`) had + * the top half of its heading cut off this way (#4439). Hoisting keeps the box + * in the same place while letting the element sit flush inside it. Positive and + * auto margins are left alone, so a genuinely over-wide table still counts as + * overflowing and a centered table stays centered. + */ +const MARGIN_SIDES = ['Top', 'Right', 'Bottom', 'Left'] as const; +const hoistNegativeMargins = ( + el: Element, + wrapper: HTMLElement, + win: (Window & typeof globalThis) | null, +) => { + if (!win) return; + const style = (el as HTMLElement).style; + if (!style) return; + const computed = win.getComputedStyle(el); + for (const side of MARGIN_SIDES) { + const prop = `margin${side}` as 'marginTop'; + const value = computed[prop]; + if (parseFloat(value) < 0) { + wrapper.style[prop] = value; + style[prop] = '0px'; + } + } +}; + /** * Toggle SCROLL_WRAPPER_FIT_CLASS: a table within tolerance of its column fits * (overflow:visible — not a scroll container), a wider one stays overflow:auto.