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

Code before:
        await Promise.all(
          pages.map(async ([browserName, page]) => {
            const text = await page.$eval(
              "[data-annotation-id='58R'] u",
              el => el.textContent
            );
            expect(text).withContext(`In ${browserName}`).toEqual("machine");
          })
        );
      });

      it("must check that an highlight as a squiggly/u element", async () => {
        await Promise.all(
          pages.map(async ([browserName, page]) => {
            const text = await page.$eval(
              "[data-annotation-id='60R'] u",
              el => el.textContent
            );
            expect(text).withContext(`In ${browserName}`)
              .toEqual(`paths through nested loops. We have implemented
a dynamic compiler for JavaScript based on our`);
          })
        );
      });

      it("must check that an highlight as a s element", async () => {
        await Promise.all(
          pages.map(async ([browserName, page]) => {
            const text = await page.$eval(
              "[data-annotation-id='65R'] s",
              el => el.textContent
            );
            expect(text)
              .withContext(`In ${browserName}`)
              .toEqual("Experimentation,");
          })
        );
      });
    });
  });
});


Review comment (suggestion):
`must check that the text under a strikeout annotation exist in the DOM` (to match the highlight one above)

Code after:
        await Promise.all(
          pages.map(async ([browserName, page]) => {
            const text = await page.$eval(
              "[data-annotation-id='58R'] u",
              el => el.textContent
            );
            expect(text).withContext(`In ${browserName}`).toEqual("machine");
          })
        );
      });

      it("must check that the text under a squiggly annotation exist in the DOM", async () => {
        await Promise.all(
          pages.map(async ([browserName, page]) => {
            const text = await page.$eval(
              "[data-annotation-id='60R'] u",
              el => el.textContent
            );
            expect(text).withContext(`In ${browserName}`)
              .toEqual(`paths through nested loops. We have implemented
a dynamic compiler for JavaScript based on our`);
          })
        );
      });

      it("must check that the text under a strikeout annotation exist in the DOM", async () => {
        await Promise.all(
          pages.map(async ([browserName, page]) => {
            const text = await page.$eval(
              "[data-annotation-id='65R'] s",
              el => el.textContent
            );
            expect(text)
              .withContext(`In ${browserName}`)
              .toEqual("Experimentation,");
          })
        );
      });
    });
  });
});
Repository: mozilla/pdf.js
File: src/core/intersector.js (JavaScript)

Code before:
    if (this.#extraChars.length > 0) {
      this.#text.push(this.#extraChars.join(""));
      this.#extraChars.length = 0;
    }
    this.#text.push(glyph);
    this.#canTakeExtraChars = true;

    return true;
  }

  addExtraChar(char) {
    if (this.#canTakeExtraChars) {
      this.#extraChars.push(char);
    }
  }

  disableExtraChars() {
    if (!this.#canTakeExtraChars) {
      return;
    }
    this.#canTakeExtraChars = false;
    this.#extraChars.length = 0;
  }

  setText() {
    this.#annotation.data.highlightedText = this.#text.join("");
  }
}

class Intersector {
  #intersectors = new Map();

  constructor(annotations) {
    for (const annotation of annotations) {
      if (!annotation.data.quadPoints) {
        continue;
      }
      const intersector = new SingleIntersector(annotation);
      for (const [otherIntersector, overlapping] of this.#intersectors) {
        if (otherIntersector.overlaps(intersector)) {
          if (!overlapping) {
            this.#intersectors.set(otherIntersector, new Set([intersector]));
          } else {
            overlapping.add(intersector);
          }
        }
      }
      this.#intersectors.set(intersector, null);
    }
  }


Review comment (suggestion):
`this.#annotation.data.overlaidText` (to remove "highlight" ambiguity). This also involves changing the other places (callers of this property) that are called "highlightedText" in this patch.

Code after:
    if (this.#extraChars.length > 0) {
      this.#text.push(this.#extraChars.join(""));
      this.#extraChars.length = 0;
    }
    this.#text.push(glyph);
    this.#canTakeExtraChars = true;

    return true;
  }

  addExtraChar(char) {
    if (this.#canTakeExtraChars) {
      this.#extraChars.push(char);
    }
  }

  disableExtraChars() {
    if (!this.#canTakeExtraChars) {
      return;
    }
    this.#canTakeExtraChars = false;
    this.#extraChars.length = 0;
  }

  setText() {
    this.#annotation.data.overlaidText = this.#text.join("");
  }
}

class Intersector {
  #intersectors = new Map();

  constructor(annotations) {
    for (const annotation of annotations) {
      if (!annotation.data.quadPoints) {
        continue;
      }
      const intersector = new SingleIntersector(annotation);
      for (const [otherIntersector, overlapping] of this.#intersectors) {
        if (otherIntersector.overlaps(intersector)) {
          if (!overlapping) {
            this.#intersectors.set(otherIntersector, new Set([intersector]));
          } else {
            overlapping.add(intersector);
          }
        }
      }
      this.#intersectors.set(intersector, null);
    }
  }
Repository: mozilla/pdf.js
File: src/display/annotation_layer.js (JavaScript)

Code before:
  }

  addHighlightArea() {
    const triggers = this.getElementsToTriggerPopup();
    if (Array.isArray(triggers)) {
      for (const element of triggers) {
        element.classList.add("highlightArea");
      }
    } else {
      triggers.classList.add("highlightArea");
    }
  }

  _editOnDoubleClick() {
    if (!this._isEditable) {
      return;
    }
    const {
      annotationEditorType: mode,
      data: { id: editId },
    } = this;
    this.container.addEventListener("dblclick", () => {
      this.linkService.eventBus?.dispatch("switchannotationeditormode", {
        source: this,
        mode,
        editId,
      });
    });
  }

  get width() {
    return this.data.rect[2] - this.data.rect[0];
  }

  get height() {
    return this.data.rect[3] - this.data.rect[1];
  }
}

class LinkAnnotationElement extends AnnotationElement {
  constructor(parameters, options = null) {
    super(parameters, {
      isRenderable: true,
      ignoreBorder: !!options?.ignoreBorder,
      createQuadrilaterals: true,
    });
    this.isTooltipOnly = parameters.data.isTooltipOnly;
  }

  render() {
    const { data, linkService } = this;

Review comment (suggestion):
I'm probably missing something here, but `mustEnterInEditMode` (which I think is responsible for switching the mode on click) only seems to be set to `true` here, but this is the double-click handler so I don't really understand how a single click also achieves this behavior? The integration test shows that it works, I just don't really understand how it works exactly.

Code after:

  addHighlightArea() {
    const triggers = this.getElementsToTriggerPopup();
    if (Array.isArray(triggers)) {
      for (const element of triggers) {
        element.classList.add("highlightArea");
      }
    } else {
      triggers.classList.add("highlightArea");
    }
  }

  _editOnDoubleClick() {
    if (!this._isEditable) {
      return;
    }
    const {
      annotationEditorType: mode,
      data: { id: editId },
    } = this;
    this.container.addEventListener("dblclick", () => {
      this.linkService.eventBus?.dispatch("switchannotationeditormode", {
        source: this,
        mode,
        editId,
        mustEnterInEditMode: true,
      });
    });
  }

  get width() {
    return this.data.rect[2] - this.data.rect[0];
  }

  get height() {
    return this.data.rect[3] - this.data.rect[1];
  }
}

class LinkAnnotationElement extends AnnotationElement {
  constructor(parameters, options = null) {
    super(parameters, {
      isRenderable: true,
      ignoreBorder: !!options?.ignoreBorder,
      createQuadrilaterals: true,
    });
    this.isTooltipOnly = parameters.data.isTooltipOnly;
  }

