Skip to content
Snippets Groups Projects
Unverified Commit 97575697 authored by adam-james's avatar adam-james Committed by GitHub
Browse files

Wrap non-latin characters in a span specifying working font (#44580)

* Wrap non-latin characters in a span specifying working font

Fixes: #38753

CSSBox seems to have a bug where font fallback doesn't work properly. This is noticeable when a font does not contain
glyphs that are present in the string being rendered. For example, Lato does not have many international characters,
so the rendered version of tables (that show up in Slack messages) will not render properly, making the card
unreadable.

Since this appears to be a downstream bug, I've opted to work around this limitation by wrapping any non-latin
characters in a <span> that specifies the font family to be sans-serif, which should contain the glyphs to properly
render.

This leaves Lato in place for other characters.

For now, I figured it's worth trying this solution over using Noto for 2 reasons:
- we can keep Lato, which has been the decided font style for the app for some time (this keeps things consistent
where possible)
- the Noto font containing all glyphs is quite a large font (>20mb, I think) and it would be nice to avoid bundling
that if we can.

* stub installed fonts test

* typo

* Do wrapping, but now per-string, and in Clojure data not html string

I've decided that a reasonable solution is to still wrap strings containing non-lato characters. But it's not done
with str/replace to the html string but rather in a postwalk over the Hiccup data prior to rendering.

Seems like a decent compromise of issues without patching CSSBox or fixing upstream (may be good to do, but will take
longer to get a fix in).

* add test checking that glyphs render correctly

* Add a test that directly checks the wrapping fn

* Change the string to keep the linter quiet

* Change how we check if string can be rendered to faster method, new Lato Font

With Sanya's help, the way I check if a given string is renderable with Lato is now faster.

Also use the full Lato font, not just the 'latin' lato so we can cover more chars

* change lato so that it loads the fn even in a fresh test run
parent 6f91db0e
No related branches found
No related tags found
No related merge requests found
...@@ -498,6 +498,7 @@ ...@@ -498,6 +498,7 @@
"frontend/test/metabase/lib/urls\\.unit\\.spec\\.js$" "frontend/test/metabase/lib/urls\\.unit\\.spec\\.js$"
"frontend/test/metabase/lib/formatting\\.unit\\.spec\\.js$" "frontend/test/metabase/lib/formatting\\.unit\\.spec\\.js$"
"src/metabase/shared/util/currency\\.cljc$" "src/metabase/shared/util/currency\\.cljc$"
"test/metabase/pulse/render/png_test.clj"
"#.+#$" "#.+#$"
"\\.transit\\.json$"]}} "\\.transit\\.json$"]}}
......
File added
...@@ -7,6 +7,7 @@ ...@@ -7,6 +7,7 @@
CSSBox JavaDoc is here: http://cssbox.sourceforge.net/api/index.html" CSSBox JavaDoc is here: http://cssbox.sourceforge.net/api/index.html"
(:require (:require
[clojure.walk :as walk]
[hiccup.core :refer [html]] [hiccup.core :refer [html]]
[metabase.formatter :as formatter] [metabase.formatter :as formatter]
[metabase.pulse.render.style :as style] [metabase.pulse.render.style :as style]
...@@ -14,7 +15,7 @@ ...@@ -14,7 +15,7 @@
[metabase.util.malli :as mu]) [metabase.util.malli :as mu])
(:import (:import
(cz.vutbr.web.css MediaSpec) (cz.vutbr.web.css MediaSpec)
(java.awt Graphics2D RenderingHints) (java.awt Font GraphicsEnvironment Graphics2D RenderingHints)
(java.awt.image BufferedImage) (java.awt.image BufferedImage)
(java.io ByteArrayInputStream ByteArrayOutputStream) (java.io ByteArrayInputStream ByteArrayOutputStream)
(java.nio.charset StandardCharsets) (java.nio.charset StandardCharsets)
...@@ -73,16 +74,65 @@ ...@@ -73,16 +74,65 @@
(.getSubimage image 0 0 content-width (.getHeight image)) (.getSubimage image 0 0 content-width (.getHeight image))
image))))) image)))))
(defn- font-can-fully-render?
[^Font font s]
(neg? (.canDisplayUpTo font s)))
(def ^:private get-lato
(letfn [(get-lato* []
(let [lato-names #{"Lato Regular" "Lato-Regular" "lato" "lato-regular"}
env (GraphicsEnvironment/getLocalGraphicsEnvironment)
fonts (.getAllFonts env)
font ^Font (some #(when (lato-names (.getName ^Font %)) %) fonts)]
font))]
(memoize get-lato*)))
(defn- lato-can-render?
[s]
(let [lato (get-lato)]
(when lato
(font-can-fully-render? lato s))))
(defn- wrap-non-lato-chars
"Wrap characters not supported by the installed Lato font in a span so that we can explicitly set the font to sans-serif.
We do this to work around unexpected font-fallback behaviours in CSSBox.
Lato is properly loaded/registered in `metabase.pulse.render.style/regiter-fonts!`, which means the
java.awt GraphicsEnvironment has Lato available as a Physical font. The loaded physical font does not contain
glyphs to properly render many international characters, and instead of falling back to another font on a per-glyph basis,
it simply renders a '[?]', which is no good.
If a given string, inside a `transformable-element` contains any character that isn't Lato-compatible, replace the entire string.
This is done to make the string as consistent as possible (no mixing fonts in a single string)."
[content]
(let [transformable-els #{:div :span :td :th :tr :table :p :tbody :thead}
string-wrapper (fn [part]
(if (and (string? part)
(not (lato-can-render? part)))
[:span {:style (style/style {:font-family "sans-serif"})} part]
part))]
(walk/postwalk
(fn [form]
(if (and
(not (map-entry? form))
(vector? form)
(transformable-els (first form)))
(mapv string-wrapper form)
form))
content)))
(mu/defn render-html-to-png :- bytes? (mu/defn render-html-to-png :- bytes?
"Render the Hiccup HTML `content` of a Pulse to a PNG image, returning a byte array." "Render the Hiccup HTML `content` of a Pulse to a PNG image, returning a byte array."
^bytes [{:keys [content]} :- formatter/RenderedPulseCard ^bytes [{:keys [content]} :- formatter/RenderedPulseCard
width] width]
(try (try
(let [html (html [:html [:body {:style (style/style (let [html (html [:html
{:margin 0 [:body {:style (style/style
:padding 0 {:font-family "Lato, 'Helvetica Neue', 'Lucida Grande', sans-serif"
:background-color :white})} :margin 0
content]])] :padding 0
:background-color :white})}
(wrap-non-lato-chars content)]])]
(with-open [os (ByteArrayOutputStream.)] (with-open [os (ByteArrayOutputStream.)]
(-> (render-to-png html width) (-> (render-to-png html width)
(write-image! "png" os)) (write-image! "png" os))
......
...@@ -5,7 +5,9 @@ ...@@ -5,7 +5,9 @@
[clojure.string :as str] [clojure.string :as str]
[metabase.public-settings :as public-settings] [metabase.public-settings :as public-settings]
[metabase.util.i18n :refer [trs]] [metabase.util.i18n :refer [trs]]
[metabase.util.log :as log])) [metabase.util.log :as log])
(:import
(java.awt Font GraphicsEnvironment)))
(set! *warn-on-reflection* true) (set! *warn-on-reflection* true)
...@@ -101,12 +103,13 @@ ...@@ -101,12 +103,13 @@
(defn- register-font! [filename] (defn- register-font! [filename]
(with-open [is (io/input-stream (io/resource filename))] (with-open [is (io/input-stream (io/resource filename))]
(.registerFont (java.awt.GraphicsEnvironment/getLocalGraphicsEnvironment) (.registerFont (GraphicsEnvironment/getLocalGraphicsEnvironment)
(java.awt.Font/createFont java.awt.Font/TRUETYPE_FONT is)))) (Font/createFont java.awt.Font/TRUETYPE_FONT is))))
(defn- register-fonts! [] (defn- register-fonts! []
(try (try
(doseq [weight ["regular" "700" "900"]] (register-font! "frontend_client/app/fonts/Lato/Lato-Regular.ttf")
(doseq [weight ["700" "900"]]
(register-font! (format "frontend_client/app/fonts/Lato/lato-v16-latin-%s.ttf" weight))) (register-font! (format "frontend_client/app/fonts/Lato/lato-v16-latin-%s.ttf" weight)))
(catch Throwable e (catch Throwable e
(let [message (str (trs "Error registering fonts: Metabase will not be able to send Pulses.") (let [message (str (trs "Error registering fonts: Metabase will not be able to send Pulses.")
......
(ns metabase.pulse.render.png-test (ns metabase.pulse.render.png-test
(:require (:require
[clojure.test :refer :all] [clojure.test :refer :all]
[metabase.pulse.render.png :as png])) [hiccup.core :as hiccup]
[metabase.pulse.render.png :as png]
[metabase.pulse.render.style :as style])
(:import
(java.awt Font GraphicsEnvironment)
(java.awt.image BufferedImage)
(java.io ByteArrayInputStream)
(javax.imageio ImageIO)))
(set! *warn-on-reflection* true) (set! *warn-on-reflection* true)
...@@ -13,9 +20,66 @@ ...@@ -13,9 +20,66 @@
(deftest table-width-test (deftest table-width-test
(testing "The PNG of a table should be cropped to the width of its content" (testing "The PNG of a table should be cropped to the width of its content"
(let [^java.awt.image.BufferedImage png (@#'png/render-to-png test-table-html-1 1200)] (let [^BufferedImage png (#'png/render-to-png test-table-html-1 1200)]
;; Check that width is within a range, since actual rendered result can very slightly by environment ;; Check that width is within a range, since actual rendered result can very slightly by environment
(is (< 140 (.getWidth png) 210)))) (is (< 140 (.getWidth png) 210))))
(testing "The PNG of a table should not clip any of its content" (testing "The PNG of a table should not clip any of its content"
(let [^java.awt.image.BufferedImage png (@#'png/render-to-png test-table-html-2 1200)] (let [^BufferedImage png (#'png/render-to-png test-table-html-2 1200)]
(is (< 320 (.getWidth png) 360))))) (is (< 320 (.getWidth png) 360)))))
(deftest installed-fonts-test
(testing "Are the correct fonts available for rendering?"
(is (contains?
(into #{} (map #(.getName ^Font %)) (.getAllFonts (GraphicsEnvironment/getLocalGraphicsEnvironment)))
"Lato Regular"))))
(defn- bytes->image
[bytes]
(let [input-stream (ByteArrayInputStream. bytes)]
(ImageIO/read input-stream)))
(defn- render-without-wrapping
[content width]
(-> [:html
[:body {:style (style/style
{:font-family "Lato, 'Helvetica Neue', 'Lucida Grande', sans-serif"
:margin 0
:padding 0
:background-color :white})}
content]]
hiccup/html
(#'png/render-to-png width)))
(defn- render-with-wrapping
[content width]
(-> {:content content
:attachments {}}
(png/render-html-to-png width)
bytes->image))
(deftest wrap-non-lato-characters-test
(testing "HTML Content inside tables with characters not supported by the Lato font are wrapped in a span."
(is (= [:td {:not-wrapped-in-here "안녕"}
[:span {:style "font-family: sans-serif;"} "안녕"]]
(#'png/wrap-non-lato-chars [:td {:not-wrapped-in-here "안녕"} "안녕"])))
(is (= [:table
[:tr
[:td "this is all Lato-compatible, baby!"]
[:td "What do you think about різні шрифти в одному документі?"]
[:td [:span {:style "font-family: sans-serif;"} "This part's English. This part is 英語ではありません"]]]]
(#'png/wrap-non-lato-chars
[:table
[:tr
[:td "this is all Lato-compatible, baby!"]
[:td "What do you think about різні шрифти в одному документі?"]
[:td "This part's English. This part is 英語ではありません"]]])))))
(deftest non-lato-characters-can-render-test
(testing "Strings containing characters that are not included in the Lato font can still be rendered."
(let [content [:span "안녕"]
^BufferedImage broken-render (render-without-wrapping content 200)
^BufferedImage working-render (render-with-wrapping content 200)]
;; The broken-render's width is around 17px. It is the width of 2 `[?]` charaters
;; We assert that the working render is wider based on the assumption (verified manually by
;; actually looking at the rendered images) that the correctly rendered glyphs are wider.
(is (< (.getWidth broken-render) (.getWidth working-render))))))
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment