128405 - Text Zoom leaves misaligned overlapping text

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect, P4)

Description

[u32858]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.8) Gecko/20020205
BuildID:    2002020511

Go to the very bottom of http://www.mplayerhq.hu/DOCS/video.html

Some text overlaps after zooming in.

Reproducible: Always
Steps to Reproduce:
1.Load that page. Find the "Only thing you need to do - have" text at the bottom.
2.Zoom in Ctrl+  scrol down again, I dont know the zoom on your machine, but on
mine 120% left the text as "Only thing you need toHAVE" where HAVE is bold.. but
i put capitals to emphasise
3.

Actual Results:  overlapping text. only seems to occure on 120%

Expected Results:  some look "nice"

Mozilla is really rather good :)

Comment 1

[Mats Palmgren (inactive)]

WFM, 2002-03-02-08 on Windows 98 SE.

Comment 2

[Chris Petersen]

WFM on OS X March 14th build (2002-03-14-08).  Reporter, please try with a newer
nightly build.

Comment 3

[u32858]

Hi
Still present on 2002 03 11 15 thats the build i have

Comment 4

[Christopher Hoess (gone)]

WFM on a current Linux CVS build.

Comment 5

[Koike Kazuhiko]

Attachment: testcase

Comment 6

[Koike Kazuhiko]

Reproduced with 2002042007/Linux.

attachment 80312 [details] doesn't have the end tag for '<font class="text">'.
This bug isn't reproduced if you add the end tag.

Comment 7

[Antony Mawer]

Using the attached test case, I'm seeing this on 2002-06-11-08 on Windows XP.
When you zoom in on the text, the text becomes overlapped. The first few zooms
are okay, but the zoom to 200% I first see the overlapping. Subsequent zooming
in from there exhibits the same symptoms of overlapped text.

