Closed
Bug 32191
Opened 24 years ago
Closed 23 years ago
[FIXED IN 0.9.2] incorrect line break before and after img tag
Categories
(Core :: Layout, defect, P2)
Core
Layout
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)
305 bytes,
text/html
|
Details | |
284 bytes,
text/html
|
Details | |
1.42 KB,
patch
|
Details | Diff | Splinter Review | |
3.79 KB,
patch
|
Details | Diff | Splinter Review | |
5.80 KB,
text/html
|
Details | |
5.43 KB,
patch
|
Details | Diff | Splinter Review | |
653 bytes,
text/html
|
Details |
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•24 years ago
|
||
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•24 years ago
|
||
Updated•24 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → M18
Comment 3•24 years ago
|
||
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
Comment 5•24 years ago
|
||
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•24 years ago
|
||
Comment 8•24 years ago
|
||
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
Updated•24 years ago
|
Status: NEW → ASSIGNED
Comment 10•24 years ago
|
||
Comment 11•24 years ago
|
||
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•24 years ago
|
||
David, this could be of interest to you since you've done some line layout related stuff.
Comment 13•24 years ago
|
||
I think 37656 is a dupe of this, can someone more in the know check it?
Updated•24 years ago
|
QA Contact: chrisd → lorca
Comment 14•24 years ago
|
||
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
Comment 15•24 years ago
|
||
buster: not fixed in Windows 2000 commercial build 2000091212. :-(
Comment 16•24 years ago
|
||
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
Depends on: 37656
Comment 17•24 years ago
|
||
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•24 years ago
|
||
*** Bug 37656 has been marked as a duplicate of this bug. ***
Comment 19•24 years ago
|
||
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
Comment 20•23 years ago
|
||
Nominating for mozilla0.9 and nsbeta1: 6 dups, previously happened on a top100 site (www.icq.com).
Keywords: mozilla0.9,
nsbeta1
Comment 21•23 years ago
|
||
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•23 years ago
|
||
*** Bug 75696 has been marked as a duplicate of this bug. ***
Comment 23•23 years ago
|
||
*** Bug 30596 has been marked as a duplicate of this bug. ***
Comment 24•23 years ago
|
||
*** Bug 76521 has been marked as a duplicate of this bug. ***
Comment 25•23 years ago
|
||
Adding mostfreq keyword: 5 dups here, 4 additional dups on bug 37656.
Keywords: mostfreq
Comment 26•23 years ago
|
||
Dupe bug 30596 was targetted at 0.9.1.
Comment 27•23 years ago
|
||
*** Bug 74259 has been marked as a duplicate of this bug. ***
Comment 28•23 years ago
|
||
*** Bug 46950 has been marked as a duplicate of this bug. ***
Comment 29•23 years ago
|
||
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•23 years ago
|
||
Note the opposite behavior in bug 84378.
Comment 31•23 years ago
|
||
*** Bug 84594 has been marked as a duplicate of this bug. ***
Keywords: mozilla0.9 → mozilla1.0
Comment 32•23 years ago
|
||
*** Bug 91553 has been marked as a duplicate of this bug. ***
Comment 33•23 years ago
|
||
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
Assignee | ||
Comment 34•23 years ago
|
||
*** Bug 93736 has been marked as a duplicate of this bug. ***
Comment 35•23 years ago
|
||
*** Bug 68186 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 36•23 years ago
|
||
Taking since I think I have a handle on this now...
Assignee: waterson → attinasi
Status: ASSIGNED → NEW
Assignee | ||
Comment 37•23 years ago
|
||
Addig topembed keyword because this is related to topembed bug bug 54565...
Comment 38•23 years ago
|
||
Woohoo! Go attinasi!
Assignee | ||
Comment 39•23 years ago
|
||
Assignee | ||
Comment 40•23 years ago
|
||
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   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•23 years ago
|
||
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 ;-).
Assignee | ||
Comment 42•23 years ago
|
||
Thanks Bernd - will do.
Assignee | ||
Comment 43•23 years ago
|
||
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.
Assignee | ||
Comment 44•23 years ago
|
||
Assignee | ||
Comment 45•23 years ago
|
||
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.
Assignee | ||
Comment 46•23 years ago
|
||
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
Assignee | ||
Comment 47•23 years ago
|
||
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.
Assignee | ||
Comment 48•23 years ago
|
||
Assignee | ||
Comment 49•23 years ago
|
||
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?
Comment 52•23 years ago
|
||
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
Assignee | ||
Comment 53•23 years ago
|
||
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...
Assignee | ||
Comment 55•23 years ago
|
||
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.
Assignee | ||
Comment 56•23 years ago
|
||
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.
Assignee | ||
Comment 61•23 years ago
|
||
>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.
Assignee | ||
Comment 63•23 years ago
|
||
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).
Assignee | ||
Comment 64•23 years ago
|
||
*** Bug 91713 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 65•23 years ago
|
||
fix checked into 0.9.2 branch. Awaiting approval for 0.9.4 checkin
Whiteboard: chk'd into 0.9.2
Assignee | ||
Updated•23 years ago
|
Summary: incorrect line break before and after img tag → [FIXED IN 0.9.2] incorrect line break before and after img tag
Comment 66•23 years ago
|
||
a=asa on behalf of drivers.
Assignee | ||
Comment 67•23 years ago
|
||
/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: 23 years ago
Resolution: --- → FIXED
Whiteboard: chk'd into 0.9.2 → chk'd into 0.9.2, 0.9.4
Comment 68•23 years ago
|
||
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•23 years ago
|
||
ignore previous comment - must have been external network hickup.
Comment 70•23 years ago
|
||
*** Bug 91713 has been marked as a duplicate of this bug. ***
Comment 71•23 years ago
|
||
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•23 years ago
|
||
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.
Assignee | ||
Comment 73•23 years ago
|
||
Rick, can you just attach a reduced testcase based on that URL?
Comment 74•23 years ago
|
||
Assignee | ||
Comment 75•23 years ago
|
||
This is being taken up in bug 97619. I have identified what I did that 'unfixed' it and will be 'refixing' it presently.
You need to log in
before you can comment on or make changes to this bug.
Description
•