Closed Bug 1367860 Opened 8 years ago Closed 8 years ago

CSS font-family inherits 'used' value, including an appended (implicit) CSS generic, instead of 'computed' value (was: Firefox picks different fall-back Chinese font when told twice to use one that doesn't work)

Categories

(Core :: Layout: Text and Fonts, defect, P3)

Product:
Core
Component:
Layout: Text and Fonts
Version:
53 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: john_thomson, Assigned: kevin.hsieh)

References

Details

Attachments

(7 files, 12 obsolete files)

reporter's testcase
547 bytes, text/html; charset=UTF-8
Details
Bug 1367860 (part 1) - Handle CSS font-family units using switch instead of if.
3.57 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
Bug 1367860 (part 2) - Update CSS fallback font when lang changes.
1.44 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
Bug 1367860 (part 3) - Construct language-specific default variable font as fallback font, not specified font.
4.52 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
Bug 1367860 (part 5) - Add testcases.
3.68 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
Additional testcase variations
1.50 KB, application/zip
Details
Bug 1367860 (part 4) - Update CSS fallback font when lang changes (stylo). r=manishearth
1.24 KB, patch
manishearth
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.36 Steps to reproduce: Open a file containing the following text: <!DOCTYPE html> <html> <head> <meta http-equiv="content-type" content="text/html; charset=UTF-8"> <meta charset="UTF-8"> </head> <body style="font-family: 'Times New Roman'"> <div lang="zh-CN" style="font-family: 'Times New Roman'">您可以自由地将本作品用于商业目的。 您可以自由地修改、转换或以本作品为基础进行创作。</div> <div lang="zh-CN">您可以自由地将本作品用于商业目的。 您可以自由地修改、转换或以本作品为基础进行创作。</div> </body> </html> (Using FF 53.0.3 (32-bit)) Actual results: The two identical blocks of Chinese are displayed in different fonts. (On my system FF chooses Microsoft YaHei for the top block and SimSun for the bottom one.) Expected results: I'm not sure what the best font to choose is, but I certainly expected the same one to be chosen for identical blocks of text in the same language when both styles resolve to the same requested font. Chrome uses Microsoft Yahei for both. Internet Explorer and MS Edge both use the same font for each, though I'm not sure which...looks more like SimSun. This can also be reproduced with CSS rules. In the same file, you can get a different font on the first block if it has a class and style rule that request Times New Roman, and another style rule requests that for the body. In different files, you can see a difference with and without a CSS rule that requests the body font for language zh-CN. It's a fairly minor problem...the obvious workaround is to request a font that has the necessary characters...but it cost us considerable time to figure out why two documents looked different.
Component: Untriaged → Layout: Text
Could you try this with the latest Nightly build (https://nightly.mozilla.org/) and see if it still happens? I'm guessing the recent fix for bug 734008 might have had an effect here. Thanks!
(In reply to Jonathan Kew (:jfkthame) from comment #1) > Could you try this with the latest Nightly build > (https://nightly.mozilla.org/) and see if it still happens? I'm guessing the > recent fix for bug 734008 might have had an effect here. Thanks! Actually, never mind about that. I can reproduce similar odd behavior with current Nightly on macOS, so it's clearly still an issue. I think "best practice" for Web authoring would suggest including one of the CSS generics (serif, sans-serif, monospace) at the end of your font-family list, for better control of what users will see if they don't have the specific face(s) listed. If you do that, e.g. specifying "font-family: 'Times New Roman', serif;" in your styles, you should get consistent fallback. It looks like it's only in the case where no generic is specified that we're doing something inconsistent. Still looks like a bug, though. I wonder if it's always behaved like this, or did we regress something recently?
Not a recent regression, it's been this way for a couple dozen releases (or perhaps forever).
Status: UNCONFIRMED → NEW
Ever confirmed: true
I think the bug here is that font-family is being inherited as "used", when it should inherit as "computed". Here's roughly what seems to be happening (based on observed behavior, I didn't try to track all this through the font-matching code yet...) - First, note that font preferences are configured per script, and this includes the choice of whether the default content font is 'serif' or 'sans-serif'; see, for example, font.default.x-western (which is 'serif' by default) vs font.default.zh-CN (which is 'sans-serif' by default). - When no CSS generic is included in the font-family property, we implicitly add the default generic to the end of the list. So a CSS declaration that says "font-family: foo;" becomes, in effect, either "font-family: foo, serif;" or "font-family: foo, sans-serif;", depending on the relevant font.default.<langGroup> preference. - In this case, the <body> element, where the font is initially specified, has no lang attribute, so font preferences will be resolved based on a default, probably en-US, which is Latin script. So its used value of font-family is "Times New Roman, serif". - The first <div> with lang=zh-CN also specifies "font-family: Times New Roman". Unlike the <body>, however, the default generic for zh-CN is sans-serif, and so the used value becomes "font-family: Times New Roman, sans-serif". Therefore, when the Chinese characters are not found in Times, the next font we try is the default sans-serif font for Simplified Chinese. - The second <div>, however, specifies lang=zh-CN but does not specify font-family. Therefore, it inherits font-family from the <body>. And it appears that what it inherits is the *used* value of font-family, where the (lang-dependent) CSS generic has already been appended, rather than the *computed* value, to which the appropriate default generic for the current element would be added. That would explain the observed behavior.
Here's a slightly different testcase that demonstrates the same issue (assuming default font prefs), perhaps marginally more clearly: data:text/html;charset=utf-8, <div style="font-family:foo" lang="en"> <div lang="zh-cn">%E6%82%A8</div> </div> <div style="font-family:foo" lang="he"> <div lang="zh-cn">%E6%82%A8</div> </div> Here, the first Chinese <div> is rendered with the default zh-CN serif font, because the font-family property specified on the outer <div>, with lang=en, was resolved to a used value of "foo, serif" and then inherited by the inner <div>; while the second Chinese <div> uses the zh-CN sans-serif font, because its outer <div> had lang=he and font.default.he=sans-serif. So I think the behavior is logical (in some sense); but it's surprising and difficult to understand, and I don't think it is justifiable per the CSS Fonts spec.
Summary: Firefox picks different fall-back Chinese font when told twice to use one that doesn't work → CSS font-family inherits 'used' value, including an appended (implicit) CSS generic, instead of 'computed' value (was: Firefox picks different fall-back Chinese font when told twice to use one that doesn't work)
Thanks for helpful analysis and suggestions!
Priority: -- → P3
Assignee: nobody → kevin.hsieh
Status: NEW → ASSIGNED
Attached file Additional testcase variations (obsolete) —
Additional testcase variations: Reference: language tag and font-family on same tag -> simplest case, known to work Case 1: language tag on ancestor / font-family on descendant -> already passes Case 2: language tag on descendant / font-family on ancestor -> unexpected behavior: triggered by the reporter's testcase and jfkthame's testcase Case 3: no font-family tag -> also unexpected behavior: language default generic preference has no effect
Try test: https://treeherder.mozilla.org/#/jobs?repo=try&revision=94b268ce46b0817a45d5300443a05b0e58e00cd1 Newly added tests fail on stylo. cc: canaltinova
Comment on attachment 8897956 [details] Bug 1367860 (part 2) - Update CSS fallback font when lang changes. https://reviewboard.mozilla.org/r/169258/#review174638 drive-by nits: ::: layout/style/nsRuleNode.cpp:3739 (Diff revision 1) > case eCSSUnit_Unset: > aConditions.SetUncacheable(); > aFont->mFont.fontlist = aParentFont->mFont.fontlist; > aFont->mFont.systemFont = aParentFont->mFont.systemFont; > aFont->mGenericID = aParentFont->mGenericID; > + U_FALLTHROUGH; // Fall through here to check for a lang change. This should be MOZ_FALLTHROUGH. (Looks like U_FALLTHROUGH does sort of the same thing, but it's only used as part of the ICU 3rd-party library.) ::: layout/style/nsRuleNode.cpp:3744 (Diff revision 1) > + U_FALLTHROUGH; // Fall through here to check for a lang change. > + case eCSSUnit_Null: > + // If we have inheritance (cases eCSSUnit_Inherit, eCSSUnit_Unset, and > + // eCSSUnit_Null) and the language is different from the parent's, then we > + // need to overwrite the inherited default generic font with one > + // appropriate for the new language (bug 1367860). Probably don't mention the bug number here, because: (1) It's already available -- anyone who's curious can find it from the "hg blame" (and tools like searchfox/DXR) (2) By convention, bug numbers in our codebase tend to indicate known-problems (and the bug is where the known-problem will eventually be addressed). So someone reading this comment will be confused about whether you're describing something that's still broken. ::: layout/style/nsStyleUtil.cpp:213 (Diff revision 1) > + if (defaultGeneric != eFamily_none) { > + // If the font list is empty but we have a default generic, then serialize > + // the default generic. See also: gfxFontGroup::BuildFontList() > + FontFamilyName(defaultGeneric).AppendToString(aResult); > + } What happens here if defaultGeneric **is** eFamily_none? It looks like in that case, we'll end up returning the empty string as our computed style, and that's probably bad. - Is this even possible? - To the extent that this is possible, is this already a problem? (or, is it a problem that is being introduced as a result of this change?) If it's possible to trigger this, you might want to file a followup for this. Or, if it's clearly not possible to trigger this, you might want to include a NS_NOTREACHED case here to validate/document that assumption, to make it clearer that we're pretty-much-guaranteed to return a nonempty string. ::: testing/web-platform/tests/css/CSS2/selectors/attribute-value-selector-002.xht:18 (Diff revision 1) > <p>Test passes if the first line of "Filler Text" below is black and the second one is green.</p> > - <div lang="ja-jp">Filler Text</div> > + <div lang="fr-fr">Filler Text</div> > <div lang="en-us">Filler Text</div> Why are you making a change to this test? It looks like it comes from the upstream "web-platform-tests" repo (per its "hg log" history, which points to Bug 1381842, and also per its <link rel="author" title="Microsoft" .../> in the head section.) It won't do any good to change the test in mozilla-central, because your changes will get overwritten the next time we pull in updates from upstream. If this test makes an invalid assumption and really needs to be changed, we probably need to fix it upstream and (temporarily) mark it as failing in our local copy of the web-platform-test manifest.
Comment on attachment 8897955 [details] Bug 1367860 (part 5) - Add testcases. https://reviewboard.mozilla.org/r/169256/#review174668 ::: layout/style/nsRuleNode.cpp:3710 (Diff revision 1) > - NS_ASSERTION(eCSSUnit_Enumerated != familyValue->GetUnit(), > - "system fonts should not be in mFamily anymore"); > - if (eCSSUnit_FontFamilyList == familyValue->GetUnit()) { > + case eCSSUnit_Enumerated: > + NS_NOTREACHED("system fonts should not be in mFamily anymore"); > + break; remove this; the assertion at the end should be sufficient
Attachment #8897955 - Flags: review?(dbaron) → review+
Comment on attachment 8897956 [details] Bug 1367860 (part 2) - Update CSS fallback font when lang changes. https://reviewboard.mozilla.org/r/169258/#review174672 This doesn't have a sufficient commit message to explain what it's changing and why, so I can't really review it as-is. However, some comments: ::: layout/base/StaticPresData.cpp:173 (Diff revision 1) > - mDefaultVariableFont.fontlist = FontFamilyList(defaultType); > + mDefaultVariableFont.fontlist = FontFamilyList(); > + mDefaultVariableFont.fontlist.SetDefaultFontType(defaultType); Could you explain why you're making this change? ::: layout/style/nsRuleNode.cpp:431 (Diff revision 1) > aGenericFontID == kGenericFont_fantasy))) { > FontFamilyType defaultGeneric = > + aDefaultVariableFont->fontlist.IsEmpty() ? > + aDefaultVariableFont->fontlist.GetDefaultFontType() : > aDefaultVariableFont->fontlist.FirstGeneric(); > - MOZ_ASSERT(aDefaultVariableFont->fontlist.Length() == 1 && > + // Fall back to the language default generic if the fontlist is empty. What if the fontlist isn't empty, but doesn't have a generic? ::: layout/style/nsRuleNode.cpp:432 (Diff revision 1) > FontFamilyType defaultGeneric = > + aDefaultVariableFont->fontlist.IsEmpty() ? > + aDefaultVariableFont->fontlist.GetDefaultFontType() : > aDefaultVariableFont->fontlist.FirstGeneric(); > - MOZ_ASSERT(aDefaultVariableFont->fontlist.Length() == 1 && > + // Fall back to the language default generic if the fontlist is empty. > + MOZ_ASSERT(aDefaultVariableFont->fontlist.Length() <= 1 && Could you explain (maybe in the commit message) why being empty is now a possibility? ::: layout/style/nsRuleNode.cpp:3740 (Diff revision 1) > aConditions.SetUncacheable(); > aFont->mFont.fontlist = aParentFont->mFont.fontlist; > aFont->mFont.systemFont = aParentFont->mFont.systemFont; > aFont->mGenericID = aParentFont->mGenericID; > + U_FALLTHROUGH; // Fall through here to check for a lang change. > + case eCSSUnit_Null: The comment here should explain that it's depending on mLanguage being in the same style struct. ::: layout/style/nsRuleNode.cpp:3745 (Diff revision 1) > + if (aFont->mLanguage != aParentFont->mLanguage) { > + aConditions.SetUncacheable(); This needs a comment to explain why it's safe to call SetUncacheable() only when the language is different from the parent language, i.e., why you'd never cache a struct when the languages were the same, and then incorrectly use that cached struct when they're different. (It's not immediately obvious to me that it is safe -- although I suspect it likely is.)
Attachment #8897956 - Flags: review?(dbaron) → review-
See Also: → 1391341
Comment on attachment 8897956 [details] Bug 1367860 (part 2) - Update CSS fallback font when lang changes. https://reviewboard.mozilla.org/r/169258/#review174672 > Could you explain why you're making this change? I'm creating mDefaultVariableFont.fontlist with defaultType as the default (fallback) font instead of as part of the font list proper. This way, it can be overwritten should there be a language change. (I'll add this explanation as a comment.) > What if the fontlist isn't empty, but doesn't have a generic? Then the MOZ_ASSERT that immediately follows will fail. > Could you explain (maybe in the commit message) why being empty is now a possibility? I'll split it off into a third commit + message.
Comment on attachment 8897956 [details] Bug 1367860 (part 2) - Update CSS fallback font when lang changes. https://reviewboard.mozilla.org/r/169258/#review175088 ::: layout/style/nsRuleNode.cpp:3739 (Diff revision 2) > + case eCSSUnit_Null: > + // If we have inheritance (cases eCSSUnit_Inherit, eCSSUnit_Unset, and > + // eCSSUnit_Null) and a (potentially different) language is explicitly > + // specified, then we need to overwrite the inherited default generic font > + // with one appropriate for the current language. > + if (aFont->mExplicitLanguage) { aFont->mExplicitLanguage is inherited. I think what you want to test here is aRuleData->ValueForLang()->GetUnit() != eCSSUnit_Null. ::: layout/style/nsRuleNode.cpp:3740 (Diff revision 2) > + // If we have inheritance (cases eCSSUnit_Inherit, eCSSUnit_Unset, and > + // eCSSUnit_Null) and a (potentially different) language is explicitly > + // specified, then we need to overwrite the inherited default generic font > + // with one appropriate for the current language. > + if (aFont->mExplicitLanguage) { > + aConditions.SetUncacheable(); I think if you change the test per my previous comment, there's no longer any justification for SetUncacheable() (which is suspicious to begin with since it's inside a condition rather than outside it). Remove this line. ::: layout/style/nsRuleNode.cpp:3741 (Diff revision 2) > + FontFamilyType defaultGeneric = > + defaultVariableFont->fontlist.GetDefaultFontType(); > + aFont->mFont.fontlist.SetDefaultFontType(defaultGeneric); The comment should point out what in these three lines actually depends on the language. I don't see it... which makes it hard to review whether changes to other things are needed.
Attachment #8897956 - Flags: review?(dbaron) → review-
Comment on attachment 8898390 [details] Bug 1367860 (part 3) - Construct language-specific default variable font as fallback font, not specified font. https://reviewboard.mozilla.org/r/169364/#review175122 r=dbaron with the following changes ::: layout/base/StaticPresData.cpp:173 (Diff revision 1) > - mDefaultVariableFont.fontlist = FontFamilyList(defaultType); > + mDefaultVariableFont.fontlist = FontFamilyList(); > + mDefaultVariableFont.fontlist.SetDefaultFontType(defaultType); To match this change (and the one just after), I think you should also change the constructor of StaticPresData so that it constructs mDefaultVariableFont with no arguments to the constructor rather than eFamily_serif. I wonder whether you should also change mDefaultFixedFont the same way -- but I guess it's OK for them to be different. ::: layout/style/nsRuleNode.cpp:428 (Diff revision 1) > + aDefaultVariableFont->fontlist.IsEmpty() ? > + aDefaultVariableFont->fontlist.GetDefaultFontType() : > aDefaultVariableFont->fontlist.FirstGeneric(); So, actually, given your change to StaticPresData -- how is it possible for this fontlist to *not* be empty? Don't you now need to handle only that case, and just call GetDefaultFontType (and assert that fontlist.Length() == 0)?
Attachment #8898390 - Flags: review?(dbaron) → review+
Attachment #8898391 - Flags: review?(dbaron) → review?(manishearth)
Comment on attachment 8898392 [details] Bug 1367860 (part 5) - Add testcases. https://reviewboard.mozilla.org/r/169392/#review175124 The reftests look fine, with comments addressed, but marking review- because of the wpt manifest. ::: layout/reftests/font-matching/1367860-1.htm:7 (Diff revision 5) > +Reftest for Bug 1367860 > +Case 1: Language tag on ancestor / font-family on descendant > + (passes before patch) > +--> > + > +<!DOCTYPE html> Please put the <!DOCTYPE html> line before the comment, in all files. (The DOCTYPE only causes standards mode when it's in the first N bytes of the file, and I don't remember what N is, so it's good practice...) ::: layout/reftests/font-matching/1367860-3.htm:3 (Diff revision 5) > +<!-- > +Reftest for Bug 1367860 > +Case 3: No font-family tag s/font-family tag/font-family/ ::: testing/web-platform/meta/MANIFEST.json:76766 (Diff revision 5) > [ > "/css/CSS2/selectors/attribute-value-selector-002.xht", > [ > [ > "/css/CSS2/selectors/attribute-value-selector-001-ref.xht", > - "==" > + "!=" This doesn't seem right. This should be addressed by adjusting failure annotations, not by changing the test between == and !=.
Attachment #8898392 - Flags: review?(dbaron) → review-
Comment on attachment 8898392 [details] Bug 1367860 (part 5) - Add testcases. https://reviewboard.mozilla.org/r/169392/#review175124 The reftests look fine, with comments addressed, but marking review- because of the wpt manifest. ::: layout/reftests/font-matching/1367860-1.htm:7 (Diff revision 5) > +Reftest for Bug 1367860 > +Case 1: Language tag on ancestor / font-family on descendant > + (passes before patch) > +--> > + > +<!DOCTYPE html> Please put the <!DOCTYPE html> line before the comment, in all files. (The DOCTYPE only causes standards mode when it's in the first N bytes of the file, and I don't remember what N is, so it's good practice...) ::: layout/reftests/font-matching/1367860-3.htm:3 (Diff revision 5) > +<!-- > +Reftest for Bug 1367860 > +Case 3: No font-family tag s/font-family tag/font-family/ ::: testing/web-platform/meta/MANIFEST.json:76766 (Diff revision 5) > [ > "/css/CSS2/selectors/attribute-value-selector-002.xht", > [ > [ > "/css/CSS2/selectors/attribute-value-selector-001-ref.xht", > - "==" > + "!=" This doesn't seem right. This should be addressed by adjusting failure annotations, not by changing the test between == and !=.
Comment on attachment 8897956 [details] Bug 1367860 (part 2) - Update CSS fallback font when lang changes. https://reviewboard.mozilla.org/r/169258/#review175130 ::: layout/style/nsRuleNode.cpp:3743 (Diff revision 3) > + // with the default generic from defaultVariableFont, which is computed > + // using aFont->mLanguage above. > + if (aRuleData->ValueForLang()->GetUnit() != eCSSUnit_Null) { > + FontFamilyType defaultGeneric = > + defaultVariableFont->fontlist.GetDefaultFontType(); > + aFont->mFont.fontlist.SetDefaultFontType(defaultGeneric); OK, so this seems like it isn't quite right, since: * when we process eCSSUnit_FontFamilyList (part of whose processing is in ComputeFontData rather than SetFont), we call SetDefaultFontType is only some conditions * when we process inheritance that involves a change in language, we call SetDefaultFontType unconditionally So this means that, while your reftests pass, we're not actually setting up the same data structures between the different cases that you test (lang on the inside vs. font-family on the inside vs. on the same element) for other values of font-family -- those where FixupNoneGeneric would do something other than aFont->fontlist.SetDefaultFontType(defaultGeneric). It seems that the fontlist should be set up the same way in both cases -- either by giving this new codepath the same conditions as FixupNoneGeneric (maybe by just calling it?), or by changing what FixupNoneGeneric does. Probably the least risky strategy would be to call FixupNoneGeneric. (It's worth checking that the "if (!ProiritizeFirstGeneric()) { PrependGeneric() }" bit is idempotent.)
Attachment #8897956 - Flags: review?(dbaron) → review-
Comment on attachment 8897956 [details] Bug 1367860 (part 2) - Update CSS fallback font when lang changes. https://reviewboard.mozilla.org/r/169258/#review175130 > OK, so this seems like it isn't quite right, since: > > * when we process eCSSUnit_FontFamilyList (part of whose processing is in ComputeFontData rather than SetFont), we call SetDefaultFontType is only some conditions > * when we process inheritance that involves a change in language, we call SetDefaultFontType unconditionally > > So this means that, while your reftests pass, we're not actually setting up the same data structures between the different cases that you test (lang on the inside vs. font-family on the inside vs. on the same element) for other values of font-family -- those where FixupNoneGeneric would do something other than aFont->fontlist.SetDefaultFontType(defaultGeneric). > > It seems that the fontlist should be set up the same way in both cases -- either by giving this new codepath the same conditions as FixupNoneGeneric (maybe by just calling it?), or by changing what FixupNoneGeneric does. > > Probably the least risky strategy would be to call FixupNoneGeneric. (It's worth checking that the "if (!ProiritizeFirstGeneric()) { PrependGeneric() }" bit is idempotent.) I'll go with a call to FixupNoneGeneric. It does appear that "if (!ProiritizeFirstGeneric()) { PrependGeneric() }" is idempotent. Thanks.
Comment on attachment 8898392 [details] Bug 1367860 (part 5) - Add testcases. https://reviewboard.mozilla.org/r/169392/#review175124 > s/font-family tag/font-family/ ?
Comment on attachment 8898392 [details] Bug 1367860 (part 5) - Add testcases. https://reviewboard.mozilla.org/r/169392/#review175124 > ? Never mind, fixing this.
Attachment #8897956 - Attachment is obsolete: true
Attachment #8898390 - Attachment is obsolete: true
Attachment #8898391 - Attachment is obsolete: true
Attachment #8898391 - Flags: review?(manishearth)
Attachment #8898392 - Attachment is obsolete: true
The MozReview patch identities are all broken. I'll no longer accept further reviews in this bug using MozReview. Please reattach them all as patch files and request review again.
Attachment #8898992 - Attachment is obsolete: true
Attachment #8898992 - Flags: review?(dbaron)
Attachment #8898993 - Attachment is obsolete: true
Attachment #8898993 - Flags: review?(dbaron)
Attachment #8898994 - Attachment is obsolete: true
Attachment #8898994 - Flags: review?(dbaron)
Attachment #8898995 - Attachment is obsolete: true
Attachment #8898995 - Flags: review?(manishearth)
Attachment #8897955 - Attachment is obsolete: true
Attachment #8899002 - Attachment description: patch2.diff → Bug 1367860 (part 2) - Update CSS fallback font when lang changes.
Attachment #8899004 - Flags: review?(manishearth)
Attachment #8899005 - Flags: review?(dbaron)
Apologies for the MozReview breakage. I've re-uploaded the revised patches.
Attachment #8896501 - Attachment is obsolete: true
Comment on attachment 8899004 [details] [diff] [review] Bug 1367860 (part 4) - Update CSS fallback font when lang changes (stylo). Review of attachment 8899004 [details] [diff] [review]: ----------------------------------------------------------------- ::: servo/components/style/properties/gecko.mako.rs @@ +2218,5 @@ > + if self.gecko.mFont.fontlist.mFontlist.is_empty() { > + let default = match self.gecko.mFont.fontlist.mDefaultFontType { > + FontFamilyType::eFamily_serif => FontFamily::Generic(atom!("serif")), > + FontFamilyType::eFamily_sans_serif => FontFamily::Generic(atom!("sans-serif")), > + _ => panic!("asdsa"), better panic message mention why it's only serif or sans serif
Attachment #8899004 - Flags: review?(manishearth) → review+
Attachment #8899004 - Attachment is obsolete: true
Attachment #8901339 - Flags: review?(manishearth)
Update addresses failing tests after rebasing on changes from Bug 1383869. Try test: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3ae6e16f327ad045bd614513c76ae7592ed6b93
Attachment #8901339 - Flags: review?(manishearth) → review+
Pushed by canaltinova@gmail.com: https://hg.mozilla.org/integration/autoland/rev/960307b8d3c7 (part 1) - Handle CSS font-family units using switch instead of if. r=dbaron https://hg.mozilla.org/integration/autoland/rev/680ccadcd055 (part 2) - Update CSS fallback font when lang changes. r=dbaron https://hg.mozilla.org/integration/autoland/rev/5bd865157802 (part 3) - Construct language-specific default variable font as fallback font, not specified font. r=dbaron https://hg.mozilla.org/integration/autoland/rev/49a87dcd3bd0 (part 4) - Update CSS fallback font when lang changes (stylo). r=manishearth https://hg.mozilla.org/integration/autoland/rev/26e771621b71 (part 5) - Add testcases. r=dbaron
See Also: → 1394762
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: