From 623d422ddb36ca14cbcb7189db89425087cf3b43 Mon Sep 17 00:00:00 2001 From: Calixte Denizet Date: Thu, 18 Sep 2025 21:23:42 +0200 Subject: [PATCH] [Editor] Make sure the comment dialog is visible on the screen (bug 1989304) --- src/display/editor/annotation_editor_layer.js | 15 +- test/integration/comment_spec.mjs | 134 ++++++++++++++++++ test/integration/jasmine-boot.js | 1 + test/pdfs/.gitignore | 1 + test/pdfs/bug1989304.pdf | Bin 0 -> 10886 bytes web/app.js | 2 + web/comment_manager.css | 3 +- web/comment_manager.js | 52 ++++++- 8 files changed, 195 insertions(+), 13 deletions(-) create mode 100644 test/integration/comment_spec.mjs create mode 100755 test/pdfs/bug1989304.pdf diff --git a/src/display/editor/annotation_editor_layer.js b/src/display/editor/annotation_editor_layer.js index fe01b9156..1284187bd 100644 --- a/src/display/editor/annotation_editor_layer.js +++ b/src/display/editor/annotation_editor_layer.js @@ -193,11 +193,16 @@ class AnnotationEditorLayer { this.toggleAnnotationLayerPointerEvents(false); const { classList } = this.div; - for (const editorType of AnnotationEditorLayer.#editorTypes.values()) { - classList.toggle( - `${editorType._type}Editing`, - mode === editorType._editorType - ); + if (mode === AnnotationEditorType.POPUP) { + classList.toggle("commentEditing", true); + } else { + classList.toggle("commentEditing", false); + for (const editorType of AnnotationEditorLayer.#editorTypes.values()) { + classList.toggle( + `${editorType._type}Editing`, + mode === editorType._editorType + ); + } } this.div.hidden = false; } diff --git a/test/integration/comment_spec.mjs b/test/integration/comment_spec.mjs new file mode 100644 index 000000000..113d7543c --- /dev/null +++ b/test/integration/comment_spec.mjs @@ -0,0 +1,134 @@ +/* Copyright 2025 Mozilla Foundation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { + closePages, + getEditorSelector, + getRect, + getSpanRectFromText, + loadAndWait, + scrollIntoView, + switchToEditor, + waitAndClick, +} from "./test_utils.mjs"; + +const switchToHighlight = switchToEditor.bind(null, "Highlight"); + +describe("Comment", () => { + describe("Comment edit dialog must be visible in ltr", () => { + let pages; + + beforeEach(async () => { + pages = await loadAndWait( + "bug1989304.pdf", + ".annotationEditorLayer", + "page-width", + null, + { enableComment: true } + ); + }); + + afterEach(async () => { + await closePages(pages); + }); + + it("must set the comment dialog in the viewport (LTR)", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + await switchToHighlight(page); + + await scrollIntoView(page, ".textLayer span:last-of-type"); + const rect = await getSpanRectFromText(page, 1, "..."); + const x = rect.x + rect.width / 2; + const y = rect.y + rect.height / 2; + // Here and elsewhere, we add a small delay between press and release + // to make sure that a pointerup event is triggered after + // selectionchange. + // It works with a value of 1ms, but we use 100ms to be sure. + await page.mouse.click(x, y, { count: 2, delay: 100 }); + await page.waitForSelector(getEditorSelector(0)); + + const commentButtonSelector = `${getEditorSelector(0)} button.comment`; + await waitAndClick(page, commentButtonSelector); + + await page.waitForSelector("#commentManagerDialog", { + visible: true, + }); + const dialogRect = await getRect(page, "#commentManagerDialog"); + const viewport = await page.evaluate(() => ({ + width: document.documentElement.clientWidth, + height: document.documentElement.clientHeight, + })); + expect(dialogRect.x + dialogRect.width) + .withContext(`In ${browserName}`) + .toBeLessThanOrEqual(viewport.width); + expect(dialogRect.y + dialogRect.height) + .withContext(`In ${browserName}`) + .toBeLessThanOrEqual(viewport.height); + }) + ); + }); + }); + + describe("Comment edit dialog must be visible in rtl", () => { + let pages; + + beforeEach(async () => { + pages = await loadAndWait( + "bug1989304.pdf", + ".annotationEditorLayer", + "page-width", + null, + { enableComment: true, localeProperties: "ar" } + ); + }); + + afterEach(async () => { + await closePages(pages); + }); + + it("must set the comment dialog in the viewport (RTL)", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + await switchToHighlight(page); + + await scrollIntoView(page, ".textLayer span:nth-of-type(4)"); + const rect = await getSpanRectFromText(page, 1, "World"); + const x = rect.x + rect.width / 2; + const y = rect.y + rect.height / 2; + await page.mouse.click(x, y, { count: 2, delay: 100 }); + await page.waitForSelector(getEditorSelector(0)); + + const commentButtonSelector = `${getEditorSelector(0)} button.comment`; + await waitAndClick(page, commentButtonSelector); + + await page.waitForSelector("#commentManagerDialog", { + visible: true, + }); + const dialogRect = await getRect(page, "#commentManagerDialog"); + const viewport = await page.evaluate(() => ({ + height: document.documentElement.clientHeight, + })); + expect(dialogRect.x + dialogRect.width) + .withContext(`In ${browserName}`) + .toBeGreaterThanOrEqual(0); + expect(dialogRect.y + dialogRect.height) + .withContext(`In ${browserName}`) + .toBeLessThanOrEqual(viewport.height); + }) + ); + }); + }); +}); diff --git a/test/integration/jasmine-boot.js b/test/integration/jasmine-boot.js index 72a8412af..51590003c 100644 --- a/test/integration/jasmine-boot.js +++ b/test/integration/jasmine-boot.js @@ -30,6 +30,7 @@ async function runTests(results) { "annotation_spec.mjs", "autolinker_spec.mjs", "caret_browsing_spec.mjs", + "comment_spec.mjs", "copy_paste_spec.mjs", "document_properties_spec.mjs", "find_spec.mjs", diff --git a/test/pdfs/.gitignore b/test/pdfs/.gitignore index 781516bde..609bbd87a 100644 --- a/test/pdfs/.gitignore +++ b/test/pdfs/.gitignore @@ -744,3 +744,4 @@ !bug1980958.pdf !tracemonkey_annotation_on_page_8.pdf !issue20232.pdf +!bug1989304.pdf diff --git a/test/pdfs/bug1989304.pdf b/test/pdfs/bug1989304.pdf new file mode 100755 index 0000000000000000000000000000000000000000..c4aa51f62f6431543432c80c8990678d22ccc09c GIT binary patch literal 10886 zcmeHNcT`hZx2KD=5$QcN0R>VC2|_4Aq!WrLASEQBLokIZN|h=gN>ikZARMTm=CI z>I3WmFjWu$tfFF%LU9s&@S9QqfBkT>K_I_$=n`;5JOP6xd6Q`x(L^t*w+|Tr17b)7 zygQCUBmuw>6bh*4?nNPxfO=k73gJ5o&_LHt9j=1_BhYX#TotXN2Zw3vz*W!(RMCh7 z+Sbtnjq)a(>69VjLjzeIZ z4=zNSM>GR1fW`zLSBe|W)lF;szWE6QS`dj8noZvbENR#PpejJCKwkpzhy}p~c-W36 z1^}iZK&FrgSZ_`?w=rjD{4OUaXD36Z-5e;b@HzhE@#AdHVXUX*0j-t0Zr3n!e*tt{ z)0$`yVdRl+S4`f`qW2)?Yl2)8J(D1v8yDM7PBt2vKji(ul}y2sC;=pb3nv?hlTB9k zfSDdA+fS@H*`Tmr2yX`+015l)ohq0%rK$*^B>;-}gVjz><9H`03uiipIUA1RQ0^ET zL4BpWA&jPxdJ$cJMHd+Q%S|-V zheGh7kZA$_E!2$)tVZUd|y;Mk&Y<1c0c1R|9gS&G>gCY2y$8baMe&Y|=mh&`tX+XyJdl1dIS}u14B&+zdrCXAet? z_tx2bX5&fZImo-QZ=TyqvVTodQU(x}U#oRirx=6ExXqcidp{{*Xnif4?CO8rp)oOH zZ{epG0gcz^>&dTBeLc39tE(KCOT93ru7mw^Ry!^za%y~yZS;SYkV&AaXtXK81C z^g6`G+;4iR_*T$*z$X_f+^$#P)o%_Ld->n4tWLj}^7TVCJCyr~t83VuIIoc<8q#p% zMuz=F%>%RS-2TD0mZ)<__w+l8iS|C~){XH)1fp)O65yI&8?C_^U9DGZgQxPd8|yQ& z9ZL~`l}E4UXxM$3pF5*u>ySrY6n&@b(3IiO2lNqNM>+PSvoHA796wU}N&tG15LZhZy!2Fl;91mP|2-9%$QRBl8CfshFFEi6La{wMrA7n34yY?WkOs@RVVEoF= zV8=>_@l4l1kl%^}L1Sfp+GCH2!iL9*8i#9pB+efQOvi8Z7d?i;g>tez8X7t?%K1+M zQ>ju5t(6dAC^4~5?Bz%SFy~R0i2ZOV>2qPG=*c08h|Vv#a_@;WmzUT13+eWQSUzNt zP;Bb8s`%Re?iaVtRYcVG+dBnIwLC>|8KpZNZY?>&@un&c+|n1$#y#ssX6c8i5}iB_ zbWB;%V1^{QdXqmKNeVd}mb?DpJYtH;jq%(9IXj9I{YPP@KE2;@pKKC;=} zIZ(M*#4m`*>v^aMaP){1oxC7#e6B^Mn>m+ir;{|uCO73)$px=B6KC6tZo5nlG;cVj zwl+Dud84Nqf^>XjkzUOwJsjtDrS&sl^7DP+d%IwQ;56~uM}#BtuwKlkIrSbyzs`&9 z(_+azb6;z97o&TEjFW?Kksw1>O3*ONwOdY;(i}*L)6LG0D_+Hx2;MjOMaOsr={4Bc zmO8YxSi}0lL}VZF>N*_+hCS@O%k^Y5{9;$EJH545SFU#;mU7Ago24M9?lcHhRBkVY z-iS$c1}nT&JJETH9HSKmmdQ8pK3HH-!x^?H6yF~d%-i+Z)JrS;(hgnrplo?=?&q$; zcISfh81Lw^l(W)@B=e^zKddNJDzGUYO=l!`Au=I1!|4*2>B|fH^8H!t zW6BU$x&Bs=;e&SSnPii&i<9Bl)F&LLn3HzBX3x$G$iYYrRoQc@2!dAhTCEL(Fc$%E z>YOICbS#?JHw^oTt@vm(#yxIFE>sM0(2eL` zmLMc}xbao>pD}ML>Zpuyhp9_l=w;y$xw-4#_NUJdj2JbaO^C7P1O*rh^AH?fGJLlrnBa%IVt#n%xOe ztADTAfRV62?`a`+W~3OXxe+NBD>_$}l&0X_X4E5`k24PeDXS)2K||`^T}e7E^EC3* z5YB$6_FQUMjiQ&x-R{-bDa5m(u}>1-#+NO#OvtR1?3j)odG%}~Nk*&eIR36r@}Qqo zh>Dr!*9FNPweOK9QzcYGlgSr=(I59ad3I4@PAB;8pP~St*CPFSo7VcXo<&b>(92w^ z0jxc=HoVi=mBLNeSnxh+Se{T`Gh-^HEf>qRo3Fa`Oh4b8QxH*?;;sRI(}|wxV!gS0 z=get+V(jJL0K=F9LJ4lMfN`zL+dHQ-R(Yoc#ZRDiVT8X1F7-rz2&muXkA33$y!uhu z$z&nh(7|_K=O5CYPz}#gH@>_Ku9n;%5x|#?Pfn9E)dqvq>sSg(2U=RB+hBaQ;Q}n# z;e7fMDO2!=ajlW(FV9q*ihLGp$$D6t$OB27dl6%7-?LN}W~61>TbY$uIKz~eVz}5P zlY;-MJzd5GHE6knK4v;odOdlg#Y!aW?)Xb4(mk+l(a|m+uQzzEw|+X)&lnPKvClT* zp7>pPa}UAN-NcS=i2lU!N@!7}L+tJ7;cFqCVuQmN0cT*)QDFX##_OiU@g{@&%&MQ1 zVQ$A`YrC}!a-7GtE%vHcOA5J~ea+7jUr^7xGu_=eG_QN@i#9__0ACvuj}G&zw#;yt zO5__y67zxlHkH+&)x^dkdVM~jbsknN!-exP)z&c_CCUvo*T|LU8UoW|OAdPF%f#wR zO`acYe8!sWYehmEb{6slzIXm)ka~N0I;eNjK8-GZKDlzeRbTcaEd6|eEbE1{1rj7~ zr+S~WxiSSt=zSn(7QVYOpls68>xgu#dW%h}?OzjBF}OOj*`_<4i1%((&Js6hF& z<>ODI&WUuSPn@PXRiH_ZtL3=yNn;GbTlnh3|K|o8YGi5U|L>uUC=CGSd zA_%=zvb|c zjx}%*N?I|YL&sfu&Gw0zeYBaE)#diR%#JYQcXr102$*0(3>rjV8nY9;b8(+pkKJNe z3aZ#b{*Y}!r`sxH8FWVF%~5GSaS@T)xOQJny=Cx&q_^vW*W=5M>lnP!&PNP@X$)8=BNIWJT zZtu8>`#!9O$1vm8b-!E+y&q~s4`eQ7_M!hgH0m_%^YI8rL#wMtTF>QWk1v@F85w~V zaTCex9op%k+^eWm5-}NvO4Qawi{cQSgeqg_Q=zI{jLR6ev_-1_!je$Xr%xQKjn8~= zIY$}n-mqls5D3_HvXkjcOLNb8w^qi|Qnk2`QQ>a!D&2-}#?9#%9TyT#N}mLw*2Q>v zt~V_C*~=04lq~{|p5l+xkA5d7B69bXUeaXMJK&_t?4gwc4FPE#R$ief`E!kPm=RtD zp|bh5CdntmX2B8T6GFv{6XYKAB2$l>E;pfC%e8ECMEAaD{07Tu5%TfWTd9TEB!Nhz zQlBf6)jEg6(1kkKy<4D*{Clje4o7wAL@tY7WpJI9>r1h0D=}pcF&wj)Qanb8m-)E& zZNLV^>+>f>^RUpD<0#b*e%s}b8Ta_nxo$A_f>xe%+?BA;`Fn)bQPWo(4la+;zg2`c zuPFdy2%|Z>e1FS25ZrQOuN_R_!^iNYEdD!|;epZnzt)GHJAUWJp>=N4#g(;SwwmXY z+ILY zzoxV;9MNLGT>56;y|YG>gXS*$wtLl-*I9k^qXyhTdWC#0`P345za4S7$+Z_G*>jbp zUQ>~8v+br1JcQK~+-pAHPR#Gc$$x>K1E|S~hMD7@#R**8m6&_qpgM&2K-JuxiVsE^ zPpOxeVn&dl|;AHiBv#$d#3_F_~b~5&x zWM^js2G%~Fp?q$8pI{r<@X$jl>2lHIqOK2^wg%ZT`Gklz68%&SN z(MFV&?zXln3(Esdhi*^N6FO?8Z+c5f*Au8ucq4%lGPX)Pt*hvSH2?!PFY!j>5j>oU zNpA^0b+h`{kHfIe8@@&Q;cRcz=SAWJ`!BClccKx_~oPlhWdAWN0;gJT2&u6>JYn=#8Xhs zwQRS-(g^QO;!JX)v@su7W=BW{`WJVMeVtjdT%5?wDVnpx&^M<~^d|4{6PQ)~P!+@0 zD!_lRCR}S+>sZhE+Y6-Ij1yV#TJ?2BV3vet|=q3N&(flL&DHAXfM(BhVC_~i)*=F-lXoL`f{!igL_25 ze&IzP8(}rMz~{^MJT^`|#iMq@-Cb3K?XuEU2ceH0j9s|A5OU8Sr;Som1=R+F=lVB> z6}$^0*N{N`)C3>8b-XmU%3AN(8$3IQw~xCRLP7U*4qX+Fyl`SpggS8`g#EHWYf49g z^(v8`g>D#h&T+dxXvBZ>2MvL3Id{{KJ(_Id(GJ=bwWQXec=kXtF6(|cH*a*Pz($_W zZvZ3Q;ax^LX#!0lcP#Yt7z3{-m4Y{uk(rZo24S4Sj| zPEu=9ORxMvbJxx=x#7HrL#oF#lBbCio(rsm2^?2JR_Cx$l=f3@8+**o;&Q9-yoX;` z4_Plnte(MMP|k6`A%8`#(wu5aK@o1?J$7G=Gag=gU*VmLZjo-X)ybWju~qoQ|LDkZ z(V_jn6+RVT<8z{`^Y(wW9eSIGYX334fxuO^<}oz%d?Gf{Lv5mm2PAv#vd|HUJ#f_E*g4)Q4SaA{T!(`cZ~J*ZH&k4(PB8n z%(k|CKi;?8S$AA!KWy(J^$3^wyy{#7A~FFnns&ze#Z;s&uqX7YvGXC!nDZfy)2oj) ziC3xCM#_n~8x1hM9pfLyy7?JD zKb229d-0a;Yv22)X2}@aQ#(2wX>giO_+>IAo}N+KXEZKxE-T#8iELflt~5z>$b0M( z+Ix7^C#>!|Srd`JGgB z7KpJ5Qc#@RXkkS|GaZ<6k!US}0{iF`>jzx+l*a zY^I7|WX^V(dZ)1xQy0(j=wVIktH6s?tt$(aj5wc-MpB`;}z-JIeSiIa%;Q3yN?(MvE^!07kh4`D^EVT{u?f$T|qeB%T4PLpM{E* zYl*F7gFr~Zjj=DneebW=$LFu#eLoxWaw_;EE5X&J>#Tor#8qzf7dR8J?k*3FH_9z^ zS9ORD+W__Nar%CI4gS3b0fxY7r`OPbb$AW21YtmjL6ue&mKdfh8S&ep;M~T4jLD(A)V|xu(#Vs-BcAE6&SV zw;R2pLT`VzGO4ggcSGYUD7tvwyAHXh11}+wm8-3*?^h&O{QT%>K~QvA4!(_VTdG$- zwy01r67u^V>xhZ*cSn9!H-5!}x@Lec7UxNz0GtV~?mik)A8s~F0o?H#QioMcKqkIu zf}6X35Q$(JWNL*A^28zWQkt|A;sCV(Z(na(tppI@?d3yO3(%0l(yD6&HQN2=Fi;Bc zjfCQr zIjM{zdIJNnzCf@t2)G>+9``$sFO}r=%``j?Nbn+fZ`OloeBf>QXqcLq{LZ?qI&bf7 zIeoX_@drNI3z|A8@OJ<^D!VKoB($ zNFDem%3lC|lYyqy6KTcIKg&R+CaF~*_J4FEiQBejfLqKpO^t)qQS++>Srpe z;{-?pQXTl~)fW11v!j?i(Hx~YP9Mfo{A%o{u`+NK>tIII&gE{Y*|O&7wNZA+A5{}LI8jN ziTL(vb4C51ettRlf2igkC4URtf8hEDuD^xA-y;4ey8eOdZz1rvi2sSM|24SSer+;n zy~$FcvnJjyKU( z{Dp6J$P``X9~s{4W*b*wFW<(P|EAeVxLL)<9sG@8&2QnkaVl^pgz~4IcVd zLN@d63RpFr4jGN}W|XEfGEz-quB=OO-Z{5kXvKVIDu2E3_Uq(GWB;a=k&a=HUWqTu9+uAiTH&wcV;-_gDKpAmD`%<_6cofV4Qc{f@6yp~ XmRF)!#I}KDOWz_GOxs-l=v@3Cy~B5S literal 0 HcmV?d00001 diff --git a/web/app.js b/web/app.js index d3f752bd1..317994e2e 100644 --- a/web/app.js +++ b/web/app.js @@ -368,6 +368,7 @@ const PDFViewerApplication = { docBaseUrl: x => x, enableAltText: x => x === "true", enableAutoLinking: x => x === "true", + enableComment: x => x === "true", enableFakeMLManager: x => x === "true", enableGuessAltText: x => x === "true", enablePermissions: x => x === "true", @@ -380,6 +381,7 @@ const PDFViewerApplication = { forcePageColors: x => x === "true", pageColorsBackground: x => x, pageColorsForeground: x => x, + localeProperties: x => ({ lang: x }), }); } diff --git a/web/comment_manager.css b/web/comment_manager.css index 8dffbc714..b8323ba47 100644 --- a/web/comment_manager.css +++ b/web/comment_manager.css @@ -20,7 +20,8 @@ min-width: 200px; position: absolute; padding: 8px 16px 16px; - margin: 0; + margin-left: 0; + margin-top: 0; box-sizing: border-box; border-radius: 8px; diff --git a/web/comment_manager.js b/web/comment_manager.js index f0683eaf0..818b9c05b 100644 --- a/web/comment_manager.js +++ b/web/comment_manager.js @@ -46,7 +46,7 @@ class CommentManager { dateStyle: "long", }); this.dialogElement = commentDialog.dialog; - this.#dialog = new CommentDialog(commentDialog, overlayManager); + this.#dialog = new CommentDialog(commentDialog, overlayManager, ltr); this.#popup = new CommentPopup(dateFormat, ltr, this.dialogElement); this.#sidebar = new CommentSidebar( sidebar, @@ -572,15 +572,19 @@ class CommentDialog { #dialogY = 0; + #isLTR; + constructor( { dialog, toolbar, title, textInput, cancelButton, saveButton }, - overlayManager + overlayManager, + ltr ) { this.#dialog = dialog; this.#textInput = textInput; this.#overlayManager = overlayManager; this.#saveButton = saveButton; this.#title = title; + this.#isLTR = ltr; const finishBound = this.#finish.bind(this); dialog.addEventListener("close", finishBound); @@ -682,7 +686,38 @@ class CommentDialog { } this.#uiManager?.removeEditListeners(); this.#saveButton.disabled = true; - + const parentDimensions = options?.parentDimensions; + if (editor.hasDefaultPopupPosition()) { + const { dialogWidth, dialogHeight } = this._dialogDimensions; + if (parentDimensions) { + if ( + this.#isLTR && + posX + dialogWidth > + Math.min( + parentDimensions.x + parentDimensions.width, + window.innerWidth + ) + ) { + const buttonWidth = this.#editor.commentButtonWidth; + posX -= dialogWidth - buttonWidth * parentDimensions.width; + } else if (!this.#isLTR) { + const buttonWidth = + this.#editor.commentButtonWidth * parentDimensions.width; + if (posX - dialogWidth < Math.max(0, parentDimensions.x)) { + posX = Math.max(0, posX); + } else { + posX -= dialogWidth - buttonWidth; + } + } + } + const height = Math.max(dialogHeight, options?.height || 0); + if (posY + height > window.innerHeight) { + posY = window.innerHeight - height; + } + if (posY < 0) { + posY = 0; + } + } this.#setPosition(posX, posY); await this.#overlayManager.open(this.#dialog); @@ -694,14 +729,17 @@ class CommentDialog { this.#finish(); } - get _dialogWidth() { + get _dialogDimensions() { const dialog = this.#dialog; const { style } = dialog; style.opacity = "0"; style.display = "block"; - const width = dialog.getBoundingClientRect().width; + const { width, height } = dialog.getBoundingClientRect(); style.opacity = style.display = ""; - return shadow(this, "_dialogWidth", width); + return shadow(this, "_dialogDimensions", { + dialogWidth: width, + dialogHeight: height, + }); } #setPosition(x, y) { @@ -1021,7 +1059,7 @@ class CommentPopup { } } - #setPosition(x, y, correctPosition = true) { + #setPosition(x, y, correctPosition) { if (!correctPosition) { this.#editor.commentPopupPosition = [x, y]; } else {