Skip to content

Commit aa92810

Browse files
calixtemancalixteman
committed
Fix tooltips on thumbnails and checkbox (bug 2019714)
And fix an issue with some integration tests where the manage button was disabled (hence not in the tab cycle).
1 parent 45b0f8b commit aa92810

5 files changed

Lines changed: 58 additions & 40 deletions

File tree

‎l10n/en-US/viewer.ftl‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -192,8 +192,9 @@ pdfjs-additional-layers = Additional Layers
192192

193193
# Variables:
194194
# $page (Number) - the page number
195-
pdfjs-thumb-page-title =
196-
.title = Page { $page }
195+
# $total (Number) - the number of pages
196+
pdfjs-thumb-page-title1 =
197+
.title = Page { $page } of { $total }
197198
198199
# Variables:
199200
# $page (Number) - the page number
@@ -202,8 +203,8 @@ pdfjs-thumb-page-canvas =
202203
203204
# Variables:
204205
# $page (Number) - the page number
205-
pdfjs-thumb-page-checkbox =
206-
.aria-label = Select page { $page }
206+
pdfjs-thumb-page-checkbox1 =
207+
.title = Select page { $page }
207208
208209
## Find panel button title and messages
209210

@@ -690,11 +691,11 @@ pdfjs-editor-add-comment-button =
690691
## - layers.
691692
## The thumbnails view is used to edit the pdf: remove/insert pages, ...
692693

693-
pdfjs-toggle-views-manager-button =
694-
.title = Toggle Sidebar
694+
pdfjs-toggle-views-manager-button1 =
695+
.title = Manage pages
695696
pdfjs-toggle-views-manager-notification-button =
696697
.title = Toggle Sidebar (document contains thumbnails/outline/attachments/layers)
697-
pdfjs-toggle-views-manager-button-label = Toggle Sidebar
698+
pdfjs-toggle-views-manager-button1-label = Manage pages
698699
699700
pdfjs-views-manager-sidebar =
700701
.aria-label = Sidebar

‎test/integration/test_utils.mjs‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -262,7 +262,7 @@ function getAnnotationSelector(id) {
262262
}
263263

264264
function getThumbnailSelector(pageNumber) {
265-
return `.thumbnailImageContainer[data-l10n-args='{"page":${pageNumber}}']`;
265+
return `.thumbnailImageContainer[data-l10n-args^='{"page":${pageNumber}']`;
266266
}
267267

