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

RESOLVED FIXED

Status

()

Core
Layout
P1
normal
RESOLVED FIXED
14 years ago
10 years ago

People

(Reporter: Federico Carbonell, Unassigned)

Tracking

({qawanted, regression, testcase})

Trunk
x86
All
qawanted, regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [reflow-refactor], URL)

Attachments

(8 attachments, 2 obsolete attachments)

(Reporter)

Description

14 years ago
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

14 years ago
Worskforme, linux trunk build 2003-08-11-05
Summary: The page in http://www.endi.com is not displayed correctly. → The page in http://www.endi.com is not displayed correctly.
Created attachment 129610 [details]
screenshot from win2k and todays nightly
a reload fixed this
Assignee: general → other
Status: UNCONFIRMED → NEW
Component: Browser-General → Layout
Ever confirmed: true
QA Contact: general → ian

Comment 4

14 years ago
Created attachment 129637 [details]
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

14 years ago
this regressed between linux trunk 2003072905 and 2003073005, apparently bug 38370
earlier builds do not exhibit the bug, even in standards mode.
Keywords: regression, testcase
OS: Windows 2000 → All
Depends on: 217590

Comment 6

14 years ago
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

14 years ago
Created attachment 130994 [details]
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.
Attachment #129637 - Attachment is obsolete: true

Comment 8

14 years ago
Created attachment 131075 [details]
testcase without hr

Comment 9

14 years ago
Created attachment 131076 [details]
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.
This wouldn't have been fixed by the checkin for bug 209066, would it?

Updated

14 years ago
Attachment #131076 - Attachment mime type: application/octet-stream → text/plain

Comment 11

14 years ago
no it would not have been fixed as the div has no margins around it specified
Whiteboard: [reflow-refactor]

Comment 12

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

Comment 13

14 years ago
*** Bug 217049 has been marked as a duplicate of this bug. ***
Priority: -- → P1
Summary: The page in http://www.endi.com is not displayed correctly. → {inc}percentage-width blocks inside table cells can make cells too big

Comment 14

14 years ago
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
Blocks: 220247

Comment 15

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

Comment 16

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



Comment 17

14 years ago
this could be a dublicate of 217369

Updated

14 years ago
Blocks: 217369
*** Bug 220502 has been marked as a duplicate of this bug. ***

Comment 19

14 years ago
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

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

Comment 21

14 years ago
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.
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

14 years ago
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

14 years ago
> 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

14 years ago
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

14 years ago
Seems to occur on http://www.collegiatetimes.com/ as well.
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
...causes the layout to behave strangely.

(Bugzilla ate my comment ;))

Updated

14 years ago
Blocks: 223906

Updated

14 years ago
Blocks: 224949

Comment 29

14 years ago
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

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

Comment 31

14 years ago
Created attachment 135119 [details]
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

14 years ago
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.
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

14 years ago
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

14 years ago
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

14 years ago
Created attachment 135631 [details]
,testcase without hr but with a table

Comment 37

14 years ago
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

14 years ago
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.

Updated

14 years ago
Flags: blocking1.6?

Comment 39

14 years ago
Created attachment 135670 [details] [diff] [review]
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

14 years ago
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

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

Comment 42

14 years ago
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

14 years ago
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).
Attachment #135670 - Attachment is obsolete: true

Comment 44

14 years ago
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

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

Comment 46

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

Comment 47

14 years ago
Created attachment 136052 [details] [diff] [review]
patch that makes the regression tests happy
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) {
Actually, I'm not sure I like this approach.  I'll write some tests and get back
to you later today...
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.
Created attachment 136149 [details] [diff] [review]
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 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...
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 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...
Attachment #136149 - Flags: superreview?(bz-vacation)
Attachment #136149 - Flags: review?(bernd.mielke)

Updated

14 years ago
Attachment #136149 - Flags: review?(bernd.mielke) → review+
Attachment #136149 - Flags: superreview?(bz-vacation) → superreview?(roc)

Comment 55

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

Updated

14 years ago
Blocks: 128601
Attachment #136149 - Flags: superreview?(roc) → superreview?(bz-vacation)

Comment 56

14 years ago
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.  :(
Attachment #136149 - Flags: superreview?(bz-vacation) → superreview+
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.
Attachment #136149 - Flags: approval1.6b?
Comment on attachment 136149 [details] [diff] [review]
patch to make HaveAutoWidth and !HaveAutoWidth generate the same m-e-w for % widths

a=brendan@mozilla.org for 1.6b.

/be
Attachment #136149 - Flags: approval1.6b? → approval1.6b+
Fix checked in to trunk, 2003-11-24 11:48 -0800.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Flags: blocking1.6?
Resolution: --- → FIXED

Comment 60

14 years ago
Probably worth going through the dependencies of this bug and bug 217590 and
seeing which ones went away....
I'm already doing a pair of builds for that purpose.
No longer blocks: 128601

Comment 62

14 years ago
Does this affect bug 224469 as well?

Comment 63

14 years ago
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)

Updated

14 years ago
Blocks: 224469

Comment 64

14 years ago
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

14 years ago
Re: Comment #64
Using firebird and not mozilla.

Comment 66

14 years ago
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

14 years ago
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

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

Comment 69

14 years ago
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

14 years ago
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

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

Comment 72

14 years ago
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.
Keywords: qawanted

Comment 73

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

Comment 74

14 years ago
the url http://www.gamespot.com in bug 217369 still displays incorrectly
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.
*** Bug 107873 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.