Closed
Bug 369698
Opened 18 years ago
Closed 18 years ago
After changing layout.css.dpi value, application and layout text looks very bad.
Categories
(Core Graveyard :: GFX, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: romaxa, Assigned: sharparrow1)
References
Details
Attachments
(7 files)
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
Open seamonkey, with new "Fixed Units" Click MozillaZine bookmark Open new tab -> about:config -> layout.css.dpi=200 Resize Browser window (workaround for simple resize reflow) Text on layout and application elements broken. I think FontMetrics need to be reinitialized also according to changed DPI value.
Reporter | ||
Comment 1•18 years ago
|
||
Reporter | ||
Comment 2•18 years ago
|
||
Reporter | ||
Comment 3•18 years ago
|
||
Attachment #254392 -
Flags: review?(roc)
Reporter | ||
Comment 4•18 years ago
|
||
Reporter | ||
Comment 5•18 years ago
|
||
Attachment #254392 -
Flags: review?(roc) → review?(pavlov)
Assignee | ||
Comment 6•18 years ago
|
||
Hmm, does calling FlushFontCache when the DPI changes do the trick? That seems cleaner than adding a field to the font metrics. I don't have any objections to this patch, though. Also, if we're going to fix this right, we really ought to trigger a resize reflow as well. Not sure what the best way to do that is, though, since you can't trigger resize reflows from GFX.
Reporter | ||
Comment 7•18 years ago
|
||
> can't trigger resize reflows from GFX.
I think it should be another bug.... because there are 2 ways
1) Do resize reflow
2) Increase scrollwidth[height] and reflow only scrolbars...
Reporter | ||
Comment 8•18 years ago
|
||
with way N2 we will get exactly the same zooming stuff, like Opera, IE ;) BUG https://bugzilla.mozilla.org/show_bug.cgi?id=4821
Reporter | ||
Comment 9•18 years ago
|
||
>Hmm, does calling FlushFontCache when the DPI changes do the trick?
I think it is wrong, because it will flush FontMetrics wich can be used in other Window.... :(,
Assignee | ||
Comment 10•18 years ago
|
||
(In reply to comment #7) > > can't trigger resize reflows from GFX. > I think it should be another bug.... because there are 2 ways > 1) Do resize reflow > 2) Increase scrollwidth[height] and reflow only scrolbars... The way I'm planning to implement zoom is with a scaling factor on the viewport rather than messing with the dpi. Therefore, we want a resize reflow. It's okay to leave it for another bug, though. (In reply to comment #9) > >Hmm, does calling FlushFontCache when the DPI changes do the trick? > I think it is wrong, because it will flush FontMetrics wich can be used in > other Window.... :(, I'm pretty sure the cache is per device context.
Assignee | ||
Comment 11•18 years ago
|
||
As far as I can tell, this patch makes changing the layout.css.dpi setting work flawlessly. Note that for the nsPresContext.cpp code, I had to go through all those steps to clean everything out properly. I'm not sure how compatible this is with what you're trying to do, though. BTW, there are a couple of extra changes mized into the patch; they're pieces of the fix to bug 370034. Should be obvious which changes those are.
Attachment #255074 -
Flags: review?(roc)
(In reply to comment #10) > The way I'm planning to implement zoom is with a scaling factor on the viewport > rather than messing with the dpi. That's what I was thinking. However, it won't work well with windowed plugins. I'm currently thinking that perhaps we should have a zoom feature that is controlled by varying the mAppUnitsPerInch and mAppUnitsPerDevUnit values in the device context.
Comment on attachment 255074 [details] [diff] [review] Possible fix + if (changed && mShell) { + mDeviceContext->FlushFontCache(); + + nsIViewManager* vm = GetViewManager(); + nscoord width = DevPixelsToAppUnits(bounds.width); + nscoord height = DevPixelsToAppUnits(bounds.height); + vm->SetWindowDimensions(width, height); + + ClearStyleDataAndReflow(); + } How does this perform for deeply nested document trees? Do we end up reflowing child documents many times? Make CheckDPIChange "virtual PRBool CheckDPIChange()".
Attachment #255074 -
Flags: review?(roc) → review+
Assignee | ||
Comment 14•18 years ago
|
||
(In reply to comment #13) > (From update of attachment 255074 [details] [diff] [review]) > + if (changed && mShell) { > + mDeviceContext->FlushFontCache(); > + > + nsIViewManager* vm = GetViewManager(); > + nscoord width = DevPixelsToAppUnits(bounds.width); > + nscoord height = DevPixelsToAppUnits(bounds.height); > + vm->SetWindowDimensions(width, height); > + > + ClearStyleDataAndReflow(); > + } > > How does this perform for deeply nested document trees? Do we end up reflowing > child documents many times? It could get bad; it depends on the order the pref listeners fire. A frame could in theory get the same number of resize reflows as it has parents if the parents get the notifications first and the frames use percentage sizes. I don't think it's a serious issue, though. The performance of a ClearStyleDataAndReflow probably isn't all that great, but that's a different issue. We might be able to get away with keeping the style data if we can somehow make the system fonts smarter. > Make CheckDPIChange "virtual PRBool CheckDPIChange()". Sure. (In reply to comment #12) > (In reply to comment #10) > > The way I'm planning to implement zoom is with a scaling factor on the viewport > > rather than messing with the dpi. > > That's what I was thinking. However, it won't work well with windowed plugins. > I'm currently thinking that perhaps we should have a zoom feature that is > controlled by varying the mAppUnitsPerInch and mAppUnitsPerDevUnit values in > the device context. It's sort of a hack, so I'd prefer not to do that if we can avoid it. Once plugins are direct children of the root window (i.e. not a child of the current mess of child widgets and views), it should be straightforward to make them account for the current zoom factor.
Assignee | ||
Comment 15•18 years ago
|
||
Should I request sr from someone else?
Comment on attachment 255074 [details] [diff] [review] Possible fix oops
Attachment #255074 -
Flags: superreview+
Assignee | ||
Updated•18 years ago
|
Assignee: general → sharparrow1
Assignee | ||
Comment 17•18 years ago
|
||
Assignee | ||
Updated•18 years ago
|
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment 18•18 years ago
|
||
Comment on attachment 255195 [details] [diff] [review] Checked-in version >+ // If it's negative, we pretend it's not set. I know you're just moving this comment, but it's wrong -- or at least makes no sense. (The logical resolution can't be "not set".) I'd say something like: If it's negative, we use the larger of 96dpi and the operating system's value (since staying at or above 96dpi is important for legibility of Web pages with point-sized fonts). Feel free to fix the comment to something along those lines without further review, if you want. Or maybe I will sometime...
Reporter | ||
Comment 19•18 years ago
|
||
- vm->SetWindowDimensions(width, height); - ClearStyleDataAndReflow(); + mShell->FrameNeedsReflow(mShell->GetRootFrame(), nsIPresShell::eStyleChange); + mShell->ResizeReflow(width ,height); With this change reflow works 3x times faster
Assignee | ||
Comment 20•18 years ago
|
||
(In reply to comment #19) > - vm->SetWindowDimensions(width, height); > - ClearStyleDataAndReflow(); > > + mShell->FrameNeedsReflow(mShell->GetRootFrame(), nsIPresShell::eStyleChange); > + mShell->ResizeReflow(width ,height); > > With this change reflow works 3x times faster I imagine that ClearStyleDataAndReflow is a lot more expensive than just a style change reflow. However, system fonts don't work with that, because their size in app units changes. There might be a less expensive way to deal with it than recalculating all the style data, but I can't think of one off the top of my head.
Updated•18 years ago
|
Attachment #254392 -
Flags: review?(pavlov)
Updated•16 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•