[webkit-reviews] review denied: [Bug 18768] onscroll and mousewheel events are not fired when iframe set to have no scrollbars : [Attachment 62988] s/\t/4 space

Thu Aug 19 21:37:13 PDT 2010

Antonio Gomes <tonikitoo at webkit.org> has denied Robin Qiu
<robin.qiu at torchmobile.com.cn>'s request for review:
Bug 18768: onscroll and mousewheel events are not fired when iframe set to have
no scrollbars
https://bugs.webkit.org/show_bug.cgi?id=18768

Attachment 62988: s/\t/4 space
https://bugs.webkit.org/attachment.cgi?id=62988&action=review

------- Additional Comments from Antonio Gomes <tonikitoo at webkit.org>
(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.