1358634 - stylo: nonexistent font families are treated differently from Gecko

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

Testcase:

   <span style="font-family: idonotexist">XXXX</span>

This renders in a serif font with Gecko, a sans-serif font with stylo.  I have not looked into what the difference in the actual computed font struct is, but presumablt there is one...

Xidorn, do you know who knows the font bits?

Comment 1

[Bobby Holley (:bholley)]

You want Manish.

Comment 2

[Manish Goregaokar [:manishearth]]

We aren't handling the useDocumentFonts stuff in http://searchfox.org/mozilla-central/source/layout/style/nsRuleNode.cpp#3695-3715

Comment 3

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1358634 - Handle fallback to default variable font in case of nonexistant generic ;

Review commit: https://reviewboard.mozilla.org/r/132570/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/132570/

Comment 4

[Manish Goregaokar [:manishearth]]

Comment on attachment 8860556 [details]
Bug 1358634 - Handle fallback to default variable font in case of nonexistant generic ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132570/diff/1-2/

Comment 5

[Manish Goregaokar [:manishearth]]

Comment on attachment 8860556 [details]
Bug 1358634 - Handle fallback to default variable font in case of nonexistant generic ;

https://reviewboard.mozilla.org/r/132570/#review135458

::: servo/components/style/properties/properties.mako.rs:2499
(Diff revision 2)
>          // and nsStyleSVG per element. We should only do this when necessary
>          // using the `seen` bitfield!
>          style.mutate_background().fill_arrays();
>          style.mutate_svg().fill_arrays();
> +
> +        style.mutate_font().fixup_none_generic(device);

I should probably use the "none generic or fantasy/cursive font in non-use-document-font mode" heuristic beforehand here to avoid always cloning the font struct.

Comment 6

[Manish Goregaokar [:manishearth]]

Of course, GetDefaultFont is not thread safe. Quick fix would be to lock, but this gets called every time. A better fix would be to be *very* conservative about calling this (only when the language changes, only when it's necessary), locking when it's called, and caching the result in the TLS cx or something. Though I'm concerned about the lifetime of the cache.

An amazing fix would be to make the cache's linked list lock-free threadsafe. I was going to do this for the system font and font metrics stuff but eventually decided not to (both of them have other threadsafety issues that can't be fixed)

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8860556 [details]
Bug 1358634 - Handle fallback to default variable font in case of nonexistant generic ;

https://reviewboard.mozilla.org/r/132570/#review135472

::: servo/components/style/properties/properties.mako.rs:2499
(Diff revision 2)
>          // and nsStyleSVG per element. We should only do this when necessary
>          // using the `seen` bitfield!
>          style.mutate_background().fill_arrays();
>          style.mutate_svg().fill_arrays();
> +
> +        style.mutate_font().fixup_none_generic(device);

Can you actually fix up the font-family here, even from a correctness point of view?

Aren't there font metrics that depend on it?

Comment 8

[Manish Goregaokar [:manishearth]]

Ah, fair point.

I guess it can be set in set_font_family itself? I'm not sure; we may need this to do stuff whilst inheriting too.

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8860556 [details]
Bug 1358634 - Handle fallback to default variable font in case of nonexistant generic ;

https://reviewboard.mozilla.org/r/132570/#review136052

I don't know the answer to whether we need to do it while inheriting.  But I think Emilio is right about needing to do this earlier so that we have the right font metrics.  Cancelling review while you look into that.

::: layout/style/nsStyleStruct.h:167
(Diff revision 2)
> +  /**
> +   * Appropriately add the correct font if we are using DocumentFonts or
> +   * overriding for XUL
> +   */
> +  void FixupNoneGeneric(const nsPresContext* aPresContext,
> +                        uint8_t aGenericFontID,
> +                        const nsFont* aDefaultVariableFont);
> +

I think it would be nice to keep this code in nsRuleNode.cpp, where all of the rest of the font handling stuff is.  Can you make it a public static method on nsRuleNode that takes an nsFont to work on?

::: layout/style/nsStyleStruct.cpp:171
(Diff revision 2)
> +void nsStyleFont::FixupNoneGeneric(const nsPresContext* aPresContext,
> +                                   uint8_t aGenericFontID,
> +                                   const nsFont* aDefaultVariableFont)

Nit: newline after "void".

::: servo/components/style/properties/gecko.mako.rs:1307
(Diff revision 2)
> +        if v.0.len() != 1 {
> +            self.gecko.mGenericID = structs::kGenericFont_NONE;
> +        }

How about we just unconditionally set it to kGenericFont_NONE here, and then overwrite it below when we get a FontFamily::Generic.

Comment 11

[Manish Goregaokar [:manishearth]]

Addressed

Comment 12

[Manish Goregaokar [:manishearth]]

Comment on attachment 8860556 [details]
Bug 1358634 - Handle fallback to default variable font in case of nonexistant generic ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132570/diff/2-3/

Comment 13

[Manish Goregaokar [:manishearth]]

Comment on attachment 8860556 [details]
Bug 1358634 - Handle fallback to default variable font in case of nonexistant generic ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132570/diff/3-4/

Comment 14

[Cameron McCormack (:heycam)]

Comment on attachment 8860556 [details]
Bug 1358634 - Handle fallback to default variable font in case of nonexistant generic ;

https://reviewboard.mozilla.org/r/132570/#review139440

::: layout/style/ServoBindings.cpp:1790
(Diff revision 4)
> +                                   RawGeckoPresContextBorrowed aPresContext)
> +{
> +  const nsFont* defaultVariableFont =
> +    aPresContext->GetDefaultFont(kPresContext_DefaultVariableFont_ID,
> +                                 aFont->mLanguage);
> +  nsRuleNode::FixupNoneGeneric(&aFont->mFont, aPresContext, aFont->mGenericID, defaultVariableFont);

Nit: please keep to 80 columns.

::: layout/style/nsRuleNode.cpp:3736
(Diff revision 4)
>    NS_ASSERTION(eCSSUnit_Enumerated != familyValue->GetUnit(),
>                 "system fonts should not be in mFamily anymore");
>    if (eCSSUnit_FontFamilyList == familyValue->GetUnit()) {
>      // set the correct font if we are using DocumentFonts OR we are overriding for XUL
>      // MJA: bug 31816
> -    bool useDocumentFonts =
> +    nsRuleNode::FixupNoneGeneric(&aFont->mFont, aPresContext, aGenericFontID, defaultVariableFont);

Nit: and here.

::: servo/components/style/properties/gecko.mako.rs:1367
(Diff revision 4)
>      pub fn font_family_at(&self, _: usize) -> longhands::font_family::computed_value::FontFamily {
>          unimplemented!()
>      }
>  
>      pub fn copy_font_family_from(&mut self, other: &Self) {
>          unsafe { Gecko_CopyFontFamilyFrom(&mut self.gecko.mFont, &other.gecko.mFont); }

Do we need to copy mGenericID too?

::: servo/components/style/properties/properties.mako.rs:2561
(Diff revision 4)
> +            % if product == "gecko":
> +                if seen.contains(LonghandId::FontFamily) || seen.contains(LonghandId::XLang) {
> +                    context.style.mutate_font().fixup_none_generic(context.device);
> +                }
> +            % endif

It seems like Gecko doesn't do this if an -x-lang declaration was found but a font-family declaration wasn't.  Why do we need to check -x-lang?

Comment 15

[Manish Goregaokar [:manishearth]]

Comment on attachment 8860556 [details]
Bug 1358634 - Handle fallback to default variable font in case of nonexistant generic ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132570/diff/4-5/

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8860556 [details]
Bug 1358634 - Handle fallback to default variable font in case of nonexistant generic ;

https://reviewboard.mozilla.org/r/132570/#review139534

Comment 17

[Manish Goregaokar [:manishearth]]

https://github.com/servo/servo/pull/16741

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/de6c23251b75
Handle fallback to default variable font in case of nonexistant generic ; r=heycam

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/62b03dd84117
Update test expectations for fallback fonts ;r=manishearth

Comment 20

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/de6c23251b75
https://hg.mozilla.org/mozilla-central/rev/62b03dd84117