394057 - need to reflow on default-font change

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Steps to reproduce:
 - View testcase using Trunk using default font. (default is "serif" on Linux)
 - Change to a wider font, in "Content" area of the Preferences dialog.
    (e.g. "FreeMono" or any monospace font should work)

Observed results:
 - Text gets wider due to wider font (expected), but table doesn't resize to accommodate wider text. (unexpected)
 
Note: if you refresh the page after changing the font, the table size is calculated correctly.


Basically, it looks like font changes aren't triggering reflows for the containing cell, and they should.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 2

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: automated testcase attempt (doesn't reproduce bug)

I tried to make an automated test case with a dynamically updated font, but I can't reproduce the bug with this testcase -- the table resizes during the font change (which is correct behavior, not buggy).  I'm posting it here anyway, in case anyone can figure out how to change it so it reproduces the bug.

Based on this testcase, it seems that changing Firefox's default font via the preferences dialog will trigger different (not enough) reflows, as compared to dynamically changing a page body's font attribute.

Comment 3

[Eli Friedman]

Probably my fault... the issue is that the default font isn't reflected into the style system, so changing it is reduced to a no-op.  I guess we need to force a style change reflow in this case, rather than using the ClearStyleDataAndReflow, which doesn't necessarily reflow.

Comment 5

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

I think this should be P3 rather than P5; it's a pretty noticeable recent regression with a trivial fix.

Comment 7

[Breght Boschker]

Attachment: Result of changing the font from Times New Roman to Verdana on Windows XP

Comment 8

[Breght Boschker]

Attachment: Result of changing the font from Times New Roman to Verdana on Windows XP after *first* refresh

Changing the default font corrupts the screen. Refreshing once doesn't seem to help (see att.). Refreshing a second time actually reflowed the page to the desired result.

Comment 9

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

I'll take this.

Comment 10

[Bill Gianopoulos [:WG9s]]

This is really not related to tables at all.  The same thing happens outside of tables.

Look at my homepage at http://www.wg9s.com/  Then change from serif to sans-serif or vice-versa for default font.  When going form serif to sans-serif large gaps appear in the text.  When changing form sans-serif-to serif, the text overlaps itself.

Comment 11

[Bill Gianopoulos [:WG9s]]

I think the issue here is that changing  the font face from preferences needs to trigger a reflow just like changing the font size does.

Comment 12

[Bill Gianopoulos [:WG9s]]

(In reply to comment #10)
> This is really not related to tables at all.  The same thing happens outside of
> tables.
> 
> Look at my homepage at http://www.wg9s.com/  Then change from serif to
> sans-serif or vice-versa for default font.  When going form serif to sans-serif
> large gaps appear in the text.  When changing form sans-serif-to serif, the
> text overlaps itself.
> 
I decided using my site a s a test was dumb because the reason I ran into this was I was trying to select a font I liked to use for my site's body text, so having it as a testcase in this bug is dumb.

Real text only, no tables, testcase to follow.

Comment 13

[Bill Gianopoulos [:WG9s]]

Attachment: text only testcase without tables

This plain text page demonstrates the issue as well.

1.  Set your default font to a sans-serif font.
2.  open the attachment.
3.  change you default font to a serif font (do not change the font size
    because that will trigger a reflow).
4.  Notice that the text now runs off the righthand side of the page and there
    is no horizontal scrollbar.

Comment 14

[Bill Gianopoulos [:WG9s]]

Attachment: Improved txt only testcase w/o tables

Same steps, better text for seeing issue.

Comment 15

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

I know what the problem is and how to fix it; no more information is needed.

Comment 16

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

Attachment: patch

This fixes the bug.  I still need to review the other callers of RebuildAllStyleData a little more carefully than I have so far (it looks like a bunch of the others needed fixing as well -- after all, it used to be called ClearStyleDataAndReflow).

Also need to write an automated test for this.

Comment 17

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

Attachment: patch

Here's the patch, with test added (and the test fails with the key line in PreferenceChanged commented out).

This patch changes the following:
  * It moves the handling of "browser.display.auto_quality_min_font_size" (added in bug 387969) from nsPresContext::PreferenceChanged and nsPresContext::Init to nsPresContext::GetUserPreferences (along with all the other preferences), where we'll re-fetch it on PreferenceChanged.
  * It fixes GetUserPreferences so a dynamic change of the animation mode pref to an invalid value produces the same result as it would initially.
  * It changes nsFrameManager::ComputeStyleChangeFor to append a change for the hint passed in as an argument, logic moved from one of the two callers (nsCSSFrameConstructor::RestyleElement) so that I don't have to add the same logic to nsCSSFrameConstructor::RebuildAllStyleData.  It then simplifies the calling code there as well (based on the fact that any change other than the one passed in would already end up in the change list).
  * It adds an |nsChangeHint aExtraHint| parameter to both RebuildAllStyleData methods (nsPresContext and nsCSSFrameConstructor).  I pass NS_STYLE_HINT_REFLOW in a whole bunch of cases where I think the thing we're restyling for isn't reflected in style data, although I'm actually not 100% sure about some of them, and I think some of them it is, but it's probably slightly less computation (a simpler change list?) to know that in advance.  I also did this for cases where ClearStyleDataAndReflow was used with the intent of forcing a reflow (the users in dom/)
  * It adds an mPrefChangePendingNeedsReflow to nsPresContext, and sets that so that we pass NS_STYLE_HINT_REFLOW in the font pref change cases for this bug.

Comment 18

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

Comment on attachment 301617 [details] [diff] [review]
patch

+  nsDependentCString prefName(aPrefName);
+  if (prefName == NS_LITERAL_CSTRING("layout.css.dpi")) {

EqualsLiteral?

Comment 19

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

Checked in to trunk, 2008-02-08 11:52/53 -0800.