We currently have language-group-specific font preferences (for default families and sizes), but we use that only on a per-document level, and based only on the character encoding of the document. We could do a lot better here, and use the correct pref for the language. (That said, it might mean users would see the effects of their different font prefs more often -- ones that they may well have no idea how to set.) (Note that in some cases I think gfx uses the same preferences to get correct language-specific fallbacks.)

(In reply to comment #0) > We currently have language-group-specific font preferences (for default > families and sizes), but we use that only on a per-document level, and based > only on the character encoding of the document. At the rendering level this is specified by the gfxFontStyle attached to a font group. > We could do a lot better here, and use the correct pref for the language. > (That said, it might mean users would see the effects of their different font > prefs more often -- ones that they may well have no idea how to set.) Hmmm, what's the "correct pref for the language" mean precisely? This is a very tricky question, see bug 390900 for an interesting example of where things get confusing, there are some really weird interactions with generics and language setting. And with CJK characters it's hard to know which pref setting to use if the charset is Unicode. > (Note that in some cases I think gfx uses the same preferences to get correct > language-specific fallbacks.) This is part of the trickiness, in the fallback case we infer the lang of individual characters and use the pref font settings for that lang before doing a system-wide font search.

At the very least, I was referring to picking up the information provided by the lang attribute, which is reflected in GetStyleVisibility()->mLangGroup.

Holy crap, we're not doing that already?!? Yes, I completely agree, that needs to get fixed.

Holy crap indeed. Is that a regression from Thebes? Do you have a test case? We certainly used to use the lang attribute.

So I think *gfx* uses the lang attribute as part of font selection. At least I think it does. (Although for a while it didn't, but I think that regression was fixed.) The issue is that we don't use the lang attribute to apply the user's preferences for default fonts -- at least when the style system applies them. I think in some cases (looks like Windows and Mac, but not GTK) the gfx code may *reapply* the same preferences (or use them for something -- I'm not sure exactly what), but a lower priority than the style system doing it, since what gfx gets from layout is a concatenation of what the author specified and the user's preference.

So, to be clear, this definitely affects the language-specific font *size* preferences, and possibly also affects the serif vs. sans-serif default preference (depending on how the two things that read that pref interact). Testcases for the font size pref issue are: * the FIXME in layout/style/test/test_bug401046.html * the fact that the same test fails on non-Western locales Also, I think the first step for fixing this bug is to move mLanguage from nsStyleVisibility to nsStyleFont -- that'll fix a bunch of potential cross-struct-dependency issues.

It's not clear to me exactly what I should be doing in nsStyleFont::CalcDifference since I don't have mVisible there, and it's not clear to me what should take priority when integrated with the existing code in that method.

Comment on attachment 581424 [details] [diff] [review] Move mLanguage from nsStyleVisibility to nsStyleFont nsRuleNode.cpp: Remove the aStyleVisibility parameters to: CalcLengthWith CalcLengthCalcOps (mStyleVisibility too) GetMetricsFor nsStyleStruct.cpp: The CalcDifference is really just weirdly optimized code: basically, the logic is things like this: if (a != other.a) return reflow; if (b != other.b) return paint; etc. However, what's confusing you is that there's a bunch of if-else nesting that uses == rather than !=. You really just need to return NS_STYLE_HINT_REFLOW when the language is different, just like when the size is different. It's probably clearer to make it: if (mSize != aOther.mSize || mLanguage != aOther.mLanguage) { return NS_STYLE_HINT_REFLOW; } return CalcFontDifference(mFont, aOther.mFont); But you also need to leave the visibility-testing logic in nsStyleVisibility::CalcDifference. r=dbaron with that, but I want to look at the revisions to the CalcDifference methods

Comment on attachment 583058 [details] [diff] [review] Move mLanguage from nsStyleVisibility to nsStyleFont gah, forgot to do a final qref

Probably a good idea to look over the entire interdiff BTW. Obviously I'll remove the try flags from the commit message before pushing.

Comment on attachment 583063 [details] [diff] [review] Move mLanguage from nsStyleVisibility to nsStyleFont Sorry, more things to remove in nsRuleNode that I missed the last time: param from SetFontSize param and member from SetFontSizeCalcOps languageVisibility variable in nsRuleNode::SetFont (and if you see more that I missed this time too, go ahead) r=dbaron with those removed and the "try: ..." not in the commit message

Actually I've had to make a change in content/html/content/src/nsGenericHTMLElement.cpp and in layout/style/nsCSSPropList.h to fix Try crashes. Even now I'm still getting failures in reftests/canvas/text-font-lang.html and reftests/text/auto-hyphenation-1.html, auto-hyphenation-1a.html and auto-hyphenation-5.html, and I'm having some trouble figuring out what I've missed/done wrong. https://tbpl.mozilla.org/?tree=Try&rev=24c2dc528f8f Once that's fixed I'll remove the extra stuff you've requested to be removed and post a new patch.

The problem is basically that the 'lang' attribute doesn't make it through to the computed style for HTML canvas elements for some reason. I don't see anything wrong with my code, and I'm having a little trouble figuring out the problem using gdb. Can you think of anything obvious that I might have missed (hidden in a macro or something?) David? If not I'll keep working on getting to grips with how style is computed until I figure it out.

I notice the following comment in the code: const void* nsRuleNode::ComputeVisibilityData(void* aStartStruct, const nsRuleData* aRuleData, nsStyleContext* aContext, nsRuleNode* aHighestNode, const RuleDetail aRuleDetail, const bool aCanStoreInRuleTree) { COMPUTE_START_INHERITED(Visibility, (mPresContext), visibility, parentVisibility) // IMPORTANT: No properties in this struct have lengths in them. We // depend on this since CalcLengthWith can call GetStyleVisibility() // to get the language for resolving fonts! I'm not sure what that means, but I wonder if it's of concern now that mLanguage is on nsStyleFont, which does have lengths.

(In reply to Jonathan Watt [:jwatt] from comment #14) > The problem is basically that the 'lang' attribute doesn't make it through > to the computed style... What seems to be going wrong is this: when nsRuleNode::ComputeFontData() is called under WalkRuleTree(), things start out okay. At the beginning of the method the '-x-lang' (eCSSProperty__x_lang) property is correctly inherited into the new nsStyleFont object, 'font'. Further down though, the nsRuleNode::SetGenericFont() call overwrites this property when it should not. I really don't understand why we're calling SetGenericFont() to do what it does - i.e. recalculate the cascade from the first ancestor that has a generic font family name (seems a bit quirks-mode'y). More to the point I was pretty surprising to discover that a setter includes it's own mini take on the whole cascade implementation. Anyway, I'll take it on faith that this is what we want to do. Digging into SetGenericFont(), the issue seems to be that when we call nsRuleNode::SetFont() with the nsRuleData that's worked out from the style context with the '-x-lang' property set, it doesn't actually do anything for this property. Since the nsStyleFont that's being worked out under SetGenericFont is then used to reset the initial one in ComputeFontData, this is how the '-x-lang' property on the initial one unexpectedly goes from having the right value to having the wrong value. So it seems that my patch just needs some code added to nsRuleNode::SetFont() to handle the '-x-lang' property. It's not clear to me at this point what that code should look like, but I'll try and figure it out tomorrow after some sleep.

Oh, actually there was one other place that I stumbled across during my debugging that looked like it _might_ need some new code to handle lang - CheckFontCallback. Not sure about that one yet though.

Other than the removal of nsStyleVisibility variables than are now unnecessary as requested in comment 12, there are only a few significant changes from the last r+'ed patch: * the change in nsGenericHTMLElement.cpp * the change in nsCSSPropList.h * an XXX comment in CheckFontCallback * the change to nsRuleNode::SetFont - not perfect, but I'm hoping you (David) can comment on what I should be doing there

Comment on attachment 588375 [details] [diff] [review] Move mLanguage from nsStyleVisibility to nsStyleFont - passes Try In nsRuleNode.cpp, in CheckFontCallback, you have: >+ // XXX check?: const nsCSSValue* langValue = aRuleData->ValueForLang(); Just remove the comment. This function is for detecting special values that act a lot like 'inherit' (in particular, require the same sort of disabling of sharing that 'inherit' does). lang doesn't have anything like that. You don't need to duplicate the code between nsRuleNode::ComputeFontData and nsRuleNode::SetFont. SetFont is a helper function for ComputeFontData, so you're doing the same work twice, and in some cases even more than twice. I think it should be *only* in ComputeFontData, but probably earlier in the function -- definitely before the calls to SetFont and SetGenericFont -- and probably basically at the beginning: I'd say put it right after the 10-line comment after the COMPUTE_START_INHERITED line. You should, in fact, note in a comment that it's important that the language be set before any of the CalcLengthWith calls in SetFont (direct calls or calls via SetFontSize) for the cases where |parentFont| is the same as |font|. r=dbaron with that

(In reply to David Baron [:dbaron] from comment #21) > You don't need to duplicate the code between nsRuleNode::ComputeFontData > and nsRuleNode::SetFont. SetFont is a helper function for > ComputeFontData, so you're doing the same work twice, and in some cases > even more than twice. I think it should be *only* in ComputeFontData That would break inheritance of mLanguage when SetGenericFont is called. As mentioned in comment 17, SetGenericFont overwrites the nsStyleFont that ComputeFontData passes to it, so setting mLanguage in ComputeFontData before the SetGenericFont call is useless when it's called. It looks like setting mLanguage at the top of SetFont is early enough to avoid any CalcLengthWith problems, so it seems best to remove the duplicate mLanguage setting code from ComputeFontData instead. (Also that passes Try.) Is that okay with you? You can see the patch I pushed to Try at: https://hg.mozilla.org/try/rev/de90114784f5

You may with to change the method that's used to cache the prefs, but funtionally this fixes the bug.

Tidy that up a little.

(In reply to Jonathan Watt [:jwatt] from comment #22) > It looks like setting mLanguage at the top of SetFont is early enough to > avoid any CalcLengthWith problems, so it seems best to remove the duplicate > mLanguage setting code from ComputeFontData instead. (Also that passes Try.) > Is that okay with you? You can see the patch I pushed to Try at: > https://hg.mozilla.org/try/rev/de90114784f5 Sounds good.

For some reason the second patch seems to be causing layout/style/test/chrome/test_moz_document_rules.html to time out _after_ all the tests in that file have passed. It happens on all platforms on Try, but I can't reproduce locally. If anyone has any ideas...

Pushed part 1 to m-i: https://hg.mozilla.org/integration/mozilla-inbound/rev/cfc432c9bfd1

https://hg.mozilla.org/mozilla-central/rev/cfc432c9bfd1 Leaving open for the rest.

Seems to pass Try now BTW: https://tbpl.mozilla.org/?tree=Try&rev=a90db6ed7bc7

Comment on attachment 593059 [details] [diff] [review] Part 2 - take lang group specific font size prefs into account Make LangGroupFontPrefs::mLangGroup an nsCOMPtr<nsIAtom> and remove all of the manual reference counting (i.e., the 2 occurrences of NS_IF_RELEASE and 1 occurrence of NS_ADDREF that your patch adds). + NS_ABORT_IF_FALSE(++count < 35, "Lang group count exceeded!!!"); This shouldn't be an abort, given that if we add new lang groups we might exceed it. Make it an assertion or have a way of actually detecting the number of lang groups at compile time (e.g., if they're listed in a header file with constants). Remove the "= nsnull" default to the parameter to nsPresContext::MinFontSize. It looks like it's unused (and if it's not, then this bug isn't fully fixed). >+ friend class nsAutoPtr<LangGroupFontPrefs>; This seems odd. Is it needed? Can it be replaced by |friend struct LangGroupFontPrefs|? Don't give nsPresContext::GetFontPrefsForLang's parameter a default value. We want callers to pass the correct language (and all the ones you've added do, so it's trivial to remove). > // See if we are in the chrome > // We only need to know this to determine if we have to use the > // document fonts (overriding the useDocumentFonts flag), or to > // determine if we have to override the minimum font-size constraint. >- if ((!useDocumentFonts || minimumFontSize > 0) && mPresContext->IsChrome()) { >+ if (!useDocumentFonts && mPresContext->IsChrome()) { Please fix up this comment to remove mention of min font size. nsRuleNode::ComputeTextData: Hmm. This is sort of bad. Technically you need to move the "canStoreInRuleTree = false" along with the GetStyleFont line, since the cross-struct dependency is what means we can't store the result in the rule tree. That's a bid of de-optimization, but I think it's probably best to just bite the bullet and do it. Effectively it only means that 'line-height: 15px' is no better than 'line-height: 1.2em', so I'm not too worried. nsStyleStruct.cpp: I think this is wrong, and hopefully even testably so (on pages where the document doesn't specify a font). You should initialize mFont after calling Init(), and pass mLanguage. And if you can, add a test, but don't spend too much time trying? r=dbaron with those things fixed

(In reply to David Baron [:dbaron] (busy through 12 Feb) from comment #32) > Comment on attachment 593059 [details] [diff] [review] > This shouldn't be an abort, given that if we add new lang groups > we might exceed it. Make it an assertion or have a way of actually > detecting the number of lang groups at compile time (e.g., if they're > listed in a header file with constants). I believe the lang groups are built up from: https://mxr.mozilla.org/mozilla-central/source/intl/locale/src/langGroups.properties?raw=1 Shall I add a LangGroupCount() method to nsILanguageAtomService (should be easy)? > Remove the "= nsnull" default to the parameter to > nsPresContext::MinFontSize. It looks like it's unused (and if it's not, > then this bug isn't fully fixed). Currently no lang argument is passed to MinFontSize by: TransferZoomLevels (in nsDocument.cpp) DocumentViewerImpl::SetMinFontSize DocumentViewerImpl::GetMinFontSize So having the nsnull default for them seemed okay. I could make it explicit though. > >+ friend class nsAutoPtr<LangGroupFontPrefs>; > > This seems odd. Is it needed? Can it be replaced by > |friend struct LangGroupFontPrefs|? It's needed to allow the nsAutoPtr<LangGroupFontPrefs> dtor to access LangGroupFontPrefs's dtor (LangGroupFontPrefs is protected, and one of its members is an nsAutoPtr). > nsRuleNode::ComputeTextData: > > Hmm. This is sort of bad. Technically you need to move the > "canStoreInRuleTree = false" along with the GetStyleFont line, > since the cross-struct dependency is what means we can't store > the result in the rule tree. That's a bid of de-optimization, > but I think it's probably best to just bite the bullet and do > it. Effectively it only means that 'line-height: 15px' is > no better than 'line-height: 1.2em', so I'm not too worried. I don't understand. Merely calling GetStyleFont means that canStoreInRuleTree must be set to false? > nsStyleStruct.cpp: > > I think this is wrong, and hopefully even testably so (on pages where > the document doesn't specify a font). You should initialize mFont > after calling Init(), and pass mLanguage. And if you can, add > a test, but don't spend too much time trying? Okay, I'll need to set nsStyleFont::mFont in nsRuleNode::SetFont() after setting mLanguage, then.

(In reply to Jonathan Watt [:jwatt] from comment #33) > (In reply to David Baron [:dbaron] (busy through 12 Feb) from comment #32) > > Comment on attachment 593059 [details] [diff] [review] > > This shouldn't be an abort, given that if we add new lang groups > > we might exceed it. Make it an assertion or have a way of actually > > detecting the number of lang groups at compile time (e.g., if they're > > listed in a header file with constants). > > I believe the lang groups are built up from: > > https://mxr.mozilla.org/mozilla-central/source/intl/locale/src/langGroups. > properties?raw=1 > > Shall I add a LangGroupCount() method to nsILanguageAtomService (should be > easy)? Sounds fine. > > Remove the "= nsnull" default to the parameter to > > nsPresContext::MinFontSize. It looks like it's unused (and if it's not, > > then this bug isn't fully fixed). > > Currently no lang argument is passed to MinFontSize by: > > TransferZoomLevels (in nsDocument.cpp) > DocumentViewerImpl::SetMinFontSize > DocumentViewerImpl::GetMinFontSize > > So having the nsnull default for them seemed okay. I could make it explicit > though. Explicit would be better. That said, what's the doc-specific min font size used by? It seems like this would sort of break it. > > nsRuleNode::ComputeTextData: > > > > Hmm. This is sort of bad. Technically you need to move the > > "canStoreInRuleTree = false" along with the GetStyleFont line, > > since the cross-struct dependency is what means we can't store > > the result in the rule tree. That's a bid of de-optimization, > > but I think it's probably best to just bite the bullet and do > > it. Effectively it only means that 'line-height: 15px' is > > no better than 'line-height: 1.2em', so I'm not too worried. > > I don't understand. Merely calling GetStyleFont means that > canStoreInRuleTree must be set to false? Exactly. Style data can be stored in the rule tree only when they're a function only of the style rules represented by that rule node (i.e., they're not a function of anything inherited from parent style contexts). Since the contents of GetStyleFont could contain values coming from inheritance from a parent style context (and in fact likely do), it means that the resulting data can't be cached in the rule tree.

(In reply to David Baron [:dbaron] from comment #34) > (In reply to Jonathan Watt [:jwatt] from comment #33) > > Currently no lang argument is passed to MinFontSize by: > > > > TransferZoomLevels (in nsDocument.cpp) > > DocumentViewerImpl::SetMinFontSize > > DocumentViewerImpl::GetMinFontSize > > > > So having the nsnull default for them seemed okay. I could make it explicit > > though. > > Explicit would be better. > > That said, what's the doc-specific min font size used by? It seems like > this would sort of break it. It comes from bug 623820, which gave the ability for some code in mobile/xul/chrome/content/content.js to use the nsIMarkupDocumentViewer interface to set the mMinFontSize member of nsPresContext so that the use of text-zoom on mobile doesn't screw up rendering. So this is a document wide thing, and we need to have a way to ask nsPresContext::MinFontSize for this value in order for it to propagate correctly. So I think what the patch does now is correct. I'll change it to pass an explicit nsnull though. > > I don't understand. Merely calling GetStyleFont means that > > canStoreInRuleTree must be set to false? > > Exactly. Style data can be stored in the rule tree only when they're a > function only of the style rules represented by that rule node (i.e., > they're not a function of anything inherited from parent style contexts). > Since the contents of GetStyleFont could contain values coming from > inheritance from a parent style context (and in fact likely do), it means > that the resulting data can't be cached in the rule tree. Just to be clear, this is a preexisting bug that I'm fixing then. Previously that min font size came from looking at the nsStyleFont - the patch just delays the point at which we looked at it.

(In reply to Jonathan Watt [:jwatt] from comment #35) > Just to be clear, this is a preexisting bug that I'm fixing then. Previously > that min font size came from looking at the nsStyleFont - the patch just > delays the point at which we looked at it. Oh, sorry, I get it. It's only now that the min font size depends on the data in the nsStyleContext that the node can't be cached in the rule tree.

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/8bdb4c87ad78 (In reply to David Baron [:dbaron] from comment #32) > nsStyleStruct.cpp: > > I think this is wrong, and hopefully even testably so (on pages where > the document doesn't specify a font). You should initialize mFont > after calling Init(), and pass mLanguage. And if you can, add > a test, but don't spend too much time trying? As explained on IRC, passing nsnull as the language makes GetDefaultFont use the document's language, which is what we want for the default. I've added a comment noting that in the nsStyleStruct.cpp caller as requested by dbaron. (In reply to David Baron [:dbaron] from comment #34) > > Shall I add a LangGroupCount() method to nsILanguageAtomService (should be > > easy)? > > Sounds fine. Actually, that turned out to be tricky, so I went the NS_ASSERTION route.