103582 - Text zooming corrupts layout

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[Jeroen Besse]

If one goes to http://dmoz.org/Arts/ , then zooming in, and out again (or the
other way around), the layout isn't the same as from the start. Reload corrects
it again.

This bug has been seen on OS/2 (0.9.4, Build ID 2001100119) and on Linux
("Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.4+) Gecko/20011005" and
"Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.4) Gecko/20010913")

Comment 1

[Niels Aufbau]

Attachment: testcase

Comment 2

[Marc Attinasi]

Bummer (thanks for the testcase Niels).

Comment 3

[m.annand]

The background on this page -

http://www.bathspa.ac.uk/markhelp/

- is unhappy after changing the text size using CTRL and + etc. Resize and
something nasty happens to the page background graphic. Reloading the page
corrects things ... apologies if this is a different bug ... 

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.5) Gecko/20011011

Comment 4

[Niels Aufbau]

[e-mail]: bug 95263

Comment 5

[Vitaly Harisov]

Attachment: Another example of layout corrupting.

1. Go to http://www.rathedg.com/ (See attached TextZoom-100%.png)
2. Zoom Text Size to 120% (See attached TextZoom-120%.png)

Is this same bug or I must create new one?

OS: Windows 2000 SP4 en (Large Fonts, 125%)
Browser: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5) Gecko/20031007

Comment 6

[Christopher Whitt]

Adding top100 keyword.  Probably occurs on more top100 sites than just dmoz.org.

Re comment 5, I don't see any problem on rathedg.com

Comment 7

[Hideo Saito]

Attachment: patch

If the reflow reason is a stylechange reflow, I think that both the width
should be recalculated, because the calculated available width has a previous
width.

Comment 8

[Bernd]

Attachment: reflow log for the text size increase

Comment 9

[Bernd]

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

This patch is not correct.
1.) This patch tries to manipulate the table reflow when the block reflow is
wrong. During incr. reflow a auto width table relies on the mMaximumWidth
information from the block reflow to expand if necessary. If  text in auto
table wraps where it should not, one should look for this parameter. If you run
a reflow log you will see it as m=xxx. In the attached reflow log you will find
the line:

block 03530910 d=1068,758 me=852 m=720 
It is clear that this line is wrong how can the mMaximumWidth on a block
without a width be smaller then the desired width? (see
https://bugzilla.mozilla.org/show_bug.cgi?id=147558#c13 for a similiar case)
One can tweak of course the layout in a lot of places to get a correct result
for one testcase but this goes on the expense of regressions. In order to
prevent these regressions a proper analysis is needed first before tweaking the
code, and making this kind of changes requires running the regression tests.

2.) it uses the bogus mechanism with CalcAvailWidth which needs fixing rather
then extension, this will happen in bug 226637

Comment 10

[Hideo Saito]

Attachment: another testcase

Comment 11

[Hideo Saito]

Attachment: patch

For the stylechange reflow, ReflowChildren at the nsTableFrame::Reflow should
be called after table cells are reset. For the testcase, the posted patch
(attachment 165805 [details] [diff] [review]) to Bug 147558 needs because the testcase has a indent. I
tested this patch using the another testcase (attachment 165871 [details]).

Comment 12

[Bernd]

I don't see how this patch addresses the first issue in comment 9. It's rather
coding around the block problem. I will not review a patch that does not fix the
block issue or explains why the block is doing the correct thing.

Comment 13

[Hideo Saito]

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

The meaningful changes were only the following codes.

-	 SetNeedStrategyBalance(PR_TRUE); // force a balance and then a pass2
reflow 
+	 if (HasPctCol())
+	   SetNeedStrategyBalance(PR_FALSE); // did a balance and a pass2
reflow 
+	 else
+	   SetNeedStrategyBalance(PR_TRUE); // force a balance and then a pass2
reflow 

In nsTableFrame::Reflow, this function is called from ReflowChildren again for
the stylechange reflow. When it returns from the multiplexed call, both values
of desWidth and prefWidt are set to the previous values in
nsTableFrame::BalanceColumnWidths, because NeedStrategyBalance is set to true.

I indicate the flow as follows.

