Bug 18768

Summary: onscroll and mousewheel events are not fired when iframe set to have no scrollbars
Product: WebKit Reporter: Joshua <admin@neurodna.com>
Component: HTML DOMAssignee: Nobody <webkit-unassigned@lists.webkit.org>
Status: RESOLVED FIXED    
Severity: Enhancement CC: abarth@webkit.org, bora.ertung@neurodna.com, charles.wei@torchmobile.com.cn, commit-queue@webkit.org, dahjelle.webkit.org@thehjellejar.com, eric@webkit.org, jon@chromium.org, manyoso@yahoo.com, mrobinson@webkit.org, robin.qiu@torchmobile.com.cn, staikos@kde.org, tonikitoo@webkit.org, webkit.review.bot@gmail.com
Priority: P2 Keywords: InRadar, ReviewedForRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
reduced test case
none
A test case shows that scrollEvents are not fired when overflow == "hidden"
none
1st patch
none
s/\t/4 space
tonikitoo: review-
Changed according to Antonio Gomes' comment.
none
Fixed a typo
none
Added ifame into test cases. none

Description From 2008-04-27 01:53:47 PST
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.
------- Comment #1 From 2008-04-27 17:41:17 PST -------
<rdar://problem/5893475>
------- Comment #2 From 2008-05-05 00:11:51 PST -------
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.
------- Comment #3 From 2008-11-18 12:15:50 PST -------
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.
------- Comment #4 From 2008-11-18 21:07:38 PST -------
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 ;).
------- Comment #5 From 2008-11-23 01:09:58 PST -------
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. 
------- Comment #6 From 2009-09-16 12:26:30 PST -------
Created an attachment (id=39655) [details]
reduced test case
------- Comment #7 From 2009-09-16 12:27:39 PST -------
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.
------- Comment #8 From 2010-07-29 08:56:02 PST -------
Created an attachment (id=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".
------- Comment #9 From 2010-07-29 13:39:41 PST -------
Created an attachment (id=62981) [details]
1st patch
------- Comment #10 From 2010-07-29 13:42:41 PST -------
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.
------- Comment #11 From 2010-07-29 13:54:44 PST -------
(In reply to comment #9)
> Created an attachment (id=62981) [details] [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 *
------- Comment #12 From 2010-07-29 13:59:08 PST -------
Created an attachment (id=62988) [details]
s/\t/4 space
------- Comment #13 From 2010-08-04 09:11:52 PST -------
Could anybody review this patch, please?
------- Comment #14 From 2010-08-09 11:31:22 PST -------
(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?
------- Comment #15 From 2010-08-09 13:08:04 PST -------
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 #16 From 2010-08-19 21:37:14 PST -------
(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?

> +
> +        * 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.
------- Comment #17 From 2010-08-24 08:26:47 PST -------
(In reply to comment #16)
> (From update of attachment 62988 [details] [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.
------- Comment #18 From 2010-08-24 13:46:30 PST -------
Created an attachment (id=65315) [details]
Changed according to Antonio Gomes' comment.
------- Comment #19 From 2010-08-25 23:21:43 PST -------
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(?)
------- Comment #20 From 2010-08-26 07:27:29 PST -------
(In reply to comment #19)
> Thank you Robin!
> 
> (In reply to comment #18)
> > Created an attachment (id=65315) [details] [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] [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.
------- Comment #21 From 2010-08-26 07:31:27 PST -------
Created an attachment (id=65561) [details]
Fixed a typo
------- Comment #22 From 2010-08-26 07:37:24 PST -------
> > Also I'd add a scrollable content inside a iframe with scrolling=no and/or overflow:hidden as in attachment 39655 [details] [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.
------- Comment #23 From 2010-08-26 08:17:37 PST -------
(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. :)
------- Comment #24 From 2010-08-26 08:51:35 PST -------
(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 :)
------- Comment #25 From 2010-08-26 12:35:05 PST -------
Created an attachment (id=65600) [details]
Added ifame into test cases.

Any comment is welcome.
------- Comment #26 From 2010-09-10 17:09:13 PST -------
(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?
------- Comment #27 From 2010-09-12 19:12:22 PST -------
(In reply to comment #26)
> (From update of attachment 65600 [details] [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 #28 From 2010-09-12 23:25:06 PST -------
(From update of attachment 65600 [details])
Clearing flags on attachment: 65600

Committed r67365: <http://trac.webkit.org/changeset/67365>
------- Comment #29 From 2010-09-12 23:25:12 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #30 From 2010-09-13 01:19:33 PST -------
http://trac.webkit.org/changeset/67365 might have broken GTK Linux 32-bit Debug
------- Comment #31 From 2010-09-13 04:33:34 PST -------
(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?