From ea06bb0e36fbc2ea2bbbd140851b94a7e05cd719 Mon Sep 17 00:00:00 2001 From: Calixte Denizet Date: Sat, 19 Dec 2020 18:12:44 +0100 Subject: [PATCH] [api-minor] Annotation -- Don't compute appearance when nothing has changed * don't set a value in annotationStorage by default: - having an undefined when the annotation is rendered for saving/printing means nothing has changed so use normal appearance - aims to fix https://bugzilla.mozilla.org/show_bug.cgi?id=1681687 * change the way to compute font size when this one is null in DA: - make fontSize proportional to line height - in multiline case, take into account the number of lines for text entered to adapt the font size --- src/core/annotation.js | 65 ++++++++++++++++++---------- src/display/annotation_layer.js | 12 ++--- src/display/annotation_storage.js | 15 ++++++- test/unit/annotation_spec.js | 4 +- test/unit/annotation_storage_spec.js | 25 +++++++---- 5 files changed, 81 insertions(+), 40 deletions(-) diff --git a/src/core/annotation.js b/src/core/annotation.js index 4863ea93d..d420c2f66 100644 --- a/src/core/annotation.js +++ b/src/core/annotation.js @@ -1268,18 +1268,25 @@ class WidgetAnnotation extends Annotation { if (!annotationStorage || isPassword) { return null; } - const value = + let value = annotationStorage[this.data.id] && annotationStorage[this.data.id].value; if (value === undefined) { // The annotation hasn't been rendered so use the appearance return null; } + value = value.trim(); + if (value === "") { // the field is empty: nothing to render return ""; } + let lineCount = -1; + if (this.data.multiLine) { + lineCount = value.split(/\r\n|\r|\n/).length; + } + const defaultPadding = 2; const hPadding = defaultPadding; const totalHeight = this.data.rect[3] - this.data.rect[1]; @@ -1297,8 +1304,12 @@ class WidgetAnnotation extends Annotation { ); } + const [defaultAppearance, fontSize] = this._computeFontSize( + totalHeight, + lineCount + ); + const font = await this._getFontData(evaluator, task); - const fontSize = this._computeFontSize(font, totalHeight); let descent = font.descent; if (isNaN(descent)) { @@ -1306,7 +1317,6 @@ class WidgetAnnotation extends Annotation { } const vPadding = defaultPadding + Math.abs(descent) * fontSize; - const defaultAppearance = this.data.defaultAppearance; const alignment = this.data.textAlignment; if (this.data.multiLine) { @@ -1389,35 +1399,44 @@ class WidgetAnnotation extends Annotation { return initialState.font; } - _computeFontSize(font, height) { - let fontSize = this.data.defaultAppearanceData.fontSize; - if (!fontSize) { - const { fontColor, fontName } = this.data.defaultAppearanceData; - let capHeight; - if (font.capHeight) { - capHeight = font.capHeight; + _computeFontSize(height, lineCount) { + let { fontSize } = this.data.defaultAppearanceData; + if (fontSize === null || fontSize === 0) { + // A zero value for size means that the font shall be auto-sized: + // its size shall be computed as a function of the height of the + // annotation rectangle (see 12.7.3.3). + + const roundWithOneDigit = x => Math.round(x * 10) / 10; + + // Represent the percentage of the font size over the height + // of a single-line field. + const FONT_FACTOR = 0.8; + if (lineCount === -1) { + fontSize = roundWithOneDigit(FONT_FACTOR * height); } else { - const glyphs = font.charsToGlyphs(font.encodeString("M").join("")); - if (glyphs.length === 1 && glyphs[0].width) { - const em = glyphs[0].width / 1000; - // According to https://en.wikipedia.org/wiki/Em_(typography) - // an average cap height should be 70% of 1em - capHeight = 0.7 * em; - } else { - capHeight = 0.7; - } + // Hard to guess how many lines there are. + // The field may have been sized to have 10 lines + // and the user entered only 1 so if we get font size from + // height and number of lines then we'll get something too big. + // So we compute a fake number of lines based on height and + // a font size equal to 10. + // Then we'll adjust font size to what we have really. + fontSize = 10; + let lineHeight = fontSize / FONT_FACTOR; + let numberOfLines = Math.round(height / lineHeight); + numberOfLines = Math.max(numberOfLines, lineCount); + lineHeight = height / numberOfLines; + fontSize = roundWithOneDigit(FONT_FACTOR * lineHeight); } - // 1.5 * capHeight * fontSize seems to be a good value for lineHeight - fontSize = Math.max(1, Math.floor(height / (1.5 * capHeight))); - + const { fontName, fontColor } = this.data.defaultAppearanceData; this.data.defaultAppearance = createDefaultAppearance({ fontSize, fontName, fontColor, }); } - return fontSize; + return [this.data.defaultAppearance, fontSize]; } _renderText(text, font, fontSize, totalWidth, alignment, hPadding, vPadding) { diff --git a/src/display/annotation_layer.js b/src/display/annotation_layer.js index 3879984b6..df43a8d9a 100644 --- a/src/display/annotation_layer.js +++ b/src/display/annotation_layer.js @@ -603,7 +603,7 @@ class TextWidgetAnnotationElement extends WidgetAnnotationElement { // NOTE: We cannot set the values using `element.value` below, since it // prevents the AnnotationLayer rasterizer in `test/driver.js` // from parsing the elements correctly for the reference tests. - const textContent = storage.getOrCreateValue(id, { + const textContent = storage.getValue(id, { value: this.data.fieldValue, }).value; const elementData = { @@ -873,7 +873,7 @@ class CheckboxWidgetAnnotationElement extends WidgetAnnotationElement { const storage = this.annotationStorage; const data = this.data; const id = data.id; - const value = storage.getOrCreateValue(id, { + const value = storage.getValue(id, { value: data.fieldValue && ((data.exportValue && data.exportValue === data.fieldValue) || @@ -962,7 +962,7 @@ class RadioButtonWidgetAnnotationElement extends WidgetAnnotationElement { const storage = this.annotationStorage; const data = this.data; const id = data.id; - const value = storage.getOrCreateValue(id, { + const value = storage.getValue(id, { value: data.fieldValue === data.buttonValue, }).value; @@ -1072,9 +1072,9 @@ class ChoiceWidgetAnnotationElement extends WidgetAnnotationElement { // are not properly printed/saved yet, so we only store the first item in // the field value array instead of the entire array. Once support for those // two field types is implemented, we should use the same pattern as the - // other interactive widgets where the return value of `getOrCreateValue` is - // used and the full array of field values is stored. - storage.getOrCreateValue(id, { + // other interactive widgets where the return value of `getValue` + // is used and the full array of field values is stored. + storage.getValue(id, { value: this.data.fieldValue.length > 0 ? this.data.fieldValue[0] : undefined, }); diff --git a/src/display/annotation_storage.js b/src/display/annotation_storage.js index abc6dacfc..ae30af211 100644 --- a/src/display/annotation_storage.js +++ b/src/display/annotation_storage.js @@ -13,6 +13,7 @@ * limitations under the License. */ +import { deprecated } from "./display_utils.js"; import { objectFromEntries } from "../shared/util.js"; /** @@ -33,7 +34,7 @@ class AnnotationStorage { /** * Get the value for a given key if it exists - * or store and return the default value + * or return the default value * * @public * @memberof AnnotationStorage @@ -41,7 +42,19 @@ class AnnotationStorage { * @param {Object} defaultValue * @returns {Object} */ + getValue(key, defaultValue) { + if (this._storage.has(key)) { + return this._storage.get(key); + } + + return defaultValue; + } + + /** + * @deprecated + */ getOrCreateValue(key, defaultValue) { + deprecated("Use getValue instead."); if (this._storage.has(key)) { return this._storage.get(key); } diff --git a/test/unit/annotation_spec.js b/test/unit/annotation_spec.js index d647fbe42..fb2a17875 100644 --- a/test/unit/annotation_spec.js +++ b/test/unit/annotation_spec.js @@ -1808,7 +1808,7 @@ describe("annotation", function () { }, done.fail) .then(appearance => { expect(appearance).toEqual( - "/Tx BMC q BT /Helv 11 Tf 0 g 1 0 0 1 0 0 Tm" + + "/Tx BMC q BT /Helv 8 Tf 0 g 1 0 0 1 0 0 Tm" + " 2.00 2.00 Td (test \\(print\\)) Tj ET Q EMC" ); done(); @@ -1848,7 +1848,7 @@ describe("annotation", function () { "\x30\x53\x30\x93\x30\x6b\x30\x61" + "\x30\x6f\x4e\x16\x75\x4c\x30\x6e"; expect(appearance).toEqual( - "/Tx BMC q BT /Goth 9 Tf 0 g 1 0 0 1 0 0 Tm" + + "/Tx BMC q BT /Goth 8 Tf 0 g 1 0 0 1 0 0 Tm" + ` 2.00 2.00 Td (${utf16String}) Tj ET Q EMC` ); done(); diff --git a/test/unit/annotation_storage_spec.js b/test/unit/annotation_storage_spec.js index f8fa46abd..8ba4ee15e 100644 --- a/test/unit/annotation_storage_spec.js +++ b/test/unit/annotation_storage_spec.js @@ -16,17 +16,21 @@ import { AnnotationStorage } from "../../src/display/annotation_storage.js"; describe("AnnotationStorage", function () { - describe("GetOrCreateValue", function () { + describe("GetOrDefaultValue", function () { it("should get and set a new value in the annotation storage", function (done) { const annotationStorage = new AnnotationStorage(); - let value = annotationStorage.getOrCreateValue("123A", { + let value = annotationStorage.getValue("123A", { value: "hello world", }).value; expect(value).toEqual("hello world"); + annotationStorage.setValue("123A", { + value: "hello world", + }); + // the second argument is the default value to use // if the key isn't in the storage - value = annotationStorage.getOrCreateValue("123A", { + value = annotationStorage.getValue("123A", { value: "an other string", }).value; expect(value).toEqual("hello world"); @@ -50,16 +54,18 @@ describe("AnnotationStorage", function () { called = true; }; annotationStorage.onSetModified = callback; - annotationStorage.getOrCreateValue("asdf", { value: "original" }); - expect(called).toBe(false); - // not changing value annotationStorage.setValue("asdf", { value: "original" }); - expect(called).toBe(false); + expect(called).toBe(true); // changing value annotationStorage.setValue("asdf", { value: "modified" }); expect(called).toBe(true); + + // not changing value + called = false; + annotationStorage.setValue("asdf", { value: "modified" }); + expect(called).toBe(false); done(); }); }); @@ -72,7 +78,10 @@ describe("AnnotationStorage", function () { called = true; }; annotationStorage.onResetModified = callback; - annotationStorage.getOrCreateValue("asdf", { value: "original" }); + annotationStorage.setValue("asdf", { value: "original" }); + annotationStorage.resetModified(); + expect(called).toBe(true); + called = false; // not changing value annotationStorage.setValue("asdf", { value: "original" });