32191 - [FIXED IN 0.9.2] incorrect line break before and after img tag

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[battery841]

From Bugzilla Helper:
User-Agent: Mozilla/4.72 [en] (X11; U; Linux 2.2.12-20 i586; Nav)
BuildID:    200031609

In www.mirabilis.com, I have found that next to the 'ICQ Search' field, there
are a few graphics which don't appear to line up correctly with the rest of the
column.  The ICQ Search graphic and the 'Go' graphic doesn't look like they line
up the way they're supposed to.

Reproducible: Always
Steps to Reproduce:
1. Load www.mirabilis.com
2. Look at first page of the site by the search ICQ field.

Actual Results:  The graphics were't aligned correctly (refer to description).

Expected Results:  The graphics should be lined up, with the orange horizontal
lines touching the horizontal orange lines with the correct aligned graphics.

Comment 1

[Richard Zach]

There is some weirdness, but most of it is the same as in NS 4.7.  Probably they
just measured the sice of the input widgets on Win and Mac and they're too big
on Linux or something.

The only thing that is different than NS 4.7 is the "Go" image.  It should line
up with the top orange line.  This is cased by the cell to the right having a
width=100% attribute, which makes the cell containing the image wrap before the
image (even though there's a   there).  Attaching a testcase.

This is the same site as icq.com, which is top100.

Comment 2

[Richard Zach]

Attachment: testcase

Comment 3

[karnaze (gone)]

Buster, the line in the test case is too tall, but since the url doesn't look as 
bad, this is probably a future.

This bug has been marked "future" because we have determined that it is not 
critical for netscape 6.0. If you feel this is an error, or if it blocks your 
work in some way -- please attach your concern to the bug for reconsideration.

Comment 4

[Peter "jag" Annema]

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

Comment 5

[Peter "jag" Annema]

Changing the URL to one where this problem is more visible.
Old url: http://www.mirabilis.com/ (did they change their html btw?)

What happens is that the line is broken before and after an image, where 
Netscape 4.7 and IE5 don't do that.

linux, build ID 2000071321
Windows 95, build ID 2000071320

Marking this All All, suspecting XP.

Working on another testcase.

Please reconsider the target milestone.

Comment 6

[Peter "jag" Annema]

Attachment: [testcase] Showing faulty line-break before and after <img>

Comment 7

[Peter "jag" Annema]

Adding testcase keyword.

Comment 8

[Peter "jag" Annema]

Updating summary.

http://www.w3.org/TR/html4/struct/global.html#h-7.5.3 ; Formatting:

"By default, block-level elements are formatted differently than inline 
elements. Generally, block-level elements begin on new lines, inline elements do 
not. For information about white space, line breaks, and block formatting, 
please consult the section on text."

This points at chapter 9, which contains the following section:

http://www.w3.org/TR/html4/struct/text.html#h-9.3.5:
"In Western scripts, for example, text should only be wrapped at white space."

In the testcase there's no whitespace between the text and the img tag, so 
wrapping should not occur.

Comment 9

[Peter "jag" Annema]

Assigning to waterson per his request.

Comment 10

[Chris Waterson]

Attachment: part of the fix

Comment 11

[Chris Waterson]

The above patch is may be part of the fix; however, it illustrates another
problem. If we let non-text frames form a "continuation" of a text run, then we
need to be sure that we account for their width when measuring text.

Fixing this may require tighter integration of the current implementation of
nsLineLayout::FindNextText() and the text measurement code. Specifically, if a
frame CanContinueTextRun() AND is a leaf frame AND is not a text frame, then we
must account for its width in the line- and word-breaking code in
nsTextFrame::MeasureText().

buster: this may be related to 14280.

Comment 12

[Hixie (not reading bugmail)]

David, this could be of interest to you since you've done some line layout
related stuff.

Comment 13

[Peter "jag" Annema]

I think 37656 is a dupe of this, can someone more in the know check it?

Comment 14

[buster]

waterson: you and I should talk.  I think this is a dup of 14280.  The fix for 
that bug is checked in already.  Could somebody with a recent build test?  I 
don't have recent code here at home.

Comment 15

[Hixie (not reading bugmail)]

buster: not fixed in Windows 2000 commercial build 2000091212. :-(

Comment 16

[buster]

Peter:  I don't interpret the portion of the HTML spec you quote to involve 
images or other inline content besides text.  Wrapping happens at word 
boundaries (considering puctuation to be part of the word for purposes of line 
breaking.)  Whitespace is a convenient indication of a word boundary. I also 
interpret inline elements such as images as being distinct from the word they 
are adjacent to.

This is easily controllable in HTML by using <NOBR>, which would give the 
desired effect, as would the CSS property white-space: nowrap

This is probably a dup of bug 37656, not 14280 which had to do with properly 
accounting for leading &nbsp;

Comment 17

[Braden]

Steve, there is no NOBR in HTML. And this issue should not be dependent on a
particular style sheet language. Thus the only safe (and rational) thing to do
in this case is *not* break. If a content author *does* want an optional break,
s/he can use a zero-width space.

Comment 18

[Braden]

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

Comment 19

[Hixie (not reading bugmail)]

Reassigning QA Contact for all open and unverified bugs previously under Lorca's
care to Gerardo as per phone conversation this morning.

Comment 20

[Jesse Ruderman]

Nominating for mozilla0.9 and nsbeta1: 6 dups, previously happened on a top100 
site (www.icq.com).

Comment 21

[Jesse Ruderman]

Forgot to mention: I just ran into this on 
http://www.cs.hmc.edu/~jruderma/mozilla/iframe-sec/.  (Click the link to 
http://www.mozillazine.org/ inside the frame on that page.)

Comment 22

[Andreas M. "Clarence" Schneider]

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

Comment 23

[Christian Mattar]

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

Comment 24

[Boris Zbarsky [:bzbarsky]]

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

Comment 25

[Jesse Ruderman]

Adding mostfreq keyword: 5 dups here, 4 additional dups on bug 37656.

Comment 26

[Andreas M. "Clarence" Schneider]

Dupe bug 30596 was targetted at 0.9.1.

Comment 27

[Keyser Sose]

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

Comment 28

[Andreas M. "Clarence" Schneider]

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

Comment 29

[Christian Mattar]

Since this is a mostfreq and top100 bug could we at least have a semi-official 
statement whether the current behaviour is right or wrong and whether Netscape 
is commited to fix this bug for 0.91 or 0.92?
Thanks!

Comment 30

[Andreas M. "Clarence" Schneider]

Note the opposite behavior in bug 84378.

Comment 31

[Andreas M. "Clarence" Schneider]

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

Comment 32

[Mats Palmgren (inactive)]

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

Comment 33

[Jacek Piskozub]

 This bug has 16 dups. Nominating for mozilla0.9.4 (as there as no mozilla0.9.5
.. mozilla0.9.9 keywords).

Comment 34

[Marc Attinasi]

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

Comment 35

[Christopher Hoess (gone)]

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

Comment 36

[Marc Attinasi]

Taking since I think I have a handle on this now...

Comment 37

[Marc Attinasi]

Addig topembed keyword because this is related to topembed bug bug 54565...

Comment 38

[jag (Peter Annema)]

Woohoo! Go attinasi!

Comment 39

[Marc Attinasi]

Attachment: PATCH: make ImageFrame 'continuable' as Waterson did, and also manage the MEW calculation accordingly

Comment 40

[Marc Attinasi]

I added a patch that takes care of a lot of the problems. It incorporates
waterson's chage, to allow the ImageFrame to continue a text run, and also looks
at whether or not the frames are 'continuable' when calculating the
MaxElementWidth. Basically, if a frame can continue a text run, then we want to
add its width to the MEW. This also takes care of the case where we jsut have
contiguous images (see bug 54565) but does not take care of some pesky &nbsp
problems, as reported in bug 93992.

I checked all of the testcase and URLs I could from this and realted / dup's
bugs - it fixes a lot of layout problems! I have not yet run the regression
tests but will soon.

Comment 41

[Bernd]

Marc could you alter the // now update the preImage flag 
comment as the following frames also CanContinueTextRun 
InlineFrame, FirstLetterFrame, TextFrame. This may give 
you a hint for some new testcases ;-).

Comment 42

[Marc Attinasi]

Thanks Bernd - will do.

Comment 43

[Marc Attinasi]

Testing the patch further, there seems to be some problem with the way a table
cell sizes around the block now in some cases (small width on the cell, but none
on table) so I'm digging into that.

The testcase I am using will be attached if anyone is interested - it tests
various arrangements of inlines in a textrun to see what causes breaks and how
the cells are formatted.

Comment 44

[Marc Attinasi]

Attachment: WIP: testcase for checking out the impact of the patch on various inlines-in-text-run in an overconstrained table scenarios

Comment 45

[Marc Attinasi]

Of course, it is complicated ;)

