Closed Bug 221784 Opened 21 years ago Closed 21 years ago

[FIX]On builds beginning with 20031009 vertical scroll bar is "too long" by appr. 4000%

Categories

(Core :: Layout, defect, P1)

Product:
Core
Component:
Layout
Platform:
x86
All
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.6alpha

People

(Reporter: jyaku, Assigned: bzbarsky)

References

(
URL
)

Details

(Keywords: regression, testcase)

Attachments

(4 files, 1 obsolete file)

Testcase (far from minimal though)
41.96 KB, text/html
Details
Minimal testcase, I think
130 bytes, text/html
Details
Testcase #3
155 bytes, text/html
Details
Patch that does what it should
2.27 KB, patch
roc
: review+
roc
: superreview+
Details | Diff | Splinter Review 
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.
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.
Keywords: qawanted
Blocks: 222451
No longer blocks: 222451
*** Bug 222451 has been marked as a duplicate of this bug. ***
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">
OS: Windows XP → All
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...
Attachment #133473 - Attachment is patch: false
Attachment #133473 - Attachment mime type: text/plain → text/html
Attached patch Proposed patch (obsolete) — — Splinter Review 
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.... ;) )
Taking.
Assignee: other → bzbarsky
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: qawanted → regression
Priority: -- → P1
Summary: 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 appr. 4000%
Target Milestone: --- → mozilla1.6alpha
Comment on attachment 133475 [details] [diff] [review]
Proposed patch

roc, can you review?  Or should I poke dbaron?
Attachment #133475 - Flags: superreview?(roc)
Attachment #133475 - Flags: review?(roc)
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
Attached file Testcase #3 — 
The suggested patch breaks this testcase, before: 100px after: auto.
This works for me:

      if (NS_AUTOHEIGHT == rs->mComputedHeight) {
        if (rs->frame->GetStyleDisplay()->IsAbsolutelyPositioned()) {
          break;
        } else {
          continue;
        }
      }
Keywords: testcase
Attachment #133475 - Flags: superreview?(roc)
Attachment #133475 - Flags: review?(roc)
Indeed... Mats, thanks for the idea.
Attachment #133475 - Attachment is obsolete: true
Attachment #133539 - Flags: superreview?(roc)
Attachment #133539 - Flags: review?(roc)
Attachment #133539 - Flags: superreview?(roc)
Attachment #133539 - Flags: superreview+
Attachment #133539 - Flags: review?(roc)
Attachment #133539 - Flags: review+
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...
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: