Last Comment Bug 145212 - Various problems when BODY tag has CSS "overflow: auto" or "overflow: scroll"
: Various problems when BODY tag has CSS "overflow: auto" or "overflow: scroll"
Status: VERIFIED FIXED
topembed
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: Future
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
: Chris Petersen
: Jet Villegas (:jet)
Mentors:
http://www.qca.org.uk/nq/subjects/mat...
: 57476 120370 145541 168787 172584 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2002-05-17 03:51 PDT by Malcolm Scott
Modified: 2003-08-29 04:15 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
promised fix (11.60 KB, patch)
2002-09-05 22:16 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
better fix (12.51 KB, patch)
2002-09-19 08:52 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
hyatt: superreview+
Details | Diff | Splinter Review
Updating patch (12.86 KB, patch)
2002-09-24 20:53 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
roc: review+
kinmoz: superreview+
Details | Diff | Splinter Review

Description Malcolm Scott 2002-05-17 03:51:11 PDT
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.0rc2)
Gecko/20020510
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.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2002-05-17 09:35:51 PDT
> 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....
Comment 2 Malcolm Scott 2002-05-17 12:07:16 PDT
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.
Comment 3 Malcolm Scott 2002-05-17 16:35:41 PDT
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.
Comment 4 Chris Petersen 2002-05-28 12:59:45 PDT
Confirming issue in the May 23 build (2002-05-23-05 1.0.0)
Comment 5 Christine Hoffman 2002-06-21 15:43:18 PDT
The problem described when 'overflow: scroll' is set on the BODY causing two
scrollbars is found on the following topsites:

http://www.wsj.com (Bugscape bug #16963)
http://www.unserding.de (Bugscape bug #13570)
Comment 6 Christine Hoffman 2002-06-21 15:45:30 PDT
The bugs referenced above are Bugscape, not Bugzilla bugs. Links go incorrectly
to closed Bugzilla bugs.
Comment 7 Eric A. Meyer (dead account) 2002-08-16 14:00:17 PDT
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.
Comment 8 Malcolm Scott 2002-08-16 15:41:21 PDT
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.
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2002-09-05 06:49:30 PDT
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...
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2002-09-05 22:16:53 PDT
Created attachment 98083 [details] [diff] [review]
promised fix

This patch fixes the testcase in the URL. It also fixes
http://www.tantek.com/projects/resume.html (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".
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2002-09-05 22:37:19 PDT
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.
Comment 12 Malcolm Scott 2002-09-13 16:23:50 PDT
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.
Comment 13 Robert O'Callahan (:roc) (email my personal email if necessary) 2002-09-18 19:31:24 PDT
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).
Comment 14 Robert O'Callahan (:roc) (email my personal email if necessary) 2002-09-18 19:38:12 PDT
*** Bug 120370 has been marked as a duplicate of this bug. ***
Comment 15 Robert O'Callahan (:roc) (email my personal email if necessary) 2002-09-19 08:52:04 PDT
Created attachment 99835 [details] [diff] [review]
better fix

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.
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2002-09-19 20:28:27 PDT
*** Bug 145541 has been marked as a duplicate of this bug. ***
Comment 17 José Jeria 2002-09-21 02:26:37 PDT
Has this patch been checked in?
If yes, did it cause bug 170040?
Comment 18 Robert O'Callahan (:roc) (email my personal email if necessary) 2002-09-21 06:47:52 PDT
This has not been checked in yet.
Comment 19 David Hyatt 2002-09-24 14:17:11 PDT
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).

sr=hyatt
Comment 20 Robert O'Callahan (:roc) (email my personal email if necessary) 2002-09-24 20:53:14 PDT
Created attachment 100508 [details] [diff] [review]
Updating patch

Renamed nsBoxLayoutState::Get/SetScrolledBoxSizeConstraint as per hyatt's
comment
Comment 21 Robert O'Callahan (:roc) (email my personal email if necessary) 2002-09-24 20:53:54 PDT
Comment on attachment 100508 [details] [diff] [review]
Updating patch

bringing forward r=hyatt (converting from sr as per discussion on IRC)
Comment 22 kinmoz 2002-09-30 10:03:11 PDT
Comment on attachment 100508 [details] [diff] [review]
Updating patch

sr=kin@netscape.com


==== 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)) {
Comment 23 Robert O'Callahan (:roc) (email my personal email if necessary) 2002-09-30 10:11:36 PDT
I'll fix those before checking in.

Thanks! Now I get to hammer the tree twice in one day :-).
Comment 24 Robert O'Callahan (:roc) (email my personal email if necessary) 2002-09-30 18:33:49 PDT
Fix checked in.
Comment 25 David Baron :dbaron: ⌚️UTC-10 2002-09-30 18:52:13 PDT
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.
Comment 26 Brendan Eich [:brendan] 2002-10-01 17:24:05 PDT
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)) {

/be
Comment 27 Robert O'Callahan (:roc) (email my personal email if necessary) 2002-10-04 11:45:11 PDT
*** Bug 172584 has been marked as a duplicate of this bug. ***
Comment 28 Boris Zbarsky [:bz] (still a bit busy) 2002-10-12 19:33:46 PDT
*** Bug 168787 has been marked as a duplicate of this bug. ***
Comment 29 Daniel Wang 2002-10-17 04:21:23 PDT
*** Bug 57476 has been marked as a duplicate of this bug. ***
Comment 30 Susie Wyshak 2002-12-19 13:28:33 PST
Marking topembed in case this ever needs to get on a branch to fix WSJ.com.
Comment 31 Malcolm Scott 2003-08-29 04:15:37 PDT
Verified fixed.

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