New issue
Advanced search Search tips

Issue 850092 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug-Regression



Sign in to add a comment

base64-encoded font isn't rendered into canvas

Reported by sean.gil...@buzzfeed.com, Jun 6 2018

Issue description

UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3359.181 Safari/537.36

Steps to reproduce the problem:
1. Download an eot font (e.g. https://www.buzzfeed.com/static/fonts/ProximaNova-Extrabold-webfont.eot) via a Promise
2. Resolve the Promise
3. Encode the font data into base64 data
4. Put the font information into a `style` block associated with a `canvas` element
5. Draw some text with this font, as an svg
6. Extract an image from the `canvas`
7. Look at the image and verify the font isn't the one you specified

What is the expected behavior?
The text in the canvas would be drawn in the supplied font, and the extracted image would be rendered using this data and appear in the correct font.

What went wrong?
It seems like this may be an error at some point during either style processing or rendering via the canvas.

You can reproduce this bug here: https://codepen.io/dreki/pen/GGqzbP
(if you test in Chrome 66 vs Chrome 67, you'll see that in 67, the font doesn't render correctly)

Did this work before? Yes 66.0.3359.181

Chrome version: 67.0.3396.62  Channel: stable
OS Version: OS X 10.13.3
Flash Version: 

Aside from the CodePen, I can supply you to a URL to the actual tool we've created that uses this technology.

Because of this bug, our whole editorial staff may have to be moved to a different browser at least temporarily. It's pretty disruptive and it doesn't seem like our issue or something we can work around.

Thank you for your time.
 
Components: -Blink Blink>Canvas
Update: It looks like this rule broke the font rendering in 67:

`font: 700 100% 118px/118px "ProximaNova Extrabold"; font-kerning: auto;`

We removed `100%` and then the text rendered as expected. Here's the updated rule:

`font: 700 118px/118px "ProximaNova Extrabold"; font-kerning: auto;`
So it sounds like one of these may happened:
- Something about the `font` rule itself changed in 67
- The code that generates this rule/generated the 'inline' version of fonts changed
- Something in SVG rendering or canvas drawing changed

We did notice that the generated font styles definitely did change in 67. The new style was much larger (~100KB larger), and included things like ` unicode-range: U+0-FF, U+131, U+152-153, U+2BB-2BC, U+2C6, U+2DA, U+2DC, U+2000-206F, U+2074, U+20AC, U+2122, U+2191, U+2193, U+2212, U+2215, U+FEFF, U+FFFD;`, when those weren't included before.
Comment from a team member:

"It's the font-stretch property (defaulting to 100%) that it's trying to insert into the font property btw

(not the text-size-adjust)
It's clearly indicated on MDN that you cannot have both font-weight and font-stretch in a font style, but Chrome is trying to."
More conversation:

"sean: so Chrome generates this?
dave: Yeah this is the CSSRule magic stuff going on.
I think it's coming from CSSRule.cssText ultimately."

Comment 6 by f...@opera.com, Jun 6 2018

Cc: cnardi@chromium.org
Labels: Needs-Bisect
Sounds like this may have been caused by  issue 827921 .
Cc: -cnardi@chromium.org
Components: -Blink>Canvas Blink>CSS
Labels: -Needs-Bisect
Owner: cnardi@chromium.org
Status: Started (was: Unconfirmed)
I see what's going on. Per https://drafts.csswg.org/css-fonts-4/#font-prop:

Values for the font-stretch property can also be included but only those supported in CSS Fonts level 3, none of the font-stretch values added in this specification can be used in the font shorthand

We're serializing font-stretch percentages (defined in Fonts 4) in the font shorthand, creating a rule that cannot be reparsed.
Thank you so much. We have a workaround for now, but will be very happy when this is released.
Project Member

Comment 9 by bugdroid1@chromium.org, Jun 19 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/43f06f172201a5435d4106c9b8317e734e257aee

commit 43f06f172201a5435d4106c9b8317e734e257aee
Author: Chris Nardi <cnardi@chromium.org>
Date: Tue Jun 19 09:51:43 2018

Serialize font-stretch values correctly in the font shorthand

Per the CSS Fonts 4 spec [1], only keyword values for font-stretch are
valid in the font shorthand. Our current serialization code ignores
this, and outputs percentage values as well in the shorthand, meaning
that the generated rule cannot be reparsed. We now check if the
percentage can be converted to a keyword, and if so, output it as that
keyword. Otherwise, we do not output a serialization for the font
shorthand, as per the CSSOM spec [2].

[1]: https://drafts.csswg.org/css-fonts-4/#font-prop
[2]: https://drafts.csswg.org/cssom/#serializing-css-values

Bug:  850092 
Change-Id: I7e3eec64723966b15abfa819213b95cba6cbc3d5
Reviewed-on: https://chromium-review.googlesource.com/1103856
Commit-Queue: Dominik Röttsches <drott@chromium.org>
Reviewed-by: Dominik Röttsches <drott@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568383}
[add] https://crrev.com/43f06f172201a5435d4106c9b8317e734e257aee/third_party/WebKit/LayoutTests/external/wpt/css/css-fonts/font-shorthand-serialization-font-stretch.html
[modify] https://crrev.com/43f06f172201a5435d4106c9b8317e734e257aee/third_party/blink/renderer/core/css/style_property_serializer.cc
[modify] https://crrev.com/43f06f172201a5435d4106c9b8317e734e257aee/third_party/blink/renderer/core/css/style_property_serializer.h

I saw that y'all made this changed, reviewed it, and merged it into 'master'. Thank you so much for your very fast work on this. We feel very heard and I personally have a lot of respect for you all -- you've been very receptive and helpful in all my interactions with you.
Status: Fixed (was: Started)
I'm glad you're happy with the speed of change, only sorry I didn't notice this bug in my original change! The fix should be in the next canary build, released tomorrow. I'm not sure that this change meets the bar for merging to M68, so the fix should be in M69.

Sign in to add a comment