ronantakizawa/codereview-bench huggingface dataset:
Repository: mozilla/pdf.js
File: test/integration/test_utils.mjs (JavaScript)

Code before:

// Unicode bidi isolation characters, Fluent adds these markers to the text.
const FSI = "\u2068";
const PDI = "\u2069";

export {
  applyFunctionToEditor,
  awaitPromise,
  clearEditors,
  clearInput,
  closePages,
  closeSinglePage,
  copy,
  copyToClipboard,
  createPromise,
  dragAndDrop,
  firstPageOnTop,
  FSI,
  getAnnotationSelector,
  getAnnotationStorage,
  getComputedStyleSelector,
  getEditorDimensions,
  getEditors,
  getEditorSelector,
  getFirstSerialized,
  getNextIdEditor,
  getQuerySelector,
  getRect,
  getSelector,
  getSerialized,
  getSpanRectFromText,
  getXY,
  highlightSpan,
  isCanvasMonochrome,
  kbBigMoveDown,
  kbBigMoveLeft,
  kbBigMoveRight,
  kbBigMoveUp,
  kbDeleteLastWord,
  kbFocusNext,
  kbFocusPrevious,
  kbGoToBegin,
  kbGoToEnd,
  kbModifierDown,
  kbModifierUp,
  kbRedo,
  kbSave,
  kbSelectAll,
  kbUndo,
  loadAndWait,
  mockClipboard,

Review comment (suggestion):
This doesn't seem to be used outside of this file, so let's not export it to keep it local to this file.

Code after:

// Unicode bidi isolation characters, Fluent adds these markers to the text.
const FSI = "\u2068";
const PDI = "\u2069";

export {
  applyFunctionToEditor,
  awaitPromise,
  clearEditors,
  clearInput,
  closePages,
  closeSinglePage,
  copy,
  copyToClipboard,
  createPromise,
  dragAndDrop,
  firstPageOnTop,
  FSI,
  getAnnotationSelector,
  getAnnotationStorage,
  getComputedStyleSelector,
  getEditorDimensions,
  getEditors,
  getEditorSelector,
  getFirstSerialized,
  getQuerySelector,
  getRect,
  getSelector,
  getSerialized,
  getSpanRectFromText,
  getXY,
  highlightSpan,
  isCanvasMonochrome,
  kbBigMoveDown,
  kbBigMoveLeft,
  kbBigMoveRight,
  kbBigMoveUp,
  kbDeleteLastWord,
  kbFocusNext,
  kbFocusPrevious,
  kbGoToBegin,
  kbGoToEnd,
  kbModifierDown,
  kbModifierUp,
  kbRedo,
  kbSave,
  kbSelectAll,
  kbUndo,
  loadAndWait,
  mockClipboard,
  moveEditor,
Repository: mozilla/pdf.js
File: src/core/catalog.js (JavaScript)

Code before:
    }
    return shadow(this, "markInfo", markInfo);
  }

  #readMarkInfo() {
    const obj = this.#catDict.get("MarkInfo");
    if (!(obj instanceof Dict)) {
      return null;
    }

    const markInfo = {
      Marked: false,
      UserProperties: false,
      Suspects: false,
    };
    for (const key in markInfo) {
      const value = obj.get(key);
      if (typeof value === "boolean") {
        markInfo[key] = value;
      }
    }

    return markInfo;
  }

  hasStructTree() {
    return this.#catDict.has("StructTreeRoot");
  }

  get structTreeRoot() {
    let structTree = null;
    try {
      structTree = this.#readStructTreeRoot();
    } catch (ex) {
      if (ex instanceof MissingDataException) {
        throw ex;
      }
      warn("Unable read to structTreeRoot info.");
    }
    return shadow(this, "structTreeRoot", structTree);
  }

  #readStructTreeRoot() {
    const rawObj = this.#catDict.getRaw("StructTreeRoot");
    const obj = this.xref.fetchIfRef(rawObj);
    if (!(obj instanceof Dict)) {
      return null;
    }

    const root = new StructTreeRoot(this.xref, obj, rawObj);
    root.init();

Review comment (suggestion):
If `ensureCatalog` is used in the other comment, this should probably become `get hasStructTree` (which we may also want to shadow afterwards).

Code after:
    }
    return shadow(this, "markInfo", markInfo);
  }

  #readMarkInfo() {
    const obj = this.#catDict.get("MarkInfo");
    if (!(obj instanceof Dict)) {
      return null;
    }

    const markInfo = {
      Marked: false,
      UserProperties: false,
      Suspects: false,
    };
    for (const key in markInfo) {
      const value = obj.get(key);
      if (typeof value === "boolean") {
        markInfo[key] = value;
      }
    }

    return markInfo;
  }

  get hasStructTree() {
    return this.#catDict.has("StructTreeRoot");
  }

  get structTreeRoot() {
    let structTree = null;
    try {
      structTree = this.#readStructTreeRoot();
    } catch (ex) {
      if (ex instanceof MissingDataException) {
        throw ex;
      }
      warn("Unable read to structTreeRoot info.");
    }
    return shadow(this, "structTreeRoot", structTree);
  }

  #readStructTreeRoot() {
    const rawObj = this.#catDict.getRaw("StructTreeRoot");
    const obj = this.xref.fetchIfRef(rawObj);
    if (!(obj instanceof Dict)) {
      return null;
    }

    const root = new StructTreeRoot(this.xref, obj, rawObj);
    root.init();
Repository: mozilla/pdf.js
File: web/struct_tree_layer_builder.js (JavaScript)

Code before:
              "scriptlevel",
              "encoding",
              "display",
              "linethickness",
              "intent",
              "arg",
              "form",
              "fence",
              "separator",
              "lspace",
              "rspace",
              "stretchy",
              "symmetric",
              "maxsize",
              "minsize",
              "largeop",
              "movablelimits",
              "width",
              "height",
              "depth",
              "voffset",
              "accent",
              "accentunder",
              "columnspan",
              "rowspan",
            ].map(name => ({
              name,
            })),
            comments: false,
          })
        : null
    );
  }
}

const HEADING_PATTERN = /^H(\d+)$/;

/**
 * @typedef {Object} StructTreeLayerBuilderOptions
 * @property {PDFPageProxy} pdfPage
 * @property {Object} rawDims
 */

class StructTreeLayerBuilder {
  #promise;

  #treeDom = null;

  #treePromise;

  #elementAttributes = new Map();

Review comment (suggestion):
You can just remove the `map` completely. Attributes can just be lists of strings.

Code after:
              "scriptlevel",
              "encoding",
              "display",
              "linethickness",
              "intent",
              "arg",
              "form",
              "fence",
              "separator",
              "lspace",
              "rspace",
              "stretchy",
              "symmetric",
              "maxsize",
              "minsize",
              "largeop",
              "movablelimits",
              "width",
              "height",
              "depth",
              "voffset",
              "accent",
              "accentunder",
              "columnspan",
              "rowspan",
            ],
            comments: false,
          })
        : null
    );
  }
}

const HEADING_PATTERN = /^H(\d+)$/;

/**
 * @typedef {Object} StructTreeLayerBuilderOptions
 * @property {PDFPageProxy} pdfPage
 * @property {Object} rawDims
 */

class StructTreeLayerBuilder {
  #promise;

  #treeDom = null;

  #treePromise;

  #elementAttributes = new Map();

  #rawDims;
Repository: mozilla/pdf.js
File: test/unit/api_spec.js (JavaScript)

Code before:
          buildGetDocumentParams("labelled_pages.pdf")
        );
        const pdfDoc = await loadingTask.promise;
        let labels = await pdfDoc.getPageLabels();
        expect(labels).toEqual([
          "i" /* Page 0 */,
          "ii" /* Page 1 */,
          "iii" /* Page 2 */,
          "iv" /* Page 3 */,
          "1" /* Page 4 */,
          "2" /* Page 5 */,
          "3" /* Page 6 */,
          "a" /* Page 7 */,
          "b" /* Page 8 */,
          "4" /* Page 9 */,
          "5" /* Page 10 */,
        ]);

        const data = await pdfDoc.extractPages({
          keptIndices: new Set([0, 1, 5, 7, 10]),
        });
        await loadingTask.destroy();
        loadingTask = getDocument(data);
        const newPdfDoc = await loadingTask.promise;
        labels = await newPdfDoc.getPageLabels();
        expect(labels).toEqual(["i", "ii", "1", "a", "4"]);
        await loadingTask.destroy();
      });
    });
  });
});


Review comment (suggestion):
This expectation doesn't seem to match the labels checked just above. Based on that list I would have expected to see `["i", "ii", "2", "a", "5"]` here for the given `[0, 1, 5, 7, 10]` indices.

Code after:
        const pdfDoc = await loadingTask.promise;
        const pdfData1 = await DefaultFileReaderFactory.fetch({
          path: TEST_PDFS_PATH + "doc_1_3_pages.pdf",
        });

        const data = await pdfDoc.extractPages([
          { document: pdfData1, includePages: [[0, 0], 2] },
          { document: null, includePages: [[2, 4], 7] },
        ]);
        const newLoadingTask = getDocument(data);
        const newPdfDoc = await newLoadingTask.promise;
        expect(newPdfDoc.numPages).toEqual(6);

        for (let i = 1; i <= 2; i++) {
          const pdfPage = await newPdfDoc.getPage(i);
          const { items: textItems } = await pdfPage.getTextContent();
          expect(mergeText(textItems)).toEqual(`Document 1:Page ${2 * i - 1}`);
        }

        const expectedPagesText = [
          "v0 := ld s",
          "i=4. On th",
          "resentatio",
          "5.1 Optimi",
        ];
        for (let i = 3; i <= 6; i++) {
          const pdfPage = await newPdfDoc.getPage(i);
          const { items: textItems } = await pdfPage.getTextContent();
          const text = mergeText(textItems);
          expect(text.substring(0, 10)).toEqual(expectedPagesText[i - 3]);
        }

        await newLoadingTask.destroy();
        await loadingTask.destroy();
      });

      it("should merge two PDFs with page excluded ranges", async function () {
        const loadingTask = getDocument(
          buildGetDocumentParams("tracemonkey.pdf")
        );
        const pdfDoc = await loadingTask.promise;
        const pdfData1 = await DefaultFileReaderFactory.fetch({
          path: TEST_PDFS_PATH + "doc_1_3_pages.pdf",
        });

        const data = await pdfDoc.extractPages([
          { document: pdfData1, excludePages: [[1, 1]] },
          {
            document: null,
            excludePages: [
              [0, 1],
Repository: mozilla/pdf.js
File: test/integration/stamp_editor_spec.mjs (JavaScript)

Code before:
            page,
            `.page[data-page-number="2"] .annotationEditorLayer`
          );

          const deltaX =
            page2Rect.x +
            page2Rect.width / 2 -
            (editorRect.x + editorRect.width / 2);
          const deltaY =
            page2Rect.y +
            page2Rect.height / 3 -
            (editorRect.y + editorRect.height / 2);

          await dragAndDrop(page, editorSelector, [[deltaX, deltaY]], 10);

          await page.waitForFunction(
            sel => {
              const editorDiv = document.querySelector(sel);
              const pageDiv = editorDiv?.closest(".page");
              return pageDiv?.getAttribute("data-page-number") === "2";
            },
            {},
            editorSelector
          );

          const pageIndex = await page.evaluate(sel => {
            const editorDiv = document.querySelector(sel);
            const pageDiv = editorDiv?.closest(".page");
            const pageNum = pageDiv?.getAttribute("data-page-number");
            return pageNum ? parseInt(pageNum, 10) - 1 : -1;
          }, editorSelector);

          expect(pageIndex).withContext(`In ${browserName}`).toEqual(1);
        })
      );
    });
  });
});


Review comment (refactor):
We're already waiting for the page number field to be correct above (string value "2"), so if that's the case we know that the page index will automatically also be what we expect because it's based on that string value, and if it's not the case we'll never get past that block anyway. We should therefore be able to end the test after the `await page.waitForFunction` call and thus remove these `const pageIndex` and `expect` lines to simplify the test.

Code after:

          const editorRect = await getRect(page, editorSelector);
          const page2Rect = await getRect(
            page,
            `.page[data-page-number="2"] .annotationEditorLayer`
          );

          const deltaX =
            page2Rect.x +
            page2Rect.width / 2 -
            (editorRect.x + editorRect.width / 2);
          const deltaY =
            page2Rect.y +
            page2Rect.height / 3 -
            (editorRect.y + editorRect.height / 2);

          await dragAndDrop(page, editorSelector, [[deltaX, deltaY]], 10);

          await page.waitForFunction(
            sel => {
              const editorDiv = document.querySelector(sel);
              const pageDiv = editorDiv?.closest(".page");
              return pageDiv?.getAttribute("data-page-number") === "2";
            },
            {},
            editorSelector
          );
        })
      );
    });
  });
});
Repository: mozilla/pdf.js
File: test/integration/accessibility_spec.mjs (JavaScript)

Code before:
          );
          expect(ariaHidden).withContext(`In ${browserName}`).toEqual("true");
        })
      );
    });
  });

  describe("No alt-text with MathML", () => {
    let pages;

    beforeEach(async () => {
      pages = await loadAndWait("bug2004951.pdf", ".textLayer");
    });

    afterEach(async () => {
      await closePages(pages);
    });

    it("must check that there's no alt-text on the MathML node", async () => {
      await Promise.all(
        pages.map(async ([browserName, page]) => {
          const ariaLabel = await page.$eval(
            "span[aria-owns='p3R_mc2']",
            el => el.getAttribute("aria-label") || ""
          );
          if (browserName === "firefox") {
            expect(ariaLabel).withContext(`In ${browserName}`).toEqual("");
          } else {
            expect(ariaLabel)
              .withContext(`In ${browserName}`)
              .toEqual("cube root of , x plus y end cube root ");
          }
        })
      );
    });
  });
});


Review comment (suggestion):
It's not really clear to me why this expectation is Firefox-specific given that I don't see Firefox-specific handling in the struct tree builder code itself? If a specific feature like MatML support is missing in e.g. Chrome we should probably use a feature test instead to make that more clear/explicit.

Code after:
          );
          expect(ariaHidden).withContext(`In ${browserName}`).toEqual("true");
        })
      );
    });
  });

  describe("Artifacts must be aria-hidden", () => {
    let pages;

    beforeEach(async () => {
      pages = await loadAndWait("bug1937438_mml_from_latex.pdf", ".textLayer");
    });

    afterEach(async () => {
      await closePages(pages);
    });

    it("must check that some artifacts are aria-hidden", async () => {
      await Promise.all(
        pages.map(async ([browserName, page]) => {
          const parentSquareRootHidden = await page.evaluate(() => {
            for (const span of document.querySelectorAll(".textLayer span")) {
              if (span.textContent === "√") {
                return span.parentElement.getAttribute("aria-hidden");
              }
            }
            return false;
          });
          expect(parentSquareRootHidden)
            .withContext(`In ${browserName}`)
            .toEqual("true");
        })
      );
    });
  });

  describe("No alt-text with MathML", () => {
    let pages;

    beforeEach(async () => {
      pages = await loadAndWait("bug2004951.pdf", ".textLayer");
    });

    afterEach(async () => {
      await closePages(pages);
    });

    it("must check that there's no alt-text on the MathML node", async () => {
      await Promise.all(
        pages.map(async ([browserName, page]) => {
Repository: mozilla/pdf.js
File: gulpfile.mjs (JavaScript)

Code before:
    },
    function createGenericLegacy() {
      console.log();
      console.log("### Creating generic (legacy) viewer");
      const defines = { ...DEFINES, GENERIC: true, SKIP_BABEL: false };

      return buildGeneric(defines, GENERIC_LEGACY_DIR);
    }
  )
);

function buildComponents(defines, dir) {
  fs.rmSync(dir, { recursive: true, force: true });

  const COMPONENTS_IMAGES = [
    "web/images/annotation-*.svg",
    "web/images/loading-icon.gif",
    "web/images/altText_*.svg",
    "web/images/editor-toolbar-*.svg",
    "web/images/messageBar_*.svg",
    "web/images/toolbarButton-*.svg",
    "web/images/cursor-*.svg",
    "web/images/comment-*.svg",
    "web/images/treeitem-*.svg",
    "web/images/checkmark.svg",
    "web/images/pages_*.svg"
  ];

  return ordered([
    createComponentsBundle(defines).pipe(gulp.dest(dir)),
    gulp
      .src(COMPONENTS_IMAGES, { encoding: false })
      .pipe(gulp.dest(dir + "images")),
    preprocessCSS("web/pdf_viewer.css", defines)
      .pipe(
        postcss([
          postcssDirPseudoClass(),
          discardCommentsCSS(),
          postcssNesting(),
          postcssLightDarkFunction({ preserve: true }),
          autoprefixer(AUTOPREFIXER_CONFIG),
        ])
      )
      .pipe(gulp.dest(dir)),
  ]);
}

gulp.task(
  "components",
  gulp.series(createBuildNumber, function createComponents() {
    console.log();

Review comment (bug):
This should have a trailing comma. Note that Prettier, the linter, also fails on this; you can fix it by running `npx gulp lint --fix`.

Code after:
    },
    function createGenericLegacy() {
      console.log();
      console.log("### Creating generic (legacy) viewer");
      const defines = { ...DEFINES, GENERIC: true, SKIP_BABEL: false };

      return buildGeneric(defines, GENERIC_LEGACY_DIR);
    }
  )
);

function buildComponents(defines, dir) {
  fs.rmSync(dir, { recursive: true, force: true });

  const COMPONENTS_IMAGES = ["web/images/*.svg", "web/images/*.gif"];

  return ordered([
    createComponentsBundle(defines).pipe(gulp.dest(dir)),
    gulp
      .src(COMPONENTS_IMAGES, { encoding: false })
      .pipe(gulp.dest(dir + "images")),
    preprocessCSS("web/pdf_viewer.css", defines)
      .pipe(
        postcss([
          postcssDirPseudoClass(),
          discardCommentsCSS(),
          postcssNesting(),
          postcssLightDarkFunction({ preserve: true }),
          autoprefixer(AUTOPREFIXER_CONFIG),
        ])
      )
      .pipe(gulp.dest(dir)),
  ]);
}

gulp.task(
  "components",
  gulp.series(createBuildNumber, function createComponents() {
    console.log();
    console.log("### Creating generic components");
    const defines = { ...DEFINES, COMPONENTS: true, GENERIC: true };

    return buildComponents(defines, COMPONENTS_DIR);
  })
);

gulp.task(
  "components-legacy",
  gulp.series(createBuildNumber, function createComponentsLegacy() {
    console.log();
    console.log("### Creating generic (legacy) components");
Repository: mozilla/pdf.js
File: test/integration/freetext_editor_spec.mjs (JavaScript)

Code before:
            );
          }

          await page.waitForFunction(
            () =>
              document.querySelectorAll(
                `.annotationLayer > section:is(.stampAnnotation, .inkAnnotation, .highlightAnnotation, .freeTextAnnotation)[hidden = ""]`
              ).length === 4
          );
        })
      );
    });
  });

  describe("No exception when moving (issue 20571)", () => {
    let pages;

    beforeEach(async () => {
      pages = await loadAndWait(
        "tracemonkey.pdf",
        ".annotationEditorLayer",
        100,
        {
          eventBusSetup: eventBus => {
            eventBus.on("annotationeditorstateschanged", ({ details }) => {
              window.editingEvents?.push(details);
            });
          },
        }
      );
    });

    afterEach(async () => {
      await closePages(pages);
    });

    it("must check that the buttons works correctly", async () => {
      await Promise.all(
        pages.map(async ([browserName, page]) => {
          await switchToFreeText(page);

          const rect = await getRect(page, ".annotationEditorLayer");
          await createFreeTextEditor({
            page,
            x: rect.x + 100,
            y: rect.y + 100,
            data: "Hello PDF.js World !!",
          });

          await switchToFreeText(page, /* disable = */ true);
          await switchToFreeText(page);

Review comment (refactor):
It looks like we don't actually use this in the test, so the whole event bus setup can probably be removed to simplify the test?

Code after:
            );
          }

          await page.waitForFunction(
            () =>
              document.querySelectorAll(
                `.annotationLayer > section:is(.stampAnnotation, .inkAnnotation, .highlightAnnotation, .freeTextAnnotation)[hidden = ""]`
              ).length === 4
          );
        })
      );
    });
  });

  describe("No exception when moving (issue 20571)", () => {
    let pages;

    beforeEach(async () => {
      pages = await loadAndWait(
        "tracemonkey.pdf",
        ".annotationEditorLayer",
        100
      );
    });

    afterEach(async () => {
      await closePages(pages);
    });

    it("must check that the buttons work correctly", async () => {
      await Promise.all(
        pages.map(async ([browserName, page]) => {
          await switchToFreeText(page);

          const rect = await getRect(page, ".annotationEditorLayer");
          await createFreeTextEditor({
            page,
            x: rect.x + 100,
            y: rect.y + 100,
            data: "Hello PDF.js World !!",
          });

          await switchToFreeText(page, /* disable = */ true);
          await switchToFreeText(page);

          const editorSelector = getEditorSelector(0);
          await selectEditor(page, editorSelector);
          await dragAndDrop(page, editorSelector, [[10, 10]]);

          await switchToFreeText(page, /* disable = */ true);
          await switchToFreeText(page);