<marquee> regression causes unwanted horizontal page-widening

VERIFIED FIXED

Status

()

Core
Layout
VERIFIED FIXED
13 years ago
12 years ago

People

(Reporter: Mathieu Pellerin, Assigned: roc)

Tracking

({regression, testcase})

Trunk
x86
Windows XP
regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b2 -
blocking1.8b3 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(20 attachments, 3 obsolete attachments)

422 bytes, text/html
Details
251 bytes, text/html
Details
604 bytes, application/xhtml+xml
Details
fix
1.63 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
226 bytes, text/html
Details
167 bytes, text/html
Details
1.92 KB, patch
Bernd
: review+
Details | Diff | Splinter Review
4.54 KB, text/plain
Details
2.54 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
7.61 KB, text/html
Details
7.63 KB, text/html
Details
1.44 KB, patch
chris hofmann
: approval1.8b2+
Details | Diff | Splinter Review
1.03 KB, text/xml
Details
22.81 KB, message/rfc822
Details
22.81 KB, message/rfc822
Details
15.86 KB, text/html
Details
15.86 KB, text/html
Details
15.86 KB, text/html
Details
15.86 KB, text/html
Details
12.93 KB, application/xhtml+xml
Details
(Reporter)

Description

13 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050502 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050502 Firefox/1.0+

with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050502
Firefox/1.0+

Reproducible: Always

Steps to Reproduce:
1. Open http://www.dac.org.kh/


Actual Results:  
Notice the page width goes way of the window width

Expected Results:  
Page should be contained within the window width

Comment 1

13 years ago
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050501

JS console is showing a CSS error:

Error: Error in parsing value for property 'clip'.  Declaration dropped.
Source File: http://www.dac.org.kh/
Line: 12

#logo {  height: 158px; width: 120px; position: absolute; z-index: 1; left: 9px;
top: 13px ; visibility: visible; clip:  rect(   )}

If you look at it in DOM inspector, you see a <div> and a <table> in <body>.
After deleting the 4th <TR> in <table> the width is normal.

Comment 2

13 years ago
I see the JS error in console, but the page width lays out OK and looks fine with
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050428

Comment 3

13 years ago
related to bug 292552 / bug 292370 ? 
Looks fine in Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2)
Gecko/20050422 Firefox/1.0+
Looks all wrong in Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2)
Gecko/20050502 Firefox/1.0+

Comment 3 is probably right - we'll see when its patch lands :)

Comment 5

13 years ago
wfm Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050428 Firefox/1.0+
though CSS error seen in JS console

regressed Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050429
Firefox/1.0+

same day like regression of 
Bug 292727	Page formatting goes extremely wide 
Bug 292552	tables lay out too wide
This problem is still present even after bug 292370 has landed. I'll try and
sort a testcase within 24 hrs.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Or maybe to do with bug 47710
Created attachment 182545 [details] [diff] [review]
testcase

As per comment 3 - Regressed between 20050428 (worked) & 20050429 (broken)

1. Load testcase in firefox 20050428
2. Observe the yellow bar is as wide as the browser window, and the text starts
to scroll from the right hand edge. Note no horizontal scrollbar.
3. Load testcase in firefox 20050429
4. Observe the yellow bar is as wide as the resolution of your screen,
regardless of the actual width of the browser window. Also notice that to the
right of the yellow bar, there is an area of deadspace that looks to be as wide
as the resolution of the screen again. Note horizontal scrollbar.

I've reduced the testcase as much as I can, but I can't get it to validate
properly because i don't know what doctype to use; this is due to the
mis-rendering code being <marquee>

I note from http://www.scit.wlv.ac.uk/encyc/marquee.html that the <marquee>
"...tag is not part of HTML 3.2 and is only supported by Microsoft Internet
Explorer..." so I don't know whether the fact that this has broken recently is
of any consequence.

Updated

13 years ago
Keywords: testcase

Updated

13 years ago
Summary: page width goes way beyond the window size for no reason → <marquee> regression causes unwanted horizontal page-widening
Created attachment 182546 [details]
testcase that works
Attachment #182545 - Attachment is obsolete: true

Comment 10

13 years ago
*** Bug 293316 has been marked as a duplicate of this bug. ***

Comment 11

13 years ago
Created attachment 182982 [details]
Different Testcase

Here is a slightly different testcase, involving an image and a marquee in a
table.	If you remove the image, the width is normal, and if you remove the
table tags the width is normal.  I've simplified this as much as possible; it
only happens as is.

Comment 12

13 years ago
Comment on attachment 182982 [details]
Different Testcase

Note that with this testcase, you sometimes have to reload the page once to
make it get REALLY wide.  Otherwise, for some reason, it may not render as wide
on the initial load...
What I really need is someone to boil down the MARQUEE XBL binding and reduce
this to plain HTML/CSS/JS.
Created attachment 183365 [details]
Testcase without xbl

This works in 2005-04-28 build, but fails (horizontal scrollbar) in 2005-04-29
build. So likely a fall-out from bug 240276.

Updated

13 years ago
Blocks: 240276

Comment 15

13 years ago
*** Bug 293893 has been marked as a duplicate of this bug. ***
*** Bug 294035 has been marked as a duplicate of this bug. ***

Comment 17

13 years ago
*** Bug 294264 has been marked as a duplicate of this bug. ***
Created attachment 183687 [details] [diff] [review]
fix

This fixes the regression. The overflow:hidden DIV now preserves the MEW of its
contents --- as I believe it should. But then this rather dubious nsBlockFrame
code kicks in and blows the TD block's width out to the MEW. Removing that
fixes the problem. The question is, of course, what else might break.

Here's where that code was added:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/layout/generic&command=DIFF_FRAMESET&file=nsBlockFrame.cpp&rev2=3.218&rev1=3.217

Unfortunately the checkin comment isn't very helpful.
Assignee: nobody → roc
Status: NEW → ASSIGNED
Attachment #183687 - Flags: superreview?(dbaron)
Attachment #183687 - Flags: review?(dbaron)
Attachment #183687 - Flags: superreview?(dbaron)
Attachment #183687 - Flags: superreview+
Attachment #183687 - Flags: review?(dbaron)
Attachment #183687 - Flags: review+
Comment on attachment 183687 [details] [diff] [review]
fix

fixed gmail layout regression
Attachment #183687 - Flags: approval1.8b2?
forget what I said about gmail. But it still fixes a layout regression.
Comment on attachment 183687 [details] [diff] [review]
fix

a=shaver.  Let's find out what it might break in 1.8b2, and not later!
Attachment #183687 - Flags: approval1.8b2? → approval1.8b2+
I tried the fix in my debug build, but the url testcase and the "testcase that
works" still show the bug. "Different Testcase" and "Testcase without xbl" seem
fixed, though. 
I just checked that in before I saw Martijn's comment. I'll leave this open for
further investigation.
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050516
Firefox/1.0+ ID:2005051613 (after landing of attachment 183687 [details] [diff] [review])
For "testcase that works" the marquee region is still too wide, but the
enourmous dead whitespace that was to the right hand side of the yellow bar has
now gone.
Looks like MARQUEE wants overflow:(not-visible) to crop its MEW (i.e., the
min-intrinsic-size) to the available width. I think we used to do that and now
we don't. Should we do that or not?
So what happens for an overflow-hidden inline-block, generally?  Does it always
fit on the line it's on right now?  Or can it get wrapped to the next line and
widen as needed?  I guess that's the preferred width, not the minimum width...
Constraining overflow:hidden MEW to the available width doesn't even fix the
original Cambodia page, because in that page the bottom MARQUEE text is still
really wide; its min-intrinsic-width is the width of all the text (because
MARQUEE uses whitespace:no-wrap) and the table code just believes that.

So beyond the fixes to MEW calculation here, we also need to fix MARQUEE. It
really should set its MEW to zero.
(In reply to comment #25)
Upon reflection, I would argue strenuously that overflow:whatever should not
affect the min-intrinsic-size ... on the grounds that overflow:hidden should
simply have no effect on elements that don't have overflow.
I'm trying to think of a way to wrap a container element to have a
min-intrinsic-width of zero without affecting its desired width, using existing
elements and CSS, and I can't.

Comment 30

13 years ago
Created attachment 183824 [details]
quirk IE mew expansion

>Unfortunately the checkin comment isn't very helpful.

This code pattern is one incarnation of the unwritten rule that the MEW is the
lower bound of a thing with a specified width. While this is still true for
tables I learned via several clashes with David that this is not the case for
blocks. (OT: I like the table layout model more :-))
While this rule does not  apply to blocks in mozilla, it does apply in IE. IE
happily wraps the mew and prevents overflow. So I guess you will need to quirk
this thing. I wonder however how this did not appear before.
bernd, you mean you want the fix that we checked in reverted for quirks mode?

Are there actual web pages that are broken by the fix?

Comment 32

13 years ago
Well the major layout breakage seems fixed but the width/overflow? issue doesn't
seem fixed. As I see by looking at bugged URL and at some of the testcases.

I notice there are a lot of things that aren't currently supported by the
current MARQUEE implementation in Firefox. If they will be supported or not, I
dunno.

I found that looking at http://www.htmlcodetutorial.com/_MARQUEE.html as a good
place for various testcases.

I found out that the LOOP tag is not honored at all. Sorry if this is a bug spam
in this bug report but I couldn't find any active bugs besides this one that
people are reading currently.
This particular code was simply wrong -- asking for the max-element-width should
never *change* the resulting width.

Comment 34

13 years ago
Created attachment 183861 [details]
Marquee in <TD> Table Tag Causes Window Overflow

I was messing around with the site I have this overflow problem with.
The testcase is not in XBL or anything because I do not know the language
(sorry)
But I do know that the overflow seems to be cause by using the MARQUEE tag
inside of a <TD></TD> element with using a Large Set of Characters.

I removed everything "fancy" and left just the basic tags needed to cause the
bug.
I hope this "helps" in some way.

Comment 35

13 years ago
s/So I guess you will need to quirk this thing/I am afraid..../
I am not arguing that this not wrong, I just wanted to point that there was
previously a believ among nscp engineers with respect to shrinking below MEW.
Robert just wait whether this will create regression bugs or not, if not then
removing this is just fine.
Created attachment 183958 [details] [diff] [review]
Patch

This patch makes it work for me. I've tested it on the testcases and the url
here and on quite some other url's with marquees in them.
I don't really know exactly why this works, though (width:0 on a xul:hbox).
So why doesn't the explicit width that's set from the presence or absence of the
width attribute also override the MEW?

Comment 38

13 years ago
Created attachment 183983 [details]
reflow log

This is a reflow problem,  it seems to be a issue outside XUL

on initial reflow we have
	   block 048FF2EC r=0 a=UC,UC c=UC,UC cnt=2420
	    scroll 048FF418 r=0 a=UC,UC c=UC,UC cnt=2421
	     area 048FF560 r=0 a=UC,UC c=UC,UC cnt=2422
	  
	     area 048FF560 d=6840,240 me=6840
	    scroll 048FF418 d=6840,240 me=6840
	   block 048FF2EC d=6840,240 me=6840

on the resize reflow we get then

	   block 048FF2EC r=2 a=6840,UC c=6840,UC cnt=2446
	    scroll 048FF418 r=2 a=6840,UC c=6840,UC cnt=2447
	     area 048FF560 r=2 a=6840,UC c=6840,UC cnt=2448

	     area 048FF560 d=20520,240 me=20520
	    scroll 048FF418 d=6840,240 me=20520
	   block 048FF2EC d=20520,240 me=20520
Could the first patch in this bug have triggered these assertions I see when
loading this bug page?

  ###!!! ASSERTION: max element width exceeded desired width: 'PR_FALSE', file
  ../../../mozilla/layout/tables/nsTableRowFrame.cpp, line 971
Yes, I've tested that with my own debug build by applying and backing out that
patch. I have a (not yet finished, though) testcase for that.
I filed bug 294823 for this.

Comment 42

13 years ago
It seems to me that percentage margins on the xul box (the 100% left and right
are added to the MEW when the block is reflown with a contrained width. This
would explain the trippling 6840 * 3 = 20520
But why doesn't the explicit width on the container outside of that override
that MEW?

Comment 44

13 years ago
In other words linelayout should not add unconditional pct margins to the MEW
http://lxr.mozilla.org/seamonkey/source/layout/generic/nsLineLayout.cpp#1712

Comment 45

13 years ago
Created attachment 184031 [details] [diff] [review]
patch

this removes the overflow in attachment 183861 [details]
Comment on attachment 184031 [details] [diff] [review]
patch

You should check that it's not Auto-based as well.  The checks in other places
are just checking that the unit is Coord.

I think this is correct, but I don't think it's the only problem in this bug.
(other places == nsBlockReflowContext::PlaceBlock, at least)
Actually, after looking at the testcase again, maybe this is the only problem.

Comment 49

13 years ago
Created attachment 184069 [details] [diff] [review]
revised patch
Attachment #184031 - Attachment is obsolete: true

Updated

13 years ago
Attachment #184069 - Flags: superreview?(dbaron)
Attachment #184069 - Flags: review?(dbaron)
Comment on attachment 184069 [details] [diff] [review]
revised patch

r+sr=dbaron if:
 * you wrap the lines at under 80 characters
 * you test margins on :first-letter (non-floating)
Attachment #184069 - Flags: superreview?(dbaron)
Attachment #184069 - Flags: superreview+
Attachment #184069 - Flags: review?(dbaron)
Attachment #184069 - Flags: review+

Comment 51

13 years ago
>* you test margins on :first-letter (non-floating)

David, sorry but I don't know line layout good enough to understand what you
mean with this.
Just try some testcases with style rules like:

p:first-letter { margin: 0 5%; }

and make sure they don't crash and do reasonable things (or at least not worse
than what they did before).

Comment 53

13 years ago
request for blocking for 1.1a/1.8b2 -- this is one of the few remaining
regressions that have generated large interest (votes)
Flags: blocking1.8b2?

Comment 54

13 years ago
Created attachment 184185 [details]
testcase with auto width table

this testcase tests for pct margins on elements within a auto table, this
requires that the element reports the correct maximumWidth as desiredWidth

Comment 55

13 years ago
Created attachment 184186 [details]
testcase with small fixed width table

this testcase verifies that the inner element with pct margin is correctly
handled under size constrained conditions

Comment 56

13 years ago
Created attachment 184226 [details] [diff] [review]
revised patch with shorter lines

This patch has the lines broken as required by r/sr
I rtest'ed the patch without any indication of regression and tested with the
scenario that David asked for. That are the two testcases. As one can see in
general the pct margins are pretty broken when it comes to maximumWidth and MEW
computation. So this bug should be no surprise but is rather what you get when
the foundations are broken. There is slight change for the first letter case
but we change only from one broken behaviour to the next broken. It does not
crash.
In other words this fix is only a first step what needs to be fixed but it
helps to make marquees usable again in tables.

Updated

13 years ago
Attachment #184226 - Flags: approval1.8b2?

Comment 57

13 years ago
Comment on attachment 184226 [details] [diff] [review]
revised patch with shorter lines

a=chofmann
Attachment #184226 - Flags: approval1.8b2? → approval1.8b2+

Comment 58

13 years ago
I just downloaded the latest hourly, and it seems to work correctly on all the
testcases here (i.e. the page doesn't seem to be too wide on any of them). 
However, the layout on 1up.com is still messed up.  I had filed bug 294264 for
that, but it got marked as a dupe of this bug.

Does that mean 1up.com is a different issue (or at least a different case of the
same issue)?  There is a testcase in bug 294264 that also still fails.

Comment 59

13 years ago
Build 20050521 also doesn't keep text within bounds on my originally reported
website: http://houseofstrauss.co.uk/index.php

Comment 60

13 years ago
Created attachment 184293 [details]
testcase for large MEW with xul:hbox

Comment 61

13 years ago
Created attachment 184294 [details]
testcase for large MEW with xul:hbox
Attachment #184293 - Attachment is obsolete: true

Comment 62

13 years ago
Comment on attachment 183958 [details] [diff] [review]
Patch

The reminder in this bug will be fixed by this patch. The core point is the
width:0 on the xul:hbox. It seems that a auto xul:hbox gives its children as
much width as they need to layout without wrapping and then reports this width
back as MEW.
Attachment #183958 - Flags: superreview?(dbaron)
Attachment #183958 - Flags: review+
Comment on attachment 183958 [details] [diff] [review]
Patch

change looks fine btw.

Comment 64

13 years ago
Created attachment 184408 [details]
E-mail showing widening of window, saved as mail file.

Comment 65

13 years ago
Created attachment 184409 [details]
E-mail showing widening of window, saved as mail file.

Comment 66

13 years ago
Created attachment 184410 [details]
E-mail showing widening of window, saved as HTML file

Comment 67

13 years ago
Created attachment 184411 [details]
E-mail showing widening of window, saved as HTML file

The list above describes it as a mail file, but it is actually an HTML one.

Comment 68

13 years ago
Created attachment 184412 [details]
E-mail showing widening of window, saved as HTML file

The list above describes the second e-mail attachment as a mail file, but it is
actually an HTML one.

Comment 69

13 years ago
Created attachment 184413 [details]
E-mail showing widening of window, saved as HTML file

The list above describes the second e-mail attachment as a mail file, but it is
actually an HTML one.
Comment on attachment 183958 [details] [diff] [review]
Patch

sr=bzbarsky to fix this regression up...

The answer to David's question is that the MEW calculation in nsBoxFrame is a
little wacky.  uses the minSize only if the CSS computed width is
NS_INTRINSICSIZE; otherwise it uses the mRect.width.  Now the minSize is not so
minimal in this case, and XUL attributes are not mapped into style (so if there
is no style width set, the CSS computed width is intrinsicsize)...
Attachment #183958 - Flags: superreview?(dbaron) → superreview+
Comment on attachment 183958 [details] [diff] [review]
Patch

Requesting 1.8b2 approval for this layout regression when marquees are around.
Attachment #183958 - Flags: approval1.8b2?
Created attachment 184443 [details]
testcase9

I'm afraid my patch (https://bugzilla.mozilla.org/attachment.cgi?id=183958)
doesn't work anymore (you can see it with the testcase which is basically a
patched marquee with the testcase based from the url).
did work in 2005-05-22 build, doesn't work in 2005-05-23 build. It seems
Bernd's patch (https://bugzilla.mozilla.org/attachment.cgi?id=184226) caused
the changed behavior (I checked it by patching/unpatching in my own debug
build).
So I guess the marquee code needs some different rewriting?

