Last Comment Bug 221784 - [FIX]On builds beginning with 20031009 vertical scroll bar is "too long" by appr. 4000%
: [FIX]On builds beginning with 20031009 vertical scroll bar is "too long" by a...
Status: RESOLVED FIXED
: regression, testcase
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86 All
: P1 minor (vote)
: mozilla1.6alpha
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
: Hixie (not reading bugmail)
Mentors:
http://www.xsibase.com/
: 222451 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2003-10-10 04:45 PDT by amano
Modified: 2003-10-17 17:43 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (far from minimal though) (41.96 KB, text/html)
2003-10-16 21:28 PDT, Mats Palmgren (:mats)
no flags Details
Minimal testcase, I think (130 bytes, text/html)
2003-10-16 21:43 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
Proposed patch (2.36 KB, patch)
2003-10-16 21:51 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Testcase #3 (155 bytes, text/html)
2003-10-17 09:50 PDT, Mats Palmgren (:mats)
no flags Details
Patch that does what it should (2.27 KB, patch)
2003-10-17 16:51 PDT, Boris Zbarsky [:bz] (still a bit busy)
roc: review+
roc: superreview+
Details | Diff | Splinter Review

Description amano 2003-10-10 04:45:56 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6a) Gecko/20031009 Firebird/0.7+ (aebrahim)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6a) Gecko/20031009 Firebird/0.7+ (aebrahim)

From Mozillazine Thread
http://forums.mozillazine.org/viewtopic.php?t=28590&postdays=0&postorder=asc&start=0
:
debris303 wrote:
on http://www.xsibase.com the menu bar on the left is stretched by ~4000% on the
y-axis. This happens on aebrahims 20031008 SSE build and on the 20031009 Mozilla ni.

I can confirm the bug. I think it's a bug, although the site is not valid html.
but before firebird's quirks mode did handle the code properly.

Reproducible: Always

Steps to Reproduce:
1.Go to site www.xsibase.com
2.Scroll Down
Note that the content ends, but you can scroll down a long empty space

Actual Results:  
Note that the content ends, but you can scroll down a long empty space


Expected Results:  
When the content ends, the vertiacal scrollbar should reach the bottom.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2003-10-10 09:55:18 PDT
Hmm.. This regressed as a result of the fix for bug 219693, I bet.  The site has
height:100% tables around that part of the woods.

If someone could create a minimal testcase, that would be much appreciated.
Comment 2 Mats Palmgren (:mats) 2003-10-16 20:41:32 PDT
*** Bug 222451 has been marked as a duplicate of this bug. ***
Comment 3 Mats Palmgren (:mats) 2003-10-16 21:28:22 PDT
Created attachment 133472 [details]
Testcase (far from minimal though)

Loading this in 2003-10-16-05 generates the content (see first textarea):
<div id="AWMEL0" ... style="... height: 12618px; ..." ...>

In Mozilla 1.5 I get:
<div ... style="... height: 376px; ..." id="AWMEL0">
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2003-10-16 21:43:15 PDT
Created attachment 133473 [details]
Minimal testcase, I think

The site's menu system does some funky stuff, but the heart of it is that one
table gets too big and the menu system scales everything else accordingly...
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2003-10-16 21:51:25 PDT
Created attachment 133475 [details] [diff] [review]
Proposed patch

This fixes the site, the "far from minimal" testcase, and the minimal-ish
testcase, as well as the URL in bug 222451.

The core issue is that with the change in bug 219693 we now examine the first
two ancestor block/area frames.  This is necessary because in cases when the
body has overflow set both the body and the html are area frames, and we want
to use the height of the html if the body has auto height.

Now a positioned div gets an _area_ frame, not a block frame.  So for a 100%
height table in a positioned div that has no positioned ancestors we used to
look at its area frame and then not look at the area frame of the <html>.  With
the patch in bug 219693 we suddenly set that 100% height table to the height of
the <html> frame.

This patch makes us stop going up the frame tree once we hit a positioned
(absolutely or fixed) ancestor frame.  I think that's a much more intuitive
behavior anyway, since I would not expect kids of a positioned frame to size
based on the containing block of the positioned frame...

I've run the regression tests and they pass (though I suspect that's just a
matter of us having no regression tests testing this.... ;) )
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2003-10-16 21:55:16 PDT
Taking.
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2003-10-16 21:55:46 PDT
Comment on attachment 133475 [details] [diff] [review]
Proposed patch

roc, can you review?  Or should I poke dbaron?
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2003-10-17 07:36:13 PDT
This code doesn't match the comment. If the frame is positioned then you always
break and you never use it as the percentage base.

Are you sure you want IsAbsolutelyPositioned() instead of just IsPositioned()?

Also,
> nsCOMPtr<nsIAtom> pseudo = 
Make this a regular ptr
Comment 9 Mats Palmgren (:mats) 2003-10-17 09:50:25 PDT
Created attachment 133508 [details]
Testcase #3

The suggested patch breaks this testcase, before: 100px after: auto.
Comment 10 Mats Palmgren (:mats) 2003-10-17 09:59:09 PDT
This works for me:

      if (NS_AUTOHEIGHT == rs->mComputedHeight) {
        if (rs->frame->GetStyleDisplay()->IsAbsolutelyPositioned()) {
          break;
        } else {
          continue;
        }
      }
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2003-10-17 16:51:32 PDT
Created attachment 133539 [details] [diff] [review]
Patch that does what it should

Indeed... Mats, thanks for the idea.
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2003-10-17 17:43:32 PDT
Patch and testcases all checked in.  

roc, I'm not in fact sure I want IsAbsolutelyPositioned() as opposed to
IsPositioned(), but I think that I want layout to not change if someone
relatively positions something without specifying an offset...

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