[FIX]Table layout with images broken

RESOLVED FIXED in mozilla0.9.5

Status

()

P3
major
RESOLVED FIXED
17 years ago
4 years ago

People

(Reporter: moz-bugzilla2, Assigned: attinasi)

Tracking

({regression, top100, topembed})

Trunk
mozilla0.9.5
x86
All
regression, top100, topembed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: pdt, crtical for 0.9.4, [landed on: trunk, 0.9.2 branch, 0.9.4 branch], URL)

Attachments

(8 attachments, 3 obsolete attachments)

545 bytes, text/html
Details
297 bytes, text/html
Details
525 bytes, text/html
Details
1.99 KB, text/html
Details
2.86 KB, text/html
Details
6.26 KB, patch
bernd_mozilla
: review+
waterson
: superreview+
Details | Diff | Splinter Review
6.32 KB, patch
Details | Diff | Splinter Review
1.45 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

17 years ago
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.2)
Gecko/20010726 Netscape6/6.1
BuildID:    2001083003

In the page, http://www.ziskind.com/wardell/mozilla.html the table layout is
being displayed incorrectly. This page looks fine in a Mozilla build from a day
or two ago and still currently looks correct in Netscape v6.1. Using an older
build or Netscape 6.1 will show you the correct layout.

Reproducible: Always
Steps to Reproduce:
1. Load http://www.ziskind.com/wardell/mozilla.html

Actual Results:  The display is incorrect.

Expected Results:  Look at http://www.ziskind.com/wardell/mozilla.html using an
older Mozilla or Netscape 6.1 to see how it should display.
(Assignee)

Comment 1

17 years ago
Looks like it might be a regerssion from my change to handle inline-images in
text runs (bug 32191). Taking to investigate. BTW: if anyone can help out, just
#undefine MEW_HACK in nsLineLayout and see if that fixes the page. Or, reduce
the testcase. Thanks.
Assignee: asa → attinasi
Severity: major → normal
Status: UNCONFIRMED → NEW
Component: Viewer App → Layout
Ever confirmed: true
Priority: -- → P3
Target Milestone: --- → mozilla0.9.5

Comment 2

17 years ago
Created attachment 47690 [details]
testcase

Comment 3

17 years ago
Created attachment 47692 [details]
further reduced testcase

Comment 4

17 years ago
Created attachment 47696 [details]
watch this with IE ( and of course nothing more )
(Assignee)

Comment 5

17 years ago
Interesting: so the images wrap if the cell width is specified, but not if the
table width is specified. Ack, sounds like a table issue (CC'ing karnaze)

Comment 6

17 years ago
Looking a little bit longer on this issue, I think what we are doing here is to
emulate a IE quirk. Looks that we have not enough NavQuirks that we start to
bother about them ;-). From this point of view we should disable Marc's patch.
Make the <td nowrap> <img><img> </td> case working and send the majority of
these bugs to evangelism, because the page author has then complete control over
the behavior. The other way would require to break the line layout independency
from table layout or would somebody like to see a line like

if ((gettag(line.parentReflowState->parentreflowState) == tablecell) &&
(line.parentReflowState->parentReflowState.stylewidth == unspecified) &&
(mode==NavQuirks)) 
and in the worst case get this line executed every time we layout a line.

Analysing what went wrong in this case, I think we got trapped between our table
nowrap hack, a insufficient qa analysis what bug 32191 is about and a over
optmistic reviewer.
  

Comment 7

17 years ago
Created attachment 47819 [details]
some more ie quirk digging

Comment 8

17 years ago
*** Bug 97790 has been marked as a duplicate of this bug. ***

Comment 9

17 years ago
Confirming with 2001083106 Linux, thus OS->all.
http://usatoday.com is affected, too.
In the latest testcase _only_ "cell width + nowrap" with the 'xxxx' wraps. With
Konqueror everything wraps.
OS: Windows 2000 → All

Updated

17 years ago
Keywords: top100

Comment 10

17 years ago
the difference between <td nowrap> and <td style="white-space:no-wrap">
is at 
http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsHTMLTableCellElement.cpp#441
it is a result from busters checkin 1.4 from 98

Comment 11

17 years ago
To summarize chats with bernd, attinasi. attinasi's patch [to fix the problem 
with images without whitespace wrapping inside table cells] needs to be disabled 
if there is a pixel (not percent) width  on the cell. 

Updated

17 years ago
Severity: normal → major
Keywords: regression, topembed

Comment 12

17 years ago
or we are not in a table cell ...

Comment 13

17 years ago
*** Bug 97809 has been marked as a duplicate of this bug. ***

Comment 14

17 years ago
(latest duplicate found this problem on www.be.com)
Showstopper for 0.9.4 ?

Comment 15

17 years ago
we (me and Mark Attinasi) looked at the "some more ie quirk digging" testcase
and there is a restriction when we map "nowrap" to "white-space: nowrap;"

file: nsHTMLTableCellElement.cpp

void MapAttributesIntoRule(c
....
        nsHTMLValue value;
        aAttributes->GetAttribute(nsHTMLAtoms::nowrap, value);
        if (value.GetUnit() != eHTMLUnit_Null) {
          // See if our width is not a pixel width.
          nsHTMLValue widthValue;
          aAttributes->GetAttribute(nsHTMLAtoms::width, widthValue);
          if (widthValue.GetUnit() != eHTMLUnit_Pixel)
            aData->mTextData->mWhiteSpace.SetIntValue(
              NS_STYLE_WHITESPACE_NOWRAP, eCSSUnit_Enumerated);
        }
....

the problem we have is that IE wraps regadless if quirk or standard mode
(does not respect the CSS spec i think)

Comment 16

17 years ago
i know also that IE is ignoring the "nowrap" if a window is resized and a nowrap
cell does not get enough place to display nowrap-ed lines
(Assignee)

Comment 17

17 years ago
I have a fix to restrict my fix for bug 32191 to situations where we are in a
table cell with no width constraint. It fixes a lot of testcases, and breaks
some of the sites I originally fixed (newport-news.com, for example).

I'm going to go through all of the sites and testcases again, and see what is
re-broken and what remains fixed. Some new bugs will likely result ;)
Status: NEW → ASSIGNED
(Assignee)

Comment 18

17 years ago
Created attachment 47870 [details] [diff] [review]
PATCH: restrict the MEW accumulation to table cells with unconstrained width, in Quirks mode
(Assignee)

Comment 19

17 years ago
Comment on attachment 47870 [details] [diff] [review]
PATCH: restrict the MEW accumulation to table cells with unconstrained width, in Quirks mode

Uh, patch is wrong (last minute cleanup wrecked it!)
Attachment #47870 - Attachment is obsolete: true
(Assignee)

Comment 20

17 years ago
Created attachment 47880 [details] [diff] [review]
PATCH: restrict accumulation of adjacent image widths in MEW calculation to table cells with unconstrained widths, in Quirks mode

Updated

17 years ago
Whiteboard: critical for 0.9.4
(Assignee)

Comment 21

17 years ago
Attached a patch that restricts accumulation of adjacent image widths to cases
where we are in QuirksMode, in a table cvell, and the cell has unconstrained or
percentage constrained width.

Anyone want to review it? I am in the process of testing all of the testcases
and URLs that the original patch to bug 32191 fixed - so far it is looking
pretty good.
Whiteboard: critical for 0.9.4
Whiteboard: crtical for 0.9.4
(Assignee)

Comment 22

17 years ago
Nominating 0.9.4
Keywords: mozilla0.9.4
Target Milestone: mozilla0.9.5 → mozilla0.9.4
My gut feeling is that this is too much of a hack and that IE is probably doing
something more sensible (and perhaps more complicated) that we haven't figured
out.  But I could certainly be wrong.  Even if that's not the case, why should
the fix be quirks-mode only?

Comment 24

17 years ago
couldn't we place the call to InUnconstrainedTableCell

 outside the while loop and hold the result in a PR_BOOL, I am just afraid of
the performance penalty.

Comment 25

17 years ago
> Even if that's not the case, why should the fix be quirks-mode only? 

Because we all fear Hixie ;-). I think, what we do here, is to emulate some
strange behaviour of another browser in order to fullfill the page authors
expectations based on his IE experience, it is not clear that this quirky
behaviour is backed by some spec.  

But if we make this quirks-mode only, we'll end up with a fork that isn't
required by any standard and we'll have one half of the fork not very well
tested (consider our standard-mode form controls, until very recently).

Comment 27

17 years ago
*** Bug 97936 has been marked as a duplicate of this bug. ***

Comment 28

17 years ago
*** Bug 97937 has been marked as a duplicate of this bug. ***

Comment 29

17 years ago
*** Bug 97950 has been marked as a duplicate of this bug. ***

Comment 30

17 years ago
*** Bug 97612 has been marked as a duplicate of this bug. ***
Created attachment 48045 [details]
additional testcases (nav4 and IE5.5 differ; try narrow window)

Comment 33

17 years ago
changing window size on dbaron testcase also yields disaster under moz 2001083003

Comment 34

17 years ago
The problem we have arises from the fact that images now CanContinueTextRun. A
pattern word1-image-word2 gives a MaxElementSize = word1+word2 but skippes the
image size.

http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsTextFrame.cpp#4765
we search for the next text frame aka word2 and skip all frames inbetween. I
think a correct fix would move the IE quirk emulation to
imageframe::CanContinueTextFrame
and add a accumulation of skipped frames behind the above posted link. Otherwise
we will have a wrong minimum content width and all kind of trouble in table layout.

Comment 35

17 years ago
is the horkage at www.msnbc.com (top100?) caused bug this bug? (bug 98072)

Comment 36

17 years ago
*** Bug 98072 has been marked as a duplicate of this bug. ***

Comment 37

17 years ago
*** Bug 98104 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 38

17 years ago
Bernd, regarding your comment from 2001-09-03 01:37, I agree (there was even an
XXX comment in the original checkin about educating the TextFrame). Given what
you said, I should probably just turn OFF the change (#undef HACK_MEW) and work
harder on getting the TextFrame to handle images. I did try before, but the
TextFrame did not cooperate - I'll have to be more insistent I guess. This will
probaly make dbaron happier too.
Except, if you look at IE's behavior in my testcase, it actually does depend on
whether the table cell is unconstrained, I think.  I suspect it's derived from
the slightly different, and perhaps more logical to its author, behavior in Nav4...
(Assignee)

Comment 40

17 years ago
So, here is the current sate of this bug. The patch I previously attached is now
obsolete - as Bernd pointed out, the imageFrame was reporting that it could
continue a text run when it really couldn't in some cases, and so the text frame
and the line that contained it were out of synch WRT the inclusion of the image
frame and surrounding text frames' widths in the max element width. I chatted
with Bernd and he suggested a different approach where we do NOT allow images to
continue text runs, but still implement the MEW_HACK similar to the original way
it was done (but restricted to unconstrained table cells). That way we make sure
that the text runs are never treating imageFrames like inline style frames but
still allow the table to keep the text run with images intact. I have a working
version of this approach now, but...

David Baron posted a great testcase that shows many different scenarios and how
IE treats them. Looking at the testcase there is an interesting discrepency
between how the new patch and IE treat consecutive images in *constrained*
cells: IE breaks between images, but it tries to only break after an image
instead of before it. Mozilla, with the new patch or before the patch for bug
32191 that started this mess, will break before the image. Nav 4.x is different
altogether: it does not care if the cell is constrained or not, it always tries
to keep the images and textruns together. 

The new patch (to be attached shortly) fixes the regression with usatoday.com,
and makes msnbc.com render like it used to (still not correct, but for unrelated
reasons), and keeps the fix working for the sites originally reported in bug
32191 (like newport-news.com catalog pages). There are STILL discrepencies
between IE and Mozilla's rendering of some of the tests in the attached dbaron
test page, but I believe that these should be treated seperately. This patch
will cause Mozilla to only deal with images specially if they are in
unconstrained table cells, in Quirks mode.

Bottom line: we need to either disable the original patch from bug 32191, or get
theis new patch tested and in very soon. I'm happy to go either way and
encourage suggestions or ideas.
(Assignee)

Comment 41

17 years ago
Comment on attachment 47880 [details] [diff] [review]
PATCH: restrict accumulation of adjacent image widths in MEW calculation to table cells with unconstrained widths, in Quirks mode

Obsoleting patch: too many problems with imageFrames reporting that they CanContinueTextRuns
Attachment #47880 - Attachment is obsolete: true
(Assignee)

Comment 42

17 years ago
Created attachment 48212 [details] [diff] [review]
Another attempt: prevents images from indicating that they CanContinueTextRuns, but still does the MEW hackery (in quirky unconstrained table cells)
Updated status
Whiteboard: crtical for 0.9.4 → crtical for 0.9.4 (Patch needs further testing + review/super-review)
Added nsbranch keyword, set milestone to Mozilla0.9.5
Keywords: nsbranch
Target Milestone: mozilla0.9.4 → mozilla0.9.5
(Assignee)

Updated

17 years ago
Blocks: 98291

Comment 45

17 years ago
the latest patch crashes build on linux in 
nsLineLayout.cpp: In method `void nsLineLayout::VerticalAlignLine 
(nsLineBox *, nsSize &, nscoord &)':
nsLineLayout.cpp:1903: `frameCount' undeclared (first use this 
function)
nsLineLayout.cpp:1903: (Each undeclared identifier is reported only 
once for each function it appears in.)
gmake[5]: *** [nsLineLayout.o] Error 1
(Assignee)

Comment 46

17 years ago
Yea, I saw that too - sorry, just comment that line out (or wrap it in the
#ifdef NOISY_MAX_ELEMENT_SIZE like I did). I'm sorry I missed that, please try
again.
Summary: Table layout with images broken → [FIX?]Table layout with images broken

Comment 47

17 years ago
commenting out worked fine.

Comment 48

17 years ago
something is wrong. Check out http://www.cnn.com with build with and without
this patch: Around middle of page in a purple field under heading "FRONT &
CENTER" an image vanishes with the patch. (2001083008 displays it fine)

Comment 49

17 years ago
*** Bug 98336 has been marked as a duplicate of this bug. ***

Comment 50

17 years ago
Sorry - false alarm. Removed dist, rebuilt with the patch, deleted cache:
Afore mentioned image(s) display OK.

Comment 51

17 years ago
I believe I'm seeing the same problem at Apple's page:

http://store.apple.com/1-800-MY-APPLE/WebObjects/AppleStore

Last column of table (which contains several images) is rendered too wide.
Tested with the Sept 5th build under Mac OS X and Windows ME.

Comment 52

17 years ago
*** Bug 98485 has been marked as a duplicate of this bug. ***

Comment 53

17 years ago
Is that page suffering from the same problem?
http://zazie.org.free.fr/images/album/album-index.php3

I'm using today's build (20010906), and with this release, the images are all on
one line. The page has a HTML 4.01 'loose' doctype, validates against 4.01, and
the layout is fine under Opera 5 and other browsers. What's wrong?

Comment 54

17 years ago
>Is that page suffering from the same problem? : YES
(Assignee)

Comment 55

17 years ago
Created attachment 48457 [details] [diff] [review]
NEW patch against trunk: handles wrapping when the text has whitespace much more like IE (responding to Bernd's comments and testcase)
(Assignee)

Updated

17 years ago
Attachment #48212 - Attachment is obsolete: true

Comment 56

17 years ago
Comment on attachment 48457 [details] [diff] [review]
NEW patch against trunk: handles wrapping when the text has whitespace much more like IE (responding to Bernd's comments and testcase)

r=bernd
Attachment #48457 - Flags: review+

Comment 57

17 years ago
Comment on attachment 48457 [details] [diff] [review]
NEW patch against trunk: handles wrapping when the text has whitespace much more like IE (responding to Bernd's comments and testcase)

sr=waterson
Attachment #48457 - Flags: superreview+
(Assignee)

Updated

17 years ago
Summary: [FIX?]Table layout with images broken → [FIX]Table layout with images broken
Whiteboard: crtical for 0.9.4 (Patch needs further testing + review/super-review) → crtical for 0.9.4 (have r,sr, will land on trunk and 0.9.2 branch)
(Assignee)

Comment 58

17 years ago
*** Bug 98630 has been marked as a duplicate of this bug. ***

Comment 59

17 years ago
Thanks Marc, adding PDT to this one and also comments from arun (from the bug
98630):

..."Nominating PDT (hoping for a '+') and also 'nsbranch.'  This regression
shows up in mfc-embed, and so our embedding partners are bit by this :-(  ".. arun

Two example sites demonstrating the problem:
-----
http://www.golfsociety.com
http://elisabeth.com
Whiteboard: crtical for 0.9.4 (have r,sr, will land on trunk and 0.9.2 branch) → pdt, crtical for 0.9.4 (have r,sr, will land on trunk and 0.9.2 branch)

Comment 60

17 years ago
One more!!! :-) with this problem!!
http://www.livesidebar.com (the 4 sidebar tab images.. should be in 2 columns)

Comment 61

17 years ago
*** Bug 98666 has been marked as a duplicate of this bug. ***

Comment 62

17 years ago
current patch horks http://www.vg.no which used to display just fine.
(Assignee)

Comment 63

17 years ago
Hmm. The part of http://www.vg.no/ that is getting whacked is that
bisque-colored section on the left hand side - it is too wide now, and so it
pushes everything else to the right. I'll try to figure out what is oging on in
there... Thanks.
(Assignee)

Comment 64

17 years ago
OK, I figured it out (will this ever end?)

There are two images with only a space between them, and in the previous patch
that space was allowing the width of the two images to be added together, when
really it should have prevented it. I added another check to see if the frame in
question is only whitespace, and if so, do not accumulate widths. I'll attach it
- thanks for finding that site, R.K.Aa.
(Assignee)

Comment 65

17 years ago
Created attachment 48586 [details] [diff] [review]
Added check for whitespace-only frames to fix www.vg.no (and other cases of that problem, which must certainly arise)
(Assignee)

Comment 66

17 years ago
fix checked into trunk.
Whiteboard: pdt, crtical for 0.9.4 (have r,sr, will land on trunk and 0.9.2 branch) → pdt, crtical for 0.9.4, landed on: trunk

Comment 67

17 years ago
*** Bug 98894 has been marked as a duplicate of this bug. ***

Comment 68

17 years ago
The patch wfm (Thanks =). Testcases are now OK except in the "some more ie quirk
digging" testcase the "cell width +nowrap specified"-parts are not honored.

Intended ?
(Assignee)

Comment 69

17 years ago
Not intended, really, just the most I wanted to land right now. There is still
more work to do to get our emulation of IE's table quirks right, but there were
so many serious regressions coming up regularly since my landing for bug 32191
that I wanted to get the bulk of the issues fixed now, and attack the rest as
needed later. This quirk-hunting game can get tedius...
(Assignee)

Comment 70

17 years ago
Landed on 0.9.2 branch, waiting for approval for 0.9.4 branch
Whiteboard: pdt, crtical for 0.9.4, landed on: trunk → pdt, crtical for 0.9.4, [landed on: trunk, 0.9.2 branch]

Comment 71

17 years ago
Marc, I don't think you need to attack the cell width - nowrap stuff. See Alex's
comment from 2001-08-31 13:27 and  mine from 2001-08-31 10:20. Hopefully the
nightmare is over.

Comment 72

17 years ago
Using build 2001090906 on RH6.2 Linux (i.e. first nightly build with the patch),
here are sites that do not work as usual:
 - http://freshmeat.net/ (the top table borders are thick, do not render as with
previous Mozilla 2001090808 or IE6).
 - http://www.linuxgames.com/ the white table borders are going too far than
they should.

Therefore, all previous URLs seem fixed (be.com, apple.com, ziskind.com).

Comment 73

17 years ago
Marc, calm down:

freshmeat doctype: 
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 TRansitional//EN"
	"http://www.w3.org/TR/1999/REC-html401-19991224/loose.dtd">
linux games doctype:

<!DOCTYPE HTML PUBLIC "-//W3C//DTD W3 HTML//EN">

and David Baron checked in a new doctype parser: bug 55264
freshmeat / linuxgames covered in bug 98977.
*** Bug 98668 has been marked as a duplicate of this bug. ***

Comment 76

17 years ago
*** Bug 86463 has been marked as a duplicate of this bug. ***

Comment 77

17 years ago
*** Bug 99027 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 78

17 years ago
*** Bug 98803 has been marked as a duplicate of this bug. ***

Comment 79

17 years ago
Comment on attachment 48457 [details] [diff] [review]
NEW patch against trunk: handles wrapping when the text has whitespace much more like IE (responding to Bernd's comments and testcase)

a=asa for checkin to 0.9.4 branch.
Attachment #48457 - Flags: approval+
(Assignee)

Comment 80

17 years ago
Landed on 0.9.4 branch - marking FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
Whiteboard: pdt, crtical for 0.9.4, [landed on: trunk, 0.9.2 branch] → pdt, crtical for 0.9.4, [landed on: trunk, 0.9.2 branch, 0.9.4 branch]

Updated

17 years ago
Blocks: 99225
*** Bug 98767 has been marked as a duplicate of this bug. ***

Comment 82

17 years ago
*** Bug 75696 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 83

17 years ago
The late-addition of the check for whitespace-only frames messed this fix up.
Bug 32191 has lots of reports about the bug not being fixed, and they are
correct. I probably need to remove that check for now, while I search for a way
to handle it and fix the original problem...

Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 84

17 years ago
Removing the check for WHTESPACE-ONLY frames fixes this. The problem is that it
break now www.vg.no  At this point, I'd rather break that site and fix all of
the others while I search for a fix.
Status: REOPENED → ASSIGNED
(Assignee)

Comment 85

17 years ago
Created attachment 49658 [details] [diff] [review]
Patch to remove the check for whitespace-only frames, which I should not have put in to begin with.
(Assignee)

Comment 86

17 years ago
Bug 100194 opened to track / fix problem on www.vg.no

Fixed the patch on trunk and 0.9.4 to be what was reviewed and tested.

(Assignee)

Comment 87

17 years ago
OK, it is fixed now.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED

Comment 88

17 years ago
*** Bug 99851 has been marked as a duplicate of this bug. ***

Comment 89

17 years ago
This appears to have been broken again. When I visit
http://www.robotbastard.com/ the "HelpBot" has a space visable when using the
".../nightly/latest/" and the ".../nightly/latest-0.9.5/" builds. 

The space is not there if I use the ".../nightly/latest-0.9.4/" build!
(Assignee)

Comment 90

17 years ago
Looks OK in my CVS build from 10-09 - I'll update to today's source and try again.
(Reporter)

Comment 91

17 years ago
I see this problem also in 2001101003 on W2K
(Assignee)

Comment 92

17 years ago
OK, I see it. I was not seeing it before because I was in Viewer and forcing the
layout to Quirks mode, but in Mozilla, or in Viewer in Standards Mode, I see the
problem.

The problem has to do with the doctype. The doctype is triggering a Standards
Mode layout. If you remove the doctype (or force Quirks mode) then the little
help robot's head gets attached back to his body like it should be.

See bug 98977 for more information on how this DOCTYPE

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
        "http://www.w3.org/TR/1999/REC-html401-19991224/loose.dtd">

now triggers Standards mode (where it used to trigger Quirks mode).
(Assignee)

Comment 93

17 years ago
BTW: the reason for the gap is that in Standard mode the baseline that the image
sits on is the text baseline, which leaves room for the text descenders. If you
want to make the image sit on the bottom of the box, use the 'vertical-align:
bottom' property as in:

<img style="vertical-align:bottom" src="images/common/help_bot_top.gif" border="0">
Another potential fix, given the markup on this page, is:

<img style="display:block" src="images/common/help_bot_top.gif" border="0">

You need to log in before you can comment on or make changes to this bug.