Skip to content

fix: new_from_descriptor lose font variations, such as font-weight#430

Open
kaiwk wants to merge 1 commit into
servo:mainfrom
kaiwk:new-from-name
Open

fix: new_from_descriptor lose font variations, such as font-weight#430
kaiwk wants to merge 1 commit into
servo:mainfrom
kaiwk:new-from-name

Conversation

@kaiwk

@kaiwk kaiwk commented Dec 15, 2020

Copy link
Copy Markdown
Contributor

If we create CTFont from font descriptor, we will lose font variations, and use new_from_name instead will fix it.

CTFontDescriptor is essentially a dictionary of font attributes.

@jrmuizel

Copy link
Copy Markdown
Collaborator

Can you add a test?

@kaiwk

kaiwk commented Dec 15, 2020

Copy link
Copy Markdown
Contributor Author

@jrmuizel I have to add CTFontManagerCreateFontDescriptorsFromData, because it happens when loading font from 'otc/ttc'.

I encountered the issue when using font-kit, font-kit manually unpack 'otc/ttc', I suppose it won't make any difference here.

@kaiwk

kaiwk commented Dec 15, 2020

Copy link
Copy Markdown
Contributor Author

The test failed on xcode 7.3/9.2 and passed others.

Comment thread core-text/src/font.rs
pub fn new_from_buffer(buffer: &[u8]) -> Result<CTFont, ()> {
let ct_font_descriptor = create_font_descriptor(buffer)?;
Ok(new_from_descriptor(&ct_font_descriptor, 16.0))
Ok(new_from_name(&ct_font_descriptor.font_name(), 16.0)?)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this break if the font is not installed or if two different fonts use the same name? i.e. new_from_name won't find the font.

@kaiwk kaiwk May 18, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, looks like WebRender also creates CTFont from postscript name, see: https://phabricator.services.mozilla.com/D93518

   // there's no way great way to go from a CGFont to a CTFontDescriptor
   // so we use the postscript name. Ideally NativeFontHandle would
   // just use a CTFontDescriptor
   let name = native_font_handle.0.postscript_name();
   let font = core_text::font_descriptor::new_from_postscript_name(&name);

   self.ct_font_descs
       .insert(*font_key, font);

Also, font_name() returns postscript name, and new_fomr_name receives postscript name.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WebRender only uses the postscript name for installed system fonts. We don't use the name for data fonts:
https://searchfox.org/mozilla-central/source/gfx/wr/webrender/src/platform/macos/font.rs#382

@kaiwk kaiwk force-pushed the new-from-name branch 4 times, most recently from a120e4a to 1605589 Compare May 18, 2021 13:16
@kaiwk

kaiwk commented May 18, 2021

Copy link
Copy Markdown
Contributor Author

I also add an api called new_from_arc_buffer, so maybe font-kit can benefit from it : )

@bors-servo

Copy link
Copy Markdown
Contributor

☔ The latest upstream changes (presumably #485) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants