Bug 106040

Summary: gesture event scrolling of iframe in overflow div scrolls wrong target
Product: WebKit Reporter: Robert Kroeger <rjkroege>
Component: UI EventsAssignee: Robert Kroeger <rjkroege>
Status: RESOLVED INVALID    
Severity: Normal CC: jamesr, rjkroege, tdanderson
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Robert Kroeger 2013-01-03 14:23:47 PST
When a scrollable iframe is placed in a fixed size overflow-scroll div, the (synthetic) wheel events used to implement the gesture scroll are dispatched on the overflow div rather than the iframe. As a result the contents of the iframe cannot be scrolled with gesture events.
Comment 1 Robert Kroeger 2013-01-03 14:36:01 PST
Created attachment 181222 [details]
Patch
Comment 2 Robert Kroeger 2013-01-03 14:39:21 PST
jamesr@: Please take a look?
Comment 3 James Robinson 2013-01-03 16:04:44 PST
Comment on attachment 181222 [details]
Patch

I'm a little confused by the naming in here.  Why would wheel events ever be latched?
Comment 4 Robert Kroeger 2013-01-04 08:21:07 PST
(In reply to comment #3)
> (From update of attachment 181222 [details])
> I'm a little confused by the naming in here.  Why would wheel events ever be latched?

AFAIK: the "latching" here arranges for all wheel events comprising a single touchscreen scroll gesture to be delivered to the same original node.

Aside: I am not convinced that I like the pre-existing code here -- I think that https://bugs.webkit.org/show_bug.cgi?id=103952 is going to make it better -- but I wanted to fix http://crbug.com/167482 in a timely fashion.
Comment 5 James Robinson 2013-01-04 10:25:37 PST
(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 181222 [details] [details])
> > I'm a little confused by the naming in here.  Why would wheel events ever be latched?
> 
> AFAIK: the "latching" here arranges for all wheel events comprising a single touchscreen scroll gesture to be delivered to the same original node.

That doesn't make any sense - touchscreen interactions should not generate wheel events at all and touchpad interactions (which do generate wheels) should deliver events at a point, not at a node.

> 
> Aside: I am not convinced that I like the pre-existing code here -- I think that https://bugs.webkit.org/show_bug.cgi?id=103952 is going to make it better -- but I wanted to fix http://crbug.com/167482 in a timely fashion.
Comment 6 Robert Kroeger 2013-01-04 12:58:25 PST
(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (From update of attachment 181222 [details] [details] [details])
> > > I'm a little confused by the naming in here.  Why would wheel events ever be latched?
> > 
> > AFAIK: the "latching" here arranges for all wheel events comprising a single touchscreen scroll gesture to be delivered to the same original node.
> 
> That doesn't make any sense - touchscreen interactions should not generate wheel events at all and touchpad interactions (which do generate wheels) should deliver events at a point, not at a node.

You're right. http://crbug.com/134520 tracks in-progress work to fix.

I had intended this patch to fix http://crbug.com/167482 in m25 but if this particular patch is pointless, perhaps we could discuss alternatives there?

> 
> > 
> > Aside: I am not convinced that I like the pre-existing code here -- I think that https://bugs.webkit.org/show_bug.cgi?id=103952 is going to make it better -- but I wanted to fix http://crbug.com/167482 in a timely fashion.
Comment 7 Terry Anderson 2013-01-08 13:05:49 PST
(In reply to comment #1)
> Created an attachment (id=181222) [details]
> Patch

Note: I have included this change in my latest WIP patch for https://bugs.webkit.org/show_bug.cgi?id=103952
Comment 8 Terry Anderson 2013-01-08 16:52:43 PST
Comment on attachment 181222 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181222&action=review

> LayoutTests/fast/events/touch/gesture/resources/scroll-inside-editable-iframe.html:9
> +  margin: 0px;a

should that 'a' be there?

> LayoutTests/fast/events/touch/gesture/resources/scroll-inside-editable-iframe.html:35
> +<body>

</body>
Comment 9 Terry Anderson 2013-01-16 12:43:35 PST
Comment on attachment 181222 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181222&action=review

> LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-iframe-editable.html:75
> +    touchtarget.contentDocument.body.addEventListener("mousewheel", recordWheel);

Why do you add the event listener to contentDocument for a scroll, but to contentDocument.body for a mousewheel? Shouldn't these both be set on contentDocument.body?
Comment 10 Robert Kroeger 2013-02-04 12:50:36 PST
Was fixed properly via other changes.
Comment 11 Eric Seidel (no email) 2013-03-01 02:49:15 PST
Comment on attachment 181222 [details]
Patch

Cleared review? from attachment 181222 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).