Bug 18768

Summary: onscroll and mousewheel events are not fired when iframe set to have no scrollbars
Product: WebKit Reporter: Joshua <admin>
Component: HTML DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Enhancement CC: abarth, bora.ertung, charles.wei, commit-queue, dahjelle.webkit.org, eric, jon, manyoso, mrobinson, robin.qiu, staikos, tonikitoo, webkit.review.bot
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 Joshua 2008-04-27 01:53:47 PDT
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 Mark Rowe (bdash) 2008-04-27 17:41:17 PDT
<rdar://problem/5893475>
Comment 2 Joshua 2008-05-05 00:11:51 PDT
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 Jon@Chromium 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 Joshua 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 Bora Ertung 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 David Alan Hjelle 2009-09-16 12:26:30 PDT
Created attachment 39655 [details]
reduced test case
Comment 7 David Alan Hjelle 2009-09-16 12:27:39 PDT
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 Robin Qiu 2010-07-29 08:56:02 PDT
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".
Comment 9 Robin Qiu 2010-07-29 13:39:41 PDT
Created attachment 62981 [details]
1st patch
Comment 10 WebKit Review Bot 2010-07-29 13:42:41 PDT
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 Robin Qiu 2010-07-29 13:54:44 PDT
(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 *
Comment 12 Robin Qiu 2010-07-29 13:59:08 PDT
Created attachment 62988 [details]
s/\t/4 space
Comment 13 Robin Qiu 2010-08-04 09:11:52 PDT
Could anybody review this patch, please?
Comment 14 Adam Treat 2010-08-09 11:31:22 PDT
(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 Robin Qiu 2010-08-09 13:08:04 PDT
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 Antonio Gomes 2010-08-19 21:37:14 PDT
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.
Comment 17 Robin Qiu 2010-08-24 08:26:47 PDT
(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.
Comment 18 Robin Qiu 2010-08-24 13:46:30 PDT
Created attachment 65315 [details]
Changed according to Antonio Gomes' comment.
Comment 19 Antonio Gomes 2010-08-25 23:21:43 PDT
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(?)
Comment 20 Robin Qiu 2010-08-26 07:27:29 PDT
(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.
Comment 21 Robin Qiu 2010-08-26 07:31:27 PDT
Created attachment 65561 [details]
Fixed a typo
Comment 22 Antonio Gomes 2010-08-26 07:37:24 PDT
> > 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.
Comment 23 Robin Qiu 2010-08-26 08:17:37 PDT
(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 Antonio Gomes 2010-08-26 08:51:35 PDT
(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 Robin Qiu 2010-08-26 12:35:05 PDT
Created attachment 65600 [details]
Added ifame into test cases.

Any comment is welcome.
Comment 26 Antonio Gomes 2010-09-10 17:09:13 PDT
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?
Comment 27 Robin Qiu 2010-09-12 19:12:22 PDT
(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 28 WebKit Commit Bot 2010-09-12 23:25:06 PDT
Comment on attachment 65600 [details]
Added ifame into test cases.

Clearing flags on attachment: 65600

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