268268
async function getSpanRectFromText(page, pageNumber, text) {
@@ -645,6 +645,7 @@ function waitForEditorMovedInDOM(page) {
645645
}
646646

647647
async function scrollIntoView(page, selector) {
648+
await page.waitForSelector(selector, { visible: true });
648649
const handle = await page.evaluateHandle(
649650
sel => [
650651
new Promise(resolve => {
@@ -982,6 +983,9 @@ async function showViewsManager(page) {
982983
"#outerContainer:not(.viewsManagerMoving).viewsManagerOpen",
983984
{ visible: true }
984985
);
986+
await page.waitForSelector("#viewsManagerStatusActionButton:not(:disabled)", {
987+
visible: true,
988+
});
985989
}
986990

987991
// Unicode bidi isolation characters, Fluent adds these markers to the text.

‎test/integration/thumbnail_view_spec.mjs‎

Lines changed: 26 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -2,17 +2,15 @@ import {
22
awaitPromise,
33
closePages,
44
FSI,
5+
getThumbnailSelector,
56
kbFocusNext,
67
loadAndWait,
78
PDI,
89
showViewsManager,
910
} from "./test_utils.mjs";
1011

1112
function waitForThumbnailVisible(page, pageNum) {
12-
return page.waitForSelector(
13-
`.thumbnailImageContainer[data-l10n-args='{"page":${pageNum}}']`,
14-
{ visible: true }
15-
);
13+
return page.waitForSelector(getThumbnailSelector(pageNum), { visible: true });
1614
}
1715

1816
async function waitForMenu(page, buttonSelector, visible = true) {
@@ -56,6 +54,14 @@ describe("PDF Thumbnail View", () => {
5654
await page.waitForSelector(`${thumbSelector}[src^="blob:http:"]`, {
5755
visible: true,
5856
});
57+
58+
const title = await page.$eval(
59+
getThumbnailSelector(1),
60+
el => el.title
61+
);
62+
expect(title)
63+
.withContext(`In ${browserName}`)
64+
.toBe(`Page ${FSI}1${PDI} of ${FSI}14${PDI}`);
5965
})
6066
);
6167
});
@@ -110,7 +116,7 @@ describe("PDF Thumbnail View", () => {
110116

111117
for (const pageNum of [14, 1, 13, 2]) {
112118
await goToPage(page, pageNum);
113-
const thumbSelector = `.thumbnailImageContainer[data-l10n-args='{"page":${pageNum}}']`;
119+
const thumbSelector = getThumbnailSelector(pageNum);
114120
await page.waitForSelector(
115121
`.thumbnail ${thumbSelector}[aria-current="page"]`,
116122
{ visible: true }
@@ -158,26 +164,25 @@ describe("PDF Thumbnail View", () => {
158164

159165
await kbFocusNext(page);
160166
await page.waitForSelector(
161-
`#thumbnailsView .thumbnailImageContainer[data-l10n-args='{"page":1}']:focus`,
167+
`#thumbnailsView ${getThumbnailSelector(1)}:focus`,
162168
{ visible: true }
163169
);
164170

165171
await page.keyboard.press("ArrowDown");
166172
await page.waitForSelector(
167-
`#thumbnailsView .thumbnailImageContainer[data-l10n-args='{"page":2}']:focus`,
173+
`#thumbnailsView ${getThumbnailSelector(2)}:focus`,
168174
{ visible: true }
169175
);
170176

171177
await page.keyboard.press("ArrowUp");
172-
await page.waitForSelector(
173-
`#thumbnailsView .thumbnailImageContainer[data-l10n-args='{"page":1}']:focus`,
174-
{ visible: true }
175-
);
178+
await page.waitForSelector(`${getThumbnailSelector(1)}:focus`, {
179+
visible: true,
180+
});
176181

177182
await page.keyboard.press("ArrowDown");
178183
await page.keyboard.press("ArrowDown");
179184
await page.waitForSelector(
180-
`#thumbnailsView .thumbnailImageContainer[data-l10n-args='{"page":3}']:focus`,
185+
`#thumbnailsView ${getThumbnailSelector(3)}:focus`,
181186
{ visible: true }
182187
);
183188

@@ -190,13 +195,13 @@ describe("PDF Thumbnail View", () => {
190195

191196
await page.keyboard.press("End");
192197
await page.waitForSelector(
193-
`#thumbnailsView .thumbnailImageContainer[data-l10n-args='{"page":14}']:focus`,
198+
`#thumbnailsView ${getThumbnailSelector(14)}:focus`,
194199
{ visible: true }
195200
);
196201

197202
await page.keyboard.press("Home");
198203
await page.waitForSelector(
199-
`#thumbnailsView .thumbnailImageContainer[data-l10n-args='{"page":1}']:focus`,
204+
`#thumbnailsView ${getThumbnailSelector(1)}:focus`,
200205
{ visible: true }
201206
);
202207
})
@@ -322,17 +327,17 @@ describe("PDF Thumbnail View", () => {
322327
await closePages(pages);
323328
});
324329

325-
it("should have accessible label on checkbox", async () => {
330+
it("should have a title on the checkbox", async () => {
326331
await Promise.all(
327332
pages.map(async ([browserName, page]) => {
328333
await showViewsManager(page);
329334
await waitForThumbnailVisible(page, 1);
330335

331-
const ariaLabel = await page.$eval(
336+
const title = await page.$eval(
332337
`.thumbnail[page-number="1"] input[type="checkbox"]`,
333-
el => el.getAttribute("aria-label")
338+
el => el.title
334339
);
335-
expect(ariaLabel)
340+
expect(title)
336341
.withContext(`In ${browserName}`)
337342
.toBe(`Select page ${FSI}1${PDI}`);
338343
})
@@ -478,10 +483,9 @@ describe("PDF Thumbnail View", () => {
478483
await kbFocusNext(page);
479484

480485
// Verify we're on the first thumbnail
481-
await page.waitForSelector(
482-
`#thumbnailsView .thumbnailImageContainer[data-l10n-args='{"page":1}']:focus`,
483-
{ visible: true }
484-
);
486+
await page.waitForSelector(`${getThumbnailSelector(1)}:focus`, {
487+
visible: true,
488+
});
485489

486490
// Tab to checkbox
487491
await kbFocusNext(page);

‎web/pdf_thumbnail_view.js‎

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,8 @@ class PDFThumbnailView extends RenderableView {
131131
imageContainer.tabIndex = -1;
132132
imageContainer.draggable = false;
133133
imageContainer.setAttribute("page-number", id);
134+
imageContainer.setAttribute("data-l10n-id", "pdfjs-thumb-page-title1");
135+
imageContainer.setAttribute("data-l10n-args", this.#getPageL10nArgs(true));
134136

135137
const image = (this.image = document.createElement("img"));
136138
imageContainer.append(image);
@@ -139,8 +141,8 @@ class PDFThumbnailView extends RenderableView {
139141
const checkbox = (this.checkbox = document.createElement("input"));
140142
checkbox.type = "checkbox";
141143
checkbox.tabIndex = -1;
142-
checkbox.setAttribute("data-l10n-id", "pdfjs-thumb-page-checkbox");
143-
checkbox.setAttribute("data-l10n-args", this.#pageL10nArgs);
144+
checkbox.setAttribute("data-l10n-id", "pdfjs-thumb-page-checkbox1");
145+
checkbox.setAttribute("data-l10n-args", this.#getPageL10nArgs());
144146
thumbnailContainer.append(checkbox);
145147
this.pasteButton = null;
146148
}
@@ -331,8 +333,8 @@ class PDFThumbnailView extends RenderableView {
331333
reducedCanvas.toBlob(resolve);
332334
const blob = await promise;
333335
image.src = URL.createObjectURL(blob);
334-
imageContainer.setAttribute("data-l10n-id", "pdfjs-thumb-page-canvas");
335-
imageContainer.setAttribute("data-l10n-args", this.#pageL10nArgs);
336+
image.setAttribute("data-l10n-id", "pdfjs-thumb-page-canvas");
337+
image.setAttribute("data-l10n-args", this.#getPageL10nArgs());
336338
imageContainer.classList.remove("missingThumbnailImage");
337339
if (!FeatureTest.isOffscreenCanvasSupported) {
338340
// Clean up the canvas element since it is no longer needed.
@@ -525,17 +527,24 @@ class PDFThumbnailView extends RenderableView {
525527
return canvas;
526528
}
527529

528-
get #pageL10nArgs() {
529-
return JSON.stringify({ page: this.pageLabel ?? this.id });
530+
#getPageL10nArgs(hasTotal = false) {
531+
return JSON.stringify({
532+
page: this.pageLabel ?? this.id,
533+
total: hasTotal ? this.linkService.pagesCount : undefined,
534+
});
530535
}
531536

532537
/**
533538
* @param {string|null} label
534539
*/
535540
setPageLabel(label) {
536541
this.pageLabel = typeof label === "string" ? label : null;
537-
this.imageContainer.setAttribute("data-l10n-args", this.#pageL10nArgs);
538-
this.checkbox?.setAttribute("data-l10n-args", this.#pageL10nArgs);
542+
this.imageContainer.setAttribute(
543+
"data-l10n-args",
544+
this.#getPageL10nArgs(true)
545+
);
546+
this.image.setAttribute("data-l10n-args", this.#getPageL10nArgs());
547+
this.checkbox?.setAttribute("data-l10n-args", this.#getPageL10nArgs());
539548
}
540549
}
541550

‎web/viewer.html‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -111,12 +111,12 @@
111111
class="toolbarButton"
112112
type="button"
113113
tabindex="0"
114-
data-l10n-id="pdfjs-toggle-views-manager-button"
114+
data-l10n-id="pdfjs-toggle-views-manager-button1"
115115
aria-expanded="false"
116116
aria-haspopup="true"
117117
aria-controls="viewsManager"
118118
>
119-
<span data-l10n-id="pdfjs-toggle-views-manager-button-label"></span>
119+
<span data-l10n-id="pdfjs-toggle-views-manager-button1-label"></span>
120120
</button>
121121
<div
122122
id="viewsManager"

0 commit comments

Comments
 (0)