93520 - "overflow" CSS rule (not) honored on HTML BODY elements

Status: RESOLVED DUPLICATE of bug 234851

(Core :: Layout, defect, P1)

Description

[Andy.J.S]

when applying overflow:hidden to the body tag scrollbars still appear and 
content still overflows wether the browser is in strict mode or not

Comment 1

[Johnny Stenback (:jst)]

Over to layout.

Comment 2

[karnaze (gone)]

Reassigning to attinasi.

Comment 3

[Greg K.]

We must first confirm that overflow is a valid property for body elements. The HTML 4.0.1 
specification seems to indicate that body is a block element (see [http://www.w3.org/TR/html4/
struct/global.html#edef-BODY]), and is thus appropriate for overflow settings.

If I read the specification for hidden overflow right, then "no scrolling mechanism should be 
provided to view the content outside the clipping region;...The size and shape of the clipping 
region is specified by the 'clip' property."

However, the default value for the 'clip' property is auto, and that means, "The clipping region 
has the same size and location as the element's box(es)."

So, the body has 100% width by default, and a height equal to the height of the body's contents. 
Therefore, nothing is outside the body's box, and so nothing is clipped. It is appropriate, then, 
to still display scrollbars on the body since all content should be viewable.

Now, if I were to set a clip to cut off the bottom inch of the content, then that part of the content 
should not be visible and it should not be possible to scroll to view it. So, this bug is technically 
correct; setting 'overflow:hidden' doesn't work, but only because the clip property value set on 
the body is not honored.

As a result, this bug is confirmed using Fizzilla/2001080316. I'll attach a testcase momentarily.

Comment 4

[Greg K.]

Attachment: Testcase of hidden body overflow with a rectangular 1-inch clip area all around

Comment 5

[Kevin McCluskey (gone)]

Note: IE6 behaves the same as Mozilla in this respect.

Comment 6

[Andy.J.S]

Having noted that when IE6 is in strict mode it too doesnt recognise 
overflow:hidden on the body via css, but can be set using the correct dom 
javascript 
document.documentElement.style.overflow="hidden"
as when in strict mode the body becomes part of the document element
otherwise there is no way to turn off scrollbars on the body in Mozilla without 
using a frames hack

Comment 7

[lchiang]

nominate for topembed.  

(for reference, pls see http://bugscape.netscape.com/show_bug.cgi?id=12168. 
Sorry, this is internal to Netscape for embedding customer).

Comment 8

[Greg K.]

Editing Summary to reflect my Comment #3.

Comment 9

[Henry Krempel]

It looks like ie6 does have a bug related to clip, but works fine with
overflow:hidden otherwise. I tested the page in
http://bugscape.netscape.com/show_bug.cgi?id=12168 on ie6, and no scrollbars
appeared (as expected).

Comment 10

[Marc Attinasi]

What I am seeing is that IE6 ignores the clip property totally.  However, the
overflow:hidden is honored unless the document is strict.  For the testcase, as
attached (http://bugzilla.mozilla.org/attachment.cgi?id=47572&action=view) IE6
will produce scrollbars for me.  If I remove the DOCTYPE, then IE6 will not give
scroll bars, and thus the overflow:hidden is honored in non-strict mode.  Strange...

Anyway, we do not honor the clip or the overflow:hidden on the body.

Comment 11

[Marc Attinasi]

We need to honor the overflow:hidden on the body in quirks mode at least,
preferably in both modes.

Comment 12

[Kevin McCluskey (gone)]

Marking nsbeta1+, topembed+

Comment 13

[Marc Attinasi]

Attachment: PATCH to check for overflow: none on the HTML and BODY elements

This patch checks the style of the docElement to see if it should scroll or
not.  If the document is HTML, and the docElement does not have overflow:hidden
specified, then we also looks for a BODY element as a child of the doc element
and check it for overflow:hidden.  The end result is that we will suppess the
scrollbars if either the HTML or BODY element have overflow:hidden specified
(also checks for -moz-scrollbars-none, though I am not sure how that gets
specified in reality).

If the overflow is changed via the DOM, then we do not currently fix the
scrolling of the root frame correctly - something I could not easily figure out
a fix for yet.

Comment 14

[Marc Attinasi]

Updating summary - removed 'clip' complaint since it is not relevant to this bug
(another bug should be filed for that, less urgent I believe)

Comment 15

[Alexandru Savulov]

Comment on attachment 72091 [details] [diff] [review]
PATCH to check for overflow: none on the HTML and BODY elements

r= alexsavulov

Comment 16

[John Keiser (jkeiser)]

This looks good to me, but two questions/comments:

1. document.getElementsByTagName("body") might be better for this situation in
the HTML portion
2. why don't we handle document scrolling the same way as any other DIV?

Comment 17

[Marc Attinasi]

>1. document.getElementsByTagName("body") might be better for this situation in
>the HTML portion
Really? Why is it better? It would certainly make the code look simpler, but
that method results in allocation of a DOMNodeList, and is less efficient I
think. That said, I am not familiar with the implementation of that method -
does it use internal knowledge to avoid walking the content tree?

>2. why don't we handle document scrolling the same way as any other DIV?
In some respects we do - both use a GFXScrollFrame to do the scrolling...  But,
there are some differences: a scrolling DIV has a direct mapping from the
content (<div>) to the presentation (block frame[div]) whereas a scrolling
viewport has no such direct correlation to the content.  For example, the
viewport contains the HTML and BODY elements, either of which can control the
scrolling of the viewport.  Beyond that the scrolling mechanisms are pretty much
the same.

Alex pointed out an opimization in the for loop (break when body found) so I
will post an updated patch, and assume the r=alexsavulov applies to that.

Comment 18

[Marc Attinasi]

Attachment: Updated patch to fix 'for' loop (break out when body found)

Slightly improved patch...

Comment 19

[Marc Attinasi]

Comment on attachment 73248 [details] [diff] [review]
Updated patch to fix 'for' loop (break out when body found)

r=alexsavulov (from previous patch)

Comment 20

[kinmoz]

Comment on attachment 73248 [details] [diff] [review]
Updated patch to fix 'for' loop (break out when body found)

I suppose parentContext is just for readability since it's not really used
anywhere?


+    nsIStyleContext *parentContext = nsnull;
+    aPresContext->ResolveStyleContextFor(aDocElement, parentContext, PR_FALSE,
+					  getter_AddRefs(styleContext));


On nsIHTMLDocument there is a GetBodyElement() method which does the same thing
your loop does and avoids creating the child list you mentioned above. See if
you can QI your doc to an nsIHTMLDocument.

Other than that, sr=[e-mail].

Comment 21

[Marc Attinasi]

Thanks Kin - I added a comment about why I cannot use nsIHTMLDocument in layout
(the seperation between content and layout is such that I cannot include
nsIHTMLDocument.h in nsCSSFrameConstructor).  If we ever get the content and
layout modules combined, or feel like adding more to content/shared, then we
shoudl use that approach.  For now though, we cannot.

Comment 22

[Asa Dotzler [:asa]]

Comment on attachment 73248 [details] [diff] [review]
Updated patch to fix 'for' loop (break out when body found)

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 23

[John Keiser (jkeiser)]

re: getElementsByTagName() - I don't think they are any faster.  In fact, in
some cases they might be slower.  The main thing it buys you is the ability to
search the depths in case the BODY is not directly under the HTML (which might
not ever happen here).

Comment 24

[Marc Attinasi]

Checked in fix:

/cvsroot/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,v  <-- 
nsCSSFrameConstructor.cpp
new revision: 1.714; previous revision: 1.713

Comment 25

[Hixie (not reading bugmail)]

Woah, this is totally invalid. The scrollbar on the viewport has nothing to do
with either the HTML or BODY elements' 'overflow' properties.

This should be backed out or, at worst, be moved to quirks mode.

Comment 26

[Brendan Eich [:brendan]]

Hixie: shouldn't this be reopened, at least?

/be

Comment 27

[Andy.J.S]

Can you please explain why this is invalid because Gregs explanation (#3) of 
validity makes sense when compared with the w3c reccomendations of overflow, 
maybe a more detailed explanation of why this "bug" in your view is invalid 
before backing out already commited and deployed patches would be more 
appropriate instead of "whoa this is wrong"
has he/we read the w3c reccomendations on overflow and clip incorrectly and 
why this patch needs to be withdrawn after being marked as fixed ? 

Comment 28

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment 3 has very little to do with this, as far as I can tell.  The issue is
what the scrollbar that you see by default *is*.  If you read the CSS spec,
you'll see that the scrollbar is the result of application of section 9.1.1, not
the application of the 'overflow' property to a certain element.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

See related bug 170040 where applying the *correct* meaning of overflow:hidden
to BODY is 'surprising' some developers.

Comment 30

[Madhur Bhatia]

testcase provided in comment 4 .... attachment 47572 [details].
replacing KW : 'qawanted' with 'testcase'

Comment 31

[Kevin McCluskey (gone)]

Removing topembed+, nsbeta1+ since they were put on this bug to get the
overflow:hidden removes scrollbar behavior in quirks mode. This bug now seems to
be about removing this behavior from standard mode.

Comment 32

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Taking bug.

Comment 33

[Boris Zbarsky [:bzbarsky]]

If we plan to remove this, we should remove it... the longer we leave it in, the
more noise there will be when it comes out.

At this point, it's almost tempting to give this one up as a lost cause...

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We're not taking it out in 1.4, that's for sure.

I'd support taking it out when and if we had 100%-guaranteed-by-the-CSS-WG way
to control the viewport scrollbar. As far as I know, we don't have that yet.

Comment 35

[Hixie (not reading bugmail)]

As of CSS2.1, 'overflow' on <body> or <html> of an HTML document (only, i.e. not
in the case of XHTML) MAY be treated as 'overflow' on the viewport.

On the long run we expect to introduce @viewport, but we figured that we needed
an emergency short-term solution, and as this was already implemented in WinIE,
we'd go with it.

Comment 36

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

FWIW, the patch here has little to do with how WinIE really works.  WinIE5 and
WinIE6 in quirks mode treat 'overflow' according to the content tree rather than
the containing block tree.

We really need to find a way to get rid of this.

Comment 37

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(Actually, that's not the whole story, so never mind.)

Comment 38

[Anne (:annevk)]

We seem to be supporting the 'overflow' property on a BODY element in a
text/html document. Is this bug WORSKFORME or is the issue different from the
summary or comment 35?

Comment 39

[Boris Zbarsky [:bzbarsky]]

Anne, see comment 25 and comment 36...

Comment 40

[Anne (:annevk)]

Per CSS 2.1 comment 25 is incorrect I believe. See comment 35. Not sure about
WinIE though, it seems to behave similar to Mozilla for most documents though.

Comment 41

[Boris Zbarsky [:bzbarsky]]

Right.  This bug is open pending us figuring out exactly what behavior we want.
 If we decide that what we have now is what we want, we'll re-resolve it as fixed.

Comment 42

[Julien "_FrnchFrgg_" RIVAUD]

Current behaviour on Linux is strange (regardless of what is decided considering
the validity of the bug) :
The scrollbar isn't visible, but using the mouse wheel still scrolls, and using
the keyboard to move the caret down scrolls the page too when the caret is at
the bottom.

It thus seems that instead of making the viewport non scrollable, the fix only
hid the scrollbar.

This remark is no good until you've taken a decision, but when so, if you decide
to keep the current way, it may be wise then to correct the behaviour. Do what
you decide, but not half a way and half the other...

_FrnchFrgg_

Comment 43

[Boris Zbarsky [:bzbarsky]]

> It thus seems that instead of making the viewport non scrollable, the fix only
> hid the scrollbar.

Correct.  That's the behavior of "overflow:hidden".  The question in this bug is
whether to apply that behavior to the viewport and if so how.

Comment 44

[Julien "_FrnchFrgg_" RIVAUD]

Are you sure that overflow: hidden should only hide the scrollbar instead of
preventing scrolling by any means ?

_FrnchFrgg_

Comment 45

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Yes.

I don't know of anything to do here, and this bug is really way too confusing,
so on that basis alone it should really be invalid, but marking duplicate.

*** This bug has been marked as a duplicate of 234851 ***

Comment 46

[Julien "_FrnchFrgg_" RIVAUD]

In reply to last comment

David Baron: from http://www.w3.org/TR/REC-CSS2/visufx.html#propdef-overflow

"hidden
    This value indicates that the content is clipped and that no scrolling
mechanism should be provided to view the content outside the clipping region;
users will not have access to clipped content."

As I understand it, even selecting the text and moving the mouse way down
shouldn't cause the content to scroll. Same conclusion for the reaction to
mouse-wheel, which is a scrolling mechanism on its own. I may be wrong, though.

Sorry for spamming a bug marked as duplicate (though I did not see the
correlation between this bug and that one).

Comment 47

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Bugs in trunk or 1.8 branch builds where we allow scrolling should be filed as
bugs.  There have been a number of those bugs fixed since 1.7 / aviary 1.0.x, so
don't file such bugs based on those builds.