If I then zoom out (just once, from wherever I'm at) then the display appears
correctly without any overlapped text. Zoom again and once more the overlapped
text appears.

Comment 8

[Felix Miata]

Attachment: screenshot of testcase on OS/2 1.4 branch 20030619

Comment 9

[Felix Miata]

Also happens on W32 1.4rc2

Comment 10

[Hideo Saito]

Attachment: patch

This patch checks change of the size of a font.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Is this still an issue?  This worksforme with a current Linux build.....

Comment 12

[Felix Miata]

Still happens using testcase in 1.5 on W98 at 200% zoom.

Comment 13

[Felix Miata]

Still happens using testcase in OS/2 trunk at 200% zoom.

Comment 14

[djpohly]

Attachment: Testcase and variations

This bug is still around in Firefox 0.9rc (Gecko/20040608) with WinXP.  It
seems to be a problem with handling nested tags while resizing.  For the
testcase, I took the original paragraph giving me problems and changed all the
tags to <SPAN> tags to ensure the problem wasn't formatting or the anchor tag,
then changed them to dummy tags <FOO><BAR><BAZ><QUUX> which should be basically
skipped by the browser anyway, but the layout still barfed when resizing.
   This is a rather frustrating bug as I like to use the text resize feature to
save on eye strain when reading long passages of text.

Comment 15

[Hideo Saito]

Attachment: simple testcase

Comment 16

[Hideo Saito]

Attachment: screenshot for simple testcase

Comment 17

[Hideo Saito]

Attachment: patch

Comment 18

[Hideo Saito]

When the reflow reason is a resize reflow, the |measureText| boolean is set to
false although the font-size is changed. I think that the font-size should be
checked for the Text Zoom.

Comment 19

[rbs]

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

I don't think this is the right fix.
1/ Why is the textzoom triggering a resize reflow? It should trigger a
stylechange reflow, not a resize reflow. If you find out the root of this, you
will get the right fix.
2/ To add a new member data (mFontSize) to a common frame class such as
nsTextFrame, one really-really has to need it.

Comment 20

[Hideo Saito]

Attachment: patch

Comment 21

[Hideo Saito]

rbs, please refer to the following code.

nsLineLayout::ReflowFrame(nsIFrame* aFrame,

  const nsHTMLReflowState* rs = psd->mReflowState;
  nsReflowReason reason = eReflowReason_Resize;
  if (NS_FRAME_FIRST_REFLOW & aFrame->GetStateBits()) {
    reason = eReflowReason_Initial;
  }
  else if (rs->reason == eReflowReason_Incremental) { // XXX

  }
  else if (rs->reason == eReflowReason_StyleChange) {
    reason = eReflowReason_StyleChange;
  }
  else if (rs->reason == eReflowReason_Dirty) {
    if (aFrame->GetStateBits() & NS_FRAME_IS_DIRTY)
      reason = eReflowReason_Dirty;
  }

I think that the cause of this problem is in the reflow of the inline frame.

The following table indicates the flow of the reflow reason after the contents
are wrapped by the Text Zoom.

  first:
    the reflow reason of the parent frame: StyleChanged
    actual reason: Inital (because NS_FRAME_FIRST_REFLOW bit is 1)

  second:
    the reflow reason of the parent-parent frame: StyleChanged
    the reflow reason of the parent frame: Inital
    actual reason: Resize (default setting, NS_FRAME_FIRST_REFLOW bit is 0)

Comment 22

[Hideo Saito]

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

It is necessary to check other codes which are calling |CreateNextInFlow|.

Comment 23

[Hideo Saito]

Attachment: simple testcase #2

Comment 24

[Hideo Saito]

Attachment: patch

Comment 25

[Hideo Saito]

Attachment: simple patch

This patch is simpler than attachment 160840 [details] [diff] [review], although both the patches have
the same effect.

Comment 26

[rbs]

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

@@ -326,16 +327,23 @@ nsHTMLContainerFrame::CreateNextInFlow(n
     }
+      nextInFlow->RemoveStateBits(NS_FRAME_FIRST_REFLOW);

This patch is incorrect. One shouldn't clear the first_reflow bit on a _new_
frame.

The second ("simple") patch is actually more promising. The bad reflow reason
is the root cause of this bug. You are on the right track. But there could be
an even simpler fix than what you did. I have to go now. I will get back to it
later.

Comment 27

[rbs]

Attachment: patch

To understand what your patch was attempting to do, I basically had to trace
what is happening and see the effect of your patch in the debugger (this is the
problem with this dreaded layout because it is quite time consuming...). In the
process, this patch came naturally. Thanks for the lead way.

What happens is this. We start with one line, say:

LINE1: <u>aaaaaaaaaa bbbbbbbbbbb<b>cccccccccccc</b></u>

Then, after text zooming, the first line is split and a second line is created:


LINE1: <u>aaaaaaaaaa
LINE2: bbbbbbbbbbb<b>cccccccccccc</b></u>
       <<<< we are inside an overflow >>>

Content is thus pushed into a _new_ overflow inline frame within the second
line. Then,

This new overflow inline frame is reflow as _initial_ (as it should). 

The "bbbbbbbbbbb" fragment is now a _new_ continuing text frame which is
reflowed as _initial_ (as it should).

The "<b>cccccccccccc</b>" fragment is kept in its _old_ inline frame, and this
is getting reflowed as _resize_ (in error...). The code in nsLineLayout that
fixes up the reflow reason didn't anticipate this situation.

Thus the patch makes sure that an _old_ frame inside a _new_ frame doesn't get
reflowed with the wrong reason.

Comment 28

[Boris Zbarsky [:bzbarsky]]

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

sr=bzbarsky, but asking roc for the r=.  I don't know this code as well as I
ought to, and he's been dealing with it some (for columns).

Comment 29

[rbs]

Checked in.

Comment 30

[rbs]

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

Comment 31

[rbs]

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

Comment 32

[rbs]

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

Comment 33

[rbs]

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