Closed Bug 292690 Opened 19 years ago Closed 19 years ago

<marquee> regression causes unwanted horizontal page-widening

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: nirvn.asia, Assigned: roc)

References

(
URL
)

Details

(Keywords: regression, testcase)

Attachments

(20 files, 3 obsolete files)

testcase that works
422 bytes, text/html
Details
Different Testcase
251 bytes, text/html
Details
Testcase without xbl
604 bytes, application/xhtml+xml
Details
fix
1.63 KB, patch
dbaron
: review+
dbaron
: superreview+
shaver
: approval1.8b2+
Details | Diff | Splinter Review
quirk IE mew expansion
226 bytes, text/html
Details
Marquee in <TD> Table Tag Causes Window Overflow
167 bytes, text/html
Details
Patch
1.92 KB, patch
bernd_mozilla
: review+
bzbarsky
: superreview+
asa
: approval1.8b2-
shaver
: approval1.8b3+
Details | Diff | Splinter Review
reflow log
4.54 KB, text/plain
Details
revised patch
2.54 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
testcase with auto width table
7.61 KB, text/html
Details
testcase with small fixed width table
7.63 KB, text/html
Details
revised patch with shorter lines
1.44 KB, patch
chofmann
: approval1.8b2+
Details | Diff | Splinter Review
testcase for large MEW with xul:hbox
1.03 KB, text/xml
Details
E-mail showing widening of window, saved as mail file.
22.81 KB, message/rfc822
Details
E-mail showing widening of window, saved as mail file.
22.81 KB, message/rfc822
Details
E-mail showing widening of window, saved as HTML file
15.86 KB, text/html
Details
E-mail showing widening of window, saved as HTML file
15.86 KB, text/html
Details
E-mail showing widening of window, saved as HTML file
15.86 KB, text/html
Details
E-mail showing widening of window, saved as HTML file
15.86 KB, text/html
Details
testcase9
12.93 KB, application/xhtml+xml
Details 
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
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.
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
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 :)
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
Attached patch testcase (obsolete) — — Splinter Review 
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.
Keywords: testcase
Summary: page width goes way beyond the window size for no reason → <marquee> regression causes unwanted horizontal page-widening
Attached file testcase that works — 

        Attachment #182545 -
        Attachment is obsolete: true

    
    

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

    
    

    
  

      
      
      
      

        Attached file
          
        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 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.

    
    

    
  

      
      
      
      

        Attached file
          
        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.
Blocks: 240276

    
    

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

    
    

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

    
    

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

    
    

    
  

      
      
      
      

        Attached patch
          
          
        fix
        — 
          — Splinter Review
      

    


  
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.

    
    

    
  

      
      
      
      

        Attached file
          
        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?

    
    

    
  
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.

    
    

    
  


  
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.

    
    

    
  
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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch
        — 
          — Splinter Review
      

    


  
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?

    
    

    
  

      
      
      
      

        Attached file
          
        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.

    
    

    
  
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?

    
    

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

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  
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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        revised patch
        — 
          — Splinter Review
      

    


  

        Attachment #184031 -
        Attachment is obsolete: true

    
  

        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+

    
    

    
  
>* 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).

    
    

    
  
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?

    
    

    
  


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

    
    

    
  


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

    
    

    
  


  
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.

    
  

        Attachment #184226 -
        Flags: approval1.8b2?

    
    

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

a=chofmann

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

    
    

    
  
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.

    
    

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

    
    

    
  

      
      
      
      

        Attached file
          
        testcase for large MEW with xul:hbox (obsolete)
        — 
      

    


  

    
    

    
  


  

        Attachment #184293 -
        Attachment is obsolete: true

    
    

    
  
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.

    
    

    
  


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

    
    

    
  


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

    
    

    
  


  
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?

    
    

    
  

      
      
      
      

        Attached file
          
        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?

    
    

    
  
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. 

    
    

    
  
(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.

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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 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-

    
    

    
  
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+

    
    

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

    
    

    
  
fixed by checkin for bug 295459.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: