Closed Bug 145212 Opened 23 years ago Closed 22 years ago

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

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Future

People

(Reporter: m, Assigned: roc)

References

(
URL
)

Details

(Whiteboard: topembed)

Attachments

(1 file, 2 obsolete files)

Updating patch
12.86 KB, patch
roc
: review+
kinmoz
: superreview+
Details | Diff | Splinter Review
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.
> 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)
Status: UNCONFIRMED → NEW
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: http://www.wsj.com (Bugscape bug #16963) http://www.unserding.de (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 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".
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). sr=hyatt
Attachment #99835 - Flags: superreview+
Attached patch Updating patch — — Splinter Review
Renamed nsBoxLayoutState::Get/SetScrolledBoxSizeConstraint as per hyatt's comment
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 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)) {
I'll fix those before checking in. Thanks! Now I get to hammer the tree twice in one day :-).
Fix checked in.
Status: NEW → RESOLVED
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)) { /be
*** 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 WSJ.com.
Whiteboard: topembed
Verified 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: