Closed Bug 292690 Opened 20 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
(deleted), text/html
Details
Different Testcase
(deleted), text/html
Details
Testcase without xbl
(deleted), application/xhtml+xml
Details
fix
(deleted), patch
dbaron
: review+
dbaron
: superreview+
shaver
: approval1.8b2+
Details | Diff | Splinter Review
quirk IE mew expansion
(deleted), text/html
Details
Marquee in <TD> Table Tag Causes Window Overflow
(deleted), text/html
Details
Patch
(deleted), patch
bernd_mozilla
: review+
bzbarsky
: superreview+
asa
: approval1.8b2-
shaver
: approval1.8b3+
Details | Diff | Splinter Review
reflow log
(deleted), text/plain
Details
revised patch
(deleted), patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
testcase with auto width table
(deleted), text/html
Details
testcase with small fixed width table
(deleted), text/html
Details
revised patch with shorter lines
(deleted), patch
chofmann
: approval1.8b2+
Details | Diff | Splinter Review
testcase for large MEW with xul:hbox
(deleted), text/xml
Details
E-mail showing widening of window, saved as mail file.
(deleted), message/rfc822
Details
E-mail showing widening of window, saved as mail file.
(deleted), message/rfc822
Details
E-mail showing widening of window, saved as HTML file
(deleted), text/html
Details
E-mail showing widening of window, saved as HTML file
(deleted), text/html
Details
E-mail showing widening of window, saved as HTML file
(deleted), text/html
Details
E-mail showing widening of window, saved as HTML file
(deleted), text/html
Details
testcase9
(deleted), 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) (deleted) — — 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 (deleted) —
Attachment #182545 - Attachment is obsolete: true
*** Bug 293316 has been marked as a duplicate of this bug. ***
Attached file Different Testcase (deleted) —
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 (deleted) —
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 (deleted) — — 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 (deleted) —
>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 (deleted) — — 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 (deleted) —
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) (deleted) — — 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 (deleted) — — 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?
Attached file testcase with auto width table (deleted) —
this testcase tests for pct margins on elements within a auto table, this requires that the element reports the correct maximumWidth as desiredWidth
Attached file testcase with small fixed width table (deleted) —
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) (deleted) —
Attached file testcase for large MEW with xul:hbox (deleted) —
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 (deleted) —
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: