215857 - {inc}percentage-width blocks inside table cells can make cells too big

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Federico Carbonell]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5b) Gecko/20030810
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5b) Gecko/20030810

The page in http://www.endi.com is not displayed correctly. The main contents
are displayed with excessive space to the left. It looks like an improper
rendering of the html <table> tag.

Reproducible: Always

Steps to Reproduce:
1. Go to http://www.endi.com
2.
3.

Actual Results:  
The page actually looks wrong with the main contents showing excessive blank
spaces to the left, making necessary to scroll to the right to see content. When
I refresh the page, the problem goes away. It happens when I load the page for
the first time.

Expected Results:  
Display the page with the main contents centered without the excessive blank
spaces on the left.

This problem does not crash Mozilla.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Worskforme, linux trunk build 2003-08-11-05

Comment 2

[Matthias Versen [:Matti]]

Attachment: screenshot from win2k and todays nightly

Comment 3

[Matthias Versen [:Matti]]

a reload fixed this

Comment 4

[Andrew Schultz]

Attachment: testcase

testcase usually shows the bug with linux trunk 2003081005, less so with
previous builds.  the left cell is too wide (wider than the right cell). 
reloading fixes it.

I served this up with a php script that delayed a bit between each line. 
bugzilla might provide enough delay, maybe not.

Comment 5

[Andrew Schultz]

this regressed between linux trunk 2003072905 and 2003073005, apparently bug 38370
earlier builds do not exhibit the bug, even in standards mode.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Andrew, could you possibly narrow down where the layout needs to happen in
mid-load?  See http://weblogs.mozillazine.org/hyatt/archives/2003_08.html#003963
for a technique for doing that....

Comment 7

[Andrew Schultz]

Attachment: testcase using JS trick

using the JS trick bz pointed out, this is simpler and 100% reproducible,
without resorting to php tricks.
the left cell should be small (135 pixels) but it's wider than the right cell.

Comment 8

[Bernd]

Attachment: testcase without hr

Comment 9

[Bernd]

Attachment: reflow log

This error is not new, it only was made visible due to hr checkin. See the new
testcase. The issue here is that on dirty reflow the block code returns the
wrong MEW.
Lets mind execute the test case:
On the initial run, due to the break point the table will have only one cell.
The breakpoint excludes the div and the second cell. The cell has a specified
fixed width but the table needs to redistribute the remaining space to this
cell so it will grow to fill the table. On the second run the div will be added
to the table cell. The cell has the computed width as large as necessary to
fill the table. The inner div computes easily the nessary width and return also
this width as the MEW. On the third pass the second cell is added to the row.
This has a side effect on the first cell with the div. The first cell sees a
dirty reflow. More than this it sees a unconstrained computed width and it
requires the childs to give back the MEW. Now the block code marks the text
lines as dirty and reflows them but it does not reflow the inner block and
reports back the old but now incorrect MEW. The first table cell has now a
specified width but a large MEW. This requirement influences the table layout
strategy and the final resize reflow gives the wrong shape.

Comment 10

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

This wouldn't have been fixed by the checkin for bug 209066, would it?

Comment 11

[Bernd]

no it would not have been fixed as the div has no margins around it specified

Comment 12

[Andrew Schultz]

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

Comment 13

[Andrew Schultz]

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

Comment 14

[Hussam Al-Tayeb]

in bug 217049 , somebody mentioned that this bug happens only in ntfs.org and
flexbeta.net which is wrong since it happens on many websites. 
i also noticed that it tendes to happen more to people who are behind a firewall
server

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

Comment 16

[Hussam Al-Tayeb]

more test cases:
http://www.acdsystems.com/English/Products/ACDSee/index.htm
http://www.flexbeta.net
http://freshmeat.net

Comment 17

[Hussam Al-Tayeb]

this could be a dublicate of 217369

Comment 18

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

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

Comment 19

[u58943]

just discovered something new.  i am using aebrahims SSE2 build from 29th sept.
 Just tried selecting the orbitz expanded them on ntfs.org and it absolutely
refuses to display properly, regardless of refreshing or not.  It has the same
problem with everything being off the right of the screen too much, but it
absolutely wll not render properly no matter what i do, unless i choose one of
the other themes (all the others are able to display it properly, if not first
time, then after a refresh).

The other thing is that saving the page to disk and loading it from there, for
the first time ever, still exhibits the problem.  Before it had ALWAYS loaded
perfectly first time form disk, but this specific page refuses to render
properly at all.  It may change when the contents of the page changes so i have
saved the copy i have on disk if anyone wants it for testing.

Comment 20

[Hussam Al-Tayeb]

That's true, refreshing stopped helping in .7+ when viewing flexbeta.net

Comment 21

[u58943]

Just checked ntfs again.  It seems that when using the ekko expanded theme, then
clicking some of the links at the top, when it renders wrongly (most of the time
but not all the time), a refresh DOES fix it.

however now when using the orbitz expanded theme, doing the same thing, a
refresh does NOT fix it, and it renders wrongly EVERY time you click ANY of the
links at the top, but NOT when you first load the ntfs.org page.

Also I havent actually seen adslguide render wrongly at all since ive been using
this 29th sept build, but ntfs and flexbeta obviously still do.

Comment 22

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

Knowing exactly what causes and fixes this bug isn't really helpful.  We have a
simplified testcase, and all you're telling us is numerous different ways to
cause the same pattern of incremental reflows.

Comment 23

[Hussam Al-Tayeb]

Re: Knowing exactly what causes and fixes this bug isn't really helpful.

If so, any other way to find the cause of this bug? That's because I noticed
that this bug is still unassigned. Anyway, in terms of programming, can this bug
be solved by a simple fix or will it require a big change in the code?
(especially since it also showed up in mozilla 1.5b and newer builds)which could
mean that it is not a firebird specific bug?

Comment 24

[Boris Zbarsky [:bzbarsky]]

> If so, any other way to find the cause of this bug?

The minimal testcase is a lot better at demonstrating that cause than any list
of URLs which would just reduce to the same testcase.

And no, this is not firebird-specific -- that's why it's in the "Browser" product.

Comment 25

[Joe Blough]

Is this the same issue that causes Freshmeat to render incorrectly (can't see
the right sidebar) if the browser is sized too thin?

Comment 26

[Michael Moulton]

Seems to occur on http://www.collegiatetimes.com/ as well.

Comment 27

[Marek Stępień [:marcoos, inactive]]

I've got an other workaround for this. I even created a bookmarklet:

javascript:document.getElementsByTagName("body")[0].style.display='none';document.getElementsByTagName("body")[0].style.display='block';void(0);

The same code can be put into onload, so there's no need to search which table
cell causes the layout to

Comment 28

[Marek Stępień [:marcoos, inactive]]

...causes the layout to behave strangely.

(Bugzilla ate my comment ;))

Comment 29

[Hussam Al-Tayeb]

Test case (observation):
1. open www.flexbeta.net
2. refresh a couple of times untill it renders correctly
3. increase the font a lot using ctrl ++ , untill there are horizontal scrollbars
4. Decrease the font again using ctrl -- , it will not change back to the
original "font size"   (before step 3) and the scrollbars will remain. The page
will be messed up again.

Comment 30

[Steve]

I can not reproduce the www.flexbeta.net render problem it works fine for me on
2 computers.

Comment 31

[shadowlord13_1]

Attachment: Why it happened on one site in particular, which didn't get fixed by refreshing, etc

Also, this WOULD include a testcase, if SourceForge hadn't removed the
WIDTH=100% line from the project news export pages, which was apparently
conflicting with the WIDTH=70% for the part of the table said project news
export page was being displayed in.

Comment 32

[Hussam Al-Tayeb]

Re: Comment #30
That's probably because when you have a fast connection, the problem does not
seem to happen very often. In my case,the network traffic is big so this problem
seems to happen a lot.

Comment 33

[Alex Hirzel]

If it helps, this page appears to have the exact same problem:

http://evrsoft.com/download.shtml

This may also affect other pages on that site,
but I didn't really look around much.

Comment 34

[Bernd]

Dear commenters, 

especially those who added comment 25-33, could you please stop spamming this
bug. Please read carefully comment 22. If comment 22 is not explicit enough, I
will try to make it more obvious. Layout bugs are debugged with reduced
testcases like attachment 131075 [details]. There is even a first guess of a code level
explanation in comment 9. This bug needs a patch or may be a different code
level explanation. But it certainly does not need new urls.

Please no new URL's, no "if I change this it goes away", no "if I reload it goes
away..". 

Add a patch, thats all what is needed.

Comment 35

[Bernd]

When the HR Frame did go away (bug 38370) the following code went the dodo way:
(see http://bugzilla.mozilla.org/attachment.cgi?id=128831&action=view)

-  // HR's do not impact the max-element-width, unless a width is
-  // specified otherwise tables behave badly. This makes sense -- they
-  // are springy.
-  if (aDesiredSize.mComputeMEW) {
-    if (NS_UNCONSTRAINEDSIZE != aReflowState.mComputedWidth) {
-      nsStyleUnit widthUnit = aReflowState.mStylePosition->mWidth.GetUnit();
-      if ((eStyleUnit_Percent == widthUnit) || 
-          (eStyleUnit_Auto == widthUnit)) {
-        // When the HR is using an 'auto' or percentage width, make sure it
-        // remains springy.
-        aDesiredSize.mMaxElementWidth = onePixel;
-        aDesiredSize.width = PR_MAX(aDesiredSize.width, onePixel);
-      }
-      else {
-        aDesiredSize.mMaxElementWidth = aReflowState.mComputedWidth;
-      }
-    }
-    else {
-      aDesiredSize.mMaxElementWidth = onePixel;
-      aDesiredSize.width = PR_MAX(aDesiredSize.width, onePixel);
-    }
-    NS_ASSERTION(aDesiredSize.mMaxElementWidth <= aDesiredSize.width,
-                 "bad max-element-width");
-  }

What blocks currently do is
http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsBlockFrame.cpp#1128
especially 
// When style defines the width use it for the max-element-width
// because we can't shrink any smaller.

As one can see from the testcase this is not always true.

Comment 36

[Bernd]

Attachment: ,testcase without hr but with a table

Comment 37

[Boris Zbarsky [:bzbarsky]]

Bernd, is HaveAutoWidth() testing false in the testcases in this bug?  (see
http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsBlockFrame.cpp#1138)

Comment 38

[Bernd]

On the initial reflow it returns PR_TRUE as the computed width is
NS_UNCONSTRAINEDSIZE. On the resize reflow it returns of course PR_FALSE. The
problem with the block code is that it assumes that if the block has a computed
width and the style is percentage based, that the specified width of the
containing block is equal to currently computed width of the containing block.
For tables this is not true. The width specs for table cells are rather
min-width's. If one looks at the testcase without hr, the table cell has a
specified width of 135px and the table cell has spec. width of 766. Due to the
breakpoints the second cell is first not seen so while the specified width is
135px the computed width is 766. Now the computed size of the div should be
766*0.9 but the MEW should at maximum be 135 * 0.9. However the inner div can be
shrinked down to borderpadding + content so one could also argue that its
minimal size could also be borderpadding as it is empty. This is what the HR has
previously done and what tables currently report as MEW.

Comment 39

[Boris Zbarsky [:bzbarsky]]

Attachment: So like this?

This just adjusts HaveAutoWidth to be a little more likely to report "yes".
Certainly fixes the testcases in this bug...  I guess we should run the
regression tests with this sorta change, eh?  ;)

Comment 40

[Bernd]

Renaming the function to cover better the content would probably be a good idea
and ehm ... you will need a review from dbaron for this and he likes this kind
of patches ;-)

Comment 41

[Boris Zbarsky [:bzbarsky]]

Heh.  Well, all of that code looks pretty bogus to me, really... ;)

Comment 42

[u58943]

just applied this patch to a nightly i just made and can confirm it solves the
problem on all of the sites that i previously had problems with.  nice.

