122200 - pages load in one column

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[m1scha_m]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:0.9.8+) Gecko/20020127
BuildID:    2002012708

click on any catagory, and as the page loads, the second (?centre) column will
vanish, but will have loaded beneath the left hand column, with the right hand
column beneath that again. This is the first time I have seen this happen on
this site, but it happens every time on My Netscape page.

Reproducible: Always
Steps to Reproduce:
1.Click on catagory
2.watch the page load
3.

Actual Results:  two columns begin to load, but only one column exists when page
fully loaded

Expected Results:  more than one column

Comment 1

[Julien Cegarra]

I think its dupe of bug #121142

/jc

Comment 2

[Boris Zbarsky [:bzbarsky]]

layout.  more adjacent floaters...

Comment 3

[Chris Petersen]

Changing QA Contact

Comment 4

[Kevin McCluskey (gone)]

Reassigning to Alex.

Comment 5

[Alexandru Savulov]

actually there are two bugs on this page:

- the one column thing that looks good in NS621 (Mozilla094)
- the column far right does not display at the top like in IE

investigating

Comment 6

[Alexandru Savulov]

Attachment: reduced test case (the tables should be in the same height)

NS621 / IE and Opera display the two tables in the same height
mozilla 2002013103 fails to do so. is because the div does not have enough
width.

Comment 7

[Alexandru Savulov]

i know now. In nsBlockReflowState::FlowAndPlaceFloater
...
-  if ((eCompatibility_NavQuirks != mode) ||
-      (NS_STYLE_DISPLAY_TABLE != floaterDisplay->mDisplay)) {
-    // Can the floater fit here?
    while (! CanPlaceFloater(region, floaterDisplay->mFloats)) {
      // Nope. Advance to the next band.
      mY += mAvailSpaceRect.height;
      GetAvailableSpace();
    }
-  }

someone removed the '-' lines for some reason.
ned to see if we want to leave it that way or not.

Comment 8

[Alexandru Savulov]

damn... if I would only take a look at dependencies before i start to work on it
(we should put comments when we add dependencies and know why... well... spent
time on known issue...)

yeah is the patch for 99461 that broke this URL. so what are we going to do?
undo or evangelism? (ZDnet are IE addicts afaik) 

(IE, OPERA and NS621 put tables in the same line) 

Comment 9

[Alexandru Savulov]

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

Comment 10

[Alexandru Savulov]

looks like we have more items on ZDNet pages that display bad.

Comment 11

[Alexandru Savulov]

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

Comment 12

[Alexandru Savulov]

please check bug 102445 also be cause it seems that we have a conflict or at
least strong dependency between this bug and bug 102445. I marked 102445 blocks
this one but it might be reverse. However the issues are strongly related.

Comment 13

[Alexandru Savulov]

Attachment: IE displays these cases different than us with URL list Don't forget to resize the window

Comment 14

[Alexandru Savulov]

retaking

Comment 15

[Alexandru Savulov]

Attachment: patch preview to fix this bug and bug 99461 (see comment 15)

This patch basicly fixes two problems:

1. A table that is floating will resize and display in the same band with  the
previous floater if there is enough space. if not it will advance to the next
band and so on (loop). However a second reflow of the table is necessary since
the available space is a new one. (i still need to think about the possibility
of having an ocupied space there so we have to handle this recursively)
2. IE has a crappy behaviour here (handles "align" and "float" diffrerent):
If the previous floater of a floater to right table is a table that has
align=... IE does not place the floater in the next band, but is keeping it in
the initial one, letting the second floater to right to spill out of the
containing block on the right disobeying CSS2/9.5.1/1 but since an attribute
and not a rule is involved, there is not really CSS in charge, hovewer is
crappy - 
still there are pages out there that use this flaw for theyr purpose.
(www.zdnet.com for example)

Still to solve:
- a procentage rounding error that ocurs with the testcase from bug 99461
(http://bugzilla.mozilla.org/show_bug.cgi?id=99461) causing some jumping around

over bands while resizing the window
- right floaters that spill out on the left side of the containing block
causing them to spill out of the window on the left side. although this is a
consequence of obeying rule CSS2/9.5.1/1, the content on the left is unreadable
since the scrollbar cannot work with that.

(in the next comment i will attach a table that lists the differences between
IE and us with the testcase here and all the combinations of align= and float:

<body>
<table align=... style="float:...">
 <tr><td>blohblahblahblahblahbloh bloh bloh bloh</td></tr>
</table>
<table align=... style="float:...">
 <tr><td>blohblahblahblahblahbloh bloh bloh bloh</td></tr>
</table>
</body>

without using align and float simultaneously in one table.

Comment 16

[Alexandru Savulov]

Attachment: differrences between IE and Mozilla (recent 02-11-2002)

Comment 17

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

The |align| attribute is what causes the table to float.  Do you really want to
limit the quirk to when the previous table was floated using the align attribute
but not consider whether the current one was floating using the 'float' CSS
property or the align attribute?

I need to think more about the rest.

Comment 18

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

Never mind the previous comment (I read your comment now in addition to the
patch), although I wonder whether we need to emulate the quirk this perfectly. 
Also, shouldn't there be a quirk-mode check?

Comment 19

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

Is the change to nsBlockFrame.cpp needed to fix a real-world testcase?  I really
don't think we should do that if we can avoid it, and if we do, it would need a
quirks-mode check as well.

Comment 20

[Alexandru Savulov]

to comment 18:

the stupids from zdnet are puting <!DOCTYPE...> an the top of their pages and
this makes quirk-mode check useless. Somehow the html is not incorrect just the
way is designed is bad. (see first testcase on this bug)

to comment 19:

well there is that testcase on bug 96461 with the divs and the asciitable.com
page, and IE and Opera resize the tables if the tables can shrink

thanks for looking at this David

Comment 21

[Alexandru Savulov]

oh, sorry , i ment bug 99461

Comment 22

[Boris Zbarsky [:bzbarsky]]

The zdnet page in question is in quirks mode (yes it has a doctype, but the
doctype triggers quirks mode).

Comment 23

[Alexandru Savulov]

oh really?

hmmm then I should add the quirks test... testing...

Comment 24

[Alexandru Savulov]

Attachment: patch preview - added quirks mode check and rounding problem fix

this is the same patch like attachment 68976 [details] with quirks mode check and fixed
rounding problem. please also see comment 15 for further details and attachment
68987 [details] for a head to head comparsion with IE.

Comment 25

[Kevin McCluskey (gone)]

Marking nsbeta1+.

Comment 26

[Alexandru Savulov]

AFAICT the patch (attachment 69162 [details]) is ready for reviews. This fixes a bad
concept of web designers that tend to align=left tables to keep them in the same
line. Unfortunately IE (and previous releases of Mozilla and Netscape)support
that. The patch also fixes bug 99461 (see also comment 15) 

This bug encountered on:
http://www.zdnet.com/downloads/utilities.htmlhttp://www.slashcam.de
http://www.afv.se (some financial periodic in sweeden)
http://www.first2web.de
(I listed only those I know at this moment, but they may be more)

Does not repair:
http://kalx.berkeley.com since they have <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML
4.0//EN"> on the page that is not resolved to quirks mode. Is this a strict DTD?

Comment 27

[Alexandru Savulov]

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0//EN"> traslates to strict since is
not recognized, so we won't fix the kalx.berkeley.edu page. 

Comment 28

[Boris Zbarsky [:bzbarsky]]

|<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0//EN">| is the "HTML 4.0 Strict"
doctype.  So it should be strict mode.

Comment 29

[Alexandru Savulov]

well, this is even better. we won't fix the problem on the kalx.berkeley.com
page. if they think that the IE behaviour is correct, is their problem.

Comment 30

[Alexandru Savulov]

I understand that <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0//EN"> contains an
unknown doctype with a known public identifier, that's why I wrote "not
recognized". My mistake.

Comment 31

[Alexandru Savulov]

regression tests (layout/html/tests) passed.

(file:///s:/mozilla/layout/html/tests/block/bugs/5537.html is changing, but this
is what the fix for this bug is about)

Comment 32

[Alexandru Savulov]

PRINTING ISSUE:

hmmm... maybe i should change the patch not to keep the floating tables in the
same line with a previous align=left floater when printing.. investigating (IE
starts wrapping when the tables do not fit) 

Comment 33

[Alexandru Savulov]

I was wrong: IE does not wrap. We wrap anyway unles both style nad align=left
are set. Here IsPaginated helps.

Comment 34

[Alexandru Savulov]

ok, the printing issue is a problem that's beeing reported on bug 110336 just
there   the request is to not let the second table wrap.

we already do this if the previous floater has the style="float:left" set. so we
act already different (align != float) in that case while printing.

Comment 35

[Alexandru Savulov]

Attachment: added null pointer check (blame me)

i forgot to make a null pointer check after I'm trying to get the previous
floater.

Comment 36

[Marc Attinasi]

Comment on attachment 70353 [details]
added null pointer check (blame me)

r/sr=attinasi

Comment 37

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

Comment on attachment 70353 [details]
added null pointer check (blame me)

>Index: nsBlockFrame.cpp

>+      // give tables only the available space
>+      // if they can shrink we may not be constrained to place
>+      // them in the next line
>+      availWidth = aState.mAvailSpaceRect.width;

I'm against this, as I've said before, because of things like the fact
that two adjacent floating tables with some text in them can end up
doing things like (see the testcases for bug 85876 and bug 82315):

,-------------------------------------------------. ,-----------.
| This is the text of the first floating element. | | This is	|
`-------------------------------------------------' | the text	|
						    | of the	|
						    | second	|
						    | floating	|
						    | element	|
						    | and it	|
						    | will be	|
						    | stretched |
						    | a very	|
						    | long	|
						    | distance	|
						    | down the	|
						    | document. |
						    `-----------'

>+      // round down to twips per pixel so that we fit
>+      // needed when prev. floater has procentage width
>+      // (maybe is a table flaw that makes table chose to round up
>+      // but i don't want to change that, too risky)
>+      nscoord twp;
>+      float p2t;
>+      aState.mPresContext->GetScaledPixelsToTwips(&p2t);
>+      twp = NSIntPixelsToTwips(1,p2t);
>+      availWidth -=  availWidth % twp;

This seems like the opposite of what we want to do.  I'd think we would want
to round up (both when computing the available space *and* (more importantly
for the reason you did this, I think) when decided whether a floater fit or
needs to be pushed to the next band.  See bug 21193.

>Index: nsBlockReflowState.cpp

While I don't see anything particularly wrong with these changes, they add
quite a bit of complexity to the block code, which is already hard enough
to understand.	I wonder if these quirks are really important enough that
they warrant such a large addition of code.  (I'd rather see more work on
fixing the bugs in the existing code, but I guess most of those aren't going
to be 'topembed'.)

Comment 38

[Marc Attinasi]

David, in the example you showed with the two floating tables, that is exactly
what IE does, and why Alex made the change. See the testcase in the bug you
referenced, bug 82315 (testcase is
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=19136).  This legacy
behavior is important, see the URL

Comment 39

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

Should we imitate the cases where IE has text overlapping too?

Comment 40

[Alexandru Savulov]

as far as I analysed IE, it has overlapping problems when the first floating
table has align=right. that's why the patch only handles the case when the first
table has align=left, otherwise it wraps. (does anyone knows another particular
overlapping problem IE has that we don't want to simulate?)

i agree that the code gathers complexity trough this change and that IE's
behaviour disregards the standards. 

my only concern is that the authors are tempted to use this IE flaw be cause is
making their life easier. it looks like they discovered the cheat to have tables
stick together in the same block on the same line let's not forget that until
NS6.1.2 we support that, so the authors feel legitimate to use that flaw if they
test with the most used browsers today.

now is a question of pure market interest if we want to apply the patch or not.
i doubt that we can convince the guys from zdnet to change their markup. they
can argue that even our browser worked with that in previous versions. and there
are the others too.

thus we have to decide if we do apply the patch or not. i won't be sad/mad if
not :-) 

concerning the code complexity:

- i could try to pack that whole voodoo in a separate method

concerning the down-rounding:

- if i don't do that i get a jumping from band to band second floater that
  looks horrible while resizing the window in one direction. i see here another
  solution too: the procentage floater consumed width should not be odd then the 
  available space rect width will be an even value so the table won't have to
  round up/down anymore (need to investigate)

Comment 41

[Kevin McCluskey (gone)]

In general if there are more sites making use of an I.E quirk behavior than can
be addressed reasonably through evangelism we have been making Mozilla backward
compatible in quirks mode only. This bug clearly falls within that criteria. In
this case we are also making Mozilla backward compatible with Netscape6.2 since
the quirk behavior was removed after Netscape6.2 branched from Mozilla.

Comment 42

[Alexandru Savulov]

The issues from comment #40:

1. "i could try to pack that whole voodoo in a separate method"

well this won't reduce the code complexity so i decided to keep that part of the
patch for nsBlockReflowState.cpp unchanged

2. down-rounding

i investigated that part of the problem and i cannot start changing the way we
calculate procentage width just be cause of this patch. is an issue that
requires a lot of work and attention nd should be resolved apart from this issue.

Comment 43

[kinmoz]

r=[e-mail]

While I agree with dbaron that this adds to the complexity of the code, I think
backwards compatibility is important too.

Comment 44

[Asa Dotzler [:asa]]

Comment on attachment 70353 [details]
added null pointer check (blame me)

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 45

[Alexandru Savulov]

fixed on trunk

Comment 46

[Alexandru Savulov]

now with flag change:

FIXED ON TRUNK

(goddamit! ;-)

Comment 47

[Alexandru Savulov]

*******************************************************

Please check bug 99461 for dependencies with other bugs


*******************************************************

Comment 48

[Matthias Versen [:Matti]]

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

Comment 49

[Moied]

Verified fixed with build ID 20020313 on WINXP