Bug 22769

Summary:

Scrolling allowed when overflow:hidden (seen on Acid2)

Product:

WebKit

Reporter:

jasneet <jasneet>

Component:

CSS

Assignee:

Antonio Gomes <tonikitoo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

adele, ap, braja_biswal, darin, esprehn+autocc, glenn, jasneet, kondapallykalyan, Termy58, tonikitoo

Priority:

Keywords:

HasReduction

Version:

525.x (Safari 3.2)

Hardware:

All

OS:

All

URL:

http://acid2.acidtests.org

Attachments:

Description	Flags
reduced testcase	none
snack for EWS	none
actual snack for EWS	none
patch 1 - for review	darin: review-
patch 1.1 - for review	darin: review+

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	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
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.