Comment 43

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 135670 [details] [diff] [review]
So like this?

OK, this is no good.  It makes float sizing all unhappy if they have percentage
widths.  See:

http://lxr.mozilla.org/seamonkey/source//layout/html/tests/block/bugs/4515.html
(float in second div too narrow)

http://lxr.mozilla.org/seamonkey/source//layout/html/tests/block/bugs/4831-a.ht
ml
(second and third float should have equal width)

http://lxr.mozilla.org/seamonkey/source//layout/html/tests/block/bugs/4831-b.ht
ml (same)

http://lxr.mozilla.org/seamonkey/source//layout/html/tests/block/bugs/196919.ht
ml
(floats on left too narrow)

etc, etc. (all sorts of regression test failures).

Comment 44

[Bernd]

Boris, these failures show only that there is no easy solution, especially as
the HR needed several reflow hacks to work somehow properly. IMHO this bug
should waite for dbaron's reflow split up, where the MEW will be computed in a
specialized call chain. There the table width specs as min-width specs can be
handled. Everything else will result in a couple of unmaintanable hacks, like
stack crawl up. And your time is to valuable for us to go this path.

Comment 45

[Boris Zbarsky [:bzbarsky]]

Hey, sounds like a plan to me.  ;)  I'd already deprioritized it once I figured
out that the solution is Hard.  ;)

Comment 46

[Jon Henry]

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

Comment 47

[Bernd]

Attachment: patch that makes the regression tests happy

Comment 48

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

Comment on attachment 136052 [details] [diff] [review]
patch that makes the regression tests happy

This looks fine to me if the regresssion tests are happy, although I'm a bit
puzzled why bz's patch made things narrower than they were before and this
patch doesn't.	(I also don't think that what this whole chunk of code does
fits the logic of what it should be doing.)

But please wrap the comments at less than 80 characters, and:
+    const nsStylePosition* pos = GetStylePosition();
+    if (eStyleUnit_Percent == pos->mWidth.GetUnit()) {

could be written as:

+    if (GetStylePosition()->mWidth.GetUnit() == eStyleUnit_Percent) {

Comment 49

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

Actually, I'm not sure I like this approach.  I'll write some tests and get back
to you later today...

Comment 50

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

Comment on attachment 136052 [details] [diff] [review]
patch that makes the regression tests happy

I think bernd's approach is basically the right one -- this is one of the cases
where we get different max-element-width results when we reflow with a
constrained or unconstrained width, so I think this is the right place to test
for percentages in order to fix the bug.

I think I prefer the idea that we should be using the content
max-element-width, though, even though that isn't exactly right...

I admit that isn't exactly what IE/Windows is doing (See
http://dbaron.org/css/test/2003/bug215857 ), but I think we'll end up with
closer results that way, since we won't produce overflow from table cells
unless the width is specified as over 100%.

Furthermore, I prefer that idea since, for the unconstrained reflow, we're
likely to follow the second codepath rather than the first, so we should make
the two codepaths match for the percentage width case, since that case will
take one some of the time and the other some of the time.

Comment 51

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

Attachment: patch to make HaveAutoWidth and !HaveAutoWidth generate the same m-e-w for % widths

This is what I was describing in my previous comment

Comment 52

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

Comment on attachment 136052 [details] [diff] [review]
patch that makes the regression tests happy

I suspect this (Bernd's) patch will cause some regressions, but since it's only
touching what I think is an incremental reflow case (computing the m-e-w while
reflowing with a fixed containing block width -- i.e., no need to determine
max-width), I suspect they won't show up in the regression tests since they'll
only show up on incremental reflows.

I really should test my assumption that the fun of nsHTMLReflowState is causing
us to follow the HaveAutoWidth codepath for the incremental m-e-w with
constrained width case...

Comment 53

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

Also, if you want to understand what HaveAutoWidth is doing (logically), it
might help to look at how it simplifies in the changes in bug 205790 (which are
the result of simplifying the logic after removing eStyleUnit_Inherit).  I
didn't really understand what was going on here until I made those changes.

Comment 54

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

Comment on attachment 136149 [details] [diff] [review]
patch to make HaveAutoWidth and !HaveAutoWidth generate the same m-e-w for % widths

I ran through the regression tests once with a printf in the new codepath (the
% case), and  the new codepath was only hit in a single test
(formctls/bugs/bug42481.html),	so this change isn't really tested by the
regression tests...

Comment 55

[Boris Zbarsky [:bzbarsky]]

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

Comment 56

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 136149 [details] [diff] [review]
patch to make HaveAutoWidth and !HaveAutoWidth generate the same m-e-w for % widths

sr=bzbarsky... we should really refactor this code.  :(

Comment 57

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

Comment on attachment 136149 [details] [diff] [review]
patch to make HaveAutoWidth and !HaveAutoWidth generate the same m-e-w for % widths

Requesting approval for what's probably our most common incremental reflow bug.

Comment 58

[Brendan Eich [:brendan]]

Comment on attachment 136149 [details] [diff] [review]
patch to make HaveAutoWidth and !HaveAutoWidth generate the same m-e-w for % widths

a=[e-mail] for 1.6b.

/be

Comment 59

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

Fix checked in to trunk, 2003-11-24 11:48 -0800.

Comment 60

[Boris Zbarsky [:bzbarsky]]

Probably worth going through the dependencies of this bug and bug 217590 and
seeing which ones went away....

Comment 61

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

I'm already doing a pair of builds for that purpose.

Comment 62

[Oliver Klee]

Does this affect bug 224469 as well?

Comment 63

[amano]

Now it works for me :))

Bug 217590 and bug 224469 seem to be fixed as well!

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6b) Gecko/20031124
Firebird/0.7+ (Nova: MNG,DOMi)

Comment 64

[Hussam Al-Tayeb]

I'm using the official 20031124 nightly on windows xpsp1 (new directory and new
profile) and I still see this bug on www.flexbeta.net

Comment 65

[Hussam Al-Tayeb]

Re: Comment #64
Using firebird and not mozilla.

Comment 66

[Michael Lefevre]

Hussam: that's to be expected - this was checked in (dbaron helpfully gave the
time in comment 59) at 2003-11-24 11:48 -0800.  The mozilla.org Firebird Windows
build was made some time around 2003-11-24 08:00 -0800, so the change would not
be in that build.

Comment 67

[Hussam Al-Tayeb]

Re, Comment #66. Thank you for your prompt reply.Anyway I was able to find a
newer build which contained the fix. So Excellent Work people!
You see I live in different time zone so I get mixed up about those things.

Comment 68

[Boris Zbarsky [:bzbarsky]]

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

Comment 69

[Julien Muchembled]

I don't understand.
Related bugs (like 217590, 217369...) are still opened. Does it mean this bug is
different and the fix would only resolve a part of the problem ?
I thought it was fixed, but today, I've been very surprised to see that :
http://alainmuchembled.free.fr/fb1.png

url to the page : http://www.yaronet.com/sujets.php?s=&f=589

Comment 70

[u58943]

it doesnt appear to be completely fixed as adslguide.org.uk still shows up too 
wide sometimes, but it has fixed the majority of them so i think these other 
sites may be a different problem.

Comment 71

[Olivier Cahagne]

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

Comment 72

[Boris Zbarsky [:bzbarsky]]

Frankly, the other bugs are open because no one has gone through and checked
whether the problem still appears with those testcases and sites....  If someone
could, that would be nice.  Please do NOT mark this bug verified until you have
done that.

Comment 73

[Hussam Al-Tayeb]

re Comment #72 , correct since although this one is fixed, bug 217369 is still
not fixed.

Comment 74

[Hussam Al-Tayeb]

the url http://www.gamespot.com in bug 217369 still displays incorrectly

Comment 75

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

It's nice to realize that I wrote what was essentially the patch that fixed this
bug a year and a half before this bug was filed (on bug 107873) and let Alex
Savulov convince me not to check it in, even though it was clearly correct,
because it caused some sort of obscure regression.

Comment 76

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

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