Making the imageFrame continue a text run is helpful, but it causes some
problems in the way that the textFrame computes the width of a text run.
Currently, the textFrame has an implicit assumption that any continuable frame
that it runs into while prowling for the rest of a text run will not participate
in the width of the text run. This is fine for the inline frame cases (<B>, <I>,
etc) but clearly not for images. I tried first an approach that fixes up the
computation of the line max element width in the VerticalAlignFrames, taking
into account the knowledge of how the text frame will have computed its own MEW,
but it is easy to make a mess of that by having deeply nested inlines with
images and text in them. 

Next I tried hacking the text run measurement in nsTextFrams such that the
widths of any non-text inlines that continue the text run would be added to the
width of the text fragment that they were encountered in. So far, this seems the
cleanest approach, but I have a lot to learn about text measurement code!

So, some progress, but not finished. I'm afraid to checkin something that does
not handle nested inlines because I do not want to break more than I fix. People
seem to put <B> tags pretty much everywhere and anywhere.

Comment 46

[Marc Attinasi]

I have a patch (slightly modified from the one attached already) that fixes the
rendering on all of the many many dups (URLs and testcases) except for one. It
is far from perfect, but it fixes so many relatively common sites that I think
it deserves review at least, and maybe even checking in. I'm running the table
and block regression tests now (again), and if all goes well, I'll attach the
patch for ridicule, er, review (it ain't the prettiest thing).

I did put a bunch more effort into getting the TextFrame to understand image
frames, and to include the impact of images in the measurement code it does
(currently it handles frames that CanContinueTextRuns, but only if they
contribute no size of their own). Unfortunately, the images are not reflowed at
the time the text frames are measuring their text so it didn't work. Maybe I'll
look at changing the way the text frames are reflowed so that they are udpated
when the images are reflowed, but that is scary to me. Anyway, the current
approach I took involves factoring in the impact of the image frames in the
maxElementWidth calculation, after the frames in the line have been reflowed

Comment 47

[Marc Attinasi]

Regression tests passed - some actually fixed!

/////////////////////////////////////////////////////////////////////////
Files that caused bbox mismatch:

file:///s|/mozilla/layout/html/tests/table/core/row_span.html
	SAME
file:///s|/mozilla/layout/html/tests/table/bugs/bug1220.html
	SAME
file:///s|/mozilla/layout/html/tests/table/bugs/bug12910-2.html
	FIXED!
file:///s|/mozilla/layout/html/tests/table/bugs/bug1296.html
	SAME
file:///s|/mozilla/layout/html/tests/table/bugs/bug1430.html
	SAME
file:///s|/mozilla/layout/html/tests/table/bugs/bug14323.html
	FIXED
file:///s|/mozilla/layout/html/tests/table/bugs/bug15544.html
	FIXED
file:///s|/mozilla/layout/html/tests/table/bugs/bug19599.html
	SAME
file:///s|/mozilla/layout/html/tests/table/bugs/bug29157.html
	SAME
file:///s|/mozilla/layout/html/tests/table/bugs/bug4427.html
	SAME
file:///s|/mozilla/layout/html/tests/table/bugs/bug5797.html            OK		
file:///s|/mozilla/layout/html/tests/table/bugs/bug9123-1.html
	FIXED!

Will attach patch (based on trunk source) for reviews while I apply it to the
branch and test there.

Comment 48

[Marc Attinasi]

Attachment: PATCH: Allows images to continue text run

Comment 49

[Marc Attinasi]

Looking for reviews / critiques of patch (id=46950). The patch is not perfect,
there are still cases that it does not handle, but it cures the most common
symptom (the many dups are all fixed except the page at
http://students.villanova.edu in bug 84594 - I'll have to reopen that bug). 

Because I am supremenly 'cautions' about general changes to layout, I used an
#ifdef to facilitate removing this code in triaging of potential regressions -
I'll open a bug to remove the #ifdef too.

Please check the patch over, I appreciate it.

Comment 50

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

Could nsImageFrame::CanContinueTextRun just check the 'display' type from the
style context?

Comment 51

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

Actually, never mind.  Frame construction should handle that, right?

Comment 52

[Bernd]

I like this patch, it will kill a lot of table bugs, I think it  will not cause
regressions in our layout. 
May be it is suboptimal, but  I would like to see this patch checked in even as
a band-aid patch
r=bernd

Comment 53

[Marc Attinasi]

David, I tested setting an IMG to display:block and we _do_ seem to handle it,
meaning we support images as blocks. I think I need to do that check (originally
I thought we would ignore the display on the IMG frame). I'll call IsBlockLevel
on the IMG frame's display style struct in CanContinueTextRun, return FALSE if
it is a block level frame.

Still need and sr...

Comment 54

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

As I mentioned in my second comment, I think the patch is fine as is since the
frame construction for a 'display: block' image should mean that it's never
called...

Comment 55

[Marc Attinasi]

David, I saw your second post, but I still think I need to do the check. If I
create a text run with an image in it, and the image is display:block, then the
CanContinueTextRun will return TRUE unless I check check for it being a block -
I ran it in the debugger to verify this - there is nothing in frame construction
to treat thsi differently. If I don't make the change to check for display:block
then the MEW is wrong: we end up with our MEW being the sum of the text before
and after the image, as if the image did not break the text, even though it does
break to a new line.

Of course, images are probably rarely display:block, but the change is simple
enough that I think it shoudl be included.

Comment 56

[Marc Attinasi]

Proposed check for block image frame:

NS_IMETHODIMP
nsImageFrame::CanContinueTextRun(PRBool& aContinueTextRun) const
{
  NS_ASSERTION(mStyleContext, "null style context is really really bad");

  // check for block element: 
  // we only contine a text run if we are NOT acting as a block
  const nsStyleDisplay* display =
    (const nsStyleDisplay*)mStyleContext->GetStyleData(eStyleStruct_Display);
  NS_ASSERTION(display, "null display style struct - how?");

  aContinueTextRun = !(display->IsBlockLevel());
  return NS_OK;
}

Comment 57

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

I suspect the greater-than check here will break negative horizontal margins
that are more negative than the width of the image, no?  Or is this for
something else?

+          if (accumulatedWidth > mw)
+            mw = accumulatedWidth;

I think the original patch should be fine as-is without the change for block
display -- frame construction should ensure that there's only one image in a
line when it's display: block (and I would think it would be directly within the
line rather than wrapped in a block frame, although I could be wrong)

Comment 58

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

Yeah, an inline image does sit directly within the line, from sec43.htm of the
CSS1 test suite:

  line 0x827a7f8: count=1 state=block,clean,NOT Impacted,[0x1002]
{5598,4643,180,180} <
    Frame(img)(31)@0x421a7b1c {5598,4643,180,180} [state=00000024]
[content=0x421a3c80]
  >

Comment 59

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

Have you tested with images that have margin, padding, and border?  If they
work, could you remove the "XXX" comment saying they might not, and if not,
could you add  in the padding, border, and margin (or perhaps just margin)?

Comment 60

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

Other than those three things (1 at 10:55, 2 at 10:49), r=dbaron.

Comment 61

[Marc Attinasi]

>I suspect the greater-than check here will break negative horizontal margins
>that are more negative than the width of the image, no?  Or is this for
>something else?

It is just to make sure that we accumulate the widest width, the same way we
only update the MEW if the frame's width is wider than the previously saved MEW,
as was done before (and still) here:

        if (maxElementWidth < mw) {
          maxElementWidth = mw;
        }
Negative horizontal margins seem to work just fine (although in the case where
non-breaking text run has an image in it, they make very little sense).

>I think the original patch should be fine as-is without the change for block
>display -

As I said in my prior post, the problem when we do not check for a block image
is that the text frame is told that the image continues the run, and so it
accumulates the two text fragments on either side of the block-image into the
MEW for the text run. This is wrong, and it has nothing to do with how the
frames are constructed. If the line is like this:

FOOBAR<img>WHATSUPDOC

and IMG is display:block, we will get a MEW equal to the length of
FOOBARWHATSUPDOC instead of just the length of WHATSUPDOC. The block still ends
up on its own line of course, but it is the textframe that is adversly affected
by the block image saying it can continue a text run, when really it does not.

Finally, I removed the comments about margin and border, I tested and their size
is accumulated correctly.

Comment 62

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

OK, r=dbaron, but I still think VerticalAlignLine should get called only on the
frames in each line, and since a block image is in a line by itself, it
shouldn't be a problem.

Comment 63

[Marc Attinasi]

VerticalAlignLine *is* only called for the frames in each line, that is not the
problem. It is CanContinueTextRun that is the issue - it is called by the
textFrame when it is measuring the text in its Reflow. The TextFrame has no idea
about lines, it just checks to see if a frame in the middle of a run should
cause the text run to break or not, and when that frame is an image, it needs to
break the text run if it is diaplay block, but not if it is display inline. 

Thanks for the review: waterson sent me an email indicating that r=dbaron is a
proxy for sr=waterson,m so I have r and sr now (phew).

Comment 64

[Marc Attinasi]

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

Comment 65

[Marc Attinasi]

fix checked into 0.9.2 branch. Awaiting approval for 0.9.4 checkin

Comment 66

[Asa Dotzler [:asa]]

a=asa on behalf of drivers.

Comment 67

[Marc Attinasi]

/cvsroot/mozilla/layout/html/base/src/nsImageFrame.cpp,v  <--  nsImageFrame.cpp
new revision: 1.197; previous revision: 1.196

/cvsroot/mozilla/layout/html/base/src/nsImageFrame.h,v  <--  nsImageFrame.h
new revision: 1.33; previous revision: 1.32

/cvsroot/mozilla/layout/html/base/src/nsLineLayout.cpp,v  <--  nsLineLayout.cpp
new revision: 3.102; previous revision: 3.101


Marking FIXED

Comment 68

[R.K.Aa.]

not sure if this is the culprit, but yesterday this page loaded fine and now it
doesn't
http://www.dinside.no/motor
If it on the odd occation can be loaded, clicking links to main stories doesn't
work - moz just spins

Comment 69

[R.K.Aa.]

ignore previous comment - must have been external network hickup.

Comment 70

[Mats Palmgren (inactive)]

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

Comment 71

[Jesse Ruderman]

The testcases on this bug still don't work for me... (Win98 09/15 trunk build)

See also bug 99851 (which is a dup of this bug, if this bug gets reopened).

Comment 72

[Rick DeBay]

Doesn't work for me.  I have a URL that demonstrates the problem,
but I must be contacted directly in order to enable it for outside access.

Comment 73

[Marc Attinasi]

Rick, can you just attach a reduced testcase based on that URL?

Comment 74

[Michael Entin]

Attachment: Repro for one of the dups

Comment 75

[Marc Attinasi]

This is being taken up in bug 97619. I have identified what I did that 'unfixed'
it and will be 'refixing' it presently.