Closed Bug 372838 Opened 18 years ago Closed 18 years ago

print preview is broken

Categories

(Core :: Graphics, defect)

x86
Windows XP
defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: Peter6, Assigned: masayuki)

References

Details

(Keywords: regression)

Attachments

(5 files, 4 obsolete files)

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070305 Minefield/3.0a3pre ID:2007030512 [cairo] just get a print preview result: everything is off scale regressed in the past few days I'll get the regressionwindow in a sec
regressionwindow works in 20070304_0816_firefox-3.0a3pre.en-US.win32 fails in 20070304_1322_firefox-3.0a3pre.en-US.win32 http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1173024960&maxdate=1173043319 -> Bug 370588
Blocks: 370588
Severity: normal → major
Component: General → GFX: Thebes
Product: Firefox → Core
QA Contact: general → thebes
Attached image Same effect with Firefox Showcase (deleted) —
This bug also causes the same problem with the Firefox Showcase extension.
Roc: Do you know the cause? # bug 372741 may be same bug.
Flags: blocking1.9?
Attached patch Patch rv1.0 (obsolete) (deleted) — Splinter Review
fix a part of this bug. but we have another bug...
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attachment #257803 - Flags: review?(pavlov)
Attached patch Patch rv2.0 (obsolete) (deleted) — Splinter Review
we also need the same patch in uniscribe. but this cannot fix this completely too...
Attachment #257803 - Attachment is obsolete: true
Attachment #257826 - Flags: review?(pavlov)
Attachment #257803 - Flags: review?(pavlov)
Attached patch Patch rv3.0 (obsolete) (deleted) — Splinter Review
This completely fixes this bug! The new textrun caches the advances that calculated with matrix. but the textrun cache doesn't check the matrix difference. we should check it.
Attachment #257826 - Attachment is obsolete: true
Attachment #257922 - Flags: review?(pavlov)
Attachment #257826 - Flags: review?(pavlov)
why do you need a matrix == or Equals method? the default one should work fine.
(In reply to comment #8) > why do you need a matrix == or Equals method? the default one should work fine. I think so too. But if I remove them, I cannot build...
Except for hinting, the values in a textrun shouldn't depend on the matrix. All glyph advances get scaled during drawing. Maybe we need to set an identity matrix while setting up the textrun?
In other words, I don't think making the matrix part of the textrun cache key is the right thing to do.
I thought that the advances should be calculated at drawing/measuring time. But doesn't it have a damage of performance?
And MeasureText and GetAdvanceWidth doesn't have gfxContext in them params...
Right. Those calculations don't depend on the matrix, they always assume an identity matrix.
When we're scaling down the rendering for Showcase or Print Preview, the textruns used should be exactly the same as the textruns for normal drawing. The only difference is that the matrix at Draw() time is different, so all the glyph positions and the glyph sizes themselves should be scaled. Perhaps something's going wrong with gfxWindowsFont's font selection in that case?
Should we only calculate at drawing?? And the actual drawer is gfxFont::Draw. But the UpdateCTM is only for Win32. Should we override the gfxFont::Draw on Win32?
Maybe the problem is here: http://lxr.mozilla.org/seamonkey/source/gfx/thebes/src/gfxWindowsFonts.cpp#1551 Textrun setup should not depend on the current matrix. I think we should always use the identity matrix and get rid of UpdateCTM.
(In reply to comment #17) > Maybe the problem is here: > http://lxr.mozilla.org/seamonkey/source/gfx/thebes/src/gfxWindowsFonts.cpp#1551 > Textrun setup should not depend on the current matrix. I think we should always > use the identity matrix and get rid of UpdateCTM. If you meant that the identity matrix is the result of |gfxMatrix()|, it is not true. I cannot fix this bug with the approach.
I think that if textrun should not depend on matrix, we need calculate the advance at drawing and measuring every time.
This also affects printout but in a different way. The preview text is overprinted, but actual printout shows black bars where text should be. At first I thought it was the 315520 problem, but my background print is off.
(In reply to comment #20) > This also affects printout but in a different way. The preview text is > overprinted, but actual printout shows black bars where text should be. At > first I thought it was the 315520 problem, but my background print is off. > This is mentioned in bug 367036. Probably a separate issue, since the black bars bug started before the scaling issue in print preview. The tooltip preview in SeaMonkey shows the same scaling issue as print preview does, and both started at the same time.
Comment on attachment 257922 [details] [diff] [review] Patch rv3.0 I see what happens in gfxWindowsFonts.cpp. I'll attach a new patch which textrun doesn't depend on matrix.
Attachment #257922 - Flags: review?(pavlov) → review-
Attached patch Patch rv4.0 (obsolete) (deleted) — Splinter Review
maybe, this is true.
Attachment #257922 - Attachment is obsolete: true
Attachment #258214 - Flags: review?(pavlov)
Stuart, would you review the patch?
Is the "tab preview" (I forget the proper term) tied to this? You know, when you hover over a tab and it shows you the thumbnail picture? It has what looks like huge text all overlapping, just like print preview.
Flags: blocking1.9? → blocking1.9+
(In reply to comment #26) > Is the "tab preview" (I forget the proper term) tied to this? You know, when > you hover over a tab and it shows you the thumbnail picture? It has what looks > like huge text all overlapping, just like print preview. Probably. There is a mismatch scaling problem.
Aswell the scrollbars are scalled: http://img440.imageshack.us/img440/3967/printpreviewrv2.jpg Platform: windows XP
(In reply to comment #24) > Created an attachment (id=258214) [details] > Patch rv4.0 This patch has bit-rotted a might. Perhaps an updated patch is in order.
(In reply to comment #31) > (In reply to comment #24) > > Created an attachment (id=258214) [details] [details] > > Patch rv4.0 > > This patch has bit-rotted a might. Perhaps an updated patch is in order. Agreed; I can't seem to get the patch to work.
Attached patch Patch rv5.0 (deleted) — Splinter Review
Sorry for the delay. I think that we need more works for removing the UpdateCTM. Because FillLogFont uses the matrix. I don't know whether the removing is reasonable work.
Attachment #258214 - Attachment is obsolete: true
Attachment #260826 - Flags: review?(pavlov)
Attachment #258214 - Flags: review?(pavlov)
(In reply to comment #35) > Created an attachment (id=260826) [details] > Patch rv5.0 > I have verified that this patch resolves the issue.
I ran into an issue when testing this patch: Steps: http://en-us.www.mozilla.com/en-US/firefox/central/, open print preview, close print preview, open print preview. Expected: Both PPs are the same Actual: Second time PP has overlapping text/only takes up one page.
(In reply to comment #37) > Actual: Second time PP has overlapping text/only takes up one page. I cannot understand what happens your system. Could you attach a screenshot?
Attached image Screenshot of problem with patch (deleted) —
This is what I'm seeing... it's possible this problem is unique to my build, but I don't think it is.
looks like the layout bug... And I cannot reproduce it...
(In reply to comment #37) > I ran into an issue when testing this patch: > > Steps: http://en-us.www.mozilla.com/en-US/firefox/central/, open print preview, > close print preview, open print preview. > I see the same issue on my system. It is a fairly vanilla build. I could try a clobber build. It seems to happen more on some pages than others. I also get it on the Minefield start page. http://www.mozilla.org/projects/minefield/ I tried disabling disk cache and bfcache, but that did not help.
Blocks: 377201
Attached patch fix (deleted) — Splinter Review
Here's what I think is the right fix. One part of the fix is to make gfxWindowsFont not maintain a CTM. It always works assuming an identity CTM. This means that all the metrics extraction, in particular during textrun creation, assumes an identity CTM. This is what layout wants. The other part of the fix is in cairo. We were always using the font-face's HFONT for the scaled-font's scaled_hfont, so we were never scaling the font to the CTM. With the above change, this made glyphs draw too big in print preview, even though the inter-glyph advances were correct. I think cairo intends that font-faces be CTM-independent, so scaling the HFONT we pass to font_face_create would seem wrong to me. We should create the same font-face no matter what the CTM and allow cairo to choose the right scaled-font based on the current CTM when drawing. (Note that cairo automatically does this; when you pass in a scaled-font which you created with a different CTM to the current CTM, cairo automatically chooses a different scaled-font with the right CTM.) So, what I've done in this patch is basically always create a font-face from a LOGFONTW, but allow an HFONT to also be passed in to a font-face as an optimization. The HFONT will only be used for a scaled-font which is created with an identity CTM and a font matrix whose size is equal to the logfont's -lfHeight. The idea is to use the passed-in HFONT when we can be sure it's exactly the same as the HFONT the scaled-font would have created itself. If we can't be sure of that, we allow the scaled-font to create its own HFONT. This fixes the bug nicely and allows us to avoid creating extra HFONTs in common cases.
Attachment #261545 - Flags: review?(pavlov)
(In reply to comment #42) > Created an attachment (id=261545) [details] > fix OK. this seems to work for me and avoids the issues mentioned in comment #37.
Attachment #261545 - Flags: review?(pavlov) → review+
we might want to also remove the cairo_...for_hfont method entirely since it is kind of buggy.
Attachment #260826 - Flags: review?(pavlov)
Isn't it already part of public cairo 1.4 API?
Please remove the stupid cairo-thing, it makes only problems. The last functional trunk-build was 3.3. 2007!
roc: It may be if the windows api was ever made "public" -- i would at least deprecate it.
Checked into trunk. We should probably deprecate the API in upstream cairo when someone (stuart?) pushes the cairo part of this patch upstream.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Attached image Screenshot (deleted) —
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a4pre) Gecko/20070421 SeaMonkey/1.5a I'm a third party theme developer. The scrollbar of SeaMonkey Modern theme is still broken. In other words, The scrollbar of no native appearance theme is still broken. <scrollbar> and <thumb> and <scrollbarbutton> background can not themable on print preview. Is this this bug?
AKIHIRO, could you please file a new bug for that (and mention the bug number here)?
This also fixed the pagepreviews (canvas) in the tab sidebar extension
Great it works again! trunk from 22.4.2007
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: