RESOLVED FIXED 22769
Scrolling allowed when overflow:hidden (seen on Acid2) 
https://bugs.webkit.org/show_bug.cgi?id=22769
Summary Scrolling allowed when overflow:hidden (seen on Acid2)
jasneet
Reported 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
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
DetailsFormatted DiffDiff
actual snack for EWS (1.78 KB, patch)
2013-08-26 10:33 PDT, Antonio Gomes 		no flags
DetailsFormatted DiffDiff
patch 1 - for review (9.79 KB, patch)
2013-08-27 05:41 PDT, Antonio Gomes 		darin: review-
DetailsFormatted DiffDiff
patch 1.1 - for review (10.12 KB, patch)
2013-08-27 11:34 PDT, Antonio Gomes 		darin: review+
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
jasneet
Comment 1 2008-12-09 16:58:24 PST
Created attachment 25904 [details] reduced testcase
Braja
Comment 2 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.
Ryosuke Niwa
Comment 3 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>
Alexey Proskuryakov
Comment 4 2009-06-15 02:36:30 PDT
*** Bug 18189 has been marked as a duplicate of this bug. ***
Antonio Gomes
Comment 5 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..
Antonio Gomes
Comment 6 2013-08-26 09:04:51 PDT
Created attachment 209656 [details] snack for EWS
Antonio Gomes
Comment 7 2013-08-26 10:33:10 PDT
Created attachment 209663 [details] actual snack for EWS
Antonio Gomes
Comment 8 2013-08-27 05:41:50 PDT
Created attachment 209756 [details] patch 1 - for review
Darin Adler
Comment 9 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.
Antonio Gomes
Comment 10 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.
Alexey Proskuryakov
Comment 11 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.
Antonio Gomes
Comment 12 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.
Antonio Gomes
Comment 13 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.
Antonio Gomes
Comment 14 2013-08-27 21:39:26 PDT
Fixed by http://trac.webkit.org/changeset/154722
Antonio Gomes
Comment 15 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).
Note You need to log in before you can comment on or make changes to this bug.