223737 - {inc}display several extra newlines for <img>text<br clear="all">

Status: RESOLVED FIXED

(Core :: Layout: Floats, defect)

Description

[Hideo Saito]

Several extra newlines are displaied, if an attribute of the position of an IMG
is specified and the text includes tags of <br> and <br clear="all">.

This problem may be solved with the patch to append.

Comment 1

[Hideo Saito]

Attachment: testcase

Comment 2

[Hideo Saito]

Attachment: patch

Comment 3

[Hideo Saito]

Attachment: screenshot

Comment 4

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

.

Comment 5

[Martijn Wargers (dead)]

I've tried the patch, seems to work fine for the testcase. 
Hideo, why don't you ask review for the patch?

Comment 6

[Hideo Saito]

Attachment: patch

The testcase shows following two problems.
1. several extra newlines are displaied, if the window is narrow
2. 'bbbbbbbbbb' does not go back to the right side, if the window is wide

In order to mark the next line dirty, the changes make delay to clear the dirty
of the current line including BRFrame.

Comment 7

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

+      if (NS_FRAME_IS_NOT_COMPLETE(frameReflowStatus) || aLine->IsDirty()) {

Won't aLine always be dirty, since we're currently reflowing it? So you might as
well just remove this 'if' condition. That seems OK to me.

Comment 8

[Hideo Saito]

Attachment: patch

Comment 9

[Hideo Saito]

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

I think that this patch is low-risk and floated lines split by <br> are done
the layout correctly.

Comment 10

[Hideo Saito]

roc, could you check in to the trunk?

Comment 11

[Hideo Saito]

masayuki, this patch was approved for 1.8b3. Could you check in to the trunk?
and please check Bug 291757.

Comment 12

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

checked in. Thanks Hideo!!!

Comment 13

[Boris Zbarsky [:bzbarsky]]

I backed out this fix (with a=roc) because it caused severe performance issues
when typing at the beginning of a textarea with lots of text -- due to this
patch we reflowed every single line on every single keystroke...

Comment 14

[Hideo Saito]

Attachment: patch for skip to reflow lines of a textarea

I think that textarea frame does not have block frame. Should I give up this
approach?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 190116 [details] [diff] [review]
patch for skip to reflow lines of a textarea

> I think that textarea frame does not have block frame.

It sure does.  In any case, textarea is not the only case this patch made way
too slow; just the most obvious one.

Also, you may want to request reviews from roc; he knows this code much better
than I do...

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Roc:

Do you have better idea?
Mozilla Japan hopes that this bug should be fixed before Firefox 1.5.
Because bug 291757 is reproduced on "Amazon.co.jp" that is major site in Japan.

Comment 17

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

I will work on this.

Comment 18

[Hideo Saito]

Attachment: patch for skip to reflow lines of a text control frame

In order to skip to reflow lines of a text control frame, a bit of
NS_FRAME_INDEPENDENT_SELECTION is checked.

Comment 19

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

I don't think that's right. There is no logical connection between frames having
independent selection and incremental reflow of lines with BRs.

There must be a fix that fixes the actual bug here and doesn't force us to
reflow every line with a BR before it.

Comment 20

[Hideo Saito]

(In reply to comment #19)
> I don't think that's right. There is no logical connection between frames having
> independent selection and incremental reflow of lines with BRs.

A mState bit of NS_FRAME_INDEPENDENT_SELECTIONA is set for text control frame at
nsTextControlFrame::CreateAnonymousContent and SELECTION_ON is also set for
selection controller. I think that these conditions are distinguishable from
other selected frames as follows changes. Is this idea wrong?

+      PRBool needsDirty = NS_FRAME_IS_NOT_COMPLETE(frameReflowStatus);
+      if (!needsDirty) {
+        needsDirty = PR_TRUE;
+        if (aFrame->GetStateBits() & NS_FRAME_INDEPENDENT_SELECTION) {
+          nsCOMPtr<nsISelectionController> selCon;
+          rv = GetSelectionController(aState.mPresContext, getter_AddRefs(selCon));
+          //get the selection controller
+          if (NS_SUCCEEDED(rv) && selCon) 
+          {
+            PRInt16 displayresult;
+            selCon->GetDisplaySelection(&displayresult);
+            if (displayresult == nsISelectionController::SELECTION_ON)
+              needsDirty = PR_FALSE;
+          }
+        }
+      }
+      if (needsDirty) {
+        nsLineList_iterator next = aLine.next();
+        if (next != end_lines() && !next->IsBlock()) {
+          next->MarkDirty();
+        }

Comment 21

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

There is nothing special about text frames that should require them to be
reflowed differently from normal.

Comment 22

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

The newlines in the testcase are correct, by the way. align="left" floats the
image to the left, and <br clear="all"> forces that line break to happen below
the float.
http://www.w3.org/TR/1998/REC-html40-19980424/present/graphics.html#floating

The only bug is that the line bbbbbbbbb doesn't correctly move back up when the
window gets wider.

Comment 23

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

Attachment: simplified testcase

This simplifies the testcase a bit more.

Comment 24

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

Attachment: fix

The basic problem is that with a narrow width, resize reflow decides it is not
necessary to reflow line bbbbbbb because it is not "impacted by a float". It's
not impacted by a float because we've moved it below the float. Really we
should say it is impacted by float because we tried to reflow it at least once
next to a float. This patch does that.

Comment 25

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

The patch does not fix bug 291757. That bug shows another issue which I will fix
there.

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: test patch

This patch fixes this bug and bug 291757.
I think that we are not marking to dirty the next line after clearance when it
is necessary.
But I don't know when we should marking to dirty it.

Comment 27

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

Comment on attachment 190374 [details] [diff] [review]
fix

r+sr=dbaron

Comment 28

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

Comment on attachment 190374 [details] [diff] [review]
fix

Fixes one problem with amazon.co.jp

Comment 29

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

Comment on attachment 190429 [details] [diff] [review]
test patch

This is basically the same as Hideo's original patch, which caused some
performance problems.

Comment 30

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

Comment on attachment 190374 [details] [diff] [review]
fix

BTW this patch is very low risk because it just does a bit of extra dirtying,
so it should only be able to cause performance problems if any.

Comment 31

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

checked in