Closed Bug 32191 Opened 20 years ago Closed 19 years ago

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

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla0.9.4

People

(Reporter: battery841, Assigned: attinasi)

References

()

Details

(Keywords: testcase, top100, topembed, Whiteboard: chk'd into 0.9.2, 0.9.4)

Attachments

(7 files)

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.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: top100
Attached file testcase
Status: NEW → ASSIGNED
Target Milestone: --- → M18
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.
Assignee: karnaze → buster
Status: ASSIGNED → NEW
Target Milestone: M18 → Future
Component: HTMLTables → Layout
Summary: Graphics by search don't appear to line up correctly → [INLINE-H]  wrapping before image, pushing image down
*** Bug 43770 has been marked as a duplicate of this bug. ***
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.
OS: Linux → All
Hardware: PC → All
Adding testcase keyword.
Keywords: testcase
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.
Summary: [INLINE-H]  wrapping before image, pushing image down → incorrect line break before and after img tag
Assigning to waterson per his request.
Assignee: buster → waterson
Status: NEW → ASSIGNED
Attached patch part of the fixSplinter Review
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.
David, this could be of interest to you since you've done some line layout
related stuff.
I think 37656 is a dupe of this, can someone more in the know check it?
QA Contact: chrisd → lorca
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.
Depends on: 14280
buster: not fixed in Windows 2000 commercial build 2000091212. :-(
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;
Depends on: 37656
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.
*** Bug 37656 has been marked as a duplicate of this bug. ***
No longer depends on: 37656
Reassigning QA Contact for all open and unverified bugs previously under Lorca's
care to Gerardo as per phone conversation this morning.
QA Contact: lorca → gerardok
Nominating for mozilla0.9 and nsbeta1: 6 dups, previously happened on a top100 
site (www.icq.com).
Keywords: mozilla0.9, nsbeta1
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.)
*** Bug 75696 has been marked as a duplicate of this bug. ***
*** Bug 30596 has been marked as a duplicate of this bug. ***
*** Bug 76521 has been marked as a duplicate of this bug. ***
Adding mostfreq keyword: 5 dups here, 4 additional dups on bug 37656.
Keywords: mostfreq
Dupe bug 30596 was targetted at 0.9.1.
*** Bug 74259 has been marked as a duplicate of this bug. ***
*** Bug 46950 has been marked as a duplicate of this bug. ***
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!
Note the opposite behavior in bug 84378.
*** Bug 84594 has been marked as a duplicate of this bug. ***
Keywords: mozilla0.9mozilla1.0
*** Bug 91553 has been marked as a duplicate of this bug. ***
 This bug has 16 dups. Nominating for mozilla0.9.4 (as there as no mozilla0.9.5
.. mozilla0.9.9 keywords).
Keywords: mozilla0.9.4
*** Bug 93736 has been marked as a duplicate of this bug. ***
*** Bug 68186 has been marked as a duplicate of this bug. ***
Taking since I think I have a handle on this now...
Assignee: waterson → attinasi
Status: ASSIGNED → NEW
Blocks: 93992
Addig topembed keyword because this is related to topembed bug bug 54565...
Status: NEW → ASSIGNED
Keywords: topembed
Priority: P3 → P2
Target Milestone: Future → mozilla0.9.4
Woohoo! Go 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.
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 ;-).
Thanks Bernd - will do.
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.
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.
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
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.
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.
Could nsImageFrame::CanContinueTextRun just check the 'display' type from the
style context?
Actually, never mind.  Frame construction should handle that, right?
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
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...
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...
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.
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;
}
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)
Keywords: mostfreq
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]
  >
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)?
Other than those three things (1 at 10:55, 2 at 10:49), r=dbaron.
>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.
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.
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).
*** Bug 91713 has been marked as a duplicate of this bug. ***
fix checked into 0.9.2 branch. Awaiting approval for 0.9.4 checkin
Whiteboard: chk'd into 0.9.2
Summary: incorrect line break before and after img tag → [FIXED IN 0.9.2] incorrect line break before and after img tag
a=asa on behalf of drivers.
/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
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Whiteboard: chk'd into 0.9.2 → chk'd into 0.9.2, 0.9.4
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
ignore previous comment - must have been external network hickup.
*** Bug 91713 has been marked as a duplicate of this bug. ***
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).
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.
Rick, can you just attach a reduced testcase based on that URL?
This is being taken up in bug 97619. I have identified what I did that 'unfixed'
it and will be 'refixing' it presently.
QA Contact: gerardok → moied
You need to log in before you can comment on or make changes to this bug.