369698 - After changing layout.css.dpi value, application and layout text looks very bad.

Status: RESOLVED FIXED

(Core Graveyard :: GFX, defect)

Description

[Oleg Romashin (:romaxa)]

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.

Comment 1

[Oleg Romashin (:romaxa)]

Attachment: First screenshot with about:config page and broken text

Comment 2

[Oleg Romashin (:romaxa)]

Attachment: Second screenshot with broken mozillZine layout

Comment 3

[Oleg Romashin (:romaxa)]

Attachment: Possible fix for this bug

Comment 4

[Oleg Romashin (:romaxa)]

Attachment: First screenshot with my patch

Comment 5

[Oleg Romashin (:romaxa)]

Attachment: Second screenshot with applied patch mozillaZine layout

Comment 6

[Eli Friedman]

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.

Comment 7

[Oleg Romashin (:romaxa)]

> 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...

Comment 8

[Oleg Romashin (:romaxa)]

with way N2 we will get exactly the same zooming stuff, like Opera, IE ;) BUG 

https://bugzilla.mozilla.org/show_bug.cgi?id=4821

Comment 9

[Oleg Romashin (:romaxa)]

>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.... :(, 

Comment 10

[Eli Friedman]

(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.

Comment 11

[Eli Friedman]

Attachment: Possible fix

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.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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()".

Comment 14

[Eli Friedman]

(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.

Comment 15

[Eli Friedman]

Should I request sr from someone else?

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 255074 [details] [diff] [review]
Possible fix

oops

Comment 17

[Eli Friedman]

Attachment: Checked-in version

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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...

Comment 19

[Oleg Romashin (:romaxa)]

- vm->SetWindowDimensions(width, height);
- ClearStyleDataAndReflow();

+ mShell->FrameNeedsReflow(mShell->GetRootFrame(), nsIPresShell::eStyleChange);
+ mShell->ResizeReflow(width ,height);

With this change reflow works 3x times faster

Comment 20

[Eli Friedman]

(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.