Closed
Bug 391868
Opened 17 years ago
Closed 17 years ago
Page Source very small with meta charset=windows-1258
Categories
(Core :: Layout, defect)
Tracking
()
VERIFIED
FIXED
mozilla1.9beta1
People
(Reporter: martijn.martijn, Assigned: cpearce)
References
Details
(Keywords: regression, testcase)
Attachments
(2 files, 3 obsolete files)
(deleted),
text/html;charset=windows-1258
|
Details | |
(deleted),
patch
|
dbaron
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•17 years ago
|
Attachment #276302 -
Attachment mime type: text/html → text/html;charset=windows-1258
Reporter | ||
Comment 1•17 years ago
|
||
Hmm, the testcase is online when viewed already very small.
Updated•17 years ago
|
OS: Windows XP → All
Reporter | ||
Comment 2•17 years ago
|
||
Aslo happening on this page: http://www.proximusgoformusic.be/nl/report.php?id=110631 Also, scrolling with the mouse scroll wheel is slow.
Reporter | ||
Comment 3•17 years ago
|
||
And the text in the select and the text input is very small on that page.
Flags: blocking1.9?
Assignee | ||
Comment 4•17 years ago
|
||
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?
Comment 5•17 years ago
|
||
The computed 'font-size' is reported as 0px, which seems bad. (We should also probably make text that's 0px actually disappear...)
Reporter | ||
Comment 6•17 years ago
|
||
Scrolling is also slow with this testcase (although page source looks fine): https://bugzilla.mozilla.org/attachment.cgi?id=202582
Reporter | ||
Comment 7•17 years ago
|
||
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+
Assignee | ||
Comment 8•17 years ago
|
||
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.
Assignee | ||
Comment 9•17 years ago
|
||
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)
Comment 10•17 years ago
|
||
(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+
Updated•17 years ago
|
Assignee: nobody → chris
Comment 12•17 years ago
|
||
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?
Assignee | ||
Comment 13•17 years ago
|
||
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)
Comment 14•17 years ago
|
||
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?
Comment 15•17 years ago
|
||
And did you test that the new part of the patch alone, without the constructor changes, fixes the bug?
Assignee | ||
Comment 16•17 years ago
|
||
(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
Comment 17•17 years ago
|
||
I was suggesting not changing mLangGroup, but just changing what UpdateFontPreferences does with it, so that GetMetricsForInternal is not affected.
Comment 18•17 years ago
|
||
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".
Comment 19•17 years ago
|
||
(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.
Comment 20•17 years ago
|
||
I filed bug 399284, with patch.
Comment 21•17 years ago
|
||
Chris: are you planning to try what I suggested in comment 17?
Assignee | ||
Comment 22•17 years ago
|
||
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()?
Comment 23•17 years ago
|
||
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.
Assignee | ||
Comment 24•17 years ago
|
||
(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 25•17 years ago
|
||
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-
Assignee | ||
Comment 26•17 years ago
|
||
Attachment #284544 -
Flags: review?
Assignee | ||
Comment 27•17 years ago
|
||
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)
Assignee | ||
Updated•17 years ago
|
Attachment #284535 -
Attachment is obsolete: true
Comment 28•17 years ago
|
||
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+
Comment 29•17 years ago
|
||
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
Comment 30•17 years ago
|
||
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
Comment 31•17 years ago
|
||
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.
Description
•