  render() {
    const { data, linkService } = this;
Repository: mozilla/pdf.js
File: src/core/core_utils.js (JavaScript)

Code before:
    if (!key.startsWith(AnnotationEditorPrefix)) {
      continue;
    }
    let annotations = newAnnotationsByPage.get(value.pageIndex);
    if (!annotations) {
      annotations = [];
      newAnnotationsByPage.set(value.pageIndex, annotations);
    }
    annotations.push(value);
  }
  return newAnnotationsByPage.size > 0 ? newAnnotationsByPage : null;
}

function stringToAsciiOrUTF16BE(str) {
  return isAscii(str) ? str : stringToUTF16String(str, /* bigEndian = */ true);
}

function isAscii(str) {
  return !str || /^[\x00-\x7F]*$/.test(str);
}

function stringToUTF16HexString(str) {
  const buf = [];
  for (let i = 0, ii = str.length; i < ii; i++) {
    const char = str.charCodeAt(i);
    buf.push(hexNumbers[(char >> 8) & 0xff], hexNumbers[char & 0xff]);
  }
  return buf.join("");
}

function stringToUTF16String(str, bigEndian = false) {
  const buf = [];
  if (bigEndian) {
    buf.push("\xFE\xFF");
  }
  for (let i = 0, ii = str.length; i < ii; i++) {
    const char = str.charCodeAt(i);
    buf.push(
      String.fromCharCode((char >> 8) & 0xff),
      String.fromCharCode(char & 0xff)
    );
  }
  return buf.join("");
}

function getRotationMatrix(rotation, width, height) {
  switch (rotation) {
    case 90:
      return [0, 1, -1, 0, width, 0];
    case 180:
      return [-1, 0, 0, -1, width, height];

Review comment (suggestion):
This would also mean that `null` and `undefined` result in `true`; is that expected? If so we should probably add tests for those values in addition to the `""` case you added, and if not we should probably check if we're dealing with a string value here instead.

Code after:
    if (!key.startsWith(AnnotationEditorPrefix)) {
      continue;
    }
    let annotations = newAnnotationsByPage.get(value.pageIndex);
    if (!annotations) {
      annotations = [];
      newAnnotationsByPage.set(value.pageIndex, annotations);
    }
    annotations.push(value);
  }
  return newAnnotationsByPage.size > 0 ? newAnnotationsByPage : null;
}

// If the string is null or undefined then it is returned as is.
function stringToAsciiOrUTF16BE(str) {
  if (str === null || str === undefined) {
    return str;
  }
  return isAscii(str) ? str : stringToUTF16String(str, /* bigEndian = */ true);
}

function isAscii(str) {
  if (typeof str !== "string") {
    return false;
  }
  return !str || /^[\x00-\x7F]*$/.test(str);
}

function stringToUTF16HexString(str) {
  const buf = [];
  for (let i = 0, ii = str.length; i < ii; i++) {
    const char = str.charCodeAt(i);
    buf.push(hexNumbers[(char >> 8) & 0xff], hexNumbers[char & 0xff]);
  }
  return buf.join("");
}

function stringToUTF16String(str, bigEndian = false) {
  const buf = [];
  if (bigEndian) {
    buf.push("\xFE\xFF");
  }
  for (let i = 0, ii = str.length; i < ii; i++) {
    const char = str.charCodeAt(i);
    buf.push(
      String.fromCharCode((char >> 8) & 0xff),
      String.fromCharCode(char & 0xff)
    );
  }
  return buf.join("");
}
Repository: mozilla/pdf.js
File: src/display/editor/color_picker.js (JavaScript)

Code before:
    const i = this.#uiManager.highlightColors.values();
    for (const child of this.#dropdown.children) {
      child.ariaSelected = i.next().value === color.toUpperCase();
    }
  }

  destroy() {
    this.#button?.remove();
    this.#button = null;
    this.#buttonSwatch = null;
    this.#dropdown?.remove();
    this.#dropdown = null;
  }
}

class BasicColorPicker {
  #input = null;

  #editor = null;

  #uiManager = null;

  static #l10nColor = null;

  constructor(editor) {
    this.#editor = editor;
    this.#uiManager = editor._uiManager;
    this.#input = null;

    BasicColorPicker.#l10nColor ||= Object.freeze({
      freetext: "pdfjs-editor-color-picker-free-text-input",
      ink: "pdfjs-editor-color-picker-ink-input",
    });
  }

  renderButton() {
    if (this.#input) {
      return this.#input;
    }
    const editor = this.#editor;
    const { editorType, colorType, colorValue } = editor;
    const input = (this.#input = document.createElement("input"));
    input.type = "color";
    input.value = colorValue || "#000000";
    input.className = "basicColorPicker";
    input.tabIndex = 0;
    input.setAttribute("data-l10n-id", BasicColorPicker.#l10nColor[editorType]);
    input.addEventListener(
      "input",
      () => {
        this.#uiManager.updateParams(colorType, input.value);

Review comment (suggestion):
It's not immediately clear to me what the difference is between a basic color picker and the pre-existing regular color picker. It might be good to add a short docstring to both classes to explain the difference to the reader so it's clear which should be used in different scenarios.

Code after:
    }
    if (!this.#dropdown) {
      return;
    }

    const i = this.#uiManager.highlightColors.values();
    for (const child of this.#dropdown.children) {
      child.ariaSelected = i.next().value === color.toUpperCase();
    }
  }

  destroy() {
    this.#button?.remove();
    this.#button = null;
    this.#buttonSwatch = null;
    this.#dropdown?.remove();
    this.#dropdown = null;
  }
}

/**
 * BasicColorPicker class provides a simple color picker.
 * It displays an input element (with type="color") that allows the user
 * to select a color for the annotation.
 */
class BasicColorPicker {
  #input = null;

  #editor = null;

  #uiManager = null;

  static #l10nColor = null;

  constructor(editor) {
    this.#editor = editor;
    this.#uiManager = editor._uiManager;

    BasicColorPicker.#l10nColor ||= Object.freeze({
      freetext: "pdfjs-editor-color-picker-free-text-input",
      ink: "pdfjs-editor-color-picker-ink-input",
    });
  }

  renderButton() {
    if (this.#input) {
      return this.#input;
    }
    const { editorType, colorType, colorValue } = this.#editor;
    const input = (this.#input = document.createElement("input"));
    input.type = "color";
Repository: mozilla/pdf.js
File: src/display/editor/color_picker.js (JavaScript)

Code before:
    this.#button?.remove();
    this.#button = null;
    this.#buttonSwatch = null;
    this.#dropdown?.remove();
    this.#dropdown = null;
  }
}

/**
 * BasicColorPicker class provides a simple color picker.
 * It displays an input element (with type="color") that allows the user
 * to select a color for the annotation.
 */
class BasicColorPicker {
  #input = null;

  #editor = null;

  #uiManager = null;

  static #l10nColor = null;

  constructor(editor) {
    this.#editor = editor;
    this.#uiManager = editor._uiManager;
    this.#input = null;

    BasicColorPicker.#l10nColor ||= Object.freeze({
      freetext: "pdfjs-editor-color-picker-free-text-input",
      ink: "pdfjs-editor-color-picker-ink-input",
    });
  }

  renderButton() {
    if (this.#input) {
      return this.#input;
    }
    const editor = this.#editor;
    const { editorType, colorType, colorValue } = editor;
    const input = (this.#input = document.createElement("input"));
    input.type = "color";
    input.value = colorValue || "#000000";
    input.className = "basicColorPicker";
    input.tabIndex = 0;
    input.setAttribute("data-l10n-id", BasicColorPicker.#l10nColor[editorType]);
    input.addEventListener(
      "input",
      () => {
        this.#uiManager.updateParams(colorType, input.value);
      },
      { signal: this.#uiManager._signal }

Review comment (refactor):
This should already be `null` by default looking at line 318 here, so we can probably remove this.

Code after:
    this.#button?.remove();
    this.#button = null;
    this.#buttonSwatch = null;
    this.#dropdown?.remove();
    this.#dropdown = null;
  }
}

/**
 * BasicColorPicker class provides a simple color picker.
 * It displays an input element (with type="color") that allows the user
 * to select a color for the annotation.
 */
class BasicColorPicker {
  #input = null;

  #editor = null;

  #uiManager = null;

  static #l10nColor = null;

  constructor(editor) {
    this.#editor = editor;
    this.#uiManager = editor._uiManager;

    BasicColorPicker.#l10nColor ||= Object.freeze({
      freetext: "pdfjs-editor-color-picker-free-text-input",
      ink: "pdfjs-editor-color-picker-ink-input",
    });
  }

  renderButton() {
    if (this.#input) {
      return this.#input;
    }
    const { editorType, colorType, colorValue } = this.#editor;
    const input = (this.#input = document.createElement("input"));
    input.type = "color";
    input.value = colorValue || "#000000";
    input.className = "basicColorPicker";
    input.tabIndex = 0;
    input.setAttribute("data-l10n-id", BasicColorPicker.#l10nColor[editorType]);
    input.addEventListener(
      "input",
      () => {
        this.#uiManager.updateParams(colorType, input.value);
      },
      { signal: this.#uiManager._signal }
    );
    return input;
Repository: mozilla/pdf.js
File: src/display/editor/color_picker.js (JavaScript)

Code before:
 */
class BasicColorPicker {
  #input = null;

  #editor = null;

  #uiManager = null;

  static #l10nColor = null;

  constructor(editor) {
    this.#editor = editor;
    this.#uiManager = editor._uiManager;
    this.#input = null;

    BasicColorPicker.#l10nColor ||= Object.freeze({
      freetext: "pdfjs-editor-color-picker-free-text-input",
      ink: "pdfjs-editor-color-picker-ink-input",
    });
  }

  renderButton() {
    if (this.#input) {
      return this.#input;
    }
    const editor = this.#editor;
    const { editorType, colorType, colorValue } = editor;
    const input = (this.#input = document.createElement("input"));
    input.type = "color";
    input.value = colorValue || "#000000";
    input.className = "basicColorPicker";
    input.tabIndex = 0;
    input.setAttribute("data-l10n-id", BasicColorPicker.#l10nColor[editorType]);
    input.addEventListener(
      "input",
      () => {
        this.#uiManager.updateParams(colorType, input.value);
      },
      { signal: this.#uiManager._signal }
    );
    return input;
  }

  update(value) {
    if (!this.#input) {
      return;
    }
    this.#input.value = value;
  }

  destroy() {

Review comment (refactor):
We can probably remove this line if we write the one below as `const { editorType, colorType, colorValue } = this.#editor;`.

Code after:
 */
class BasicColorPicker {
  #input = null;

  #editor = null;

  #uiManager = null;

  static #l10nColor = null;

  constructor(editor) {
    this.#editor = editor;
    this.#uiManager = editor._uiManager;

    BasicColorPicker.#l10nColor ||= Object.freeze({
      freetext: "pdfjs-editor-color-picker-free-text-input",
      ink: "pdfjs-editor-color-picker-ink-input",
    });
  }

  renderButton() {
    if (this.#input) {
      return this.#input;
    }
    const { editorType, colorType, colorValue } = this.#editor;
    const input = (this.#input = document.createElement("input"));
    input.type = "color";
    input.value = colorValue || "#000000";
    input.className = "basicColorPicker";
    input.tabIndex = 0;
    input.setAttribute("data-l10n-id", BasicColorPicker.#l10nColor[editorType]);
    input.addEventListener(
      "input",
      () => {
        this.#uiManager.updateParams(colorType, input.value);
      },
      { signal: this.#uiManager._signal }
    );
    return input;
  }

  update(value) {
    if (!this.#input) {
      return;
    }
    this.#input.value = value;
  }

  destroy() {
    this.#input?.remove();
    this.#input = null;
Repository: mozilla/pdf.js
File: src/display/editor/freetext.js (JavaScript)

Code before:
      return null;
    }

    serialized.id = this.annotationElementId;

    return serialized;
  }

  #hasElementChanged(serialized) {
    const { value, fontSize, color, pageIndex } = this._initialData;

    return (
      this._hasBeenMoved ||
      serialized.value !== value ||
      serialized.fontSize !== fontSize ||
      serialized.color.some((c, i) => c !== color[i]) ||
      serialized.pageIndex !== pageIndex
    );
  }

  /** @inheritdoc */
  renderAnnotationElement(annotation) {
    const content = super.renderAnnotationElement(annotation);
    if (this.deleted) {
      annotation.updateEdited({
        popupDeleted: true,
      });
      return content;
    }
    const { style } = content;
    style.fontSize = `calc(${this.#fontSize}px * var(--total-scale-factor))`;
    style.color = this.#color;

    content.replaceChildren();
    for (const line of this.#content.split("\n")) {
      const div = document.createElement("div");
      div.append(
        line ? document.createTextNode(line) : document.createElement("br")
      );
      content.append(div);
    }

    const padding = FreeTextEditor._internalPadding * this.parentScale;
    annotation.updateEdited({
      rect: this.getRect(padding, padding),
      popupContent: this.#content,
    });

    return content;
  }


Review comment (suggestion):
`popupDeleted` no longer exists after the recent change, so this should probably be `deleted` now instead.

Code after:
      return null;
    }

    serialized.id = this.annotationElementId;

    return serialized;
  }

  #hasElementChanged(serialized) {
    const { value, fontSize, color, pageIndex } = this._initialData;

    return (
      this._hasBeenMoved ||
      serialized.value !== value ||
      serialized.fontSize !== fontSize ||
      serialized.color.some((c, i) => c !== color[i]) ||
      serialized.pageIndex !== pageIndex
    );
  }

  /** @inheritdoc */
  renderAnnotationElement(annotation) {
    const content = super.renderAnnotationElement(annotation);
    const { style } = content;
    style.fontSize = `calc(${this.#fontSize}px * var(--total-scale-factor))`;
    style.color = this.#color;

    content.replaceChildren();
    for (const line of this.#content.split("\n")) {
      const div = document.createElement("div");
      div.append(
        line ? document.createTextNode(line) : document.createElement("br")
      );
      content.append(div);
    }

    const padding = FreeTextEditor._internalPadding * this.parentScale;
    annotation.updateEdited({
      rect: this.getRect(padding, padding),
      popupContent: this.#content,
    });

    return content;
  }

  resetAnnotationElement(annotation) {
    super.resetAnnotationElement(annotation);
    annotation.resetEdited();
  }
}