Bug 18768 – onscroll and mousewheel events are not fired when iframe set to have no scrollbars

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Joshua]

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.

[Mark Rowe (bdash)]

<rdar://problem/5893475>

[Joshua]

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.

[Jon@Chromium]

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.

[Joshua]

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

[Bora Ertung]

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. 

[David Alan Hjelle]

Created attachment 39655 [details]
reduced test case

[David Alan Hjelle]

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.

[Robin Qiu]

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

[Robin Qiu]

Created attachment 62981 [details]
1st patch

[WebKit Review Bot]

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.

[Robin Qiu]

(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 *

[Robin Qiu]

Created attachment 62988 [details]
s/\t/4 space

[Robin Qiu]

Could anybody review this patch, please?

[Adam Treat]

(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?

[Robin Qiu]

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

[Antonio Gomes]

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.

[Robin Qiu]

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

[Robin Qiu]

Created attachment 65315 [details]
Changed according to Antonio Gomes' comment.

[Antonio Gomes]

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(?)

[Robin Qiu]

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

[Robin Qiu]

Created attachment 65561 [details]
Fixed a typo

[Antonio Gomes]

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

[Robin Qiu]

(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. :)

[Antonio Gomes]

(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 :)

[Robin Qiu]

Created attachment 65600 [details]
Added ifame into test cases.

Any comment is welcome.

[Antonio Gomes]

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?

[Robin Qiu]

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

[WebKit Commit Bot]

Comment on attachment 65600 [details]
Added ifame into test cases.

Clearing flags on attachment: 65600

Committed r67365: <http://trac.webkit.org/changeset/67365>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/67365 might have broken GTK Linux 32-bit Debug

[Antonio Gomes]

(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?