wip - add support for defaultAppearance fonts in text areas #12207

escapewindow · 2020-08-13T03:24:44Z

This works! If I override the DA with something like /CourierNewPSMT 8 Tf 0 g in i9.pdf, then the text I type in a textarea shows up as Courier instead of Helvetica.

I do have some questions.

I went the evaluator route because I wanted to load the font in our font cache and get a fontRefName back. I wasn't sure if we also wanted to preserve the OperatorList ?
- Because I'm using the evaluator, I need a handler and task. I added calls to both Page.getOperatorList and Page.extractTextContent to reuse those handlers and tasks. I'm not sure if there's a more elegant way.
I'm currently loading the defaultAppearanceData once per page, which might be good if we need the OperatorList or populate the font cache per page. Otherwise we might be able to load it once per PDFDocument. (I'm trying the PDFDocument solution in https://github.com/escapewindow/pdf.js/commits/page-acrodict-wip and it breaks the entire form badly. It's quite possible I'm doing something wrong, however.)
I'm unable to do anything with fontColor. I have an rgb color, but setting style.color doesn't seem to do anything. If I console.log(style.color) before and after, it's unchanged. Maybe it's read-only (or maybe I did something wrong again)?

Most of this is new code, but it's based on @dmitryskey's code from #8625. I commited the rebased patch with @dmitryskey as the author, so we both show up.

What do you think @brendandahl ?

escapewindow · 2020-08-13T03:26:35Z

I do have a text-only parsing branch here, if the evaluator solution is too heavy handed.

escapewindow · 2020-08-18T01:40:36Z

Trying to parse the DA of each widget here; that's not currently working.

escapewindow · 2020-09-10T02:42:34Z

This is rebased and working again.

@timvandermeij @Snuffleupagus @brendandahl What are your thoughts here? We no longer have the additional AnnotationWorkerTask from #8625, but we're only parsing the main document DA and DR and not parsing each widget's DA. As mentioned above, I've attempted other approaches that try to combine the approaches. I haven't yet been successful. Other options might include:

Drop the evaluator getAcroformDefaultAppearanceData in favor of a text parser like this,
add the evaluator call to each widget
go with @dmitryskey's approach of adding an AnnotationWorkerTask.

timvandermeij

First of all, sorry for the review delay. It has been quite busy lately with all the AcroForm work, so I only just now had time to take a look at this. I have added some initial review comments. I also think/hope some things can be simplified; preferably I'd have the logic mostly in the annotation code files and not in the document file, but it will require a bit more time to check if that is possible.

timvandermeij · 2020-09-14T22:32:38Z

src/core/annotation.js

@@ -242,6 +254,7 @@ class Annotation {
  constructor(params) {
    const dict = params.dict;

+    this.defaultAppearanceData = params.defaultAppearanceData;


I think this can be removed since the only place that uses defaultAppearanceData is in the TextWidgetAnnotation class and it fetches it from the params, not from this member variable.

timvandermeij · 2020-09-14T22:33:34Z

src/core/annotation.js

@@ -1285,6 +1298,14 @@ class WidgetAnnotation extends Annotation {
 class TextWidgetAnnotation extends WidgetAnnotation {
  constructor(params) {
    super(params);
+    if (params.defaultAppearanceData) {


I think you already indicated this, but individual annotations might have their own appearance, so we should parse that one instead and the global one should only be used as a fallback in that case.

What I hope that we can accomplish is some unit, let's say a class or function, that we can feed a StringStream that is the DA of the annotation or the document, that parses it using the partial evaluator and that outputs the font information. That was also what we tried to create in the original patch, but didn't finish.

timvandermeij · 2020-09-14T22:36:56Z

src/core/document.js

+    const resources = this._getInheritableProperty("Resources") || Dict.empty;
+    const acroForm = this.pdfManager.pdfDocument.catalog.acroForm;
+    if (acroForm && acroForm.get("DR")) {
+      resources.defaultResources = acroForm.get("DR");


Hm, I wouldn't expect the normal resources dictionary to be altered with a non-standard attribute. Shouldn't we merge the dictionaries instead? Perhaps the just merged https://github.com/mozilla/pdf.js/pull/12361/files can serve as inspiration.

timvandermeij · 2020-09-14T22:38:56Z

src/core/document.js

+    const opList = new OperatorList();
+
+    const appearanceStream = new StringStream(this.defaultAppearance);
+    this.defaultAppearanceData = Dict.empty;


We should use new Dict here since we should never mutate Dict.empty as it's always an empty dictionary. Note that we just merged a patch that makes calling set on Dict.empty impossible, so if you rebase this patch this will stop working.

timvandermeij · 2020-09-14T22:40:33Z

src/core/document.js

@@ -552,7 +602,7 @@ class PDFDocument {
    this.stream = stream;
    this.xref = new XRef(stream, pdfManager);
    this._pagePromises = [];
-    this._version = null;


this._version should stay.

timvandermeij · 2020-09-14T22:46:21Z

src/display/annotation_layer.js

@@ -480,22 +481,28 @@ class TextWidgetAnnotationElement extends WidgetAnnotationElement {
        element.classList.add("comb");
        element.style.letterSpacing = `calc(${combWidth}px - 1ch)`;
      }
+      if (


I don't think we actually want this in edit mode. Edit mode has other styles to clearly differentiate it from display mode. In display mode we do want this, and that's why it only lived in the else clause. The objective of this patch is also to make the final rendering on display/print/save in the right style.

timvandermeij · 2020-09-14T22:46:50Z

src/display/annotation_layer.js

@@ -515,7 +522,9 @@ class TextWidgetAnnotationElement extends WidgetAnnotationElement {
  _setTextStyle(element, font) {
    // TODO: This duplicates some of the logic in CanvasGraphics.setFont().
    const style = element.style;
-    style.fontSize = `${this.data.fontSize}px`;
+    if (this.data.fontSize) {


Shouldn't this always be set?

timvandermeij · 2020-09-14T22:47:32Z

src/display/annotation_layer.js

@@ -535,6 +544,7 @@ class TextWidgetAnnotationElement extends WidgetAnnotationElement {
    const fontFamily = font.loadedName ? `"${font.loadedName}", ` : "";
    const fallbackName = font.fallbackName || "Helvetica, sans-serif";
    style.fontFamily = fontFamily + fallbackName;
+    // XXX revisit fontColor. Setting style.color doesn't stick.


Hm, I think this worked in the original patch. Maybe the values need to be converted to RGB first?

timvandermeij · 2020-09-14T22:53:54Z

test/unit/annotation_spec.js

@@ -1863,6 +1863,36 @@ describe("annotation", function () {
          done();
        }, done.fail);
    });
+
+    it("should expose document defaultAppearance", function (done) {


"should use the default appearance of the document"

timvandermeij · 2020-09-14T22:54:09Z

test/unit/annotation_spec.js

@@ -1863,6 +1863,36 @@ describe("annotation", function () {
          done();
        }, done.fail);
    });
+
+    it("should expose document defaultAppearance", function (done) {
+      const defaultAppearanceData = Dict.empty;


Same here for new Dict instead of mutating Dict.empty.

timvandermeij · 2021-01-29T21:08:52Z

Closing since this PR is superseded by #12831 (default appearance parser) and #12828 (using the default appearance data in the display layer).

escapewindow mentioned this pull request Aug 13, 2020

WIP - Parse acroform dictionary #12139

Closed

11 tasks

timvandermeij added the annotations label Aug 16, 2020

escapewindow changed the title ~~add support for defaultAppearance fonts in text areas~~ wip - add support for defaultAppearance fonts in text areas Sep 10, 2020

getAcroformDefaultAppearanceData

52689b8

timvandermeij reviewed Sep 14, 2020

View reviewed changes

timvandermeij closed this Jan 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wip - add support for defaultAppearance fonts in text areas #12207

wip - add support for defaultAppearance fonts in text areas #12207

escapewindow commented Aug 13, 2020 •

edited

Loading

escapewindow commented Aug 13, 2020

escapewindow commented Aug 18, 2020

escapewindow commented Sep 10, 2020

timvandermeij left a comment

timvandermeij Sep 14, 2020

timvandermeij Sep 14, 2020 •

edited

Loading

timvandermeij Sep 14, 2020

timvandermeij Sep 14, 2020 •

edited

Loading

timvandermeij Sep 14, 2020

timvandermeij Sep 14, 2020

timvandermeij Sep 14, 2020

timvandermeij Sep 14, 2020

timvandermeij Sep 14, 2020

timvandermeij Sep 14, 2020

timvandermeij commented Jan 29, 2021

wip - add support for defaultAppearance fonts in text areas #12207

wip - add support for defaultAppearance fonts in text areas #12207

Conversation

escapewindow commented Aug 13, 2020 • edited Loading

escapewindow commented Aug 13, 2020

escapewindow commented Aug 18, 2020

escapewindow commented Sep 10, 2020

timvandermeij left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij Sep 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij Sep 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij commented Jan 29, 2021

escapewindow commented Aug 13, 2020 •

edited

Loading

timvandermeij Sep 14, 2020 •

edited

Loading

timvandermeij Sep 14, 2020 •

edited

Loading