NS_METHOD nsTableFrame::Reflow(nsPresContext*	       aPresContext,
{
  ...
  switch (aReflowState.reason) {
    case eReflowReason_Initial:
    case eReflowReason_StyleChange: {
      ...
	    ReflowChildren(aPresContext, reflowState, !HaveReflowedColGroups(),
      ...
	SetNeedStrategyBalance(PR_TRUE); // force a balance and then a pass2 r
      ...
  }
  ...
      ReflowTable(aPresContext, aDesiredSize, aReflowState, availHeight,
nextRea
  ...
}

nsTableFrame::ReflowTable(nsPresContext*	  aPresContext,
{
  ...
    if (NeedStrategyBalance()) {
      BalanceColumnWidths(aPresContext, aReflowState);
  ...
}

void nsTableFrame::BalanceColumnWidths(nsPresContext*	       aPresContext,
{
  ...
  SetDesiredWidth(desWidth);
  SetPreferredWidth(prefWidth);
}

I think that NeedStrategyBalance should not be set to true again.

Comment 14

[Hideo Saito]

When the table cell nests and has percent width, this problem issues as follows.
When HasPctCol() is true, the prefWidth is set to previous value because
aReflowState has previous maxWidth. If HasPctCol() is false, prefWidth is not
set to previous value though aReflowState has previous maxWidth.

nsTableFrame::Reflow(..);
  |
  +-->ReflowChildren(...);
  |     |
  |     +-->ReflowChildren(...);
  |     |
  |     +-->mTableLayoutStrategy->Initialize(aReflowState);
  |     |   (HasPctCol() is true)
  |     +-->SetNeedStrategyBalance(PR_TRUE);
  |     |
  |     +-->ReflowTable(...);
  |           |
  |           +-->BalanceColumnWidths(..);
  |           |     |
  |                 +-->mTableLayoutStrategy->BalanceColumnWidths(aReflowState);
  |                 |   (aReflowState has previaus maxWidth)
  |                 +-->SetPreferredWidth(prefWidth);
  |                     (prefWidth is set to previous value)
  |           
  +-->mTableLayoutStrategy->Initialize(aReflowState);
  |   (HasPctCol() is false)
  +-->SetNeedStrategyBalance(PR_TRUE);
  |
  +-->ReflowTable(...);
  |     |
        +-->BalanceColumnWidths(..);
        |

Comment 15

[Bernd]

Hideo, did you read my comment?

What on earth has the table reflow to do with

block 03530910 d=1068,758 me=852 m=720  ???

This is a block error. Period. m should never be smaller than me in this case. 
Please have first a carefull read of
http://www.mozilla.org/newlayout/doc/block-and-line.html especially of the
mMaximumWidth and maxElementSize parameters.
Obviously one can write code that does not exhibit this code path. But it will
be the wrong fix.

Comment 16

[Hideo Saito]

(In reply to comment #15)
> Hideo, did you read my comment?
> http://www.mozilla.org/newlayout/doc/block-and-line.html especially of the

Yes, I must read it.
Bernd, I have a question for a following flow.

nsTableRowFrame::ReflowChildren
 |
 +-->nsTableFrame::CellChangedWidth
 |   (already NeedStrategyInit is set to true and colSpan is larger than one,
 |    then only return without changing the cell width)
 +-->BasicTableLayoutStrategy::Initialize
 |   (the cell width is calculated using new font-size, and HasPctCol is set to
true)
 +-->SetNeedStrategyBalance(PR_TRUE)
 |
 +-->ReflowTable
      |
      +-->nsTableFrame::BalanceColumnWidths
           |
           +-->BasicTableLayoutStrategy::BalanceColumnWidths(aReflowState)
                |
                +-->nsTableFrame::CalcBorderBoxWidth
                     |
                     +-->width = aState.mComputedWidth
                         (this HTMLReflowState has current width)

When the font size is changed by textzoom, BalanceColumnWidths uses current
width of HTMLReflowState. Then the cell width is exchanged right width to old
width by BalanceColumnWidths. I think that the current width of HTMLReflowState
is right and should not use it. Is this flow right?

Comment 17

[Hideo Saito]

Attachment: patch for another testcase

With the following flow, ReflowChild is reflowed with the previous available
width, because aTableFrame has previous colWidth. With the patch, if the
condition of the table is forced a balance, availColWidth and availCellWidth
are not computed.

nsTableRowFrame::ReflowChildren
 +-->CalcAvailWidth(aTableFrame, ..., availColWidth, ...);
 +-->nsSize kidAvailSize(availColWidth, ...);
 +-->nsTableCellReflowState kidReflowState(..., kidAvailSize, ...);
 +-->ReflowChild(..., kidReflowState, ...);

Comment 18

[Bernd]

Hideo: I don't know what I need else to do to explain you that one needs to fix
the block error first. I give up. You need from
http://www.mozilla.org/newlayout/doc/ to read the reflow , the block and line
cheat and the talks given by karnaze and waterson. Once you understand them you
might get what I am asking you. You can of course continue to explore with
single step debugging the internals of the table reflow but without a
understanding how reflow works all this is moot.

Comment 19

[Hideo Saito]

Attachment: test patch

I marked the patch with a comment as follows and explain the patch.

nsTableFrame.cpp:#if 1 // HSAITO patch#00
nsTableFrame.cpp:#if 1 // HSAITO patch#01
For the stylechange reflow, if the table has an auto layout and a style percent
width cols, the available width of the current reflow state can not be used to
calculate cols width. However, for the first table, the available width can be
used since the width is not unchanged by the stylechange.
nsTableFrame.cpp:#if 1 // HSAITO patch#02
For the resize reflow, if the table has an auto layout and a style percent
width cols, before the table balances, all of the cols info should be updated.
nsTableFrame.cpp:#if 1 // HSAITO patch#03
nsTableFrame.cpp:#if 1 // HSAITO patch#04
nsTableFrame.cpp:#if 1 // HSAITO patch#05
nsTableFrame.cpp:#if 1 // HSAITO patch#06
The preferred width is calculated using the cols info, if the table has a
percent width cols.
nsTableFrame.cpp:#if 1 // HSAITO patch#07
For the stylechange refow, set |NS_UNCONSTRAINEDSIZE| to |availWidth|, since
the available width of the current reflow state can not be used for percent
width cols.
BasicTableLayoutStrategy.cpp:#if 1 // HSAITO patch#10
BasicTableLayoutStrategy.cpp:#if 1 // HSAITO patch#11
For the stylechange refow, set |NS_UNCONSTRAINEDSIZE| to |boxWidth| and
available width.
BasicTableLayoutStrategy.cpp:#if 1 // HSAITO patch#12
BasicTableLayoutStrategy.cpp:#if 1 // HSAITO patch#13
BasicTableLayoutStrategy.cpp:#if 1 // HSAITO patch#15
The preferred width is calculated using the cols info, if the table has a
percent width cols.
BasicTableLayoutStrategy.cpp:#if 1 // HSAITO patch#14
The cols info table of |FINAL| has the non percent width.
BasicTableLayoutStrategy.cpp:#if 1 // HSAITO patch#16
In spite of the |availWidth|, |basis| is added the table border, padding and
cell spacing.
nsTableCellFrame.cpp:#if 1 // HSAITO patch#20
nsTableOuterFrame.cpp:#if 1 // HSAITO patch#30
For the resize reflow that is the next refow of the stylechange reflow, force
to reflow the children.

Comment 20

[Hideo Saito]

Attachment: testcase for the test patch

Comment 21

[Hideo Saito]

Attachment: testcase for the test patch

Comment 22

[Hideo Saito]

Attachment: screen shot for attachment 167745

Comment 23

[Bernd]

*** Bug 281369 has been marked as a duplicate of this bug. ***

Comment 24

[Hideo Saito]

Attachment: patch

Comment 25

[Hideo Saito]

Attachment: testcase for patch

Comment 26

[Hideo Saito]

Attachment: patch part1

Comment 27

[Hideo Saito]

Attachment: patch part2

Comment 28

[Hideo Saito]

Attachment: patch part3

Comment 29

[Hideo Saito]

I updated and divided patch into three and tested layout regression tests, part1
and part3 have influence on the result.

part1 - even if availWidth has unconstrained size, the basis should be added
borderpadding and cell spacing.
part2 - HasConstrainedAvailableWidth returns false if the reflow reason is style
change and parent's availableWidth has unconstrained size. If false is returned,
it does not limit the table width. Instead, the resize reflow is forced by the
setting unconstrained size to mPriorAvailWidth.
part3 - this is rounding problem. the sum of the rounded percent col width
exceeds the basis. 

Comment 30

[Bernd]

See comment 9, comment 12, comment 15 and comment 18.
>Hideo: I don't know what I need else to do to explain you that one needs to fix
the block error first. I give up

Did the block error vanish in the mean time?

Comment 31

[Hideo Saito]

> Did the block error vanish in the mean time?

bernd, the first attached testcase was solved already, my approach is to fix for
style change reflow as to percentage col widths. In the style change reflow, the
calculation for the percentage col width is wrong and the basis of the table
width is limited using the previous available width.

Comment 32

[Bernd]

> bernd, the first attached testcase was solved already
So what does the patch then try to fix other than bug 195770? See my comment
https://bugzilla.mozilla.org/show_bug.cgi?id=195770#c16 there. 

Comment 33

[baffclan]

cannot reproduce with Firefox/2006081704-trunk/WinXP, SeaMonkey/2006081809-trunk/WinXP.

WFM?

Comment 34

[Jeroen Besse]

Cannot reproduce with Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.8.0.6) Gecko/20060803 Firefox/1.5.0.6 either, so this seems to be fixed.

Comment 35

[Magnus Melin [:mkmelin]]

Nope, I most definitely see this problem using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060819 Minefield/3.0a1 ID:2006081904 [cairo]

Tested using testcase in attachment 187578 [details]: zoom out, zoom in, the layout is not the same.

Comment 36

[Hideo Saito]

On attachment 187578 [details], it seems that this problem is already fixed by the fix for Bug 300030 as to zooming the text, in addition, by the fix for Bug 363150 as to too wide table width.

Comment 37

[Hideo Saito]

As to the original testcase (attachment 52479 [details]), we got the fix since the fix for Bug 147558, although the code was changed by the fix for Bug 300030.

Comment 38

[Magnus Melin [:mkmelin]]

V. on Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a6pre) Gecko/20070611 Minefield/3.0a6pre