Bug 22769 - Scrolling allowed when overflow:hidden (seen on Acid2)
Summary: Scrolling allowed when overflow:hidden (seen on Acid2)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 525.x (Safari 3.2)
Hardware: All All
: P3 Normal
Assignee: Antonio Gomes
URL: http://acid2.acidtests.org
Keywords: HasReduction
: 18189 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-12-09 16:56 PST by jasneet
Modified: 2013-08-28 05:10 PDT (History)
10 users (show)

See Also:


Attachments
reduced testcase (410 bytes, text/html)
2008-12-09 16:58 PST, jasneet
no flags Details
snack for EWS (1.93 KB, patch)
2013-08-26 09:04 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
actual snack for EWS (1.78 KB, patch)
2013-08-26 10:33 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
patch 1 - for review (9.79 KB, patch)
2013-08-27 05:41 PDT, Antonio Gomes
darin: review-
Details | Formatted Diff | Diff
patch 1.1 - for review (10.12 KB, patch)
2013-08-27 11:34 PDT, Antonio Gomes
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jasneet 2008-12-09 16:56:05 PST
I Steps:
Go to http://acid2.acidtests.org
Move mouse to the bottom of the face(or anywhere, placement doesn't 
matter)
Left click and hold the button down.
While holding the left mouse button down, move the mouse upward until 
the page starts to move up.

II Issue:
Page moves up when it's not supposed to, and a yellow and red line appear.

III Conclusion:
The page is not scrolling up in Firefox when you get to the anchor because of the 
style in the css  
    html { overflow: hidden;}

In Safari and Chrome, although any overflow is hidden, you can still scroll up the page.

As with the yellow and red line, this can be seen because of the scrolling issue. The lines are the background color and border of a <div>, in which its position is fixed.

IV Other Browsers:
IE7: ok
FF3: ok

V Nightly tested: 39088

Bug in Chromium : http://code.google.com/p/chromium/issues/detail?id=1701
Comment 1 jasneet 2008-12-09 16:58:24 PST
Created attachment 25904 [details]
reduced testcase
Comment 2 Braja 2009-02-26 07:13:07 PST
(In reply to comment #0)
> I Steps:
> Go to http://acid2.acidtests.org
> Move mouse to the bottom of the face(or anywhere, placement doesn't 
> matter)
> Left click and hold the button down.
> While holding the left mouse button down, move the mouse upward until 
> the page starts to move up.
> 
> II Issue:
> Page moves up when it's not supposed to, and a yellow and red line appear.
> 
> III Conclusion:
> The page is not scrolling up in Firefox when you get to the anchor because of
> the 
> style in the css  
>     html { overflow: hidden;}
> 
> In Safari and Chrome, although any overflow is hidden, you can still scroll up
> the page.
> 
> As with the yellow and red line, this can be seen because of the scrolling
> issue. The lines are the background color and border of a <div>, in which its
> position is fixed.
> 
> IV Other Browsers:
> IE7: ok
> FF3: ok
> 
> V Nightly tested: 39088
> 
> Bug in Chromium : http://code.google.com/p/chromium/issues/detail?id=1701
> 

Can any body explain me IF
style in the css  
    html { overflow: hidden;}
what happens exactly. If I am disabling overflow: hidden;
In my 1st page it self i am able to see the image in FireFox.
Is there any way we can fix it (it scould not scroll).
If any body can guide me I will try to fix it.
Comment 3 Ryosuke Niwa 2009-06-04 16:40:45 PDT
According to CSS2.1 (11.1.1):
overflow: hidden
This value indicates that the content is clipped and that no scrolling user interface should be provided to view the content outside the clipping region.

With the following markup, users can see "this content appears below window" by dragging the entire document downwards from left upper corner.  While WebKit's behavior confirms with that of Internet Explorer 8, it differs from that of Firefox, which does not allow users to see the text.
<html>
<style type="text/css">

body {overflow: hidden;}

#c_overflow { position: relative; margin: 100% 0px 200px 0px; }

#c_fixed { position: fixed; top: 100px; left: 100px; }

</style>
<body>
<div id="c_fixed">this content appears at (100,100)</div>
<div id="c_overflow">this content appears below window</div>
</body>
</html>
Comment 4 Alexey Proskuryakov 2009-06-15 02:36:30 PDT
*** Bug 18189 has been marked as a duplicate of this bug. ***
Comment 5 Antonio Gomes 2013-08-21 08:46:58 PDT
Conformed it works ok on Firefox and Opera12 (pre-blink).

Safari, Chrome and Opera15 all fail. 

I have not tried IE.

Taking..
Comment 6 Antonio Gomes 2013-08-26 09:04:51 PDT
Created attachment 209656 [details]
snack for EWS
Comment 7 Antonio Gomes 2013-08-26 10:33:10 PDT
Created attachment 209663 [details]
actual snack for EWS
Comment 8 Antonio Gomes 2013-08-27 05:41:50 PDT
Created attachment 209756 [details]
patch 1 - for review
Comment 9 Darin Adler 2013-08-27 09:48:36 PDT
Comment on attachment 209756 [details]
patch 1 - for review

View in context: https://bugs.webkit.org/attachment.cgi?id=209756&action=review

Generally looks OK to me. There are some small things that need to change, though.

> Source/WebCore/rendering/RenderBox.cpp:820
> +    bool isDocumentNode = node() == &document();

The usual way to write this is:

    node()->isDocumentNode()

I don’t think we need a local variable for this.

> Source/WebCore/rendering/RenderBox.cpp:822
> +        return &frame() && view().frameView().isScrollable();

The whole point of having frame() return a reference is that it’s guaranteed never to be zero. So it’s nonsense to check &frame() to see if it’s zero!

> Source/WebCore/rendering/RenderBox.cpp:-828
> -    // Check for a box that represents the top level of a web page.
> -    // This can be scrolled by calling Chrome::scrollRectIntoView.
> -    // This only has an effect on the Mac platform in applications
> -    // that put web views into scrolling containers, such as Mac OS X Mail.
> -    // The code for this is in RenderLayer::scrollRectToVisible.

Could you explain more about why it’s OK to remove this comment?

> Source/WebCore/rendering/RenderBox.cpp:-832
> -    return page && &page->mainFrame() == &frame() && view().frameView().isScrollable();

Why is it OK to remove the mainFrame check?

> Source/WebCore/rendering/RenderBox.cpp:828
> +    ASSERT(node() != &document());

I think it’s silly to re-assert this here. It was checked at the top of the function.
Comment 10 Antonio Gomes 2013-08-27 10:16:50 PDT
Comment on attachment 209756 [details]
patch 1 - for review

View in context: https://bugs.webkit.org/attachment.cgi?id=209756&action=review

>> Source/WebCore/rendering/RenderBox.cpp:820
>> +    bool isDocumentNode = node() == &document();
> 
> The usual way to write this is:
> 
>     node()->isDocumentNode()
> 
> I don’t think we need a local variable for this.

Ok.

>> Source/WebCore/rendering/RenderBox.cpp:822
>> +        return &frame() && view().frameView().isScrollable();
> 
> The whole point of having frame() return a reference is that it’s guaranteed never to be zero. So it’s nonsense to check &frame() to see if it’s zero!

I agree with that. Note that the "null-check" was already present in this code, so I basically decided to keep it.

A Document can be detached from its Frame, though, so frame can still be 0(?).

>> Source/WebCore/rendering/RenderBox.cpp:-828
>> -    // The code for this is in RenderLayer::scrollRectToVisible.
> 
> Could you explain more about why it’s OK to remove this comment?

Main frame scrolling is not the only case that will go through RenderLayer::scrollRectToVisible, for autoscroll. It also goes there for inner frame. So I opted for removing the comment, although the solution still comprises the main frame case (i.e. Mac's Mail app).

Please let me know your thoughts...

>> Source/WebCore/rendering/RenderBox.cpp:-832
>> -    return page && &page->mainFrame() == &frame() && view().frameView().isScrollable();
> 
> Why is it OK to remove the mainFrame check?

The previous solution made use of FrameView::isScrollable in order to determine the scrollability of a #document, however it was used exclusively if it was a main frame. However, we should respect the FrameView's (i.e. document's) scrollability in the case of non-main frame as well.

For the record, FrameView::isScrollable checks if both <html> or <body> tags have overflow:hidden, for example, among other things.

>> Source/WebCore/rendering/RenderBox.cpp:828
>> +    ASSERT(node() != &document());
> 
> I think it’s silly to re-assert this here. It was checked at the top of the function.

Ok, will get rid of it.
Comment 11 Alexey Proskuryakov 2013-08-27 10:48:21 PDT
> A Document can be detached from its Frame, though, so frame can still be 0(?).

I think that a document in back/forward cache keeps its Frame pointer (and subframe documents aren't even detached from Frame objects). A document that's never been in a Frame can't have RenderObjects.

We certainly shouldn't perform null checks on references - if these can ever be "null", then the change that converted returned type from pointer to reference was wrong, and should be reverted.
Comment 12 Antonio Gomes 2013-08-27 11:14:38 PDT
Comment on attachment 209756 [details]
patch 1 - for review

View in context: https://bugs.webkit.org/attachment.cgi?id=209756&action=review

>>> Source/WebCore/rendering/RenderBox.cpp:822
>>> +        return &frame() && view().frameView().isScrollable();
>> 
>> The whole point of having frame() return a reference is that it’s guaranteed never to be zero. So it’s nonsense to check &frame() to see if it’s zero!
> 
> I agree with that. Note that the "null-check" was already present in this code, so I basically decided to keep it.
> 
> A Document can be detached from its Frame, though, so frame can still be 0(?).

Err, there was no null-check before. I overlooked this part of the existing code, and it should be ok to remove it.
Comment 13 Antonio Gomes 2013-08-27 11:34:20 PDT
Created attachment 209788 [details]
patch 1.1 - for review

Addressed Darin's remarks.  Also added some more words in the ChangeLog saying what the comment can be removed.

Please re-look at your convenience.
Comment 14 Antonio Gomes 2013-08-27 21:39:26 PDT
Fixed by http://trac.webkit.org/changeset/154722
Comment 15 Antonio Gomes 2013-08-28 05:10:10 PDT
Just for completeness, it is related to bug https://bugs.webkit.org/show_bug.cgi?id=38267 (REGRESSION: Autoscroll does not work in Mail messages).