Closed Bug 122200 Opened 21 years ago Closed 21 years ago

pages load in one column

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.0

People

(Reporter: m1scha_m, Assigned: alexsavulov)

References

()

Details

(Keywords: top100)

Attachments

(4 files, 2 obsolete files)

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
I think its dupe of bug #121142

/jc
layout.  more adjacent floaters...
Assignee: trudelle → attinasi
Status: UNCONFIRMED → NEW
Component: XP Apps → Layout
Depends on: 99461
Ever confirmed: true
OS: Windows ME → All
QA Contact: sairuh → petersen
Hardware: PC → All
Changing QA Contact
QA Contact: petersen → moied
Reassigning to Alex.
Assignee: attinasi → alexsavulov
Target Milestone: --- → mozilla1.0
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
Keywords: top100
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.
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.
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) 
Assignee: alexsavulov → waterson
*** Bug 120582 has been marked as a duplicate of this bug. ***
looks like we have more items on ZDNet pages that display bad.
*** Bug 119433 has been marked as a duplicate of this bug. ***
Depends on: 102445
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.
Attachment #68226 - Attachment description: IE displays these cases different than us with URL list → IE displays these cases different than us with URL list Don't forget to resize the window
retaking
Assignee: waterson → alexsavulov
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.
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.
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?
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.
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
oh, sorry , i ment bug 99461
The zdnet page in question is in quirks mode (yes it has a doctype, but the
doctype triggers quirks mode).
oh really?

hmmm then I should add the quirks test... testing...
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.
Attachment #68976 - Attachment is obsolete: true
Marking nsbeta1+.
Keywords: nsbeta1+
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?
No longer depends on: 99461, 102445
Blocks: 99461
<!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. 
|<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0//EN">| is the "HTML 4.0 Strict"
doctype.  So it should be strict mode.
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.

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.

Priority: -- → P2
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)
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) 
I was wrong: IE does not wrap. We wrap anyway unles both style nad align=left
are set. Here IsPaginated helps.
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.
i forgot to make a null pointer check after I'm trying to get the previous
floater.
Attachment #69162 - Attachment is obsolete: true
Comment on attachment 70353 [details]
added null pointer check (blame me)

r/sr=attinasi
Attachment #70353 - Flags: superreview+
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'.)
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
Should we imitate the cases where IE has text overlapping too?
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)
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.
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.
r=kin@netscape.com

While I agree with dbaron that this adds to the complexity of the code, I think
backwards compatibility is important too.
Attachment #70353 - Flags: review+
Keywords: review
Comment on attachment 70353 [details]
added null pointer check (blame me)

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #70353 - Flags: approval+
fixed on trunk
now with flag change:

FIXED ON TRUNK

(goddamit! ;-)
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
*******************************************************

Please check bug 99461 for dependencies with other bugs


*******************************************************
*** Bug 129849 has been marked as a duplicate of this bug. ***
Verified fixed with build ID 20020313 on WINXP
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.