[editor] Add some UI elements in order to set font size & color, and ink thickness & color #15039

calixteman · 2022-06-13T16:26:32Z

No description provided.

Snuffleupagus

I've not really had time to test this yet, but I've added a few comments based on an initial look at the code.
While this seems mostly reasonable, there's a couple of things (e.g. related to the Toolbar-positions of the buttons) that we need to fix here.

src/display/editor/tools.js

web/annotation_editor_params.js

web/base_viewer.js

web/viewer.css

web/viewer.html

Snuffleupagus · 2022-06-15T13:28:26Z

web/viewer.css

+#editorParamsToolbarContainer .editorParamsSlider::-moz-range-progress {
+  background-color: black;
+}
+#editorParamsToolbarContainer .editorParamsSlider::-moz-range-track {
+  background-color: black;
+}
+#editorParamsToolbarContainer .editorParamsSlider::-moz-range-thumb {
+  background-color: white;
+}


Are these not available as standard CSS features instead (since it won't work in the GENERIC viewer)?

You probably want to leave these enabled everywhere though, such that the GENERIC viewer works better when used standalone in Firefox.

I was mostly wondering if there's anything similar for other browsers; sorry if that wasn't clear.

web/viewer.css

src/shared/util.js

web/viewer.css

src/display/editor/freetext.js

src/display/editor/ink.js

src/display/editor/freetext.js

src/display/editor/ink.js

web/app.js

web/toolbar.js

web/viewer.html

Snuffleupagus · 2022-06-23T10:56:51Z

I'll try to review this PR during the weekend, since the size/scope of the changes means that it takes a little time to try and understand everything here; sorry about the delay!

Snuffleupagus

I wanted to create a preview to play with this a little bit, and to see how the new CSS-rules behave, however it needs a rebase first for that to work.

src/display/display_utils.js

Snuffleupagus · 2022-06-26T10:54:57Z

src/display/editor/freetext.js

@@ -89,6 +95,94 @@ class FreeTextEditor extends AnnotationEditor {
    return editor;
  }

+  static updateDefaultParams(type, value) {


Why do we want to update the default values, and not just the values for the current instance?

I'm not sure either it's a good idea: we need to know what UX thinks about that.
Anyway it works like this in Acrobat.

src/display/editor/freetext.js

src/display/editor/ink.js

web/annotation_editor_params.js

web/app.js

web/viewer.css

Snuffleupagus · 2022-06-27T12:46:57Z

/botio-linux preview

pdfjsbot · 2022-06-27T12:46:59Z

From: Bot.io (Linux m4)

Received

Command cmd_preview from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/486fa915e784836/output.txt

pdfjsbot · 2022-06-27T12:49:45Z

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/486fa915e784836/output.txt

Total script time: 2.76 mins

Published

Viewer: http://54.241.84.105:8877/486fa915e784836/web/viewer.html
Viewer (legacy): http://54.241.84.105:8877/486fa915e784836/legacy/web/viewer.html

Snuffleupagus · 2022-06-27T13:42:00Z

web/viewer.css

+  padding-inline: 12px;
+}
+
+#editorParamsToolbarContainer .editorParamsLabel {


These labels are almost invisible in the "light"-CSS viewer mode.
Similar to the regular toolbarLabel-class, you'll definitely need to add the following rule here as well:

color: var(--main-color);

Snuffleupagus · 2022-06-27T13:52:14Z

web/viewer.html

@@ -147,6 +147,32 @@
          </div>
        </div>  <!-- findbar -->

+        <div class="editorParamsToolbar hidden doorHangerRight" id="editorFreeTextParamsToolbar">
+          <div id="editorParamsToolbarContainer">


You're adding the same id for two different elements here, which feels strange.
What you probably want, here and below, is to use a class-name instead:

Suggested change

<div id="editorParamsToolbarContainer">

<div class="editorParamsToolbarContainer">

Please remember to make the same changes throughout the CSS-code as well.

Snuffleupagus · 2022-06-27T13:52:32Z

web/viewer.html

+        </div>
+
+        <div class="editorParamsToolbar hidden doorHangerRight" id="editorInkParamsToolbar">
+          <div id="editorParamsToolbarContainer">


Suggested change

<div id="editorParamsToolbarContainer">

<div class="editorParamsToolbarContainer">

Snuffleupagus · 2022-06-27T13:53:23Z

web/viewer.css

  padding: 6px 0 10px;
  inset-inline-end: 4px;
  height: auto;
  z-index: 30000;
  background-color: var(--doorhanger-bg-color);
 }

+#editorParamsToolbarContainer > .editorParamsSetter {
+  max-width: 220px;


I'm finding it slightly inconsistent that the width of the two new toolbars aren't consistent.
So, can we perhaps remove this line, and instead add the following just above?

.editorParamsToolbarContainer { width: 220px; margin-bottom: -4px; }

(The second line again follows the secondaryToolbar code, to get consitent top/bottom margins.)

Snuffleupagus · 2022-06-27T13:56:57Z

web/viewer.html

+              <input type="color" id="editorFreeTextColor" name="fontColor" class="editorParamsColor" tabindex="100">
+              <label for="fontColor" class="editorParamsLabel" data-l10n-id="editor_free_text_font_color">Font Color</label>


Would it, perhaps, look better with the order of the labels/inputs flipped?

Snuffleupagus · 2022-06-27T14:21:04Z

web/viewer.css

+  display: flex;
+  align-items: center;
+  justify-content: space-between;
+  padding-inline: 12px;


Also, do we perhaps want to reduce this a little bit (e.g. to 10px)?

Snuffleupagus · 2022-06-28T08:35:49Z

/botio-linux preview

pdfjsbot · 2022-06-28T08:35:50Z

From: Bot.io (Linux m4)

Received

Command cmd_preview from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/fd81324e58f9554/output.txt

pdfjsbot · 2022-06-28T08:38:28Z

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/fd81324e58f9554/output.txt

Total script time: 2.63 mins

Published

Viewer: http://54.241.84.105:8877/fd81324e58f9554/web/viewer.html
Viewer (legacy): http://54.241.84.105:8877/fd81324e58f9554/legacy/web/viewer.html

Snuffleupagus

r=me, with the last round of comments addressed; thank you!

Snuffleupagus · 2022-06-28T08:40:33Z

web/viewer.html

+              <input type="color" id="editorInkColor" name="inkColor" class="editorParamsColor" tabindex="102">
+              <label for="color" class="editorParamsLabel" data-l10n-id="editor_ink_line_color">Line Color</label>


Let's flip the order of these elements as well, for consistency with the FreeText-toolbar.

Just for your information, I copied/pasted this code from:

pdf.js/web/viewer.html

Lines 131 to 136 in 4e025e1

<div id="findbarOptionsOneContainer">

<input type="checkbox" id="findHighlightAll" class="toolbarField" tabindex="94">

<label for="findHighlightAll" class="toolbarLabel" data-l10n-id="find_highlight">Highlight All</label>

<input type="checkbox" id="findMatchCase" class="toolbarField" tabindex="95">

<label for="findMatchCase" class="toolbarLabel" data-l10n-id="find_match_case_label">Match Case</label>

</div>

where it's input, label.

I understand that, however please note that in those cases only the label is actually visible; hence it's a slightly different situation :-)

Snuffleupagus · 2022-06-28T08:41:19Z

web/viewer.html

+              <input type="range" id="editorInkThickness" class="editorParamsSlider" name="lineThickness" value="1" min="1" max="20" step="1" tabindex="103">
+              <label for="lineThickness" class="editorParamsLabel" data-l10n-id="editor_ink_line_thickness">Line Thickness</label>


Let's flip the order of these elements as well, for consistency with the FreeText-toolbar.

Snuffleupagus · 2022-06-28T08:45:30Z

web/viewer.html

+              <label for="fontColor" class="editorParamsLabel" data-l10n-id="editor_free_text_font_color">Font Color</label>
+              <input type="color" id="editorFreeTextColor" name="fontColor" class="editorParamsColor" tabindex="100">


Shouldn't the for-attribute reference the input-id in order to work correctly?

Basically, I believe that you want the following (with similar changes for the new elements below):

Suggested change

<label for="fontColor" class="editorParamsLabel" data-l10n-id="editor_free_text_font_color">Font Color</label>

<input type="color" id="editorFreeTextColor" name="fontColor" class="editorParamsColor" tabindex="100">

<label for="editorFreeTextColor" class="editorParamsLabel" data-l10n-id="editor_free_text_font_color">Font Color</label>

<input type="color" id="editorFreeTextColor" class="editorParamsColor" tabindex="100">

Snuffleupagus · 2022-06-28T08:48:00Z

web/viewer.css

+}
+
+.editorParamsToolbarContainer .editorParamsLabel {
+  padding-inline-start: 10px;


This should probably be changed to padding-inline-end now?

…ink thickness & color

calixteman · 2022-06-28T10:07:08Z

/botio test

pdfjsbot · 2022-06-28T10:07:10Z

From: Bot.io (Linux m4)

Received

Command cmd_test from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/88fd07bb59f7dad/output.txt

pdfjsbot · 2022-06-28T10:07:10Z

From: Bot.io (Windows)

Received

Command cmd_test from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/534c35c559dca77/output.txt

pdfjsbot · 2022-06-28T10:33:42Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/88fd07bb59f7dad/output.txt

Total script time: 26.53 mins

Font tests: Passed
Unit tests: FAILED
Integration Tests: Passed
Regression tests: FAILED

  different ref/snapshot: 27

Image differences available at: http://54.241.84.105:8877/88fd07bb59f7dad/reftest-analyzer.html#web=eq.log

pdfjsbot · 2022-06-28T10:35:35Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/534c35c559dca77/output.txt

Total script time: 28.41 mins

Font tests: Passed
Unit tests: Passed
Integration Tests: FAILED
Regression tests: FAILED

  different ref/snapshot: 2

Image differences available at: http://54.193.163.58:8877/534c35c559dca77/reftest-analyzer.html#web=eq.log

calixteman · 2022-06-28T11:14:48Z

The "regressions" in ref tests are because I changed the linecap, hence they're acceptable.

Snuffleupagus · 2022-06-28T11:25:17Z

/botio makeref

pdfjsbot · 2022-06-28T11:25:20Z

From: Bot.io (Linux m4)

Received

Command cmd_makeref from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/7919825f1ca22cc/output.txt

pdfjsbot · 2022-06-28T11:25:20Z

From: Bot.io (Windows)

Received

Command cmd_makeref from @Snuffleupagus received. Current queue size: 1

Live output at: http://54.193.163.58:8877/b17a0e005f4040b/output.txt

pdfjsbot · 2022-06-28T11:47:59Z

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/7919825f1ca22cc/output.txt

Total script time: 22.64 mins

Lint: Passed
Make references: Passed
Check references: Passed

pdfjsbot · 2022-06-28T11:51:17Z

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/b17a0e005f4040b/output.txt

Total script time: 22.50 mins

Lint: Passed
Make references: Passed
Check references: Passed

calixteman added the editor label Jun 13, 2022

calixteman requested a review from Snuffleupagus June 13, 2022 16:26

This was linked to issues Jun 13, 2022

Support changing color to draw/write text #14972

Closed

Support changing line thickness when drawing #15017

Closed

marco-c linked an issue Jun 13, 2022 that may be closed by this pull request

Support changing font size #14973

Closed

Snuffleupagus requested changes Jun 15, 2022

View reviewed changes

Snuffleupagus reviewed Jun 15, 2022

View reviewed changes

web/viewer.css Outdated Show resolved Hide resolved

calixteman force-pushed the params branch from f6a497f to 9522bf9 Compare June 16, 2022 17:38

calixteman requested a review from Snuffleupagus June 16, 2022 17:38

calixteman force-pushed the params branch from 9522bf9 to 3874dc7 Compare June 16, 2022 18:36

Snuffleupagus reviewed Jun 16, 2022

View reviewed changes

calixteman force-pushed the params branch from 3874dc7 to ca4db4e Compare June 21, 2022 12:49

calixteman requested a review from Snuffleupagus June 21, 2022 12:50

calixteman force-pushed the params branch 4 times, most recently from 9002d3c to 79d78f6 Compare June 21, 2022 17:07

Snuffleupagus reviewed Jun 26, 2022

View reviewed changes

calixteman force-pushed the params branch from 79d78f6 to e2f3af2 Compare June 27, 2022 09:44

Snuffleupagus reviewed Jun 27, 2022

View reviewed changes

calixteman force-pushed the params branch from e2f3af2 to b213c46 Compare June 27, 2022 21:10

calixteman requested a review from Snuffleupagus June 27, 2022 21:11

Snuffleupagus approved these changes Jun 28, 2022

View reviewed changes

[editor] Add some UI elements in order to set font size & color, and …

1a3ef2a

…ink thickness & color

calixteman force-pushed the params branch from b213c46 to 1a3ef2a Compare June 28, 2022 10:06

Snuffleupagus merged commit f1d4015 into mozilla:master Jun 28, 2022

ZeroXClem mentioned this pull request Aug 12, 2024

[Snyk] Upgrade pdfjs-dist from 2.9.359 to 2.16.105 ZeroXClem/metamesa#3

Closed

earthywh mentioned this pull request Sep 24, 2024

[Snyk] Upgrade pdfjs-dist from 2.6.347 to 2.16.105 earthywh/filestash#1

Open

	<div id="editorParamsToolbarContainer">
	<div class="editorParamsToolbarContainer">

		<input type="color" id="editorFreeTextColor" name="fontColor" class="editorParamsColor" tabindex="100">
		<label for="fontColor" class="editorParamsLabel" data-l10n-id="editor_free_text_font_color">Font Color</label>

		<input type="color" id="editorInkColor" name="inkColor" class="editorParamsColor" tabindex="102">
		<label for="color" class="editorParamsLabel" data-l10n-id="editor_ink_line_color">Line Color</label>

	<div id="findbarOptionsOneContainer">
	<input type="checkbox" id="findHighlightAll" class="toolbarField" tabindex="94">
	<label for="findHighlightAll" class="toolbarLabel" data-l10n-id="find_highlight">Highlight All</label>
	<input type="checkbox" id="findMatchCase" class="toolbarField" tabindex="95">
	<label for="findMatchCase" class="toolbarLabel" data-l10n-id="find_match_case_label">Match Case</label>
	</div>

		<input type="range" id="editorInkThickness" class="editorParamsSlider" name="lineThickness" value="1" min="1" max="20" step="1" tabindex="103">
		<label for="lineThickness" class="editorParamsLabel" data-l10n-id="editor_ink_line_thickness">Line Thickness</label>

[editor] Add some UI elements in order to set font size & color, and ink thickness & color #15039

[editor] Add some UI elements in order to set font size & color, and ink thickness & color #15039

Conversation

calixteman commented Jun 13, 2022

Snuffleupagus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus Jun 16, 2022 • edited Loading

Choose a reason for hiding this comment

Snuffleupagus commented Jun 23, 2022

Snuffleupagus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus commented Jun 27, 2022

pdfjsbot commented Jun 27, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jun 27, 2022

From: Bot.io (Linux m4)

Success

Published

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

Snuffleupagus Jun 27, 2022 • edited Loading

Choose a reason for hiding this comment

Snuffleupagus Jun 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus commented Jun 28, 2022

pdfjsbot commented Jun 28, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jun 28, 2022

From: Bot.io (Linux m4)

Success

Published

Snuffleupagus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calixteman Jun 28, 2022 • 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

calixteman commented Jun 28, 2022

pdfjsbot commented Jun 28, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jun 28, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Jun 28, 2022

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Jun 28, 2022

From: Bot.io (Windows)

Failed

calixteman commented Jun 28, 2022

Snuffleupagus commented Jun 28, 2022

pdfjsbot commented Jun 28, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jun 28, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Jun 28, 2022

From: Bot.io (Linux m4)

Success

pdfjsbot commented Jun 28, 2022

From: Bot.io (Windows)

Success

Snuffleupagus Jun 16, 2022 •

edited

Loading

Snuffleupagus Jun 27, 2022 •

edited

Loading

Snuffleupagus Jun 27, 2022 •

edited

Loading

calixteman Jun 28, 2022 •

edited

Loading