52916 - [FLOAT] Floating text overlapping max-width elements

Status: RESOLVED FIXED

(Core :: Layout: Floats, defect, P2)

Description

[Karl Ove Hufthammer]

See http://home.no.net/huftis/artiklar/tabell.html for an example. The TOC menu 
is overlapping the text.

On this page, all paragraphs have a 'max-width' set, and use 'margin: auto'. 
The 'TOC' has a 'float: right' set, and is overlapping the paragraphs (it 
should of course not).

If I change 'max-width' to 'width', everything works correctly (the TOC doesn't 
overlap), so this is probably a problem computing remaining space when elements 
have a 'max-width'. This is a *very* serious bug.

Comment 1

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

Layout, not Style System.

Comment 2

[R.K.Aa.]

This is not a blocker. It does not block development or testing.
Lowering severity.

Comment 3

[Chris Petersen]

I can see this issue with the Sept 29th build. Marking new.

Comment 4

[buster]

a reduced test case would go a long way towards getting this examined before NS6
RTM.  Setting to P2.

Comment 5

[Ronald Tilby]

Attachment: testcase

Comment 6

[buster]

P2 bugs -> future

Comment 7

[Hixie (not reading bugmail)]

Upon managerial request, adding the "testcase" keyword to 84 open layout bugs that
do not have the "testcase" keyword and yet have an attachement with the word
"test" in the description field. Apologies for any mistakes.

Comment 8

[basic]

Attachment: opera screenshot

Comment 9

[Chris Waterson]

taking.

Comment 10

[Robin Lionheart]

Also see the test case for bug 69915.

Comment 11

[Marc Attinasi]

Taking, marking topembed: this was spin-off from bugscape bug 8601.

Chris, I'm taking this to work on - hope you do not mind. I'm hoping to get out
Float story in shape in the next few weeks.

Comment 12

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

Is this really related to something on a real website?  Few browsers support
'max-width'.

Comment 13

[Marc Attinasi]

Ehem - sorry. I was mistaken, not careful enough, etc.

The problem I was trying to find a bug for LOOKS just like this, but the markup
is very different. Returning this bug to it's original state, opening a new one
(until I can find a proper dup).

Comment 14

[Marc Attinasi]

re-accepting for waterson (I mistakenly took it, now giving it back).

Comment 15

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 16

[Asa Dotzler [:asa]]

don't move bugs that are in the 1.0 dependency tree. sorry.

Comment 17

[Chris Waterson]

I've looked into this a bit, and the problem appears to be that we're
determining that there's enough space to flow the line _before_ applying the
left margin. Specifically, we're not translating the space manager's coordinate
system, so the space available for the <p> is computed as if there were no
margin. I'm guessing that the margin is computed and applied later when the
max-width constraint is applied, but I need to understand this more thoroughly...

Comment 18

[Chris Waterson]

Attachment: fix?

We're not recomputing the block's side margins in the case where our width is
automatic, but constrained by max-width. I think we need to do this, but I'm
not terribly familiar with this code.

Comment 19

[Christian Reis]

Can we get review moving on this now that we have a patch (thanks Waterson)? 

Comment 20

[Kevin McCluskey (gone)]

Reassigning to Alex.

Comment 21

[Marc Attinasi]

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

Comment 22

[karnaze (gone)]

topembed+, nsbeta1+, m1.0

Comment 23

[Alexandru Savulov]

I will analyse/review the patch Waterson prepared, so if everything is ok and i
can get another review and a super review we can check it in. Otherwise we
shouldn't bother to resolve this soon.

Opera and IE act different with this test case. Seems to me that IE does not
even work with the max-width property. If we do something here, than we should
only prevent the text from overlapping. Trying to emulate here IE would not make
any sense. I also doubt that the authors will use features that do not work on IE.

Comment 24

[Karl Ove Hufthammer]

> Seems to me that IE does not
> even work with the max-width property.

FYI: This is correct. IE doesn't, and has never supported 'max-width', unfortunately.

Comment 25

[Kevin McCluskey (gone)]

Marking nsbeta1-, topembed- since Engineers are overloaded with higher priority
bugs and there isn't likely to be much content which makes use of the max-width
property since IE doesn't support it. Pushing to future-P1. We will pull this
bug back after we work through the other bugs.

Comment 26

[Alexandru Savulov]

Attachment: another testcase (resize the window to see the effect)

Comment 27

[Syd Logan]

adt agrees nsbeta1-

Comment 28

[Chris Petersen]

Still reproduces in the OS X 2002-07-31-05 NB.

Comment 29

[Karl Ove Hufthammer]

I can confirm that this bug is still present in Moz 1.2.1, using
http://bugzilla.mozilla.org/attachment.cgi?id=16455&action=view.

Comment 30

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

->Layout:Floats

Comment 31

[Robin Lionheart]

Still present in 1.3. The body text overlaps the floating menu of the front page
of my test suite at http://www.robinlionheart.com/stds/html4/.

Comment 32

[Kob]

Is this the same bug ?  Both menu at left and floaters (?) at right overlap text
at http://research.microsoft.com/projects/ilx/fsharp.htm
Seen with 
1. Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:1.4b) Gecko/20030516 Mozilla
Firebird/0.6
2. Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:1.3) Gecko/20030312

Comment 33

[James Kilfiger]

I've browsing for a few weeks with "p {max-width: 50em}" in userContent.css, and
this bug does appear in many pages, including the firebird home-page. Waterson's
patch makes sence to me, a good patch for alpha? => nominating. 

Comment 34

[James Kilfiger]

There is an unoffical build by scragz, including the patch to this bug at
http://forums.mozillazine.org/viewtopic.php?t=33958

Comment 35

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

Comment on attachment 67208 [details] [diff] [review]
fix?

This patch looks ok to me, but I think we might want to move this out a level
or two of braces (although I'm not sure)

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 67208 [details] [diff] [review]
fix?

sr=bzbarsky

Hoisting this out would indeed make the code clearer and less prone to errors
like that, I think.

David, I assume you will be checking this in?

Comment 37

[Radosław Szkodziński]

Attachment: Screenshot of the bug

The patch doesn't still fix the bug on this page:
http://www.mozilla.org/products/firebird/

The testcases look properly though.

I'm using Nova build from 2003-11-24 with the patch in.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6b) Gecko/20031124
Firebird/0.7+ (Nova: MNG,DOMi)

Link: http://forums.mozillazine.org/viewtopic.php?p=274854

Comment 38

[Boris Zbarsky [:bzbarsky]]

David, would you say this is worth trying for 1.6b?

Comment 39

[Radosław Szkodziński]

Disregard the last comment. It seems Nova didn't have the patch. :(
Sorry for spam.

Comment 40

[Mike Tigas]

AstralStorm, I did have the patch.  When I was answering your question on the
MozillaZine thread, we were both having a bit of miscommunication.  Your comment
#37 is still valid, as far as I know.

But in regard to that Firebird page:  I have snippets of importance from the
HTML and CSS of that page and the Mozilla.org style sheet quoted here:

http://forums.mozillazine.org/viewtopic.php?p=274916#274916

>The DD element on that page is set to "nowrap" so it shouldn't
>wrap the text.  Isn't it?

Am I correct in guessing that that's why the Firebird product page displays that
behavior?

I'll re-iterate that my build _does_ have the patch (attachment 67208 [details] [diff] [review]).  The
testcases display fixed behavior on my build.

Comment 41

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

Actually, moving it out a level or two wouldn't change anything since
CalculateBlockSideMargins would return immediately in all the other cases.

Comment 42

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

comment 37, comment 39, and comment 40 have nothing to do with *this* bug. 
Please discuss that issue in bug 225645.  It's a bug in the site, not a bug in
the browser.

Comment 43

[Mike Tigas]

I'd been using the patch (attachment #3 [details] [diff] [review]) for this in my builds for a while.

The fix for bug 227819 with the change to
layout/html/base/src/nsHTMLReflowState.cpp conflicts with the fix for this bug,
as far as I can tell.

It's the change around line 1971:

http://tinderbox.mozilla.org/bonsai/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/layout/html/base/src&command=DIFF_FRAMESET&file=nsHTMLReflowState.cpp&rev1=1.186&rev2=1.187&root=/cvsroot

I removed the patch for this bug to fix bustage.  Just wanted to bring this up, FYI.

Comment 44

[Boris Zbarsky [:bzbarsky]]

To David to decide whether he wants to just land that as-is.

Comment 45

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

Fix checked in to trunk, 2004-01-17 22:06 -0800.

Comment 46

[Martijn Wargers (dead)]

Could this have caused bug 244932? Backing out the patch for this bug makes the
testcase for bug 244932 work.

Comment 47

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

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