Closed
Bug 372838
Opened 18 years ago
Closed 18 years ago
print preview is broken
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: Peter6, Assigned: masayuki)
References
Details
(Keywords: regression)
Attachments
(5 files, 4 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
pavlov
:
review+
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
Details |
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
Reporter | ||
Comment 1•18 years ago
|
||
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
Assignee | ||
Updated•18 years ago
|
Component: General → GFX: Thebes
Product: Firefox → Core
QA Contact: general → thebes
Comment 2•18 years ago
|
||
This bug also causes the same problem with the Firefox Showcase extension.
Assignee | ||
Comment 3•18 years ago
|
||
Roc:
Do you know the cause?
# bug 372741 may be same bug.
I don't know the cause
Updated•18 years ago
|
Flags: blocking1.9?
Assignee | ||
Comment 5•18 years ago
|
||
fix a part of this bug. but we have another bug...
Assignee | ||
Comment 6•18 years ago
|
||
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)
Assignee | ||
Comment 7•18 years ago
|
||
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)
Comment 8•18 years ago
|
||
why do you need a matrix == or Equals method? the default one should work fine.
Assignee | ||
Comment 9•18 years ago
|
||
(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.
Assignee | ||
Comment 12•18 years ago
|
||
I thought that the advances should be calculated at drawing/measuring time. But doesn't it have a damage of performance?
Assignee | ||
Comment 13•18 years ago
|
||
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?
Assignee | ||
Comment 16•18 years ago
|
||
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.
Assignee | ||
Comment 18•18 years ago
|
||
(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.
Assignee | ||
Comment 19•18 years ago
|
||
I think that if textrun should not depend on matrix, we need calculate the advance at drawing and measuring every time.
Comment 20•18 years ago
|
||
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.
Comment 21•18 years ago
|
||
(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.
Assignee | ||
Comment 23•18 years ago
|
||
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-
Assignee | ||
Comment 24•18 years ago
|
||
maybe, this is true.
Attachment #257922 -
Attachment is obsolete: true
Attachment #258214 -
Flags: review?(pavlov)
Assignee | ||
Comment 25•18 years ago
|
||
Stuart, would you review the patch?
Comment 26•18 years ago
|
||
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+
Assignee | ||
Comment 27•18 years ago
|
||
(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.
Comment 29•18 years ago
|
||
Aswell the scrollbars are scalled:
http://img440.imageshack.us/img440/3967/printpreviewrv2.jpg
Platform: windows XP
Comment 31•18 years ago
|
||
(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.
Comment 33•18 years ago
|
||
(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.
Assignee | ||
Comment 35•18 years ago
|
||
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)
Comment 36•18 years ago
|
||
(In reply to comment #35)
> Created an attachment (id=260826) [details]
> Patch rv5.0
>
I have verified that this patch resolves the issue.
Comment 37•18 years ago
|
||
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.
Assignee | ||
Comment 38•18 years ago
|
||
(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?
Comment 39•18 years ago
|
||
This is what I'm seeing... it's possible this problem is unique to my build, but I don't think it is.
Assignee | ||
Comment 40•18 years ago
|
||
looks like the layout bug...
And I cannot reproduce it...
Comment 41•18 years ago
|
||
(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.
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)
Comment 43•18 years ago
|
||
(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.
Updated•18 years ago
|
Attachment #261545 -
Flags: review?(pavlov) → review+
Comment 45•18 years ago
|
||
we might want to also remove the cairo_...for_hfont method entirely since it is kind of buggy.
Updated•18 years ago
|
Attachment #260826 -
Flags: review?(pavlov)
Isn't it already part of public cairo 1.4 API?
Comment 47•18 years ago
|
||
Please remove the stupid cairo-thing, it makes only problems. The last functional trunk-build was 3.3. 2007!
Comment 48•18 years ago
|
||
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
Comment 50•18 years ago
|
||
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?
Comment 51•18 years ago
|
||
AKIHIRO, could you please file a new bug for that (and mention the bug number here)?
Comment 52•18 years ago
|
||
filed Bug 378272
Reporter | ||
Comment 53•18 years ago
|
||
This also fixed the pagepreviews (canvas) in the tab sidebar extension
Comment 54•18 years ago
|
||
Great it works again! trunk from 22.4.2007
Updated•18 years ago
|
Flags: in-testsuite?
You need to log in
before you can comment on or make changes to this bug.
Description
•