[FLOAT] Floating text overlapping max-width elements

RESOLVED FIXED in Future

Status

()

Core
Layout: Floats
P2
major
RESOLVED FIXED
17 years ago
9 years ago

People

(Reporter: Karl Ove Hufthammer, Assigned: dbaron)

Tracking

(Blocks: 1 bug, {css2, testcase, topembed-})

Trunk
Future
css2, testcase, topembed-
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.6a -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Hixie-P4] (py8ieh:examine) potential fix; needs careful review)

Attachments

(5 attachments)

(Reporter)

Description

17 years ago
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.
(Assignee)

Comment 1

17 years ago
Layout, not Style System.
Assignee: pierre → clayton
Component: Style System → Layout
QA Contact: chrisd → petersen

Comment 2

17 years ago
This is not a blocker. It does not block development or testing.
Lowering severity.
Severity: blocker → major

Comment 3

17 years ago
I can see this issue with the Sept 29th build. Marking new.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Updated

17 years ago
Keywords: mozilla1.0

Comment 4

17 years ago
a reduced test case would go a long way towards getting this examined before NS6
RTM.  Setting to P2.
Assignee: clayton → buster
Priority: P3 → P2

Comment 5

17 years ago
Created attachment 16455 [details]
testcase

Comment 6

17 years ago
P2 bugs -> future
Status: NEW → ASSIGNED
Target Milestone: --- → Future
Keywords: testcase
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.
(Reporter)

Updated

16 years ago
Keywords: mozilla1.0 → correctness, mozilla0.9, nsbeta1
(Reporter)

Updated

16 years ago
Keywords: css2
(Reporter)

Updated

16 years ago
Summary: Floating text overlapping max-width elements → [FLOAT] Floating text overlapping max-width elements
Blocks: 78094

Updated

16 years ago
No longer blocks: 78094
Blocks: 78094
Whiteboard: [Hixie-P4] (py8ieh:examine)

Comment 8

16 years ago
Created attachment 43771 [details]
opera screenshot

Comment 9

16 years ago
taking.
Assignee: buster → waterson
Status: ASSIGNED → NEW
Target Milestone: Future → mozilla1.0

Comment 10

16 years ago
Also see the test case for bug 69915.

Updated

16 years ago
Status: NEW → ASSIGNED

Updated

16 years ago
Blocks: 104166

Comment 11

16 years ago
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.
Assignee: waterson → attinasi
Status: ASSIGNED → NEW
Keywords: topembed
Target Milestone: mozilla1.0 → mozilla0.9.6
(Assignee)

Comment 12

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

Updated

16 years ago
Assignee: attinasi → waterson
Keywords: topembed
Target Milestone: mozilla0.9.6 → mozilla1.0

Comment 13

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

16 years ago
re-accepting for waterson (I mistakenly took it, now giving it back).
Status: NEW → ASSIGNED

Updated

16 years ago
Keywords: edt0.9.4, topembed

Updated

16 years ago
Keywords: edt0.9.4, topembed → edt0.9.4-

Comment 15

16 years ago
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)
Target Milestone: mozilla1.0 → mozilla1.0.1

Comment 16

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

Target Milestone: mozilla1.0.1 → mozilla1.0

Updated

16 years ago
Target Milestone: mozilla1.0 → Future

Updated

16 years ago
Target Milestone: Future → mozilla0.9.9

Comment 17

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

16 years ago
Created attachment 67208 [details] [diff] [review]
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.

Updated

16 years ago
Keywords: review
Whiteboard: [Hixie-P4] (py8ieh:examine) → [Hixie-P4] (py8ieh:examine) potential fix; needs careful review

Updated

16 years ago
Keywords: topembed

Comment 19

16 years ago
Can we get review moving on this now that we have a patch (thanks Waterson)? 
Keywords: edt0.9.4- → patch
Reassigning to Alex.
Assignee: waterson → alexsavulov
Status: ASSIGNED → NEW
Priority: P2 → P1
Target Milestone: mozilla0.9.9 → Future

Updated

16 years ago
Keywords: mozilla1.0+

Comment 21

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

Comment 22

16 years ago
topembed+, nsbeta1+, m1.0
Keywords: topembed → nsbeta1+, topembed+
Target Milestone: Future → mozilla1.0

Comment 23

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

(Reporter)

Comment 24

16 years ago
> 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.
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.
Keywords: nsbeta1+, topembed+ → nsbeta1-, topembed-
Target Milestone: mozilla1.0 → Future

Comment 26

16 years ago
Created attachment 75260 [details]
another testcase (resize the window to see the effect)

Comment 27

16 years ago
adt agrees nsbeta1-

Comment 28

15 years ago
Still reproduces in the OS X 2002-07-31-05 NB.
(Reporter)

Updated

15 years ago
(Reporter)

Comment 29

15 years ago
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.
(Assignee)

Comment 30

14 years ago
->Layout:Floats
Assignee: alexsavulov → float
Component: Layout → Layout: Floats
QA Contact: petersen → ian
Target Milestone: Future → ---

Updated

14 years ago
Priority: P1 → P2
Target Milestone: --- → Future

Comment 31

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

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

14 years ago
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. 
Flags: blocking1.6a?

Updated

14 years ago
Flags: blocking1.6a? → blocking1.6a-

Comment 34

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

Comment 35

14 years ago
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)
Attachment #67208 - Flags: superreview?(bz-vacation)
Attachment #67208 - Flags: review+
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?
Attachment #67208 - Flags: superreview?(bz-vacation) → superreview+
Created attachment 136247 [details]
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
David, would you say this is worth trying for 1.6b?
Disregard the last comment. It seems Nova didn't have the patch. :(
Sorry for spam.

Comment 40

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

Comment 41

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

Comment 42

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

14 years ago
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.
To David to decide whether he wants to just land that as-is.
Assignee: core.layout.floats → dbaron
(Assignee)

Comment 45

14 years ago
Fix checked in to trunk, 2004-01-17 22:06 -0800.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Could this have caused bug 244932? Backing out the patch for this bug makes the
testcase for bug 244932 work.
(Assignee)

Comment 47

13 years ago
*** Bug 249774 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.