Closed Bug 145212 Opened 22 years ago Closed 22 years ago

Various problems when BODY tag has CSS "overflow: auto" or "overflow: scroll"


(Core :: Layout, defect, P3)






(Reporter: m, Assigned: roc)




(Whiteboard: topembed)


(1 file, 2 obsolete files)

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.0rc2)
BuildID:    2002051006

In pages where "overflow: auto" or "overflow: scroll" has been set for the BODY
tag, two scrollbars are displayed for the page. The outer one lets you scroll
about 80% of the way down, then you've got to use the inner one. Also, clicking
anything at the bottom of the page causes the page to move about randomly.

Note that in the URL, an external stylesheet specifies "overflow: auto" for the
body tag. By elimination, I found that this was the statement causing the
problem. The steps to reproduce / expected & actual results below refer to this
page. Slightly different behaviour occurs with "overflow: scroll".

Reproducible: Always
Steps to Reproduce:
1. Go to the URL.
2. Notice the two scrollbars.
3. Scroll to the bottom of both scrollbars.
4. Click a link, e.g. "Advanced Search". Note that the outer scrollbar jumps to
the top, and the inner one stays at the bottom.
5. Click the "Search" button at the bottom. Note how one scrollbar vanishes,
leaving just one scrollbar as expected.

Actual Results:  a) Two scrollbars displayed.
b) Clicking a link at the bottom of the page causes the page to jump upwards.
c) Clicking a button at the bottom of the page causes one scrollbar to vanish,
effectively temporarily fixing the bug.

Expected Results:  Only one scrollbar should be displayed when "overflow: auto"
or "overflow: scroll" is set for BODY. Clicking links at the bottom shouldn't
cause the page to jump.

(Also see bug 144996 for another occurence of the page jumping.)

Also, try clicking 'Search' at the bottom of that page again, so the search is
actually carried out. Note how the search results page is squashed into the top
part of the screen only. Since these two pages LINK to the same external
stylesheet, I suspect that this is another manifestation of the same bug.
> a) Two scrollbars displayed.

This is correct if the body actually overflowed (the content area scrollbar and
the <body> scrollbar are very different things).

> b) Clicking a link at the bottom of the page causes the page to jump upwards.

This is bug 143815

> c) Clicking a button at the bottom of the page causes one scrollbar to vanish,
>    effectively temporarily fixing the bug.

This part is odd...  Sounds like the body is _not_ actually oveflowing but
thinks it is....
Still not sure why two scrollbars are appearing. Surely "overflowing" means
"being larger than the content area", so the content area scrollbar should act
as the overflow scrollbar? As it is, Mozilla appears to set an arbitrary size
for what counts as an overflow. In the URL I mentioned, that arbitrary size
seems to be about 2 or 3 times the height of the content area (but still smaller
than the length of the page), so two scrollbars appear. After clicking 'Search'
(twice, so it actually submits the form), the arbitrary size seems to be about
half the size of the content area, so only one scrollbar appears, and the lower
half of the window is blank.
On Linux, there's yet more strange behaviour. On that page, the last part is not
displayed at all (it just shows as empty white space); moving the scroll bars up
and down occasionally makes more or less of the page visible. I can't get the
'Search' button visible, so I can't check the search results page.

Changing OS & Platform -> All.
OS: Windows XP → All
Hardware: PC → All
Confirming issue in the May 23 build (2002-05-23-05 1.0.0)
Ever confirmed: true
Priority: -- → P3
Target Milestone: --- → Future
The problem described when 'overflow: scroll' is set on the BODY causing two
scrollbars is found on the following topsites: (Bugscape bug #16963) (Bugscape bug #13570)
The bugs referenced above are Bugscape, not Bugzilla bugs. Links go incorrectly
to closed Bugzilla bugs.
Comment 2: two scrollbars are appearing because the 'body' element is a
descendant of the root element, just like all its contents.  It's analagous to
having a 400px-tall 'div' in a 200px-tall table cell, and setting the cell to
'overflow: auto'.   I believe the browser window's scrollbar is the result of an
'overflow: auto' on the root element.  Is the root 'html', or a more abstract
construct?  I've never been clear on how we handle that.
An update on comment 3: in Linux (build 2002080300), scrolling either scrollbar
now produces screen corruption (and if I change to a different tab, e.g. this
bug, and back again, the screen contents stay as the Bugzilla page - Mozilla is
not redrawing the QCA page).

I managed to click the Search button by clicking randomly; this doesn't cause
the second scrollbar to vanish as I saw in WinXP when I reported the bug, but
the search page still occupies only the top third of the screen, as described at
the end of the original report.

Mozilla obviously isn't handling the overflow attribute for BODY correctly.
Perhaps Mozilla should simply ignore this attribute for BODY, since (as far as I
know) it has no real meaning? Alternatively, Mozilla could treat it as though
the attribute applied to the "root element" mentioned in comment 2, rather than
the BODY element.
I have a fix for this which will probably also fix a whole lot of related
problems with elements which have "overflow: auto/scroll" and "height: auto".
Right now nsGfxScrollFrame simply doesn't handle "height: auto" in any
reasonable way.

The basic problem is that nsBoxToBlockAdapter always chooses the preferred size
for a scrolled block without knowing about the width of the scroll area. It
chooses the size by reflowing the block in infinite width. Of course this
usually isn't high enough when the block is reflowed to the actual width of the
scroll area.

My fix adds a bit to nsBoxLayoutState which asks nsBoxToBlockAdapter to respect
the width or height constraint given in nsHTMLReflowState when choosing the
preferred/min/max dimensions. nsGFXScrollFrame sets this bit. The current patch
works for "overflow: auto" but still requires some further tweaking for
"overflow: scroll" (to take the scrollbar width/height into account when setting
the constraint to be observed by nsBoxToBlockAdapter).

This whole area is very hairy, but I believe the risk of regression will be
rather low because I only set this bit when exactly one of the width or height
of the nsGfxScrollFrame is constrained --- a situation we handle very poorly
right now.

I will attach the patch when my home machine gets network connectivity restored...
Assignee: attinasi → roc+moz
Attached patch promised fix (obsolete) — Splinter Review
This patch fixes the testcase in the URL. It also fixes (woohoo). It seems to fix a whole
lot of bugs that have been filed involving "overflow: auto" elements with
unconstrained height. It also works correctly with "overflow: scroll".
The behavior that this patch induces is both standards-compliant and what
authors expect: if you set an element to overflow:auto but don't set its height,
we give it the ability to have scrollbars, but we size it the same way as we do
with other unconstrained-height blocks: i.e., we make it tall enough to hold the
content, so no vertical scrollbar is needed. (A horizontal scrollbar may still
appear if required, so overflow:auto with unconstrained height *is* useful.) As
usual, if an element (e.g., the body) overflows the viewport, the viewport gets
a vertical scrollbar.

If you set an element to overflow:scroll then we force it to have scrollbars but
we make it tall enough to fit its content without any vertical scrolling
required. If the resulting box is too tall to fit into the viewport then the
viewport gets a vertical scrollbar. Thus we get nested scrollbars again but
that's really what the author is asking for, in this case.

This works on non-body elements too, e.g., DIVs with "overflow:auto" and no
height set.

It does *not* seem to work 100% with "overflow" set on the HTML element. The
HTML element seems to respond appropriately, actually, but then the viewport
isn't creating scrollbars when it probably should. That is probably a separate bug.
Update: in 1.2alpha on Linux (2002091016), I no longer see the display
corruption I mentioned in comment 8; the page content displays as intended.
However, I do still see two scrollbars.
More notes on this patch:

This patch makes layout of scrolling elements in HTML a little smarter. In
particular when one of the height or width of the scrolling area is
unconstrained (as is usually the case for an element with "height: auto;
overflow: auto"), we use the height or width constraint to help compute the
preferred size of the element. This ensures that a "height: auto; overflow:
auto" block will determine its preferred height by laying out in its actual
width and seeing how much vertical space is required. Currently we ignore any
available height or width constraints and always determine the preferred size by
laying out in unconstrained width, which leads to a fairly meaningless preferred
height for most blocks.

Risk: the risk is low because the new code only fires when either the width or
the height (but not both!) of a scrolling element is unconstrained. Right now we
almost always get these cases completely wrong.

This patch does NOT fix all the existing issues with overflow styles on BODY and
HTML. There are a lot of other problems to fix. However, this patch does fix a
lot of existing problems and is a big step in the right direction (IMHO).
*** Bug 120370 has been marked as a duplicate of this bug. ***
Attached patch better fix (obsolete) — Splinter Review
This patch improves on the previous patch slightly, by guessing whether a
vertical or horizontal scrollbar needs to be added and if so including space
for the scrollbar in the preferred size.
Attachment #98083 - Attachment is obsolete: true
*** Bug 145541 has been marked as a duplicate of this bug. ***
Has this patch been checked in?
If yes, did it cause bug 170040?
This has not been checked in yet.
Comment on attachment 99835 [details] [diff] [review]
better fix

I don't really like the name "GetConstrainBlockSize."  Per discussion on IRC,
"GetBlockSizeConstraint" is nicer.  Change to that (for the setter too).

Attachment #99835 - Flags: superreview+
Attached patch Updating patchSplinter Review
Renamed nsBoxLayoutState::Get/SetScrolledBoxSizeConstraint as per hyatt's
Attachment #99835 - Attachment is obsolete: true
Comment on attachment 100508 [details] [diff] [review]
Updating patch

bringing forward r=hyatt (converting from sr as per discussion on IRC)
Attachment #100508 - Flags: review+
Attachment #100508 - Flags: superreview+
Comment on attachment 100508 [details] [diff] [review]
Updating patch

==== Is it just me or do all these "nots" together look confusing. :-) I do
understand what you are trying to figure out though.

+  if ((computedSize.width != NS_INTRINSICSIZE)
+      != (computedSize.height != NS_INTRINSICSIZE)) {

==== Fix the indentation of the comments too:

@@ -349,8 +380,14 @@
     } else {
      // if we can't set the maxElementSize. Then we must reflow
      // uncontrained.
-     rect.width = NS_UNCONSTRAINEDSIZE;
-     rect.height = NS_UNCONSTRAINEDSIZE;
+      rect.width = NS_UNCONSTRAINEDSIZE;
+      rect.height = NS_UNCONSTRAINEDSIZE;

==== Put a return in after the PRBool to match convention used in the file:

+static PRBool UseHTMLReflowConstraints(nsBoxToBlockAdaptor* aAdaptor,
nsBoxLayoutState& aState) {

==== The |.get()| shouldn't be necessary:

+  if (!parentFrameType || parentFrameType.get() != nsLayoutAtoms::scrollFrame)

==== Can we redo these comments so that they flow better? It reads as if people
kept tacking on exceptions as they fixed bugs.

@@ -329,7 +358,9 @@
     // If its a resize nothing in the block could have changed.
     // so use our cached sizes instead. Well of course that is if we have
cached sizes
     // if not we have to compute them.
-    if (!DoesNeedRecalc(mBlockPrefSize) && reason == eReflowReason_Resize)
+    // Don't use the cache if we have HTMLReflowState constraints --- they
might have changed

==== Same here:

   // If the size is cached. Then we are set just return it.
-  if (!DoesNeedRecalc(mPrefSize)) {
+  // Don't use the cache if we have HTMLReflowState constraints --- they might
have changed
+  if (!DoesNeedRecalc(mPrefSize) && !UseHTMLReflowConstraints(this, aState)) {
I'll fix those before checking in.

Thanks! Now I get to hammer the tree twice in one day :-).
Fix checked in.
Closed: 22 years ago
Resolution: --- → FIXED
Since I just happened to click on the bonsai link and then read kin's comments,
a few random and too-late thoughts:

+  if ((computedSize.width != NS_INTRINSICSIZE)
+      != (computedSize.height != NS_INTRINSICSIZE)) {

Might be more clearly written (changing inner != to ==) as
  if ((computedSize.width  == NS_INTRINSICSIZE) !=
      (computedSize.height == NS_INTRINSICSIZE)) {

+  if (!parentFrameType || parentFrameType.get() != nsLayoutAtoms::scrollFrame)

The "!parentFrameType ||" isn't needed either.
Even clearer (to me, maybe I'm strange :-) and possibly more efficient (although
a good optimizer should do it):

  if ((computedSize.width  == NS_INTRINSICSIZE) ^
      (computedSize.height == NS_INTRINSICSIZE)) {

*** Bug 172584 has been marked as a duplicate of this bug. ***
*** Bug 168787 has been marked as a duplicate of this bug. ***
*** Bug 57476 has been marked as a duplicate of this bug. ***
Marking topembed in case this ever needs to get on a branch to fix
Whiteboard: topembed
Verified fixed.
You need to log in before you can comment on or make changes to this bug.