Closed Bug 391868 Opened 17 years ago Closed 17 years ago

Page Source very small with meta charset=windows-1258

Categories

(Core :: Layout, defect)

x86
All
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9beta1

People

(Reporter: martijn.martijn, Assigned: cpearce)

References

Details

(Keywords: regression, testcase)

Attachments

(2 files, 3 obsolete files)

Attached file testcase
See testcase, when doing View->Page Source, the font size of the viewed source is very small.
This regressed between 2007-02-06 and 2007-02-07, regression from bug 177805.
Attachment #276302 - Attachment mime type: text/html → text/html;charset=windows-1258
Hmm, the testcase is online when viewed already very small.
OS: Windows XP → All
Aslo happening on this page: http://www.proximusgoformusic.be/nl/report.php?id=110631
Also, scrolling with the mouse scroll wheel is slow.
And the text in the select and the text input is very small on that page.
Flags: blocking1.9?
When I add logging to gfxWindowsFont::MakeHFONT() and gfxFontStyle::gfxFontStyle() I see that all the fonts used in the ViewSource window are constructed as 1pt. Could the style rules be getting a bad default value somehow?
The computed 'font-size' is reported as 0px, which seems bad.  (We should also probably make text that's 0px actually disappear...)
Scrolling is also slow with this testcase (although page source looks fine): https://bugzilla.mozilla.org/attachment.cgi?id=202582
Sorry, never mind comment 6. I had the textZoom at a very small size, which makes scrolling apparently slow. That's bug 140676, I guess.
Flags: blocking1.9? → blocking1.9+
Ok, this was caused by Bug 177805's patch (attachment 254518 [details] [diff] [review] circa line 5010), which changed the constructor of nsPresContext. The constructor was previously initializing the default font sizes to be either 10pt or 12pt, depending on the font. This change assumed that the fonts would always be initialized by nsPresContext::GetFontPreferences() later on. That patch changed the constructor to instead initialize the fonts to be 0pt. However, this breaks in the case where the fonts are in charset windows-1258, because when nsPresContext::UpdateCharSet() calls mLangService->LookupCharSet(), that can't find an appropriate langGroup for windows-1258. Thus mLangGroup==0, and when nsPresContext::GetFontPreferences() is called, it doesn't setup the font sizes, resulting in size-0 fonts being created, which are used to display the document/sorce, hence the bug. The fix is to make the code initialize the fonts to a sensible default size, as they did before Bug 177805's patch.
Reverts nsPresContext constructor to initialize fonts to non-zero default values. That way if the langGroup is not initialized due to mLangService->LookupCharSet() failing, the fonts have sensible sizes. I had to make the constructor take the nsDeviceContext used, as that's used by nsPresContext::PointsToAppUnits(). So there was a little refactoring to move the nsDeviceContext to be passed into the constructor instead of nsPresContext::Init().
Attachment #283968 - Flags: review?(roc)
(In reply to comment #8)
> when
> nsPresContext::UpdateCharSet() calls mLangService->LookupCharSet(), that can't
> find an appropriate langGroup for windows-1258.

Can you file a follow-up bug on that? I don't think it's deliberate, and I don't see any reason that we shouldn't assign a langGroup for all charsets that we support.
Comment on attachment 283968 [details] [diff] [review]
Patch v1 - change nsPresContext constructor

Looks good. Remind me to land this for you tomorrow.
Attachment #283968 - Flags: superreview+
Attachment #283968 - Flags: review?(roc)
Attachment #283968 - Flags: review+
Assignee: nobody → chris
Blocks: 398905
Can't we instead guarantee that every language is covered by at least one of the preferences -- maybe using Western as fallback, so that we don't use font sizes that the user can't change?
This patch extends the last one, but it additionally sets the langGroup of an nsPresContext to "x-western" if a suitable lang group can't be found. This patch still assigns default font sizes in the nsPresContext constructor, in case all else fails.
Attachment #283968 - Attachment is obsolete: true
Attachment #284260 - Flags: review?(dbaron)
Hrm.  I'm not sure if we really want to change mLangGroup for everything that uses it (i.e., GetMetricsForInternal).  Maybe the check should be in nsPresContext::GetFontPreferences instead?
And did you test that the new part of the patch alone, without the constructor changes, fixes the bug?
(In reply to comment #15)
> And did you test that the new part of the patch alone, without the constructor
> changes, fixes the bug?
> 

Yup, the new part of the patch fixes the problem by itself, for this test case. The constructor changes aren't strictly necessary, I just left them in there because I'm paranoid.

We could change nsPresContext::GetFontPreferences() to fallback to x-western if !mLangGroup. GetFontPreferences() is just called by UpdateCharSet() though, so it won't have any undue side effect on this call path. I'd agrue that it makes more sense to update the langGroup when the charset is updated, rather than when the font preferences are obtained though.

Your call David.

[Taking bug]
Status: NEW → ASSIGNED
I was suggesting not changing mLangGroup, but just changing what UpdateFontPreferences does with it, so that GetMetricsForInternal is not affected.
Comment on attachment 284260 [details] [diff] [review]
Patch v2 - Fall back to x-western "lang group"

> GK_ATOM(Unicode, "x-unicode")
>+GK_ATOM(XWestern, "x-western")

I'd prefer to call it "Western", not "XWestern".
(In reply to comment #10)
> (In reply to comment #8)
> > when
> > nsPresContext::UpdateCharSet() calls mLangService->LookupCharSet(), that can't
> > find an appropriate langGroup for windows-1258.
> 
> Can you file a follow-up bug on that? 

Actually I think that shouldn't be a follow-up bug: we should fix that first (which is just a matter of adding entries to intl/uconv/src/charsetData.properties), and this should be the follow-up.
Depends on: 399284
I filed bug 399284, with patch.
Chris: are you planning to try what I suggested in comment 17?
Oops, sorry, I meant to test this yesterday. I just tried Simon's patch for bug 399284, and that fixes the problem reported in this bug. Do we still want/need to change UpdateFontPreferences()?
I think we probably do, but if we know that all encodings are covered now (and have a test for it in the future), it's probably ok to just leave it.
(In reply to comment #23)
> I think we probably do, but if we know that all encodings are covered now (and
> have a test for it in the future), it's probably ok to just leave it.
> 

Here's the code to do that, and this also fixes the bug. It's probably sensible to have this as a backup in case we encounter an unforseen charset/lang, but it's your call.
Attachment #284260 - Attachment is obsolete: true
Attachment #284535 - Flags: review?(dbaron)
Attachment #284260 - Flags: review?(dbaron)
Comment on attachment 284535 [details] [diff] [review]
Patch v3 - set default mLangGroup in GetFontPreferences()

No, what I meant was:

nsIAtom *langGroup = mLangGroup ? mLangGroup : nsGkAtoms::Western;

and then make the rest of the function use langGroup instead of mLangGroup.
Attachment #284535 - Flags: review?(dbaron) → review-
Comment on attachment 284544 [details] [diff] [review]
Patch v4 - default to "x-western" in GetFontPreferences()

Defaults langGroup used to "x-western", and only uses mLangGroup if it exists.
Attachment #284544 - Flags: review? → review?(dbaron)
Attachment #284535 - Attachment is obsolete: true
Comment on attachment 284544 [details] [diff] [review]
Patch v4 - default to "x-western" in GetFontPreferences()

r+sr=dbaron, although I'm not sure what you mean by "is safe".
Attachment #284544 - Flags: superreview+
Attachment #284544 - Flags: review?(dbaron)
Attachment #284544 - Flags: review+
Checking in layout/base/nsPresContext.cpp;
/cvsroot/mozilla/layout/base/nsPresContext.cpp,v  <--  nsPresContext.cpp
new revision: 3.334; previous revision: 3.333
done
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M9
verified fixed using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a9pre) Gecko/2007110605 Minefield/3.0a9pre. I used martijn's testcase as well as the site in Comment 2 to verify.
Status: RESOLVED → VERIFIED
Covered by layout/base/tests/test_bug399284.html
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: