[Editor] Handle correctly colors when saving a document in HCM #15115
Conversation
src/display/editor/tools.js
Outdated
| ]); | ||
|
|
||
| get _colors() { | ||
| if (typeof document !== "undefined") { |
There was a problem hiding this comment.
Why do we need this check, since I don't believe this code is expected to run outside of browsers?
Also, in the method just below you're accessing window directly without checking if it's available first.
There was a problem hiding this comment.
I added this check to fix gulp unittestcli which was unhappy with that (likely because a static property is using it)... so I chose the easiest solution.
There was a problem hiding this comment.
Ah, that explains things. In that case, since this doesn't matter in browsers, please see the suggestion below (which is based on what we do in the viewer-code in a couple of spots).
src/display/editor/tools.js
Outdated
| if (!rgb) { | ||
| return name; | ||
| } | ||
| return "#" + rgb.map(x => x.toString(16).padStart(2, "0")).join(""); |
There was a problem hiding this comment.
Wait, isn't this identical to the existing Util.makeHexColor method?
Please use that one instead, rather than reinventing the wheel here :-)
There was a problem hiding this comment.
Especially when I created this wheel myself...
src/display/editor/tools.js
Outdated
| if (typeof document !== "undefined") { | ||
| const colors = new Map([ | ||
| ["CanvasText", null], | ||
| ["Canvas", null], | ||
| ]); | ||
| getColorValues(colors); | ||
| return shadow(this, "_colors", colors); | ||
| } | ||
|
|
||
| return shadow(this, "_colors", ColorManager._colorsMapping); |
There was a problem hiding this comment.
| if (typeof document !== "undefined") { | |
| const colors = new Map([ | |
| ["CanvasText", null], | |
| ["Canvas", null], | |
| ]); | |
| getColorValues(colors); | |
| return shadow(this, "_colors", colors); | |
| } | |
| return shadow(this, "_colors", ColorManager._colorsMapping); | |
| if ( | |
| typeof PDFJSDev !== "undefined" && | |
| PDFJSDev.test("LIB") && | |
| typeof document === "undefined" | |
| ) { | |
| return shadow(this, "_colors", ColorManager._colorsMapping); | |
| } | |
| const colors = new Map([ | |
| ["CanvasText", null], | |
| ["Canvas", null], | |
| ]); | |
| getColorValues(colors); | |
| return shadow(this, "_colors", colors); | |
| } |
calixteman
force-pushed
the
editing_a11y_bis
branch
2 times, most recently
from
b01273d
to
9899d58
Compare
src/pdf.js
Outdated
| @@ -123,6 +124,7 @@ export { | |||
| getDocument, | |||
| getFilenameFromUrl, | |||
| getPdfFilenameFromUrl, | |||
| getRGB, | |||
There was a problem hiding this comment.
Why is this necessary, since I'm not finding anything that depends on this?
As a general rule, please let's not expose things in the API unless it's actually needed e.g. in the viewer.
calixteman
force-pushed
the
editing_a11y_bis
branch
from
9899d58
to
7d06392
Compare
|
/botio test |
1 similar comment
|
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/6690445c2358ce2/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @calixteman received. Current queue size: 1 Live output at: http://54.193.163.58:8877/21adb5d52137d3f/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/6690445c2358ce2/output.txt Total script time: 21.20 mins
Image differences available at: http://54.241.84.105:8877/6690445c2358ce2/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/21adb5d52137d3f/output.txt Total script time: 23.61 mins
Image differences available at: http://54.193.163.58:8877/21adb5d52137d3f/reftest-analyzer.html#web=eq.log |
|
/botio browsertest |
From: Bot.io (Linux m4)ReceivedCommand cmd_browsertest from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/932885268a25887/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_browsertest from @calixteman received. Current queue size: 0 Live output at: http://54.193.163.58:8877/8270b401e61a3fd/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/932885268a25887/output.txt Total script time: 3.95 mins
Image differences available at: http://54.241.84.105:8877/932885268a25887/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/8270b401e61a3fd/output.txt Total script time: 5.19 mins
Image differences available at: http://54.193.163.58:8877/8270b401e61a3fd/reftest-analyzer.html#web=eq.log |
|
/botio-windows browsertest |
|
So it's my fault: |
- for example in Dusk theme (Windows 11), black appears to be white, so the user will draw something in white. But if they want to print or save the used color must be black. - fix a bug with the color input which only accepts hex string colors; - adjust outline color of the selected/hovered editors in HCM.
calixteman
force-pushed
the
editing_a11y_bis
branch
from
7d06392
to
9723c5d
Compare
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/2c6b6e4d01dee2c/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @calixteman received. Current queue size: 0 Live output at: http://54.193.163.58:8877/9bf75ff6f00489c/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/2c6b6e4d01dee2c/output.txt Total script time: 25.90 mins
Image differences available at: http://54.241.84.105:8877/2c6b6e4d01dee2c/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/9bf75ff6f00489c/output.txt Total script time: 28.36 mins
Image differences available at: http://54.193.163.58:8877/9bf75ff6f00489c/reftest-analyzer.html#web=eq.log |
|
To fix the issue, I lazily get the static values computed thanks to @Snuffleupagus does that work for you ? |
the user will draw something in white. But if they want to print or
save the used color must be black.