Comment 73

13 years ago
Thanks Martijn.

OK, this looks like marquee relied on /a/a couple of/ bug(s) on the interface
between xul and normal layout. So I believe one goal should be to remove the
xul:hbox from the xbl binding at all and make it a pure  xhtml binding. 

Comment 74

13 years ago
(In reply to comment #73)

You can also find the bug verified on the following page
http://www.proofboard.com/en/index.html

If you go to another page of this website (Boards) using the same design, aou
can see that the width varies.
While trying to make a marquee binding without the xul:hbox, I stumbled upon bug
295459.

Comment 76

13 years ago
from http://msdn.microsoft.com/workshop/author/dhtml/reference/objects/marquee.asp

    The default width of the MARQUEE element is equal to the width of its parent
element. When a MARQUEE is in a TD that does not specify a width, you should
explicitly set the width of MARQUEE. If neither the MARQUEE nor the TD has a
width specified, the marquee is collapsed to a 1-pixel width.

    To create a vertically scrolling marquee, set its scrollLeft property to 0.
To create a horizontally scrolling marquee, set its scrollTop property to 0,
overriding any script setting.

    The scrollLeft and scrollTop properties are read-only while the marquee is
scrolling. When not scrolling, scrollLeft is read/write for a marquee set to
scroll horizontally and scrollTop is read/write for a marquee set to scroll
vertically.

    This element is available in HTML as of Internet Explorer 3.0, and in script
as of Internet Explorer 4.0.

    This element is a block element.

    This element requires a closing tag.

Comment 77

13 years ago
Rule 1:RTFM
Rule 2: Never trust the manual, especially if it comes from MS
a width on a TD or *a TABLE* is enough to prevent the collapsing.

Comment 78

13 years ago
the patch in bug 295459 fixes comment 74, http://www.dac.org.kh/, takes into
account comment 43 and  fixes comment 58.

And yaeah, Did I mention that working with Martijn is fun?
Depends on: 295459

Comment 79

13 years ago
Comment on attachment 183958 [details] [diff] [review]
Patch

moving request out to b3. we're wrapped on b2.
Attachment #183958 - Flags: approval1.8b3?
Attachment #183958 - Flags: approval1.8b2?
Attachment #183958 - Flags: approval1.8b2-

Comment 80

13 years ago
b2 -> b3
Flags: blocking1.8b3+
Flags: blocking1.8b2?
Flags: blocking1.8b2-
Comment on attachment 183958 [details] [diff] [review]
Patch

a=shaver
Attachment #183958 - Flags: approval1.8b3? → approval1.8b3+

Comment 82

13 years ago
attachment 183958 [details] [diff] [review] should not be checked in due to comment 72. One fix is in bug
295459.

Comment 83

13 years ago
fixed by checkin for bug 295459.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Updated

13 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.