When we create an iframe with no scrolling or have the body element's overflow hidden, iframe does not get any onscroll events when scrolled via scoll related iframe methods. Mouse wheel stops functioning as well. I think disabling onscroll event and mousewheel does not serve any good reason and must be left enabled.
<rdar://problem/5893475>
Mark, I think this should be considered as a bug not a future enhancement. I do not think that this was a design decision in Webkit. It blocks very useful development.
I am wondering how this relates to a bug that was opened against Chromium, see http://code.google.com/p/chromium/issues/detail?id=1701 The Chromium bug seems related, but kind of the opposite of this bug. In the Chromium case we can scroll when we should not. Was there a code change to resolve this issue that may be showing up as the bug reported against Chromium? URL: http://acid2.acidtests.org Other browsers tested: Add OK or FAIL after other browsers where you have tested this issue: Safari 3: FAIL Firefox 3: OK IE 7: FAIL What steps will reproduce the problem? 1. Move mouse to the bottom of the face(or anywhere, placement doesn't matter) 2. Left click and hold the button down. 3. While holding the left mouse button down, move the mouse upward until the page starts to move up. What is the expected result? A face with eyes gets drawn with "Hello world!". And when the mouse hovers over the nose, it turns blue. What happens instead? Page moves up when it's not supposed to, and a yellow and red line appear.
This bug was not filed against overflow:hidden. It was filed against iframes with scroll=no. They are very related. However, I do remember we have seen div's with overflow:hidden can be scrolled event iframe scrolling is turned off. But we didnt want to use that case because it didnt sound very consistent. On the other hand, reading the steps to reproduce chromium bug, I think Issue 1701 is not a very urgent fix ;).
Jon, I do not think this bug is related to the chromium bug #1701. On the other hand, ACID tests do not event work well on Firefox; therefore, saying test case of 1701 works on Firefox does not mean anything to me. I also think bug #1701 is not a must have fix.
Created attachment 39655 [details] reduced test case
I've added an attachment that I think is a reduced test case of this bug. In this case, I display Google within a iframe with scrolling set to "no". However, by dragging and selecting text and moving the mouse cursor to the border of the iframe, the contents can be scrolled. The content stays fixed in Firefox.
Created attachment 62952 [details] A test case shows that scrollEvents are not fired when overflow == "hidden" it's same if this html is included by an iframe, while the overflow == "inherit" and iframe scrolling == "no".
Created attachment 62981 [details] 1st patch
Attachment 62981 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/ChangeLog:7: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] LayoutTests/ChangeLog:7: Line contains tab character. [whitespace/tab] [5] LayoutTests/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] Total errors found: 4 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #9) > Created an attachment (id=62981) [details] > 1st patch Normalflow: ------------------------------------------------------------------------ ScrollView::updateScrollbars() Scrollbar::setValue() Scrollbar::setCurrentPos() FrameView::valueChanged() ScrollView::valueChanged() ScrollView::repaintFixedElementsAfterScrolling() * ScrollView::scrollContents() * EventHandler::sendScrollEvent() * // ... others * When there is no ScrollBars, old flow: (Skips a lot of routines.) ------------------------------------------------------------------------ ScrollView::updateScrollbars() ScrollView::scrollContents() * When there is no ScrollBars, new flow: ------------------------------------------------------------------------ ScrollView::updateScrollbars() FrameView::valueChanged() ScrollView::valueChanged() ScrollView::repaintFixedElementsAfterScrolling() * ScrollView::scrollContents() * EventHandler::sendScrollEvent() * // ... others *
Created attachment 62988 [details] s/\t/4 space
Could anybody review this patch, please?
(In reply to comment #13) > Could anybody review this patch, please? I'd like to see a comparison with how other browsers behave. Do they allow scrolling an iframe with mousewheel when the scrollbars are hidden? Can you please do a comprehensive report on how Firefox,Opera,IE, and others behave?
Hi, Adam, here is some test results (with the the test case: https://bugs.webkit.org/attachment.cgi?id=62952 ) FF Opera IE Safari Chrome Can scroll by JS scrollTo()? Yes Yes Yes Yes Yes Can scroll by selection(mouse drag)? No Yes Yes Yes Yes JS scrollTo() / fire scrollEvent? Yes Yes Yes NO No Scrolling by selection fire event? N/A Yes Yes No No FF: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.8) Gecko/20100722 Firefox/3.6.8 Opera: Opera/9.80 (Windows NT 5.1; U; en) Presto/2.5.24 Version/10.54 IE: Version 7.0.5730.13 Safari: 5.0(7533.16) Chrome: 5.0.375.127
Comment on attachment 62988 [details] s/\t/4 space (In reply to comment #13) > Could anybody review this patch, please? Patch generally looks good to me. Some comments below, and r-'ing because of them. 1) Should not block element like div with overflow:hidden be affect too? > + > + * scrollbars/scrollEvent-overflow-hidden-expected.txt: Copied from LayoutTests/editing/selection/5136696-expected.txt. I'd rather drop this "copied from xxx" message or write something more useful. > +++ b/LayoutTests/scrollbars/scrollEvent-overflow-hidden.html > @@ -0,0 +1,32 @@ > +<html> > + <head> > + <script> > + if (window.layoutTestController) { > + layoutTestController.dumpAsText(); > + layoutTestController.waitUntilDone(); > + } > + function scrollEventFired() > + { > + document.getElementById('console').innerHTML = "PASS"; > + if (window.layoutTestController) > + window.layoutTestController.notifyDone(); > + } > + window.onscroll = scrollEventFired; > + </script> ... > + <script> > + var node = document.getElementById('fillDIV'); > + node.style.height = window.innerHeight + 200; > + window.scrollTo(0, 100); > + if (window.layoutTestController) > + window.layoutTestController.notifyDone(); It is strange you need to call notifyDone twice. Do you really them both calls? Please also test wheel events. Bug title and ChangeLog mention that. IIRC it is support by DRTs. WebCore part itself looks good to me.
(In reply to comment #16) > (From update of attachment 62988 [details]) > (In reply to comment #13) > > Could anybody review this patch, please? > > Patch generally looks good to me. Some comments below, and r-'ing because of them. > > 1) Should not block element like div with overflow:hidden be affect too? > we should. An iframe with "scrolling=No" equals to a DIV with "overflow=hidden" in our code. > > + > > + * scrollbars/scrollEvent-overflow-hidden-expected.txt: Copied from LayoutTests/editing/selection/5136696-expected.txt. > > I'd rather drop this "copied from xxx" message or write something more useful. I'll remove that. > > > +++ b/LayoutTests/scrollbars/scrollEvent-overflow-hidden.html > > @@ -0,0 +1,32 @@ > > +<html> > > + <head> > > + <script> > > + if (window.layoutTestController) { > > + layoutTestController.dumpAsText(); > > + layoutTestController.waitUntilDone(); > > + } > > + function scrollEventFired() > > + { > > + document.getElementById('console').innerHTML = "PASS"; > > + if (window.layoutTestController) > > + window.layoutTestController.notifyDone(); > > + } > > + window.onscroll = scrollEventFired; > > + </script> > ... > > + <script> > > + var node = document.getElementById('fillDIV'); > > + node.style.height = window.innerHeight + 200; > > + window.scrollTo(0, 100); > > + if (window.layoutTestController) > > + window.layoutTestController.notifyDone(); > > It is strange you need to call notifyDone twice. Do you really them both calls? > I added the latter one because I don't want this test to be a time-out test when it fails. > Please also test wheel events. Bug title and ChangeLog mention that. IIRC it is support by DRTs. > I'll add that. FYI, here are some test results on common browsers: Can scroll by wheel when "overflow=hidden"? IE: No opera: Yes Chrome: No Saf: No FF: No > WebCore part itself looks good to me. Thanks for your review. I'll prepare a new patch soon.
Created attachment 65315 [details] Changed according to Antonio Gomes' comment.
Thank you Robin! (In reply to comment #18) > Created an attachment (id=65315) [details] > Changed according to Antonio Gomes' comment. Typo: s/scrollbas/scrollbars. Also I'd add a scrollable content inside a iframe with scrolling=no and/or overflow:hidden as in attachment 39655 [details], the initial test case of this bug. ps: even better would be have one test combining them, and other two with each separately(?)
(In reply to comment #19) > Thank you Robin! > > (In reply to comment #18) > > Created an attachment (id=65315) [details] [details] > > Changed according to Antonio Gomes' comment. > > Typo: s/scrollbas/scrollbars. > > Also I'd add a scrollable content inside a iframe with scrolling=no and/or overflow:hidden as in attachment 39655 [details], the initial test case of this bug. > > ps: even better would be have one test combining them, and other two with each separately(?) IMHO, test cases should be as simple as possible. These test cases can demonstrate the root course of this bug, personally, I think they are enough. anyway, I can add some more if you think they are necessary.
Created attachment 65561 [details] Fixed a typo
> > Also I'd add a scrollable content inside a iframe with scrolling=no and/or overflow:hidden as in attachment 39655 [details] [details], the initial test case of this bug. > > > > ps: even better would be have one test combining them, and other two with each separately(?) > > IMHO, test cases should be as simple as possible. These test cases can demonstrate the root course of this bug, personally, I think they are enough. anyway, I can add some more if you think they are necessary. I'd like to see at least one similar to the original use case reported, please.
(In reply to comment #22) > I'd like to see at least one similar to the original use case reported, please. You mean a iframe with "scrolling=no" ? No problem. :)
(In reply to comment #23) > (In reply to comment #22) > > > I'd like to see at least one similar to the original use case reported, please. > > You mean a iframe with "scrolling=no" ? Yep, and overflow:hidden on the enclosing div :)
Created attachment 65600 [details] Added ifame into test cases. Any comment is welcome.
Comment on attachment 65600 [details] Added ifame into test cases. r=me (In reply to comment #15) > Can scroll by JS scrollTo()? Yes Yes Yes Yes Yes > Can scroll by selection(mouse drag)? No Yes Yes Yes Yes > JS scrollTo() / fire scrollEvent? Yes Yes Yes NO No > Scrolling by selection fire event? N/A Yes Yes No No You are fixing _3_ and _4_, right?
(In reply to comment #26) > (From update of attachment 65600 [details]) > r=me > > (In reply to comment #15) > > Can scroll by JS scrollTo()? Yes Yes Yes Yes Yes > > Can scroll by selection(mouse drag)? No Yes Yes Yes Yes > > JS scrollTo() / fire scrollEvent? Yes Yes Yes NO No > > Scrolling by selection fire event? N/A Yes Yes No No > > You are fixing _3_ and _4_, right? Yes. Thans for you review.
Comment on attachment 65600 [details] Added ifame into test cases. Clearing flags on attachment: 65600 Committed r67365: <http://trac.webkit.org/changeset/67365>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/67365 might have broken GTK Linux 32-bit Debug
(In reply to comment #30) > http://trac.webkit.org/changeset/67365 might have broken GTK Linux 32-bit Debug Is that a false alarm? Robin, could pls verify?