Bug 66599 - Chromium Mac: PDF: Scrollsbars should be visible when scrolling using the two finger gesture on Lion
Summary: Chromium Mac: PDF: Scrollsbars should be visible when scrolling using the two...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sailesh Agrawal
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-19 15:00 PDT by Sailesh Agrawal
Modified: 2011-08-31 18:30 PDT (History)
3 users (show)

See Also:


Attachments
Patch (4.18 KB, patch)
2011-08-19 15:04 PDT, Sailesh Agrawal
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sailesh Agrawal 2011-08-19 15:00:22 PDT
Chromium Mac: PDF: Scrollsbars should be visible when scrolling using the two finger gesture on Lion
Comment 1 Sailesh Agrawal 2011-08-19 15:04:44 PDT
Created attachment 104578 [details]
Patch
Comment 2 Sailesh Agrawal 2011-08-19 15:14:39 PDT
This fixes the following chromium bug:
http://code.google.com/p/chromium/issues/detail?id=92606
Comment 3 Sailesh Agrawal 2011-08-24 11:24:07 PDT
Ping!
Comment 4 Nico Weber 2011-08-24 18:13:51 PDT
Comment on attachment 104578 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:615
> +

I'm not an expert in plugins, but I would've expected the flow to go "if plugin has touch focus, send to plugin, and only send to webview if plugin didn't handle it". Sending the event to two targets looks weird to me. But again, I'm probably not the right person to look at this.
Comment 5 Sailesh Agrawal 2011-08-31 14:51:20 PDT
Comment on attachment 104578 [details]
Patch

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

>> Source/WebKit/chromium/src/WebViewImpl.cpp:615
>> +
> 
> I'm not an expert in plugins, but I would've expected the flow to go "if plugin has touch focus, send to plugin, and only send to webview if plugin didn't handle it". Sending the event to two targets looks weird to me. But again, I'm probably not the right person to look at this.

Unfortunately this is not possible right now. Currently ScrollableArea::handleGestureEvent() just swallows all events.

There's a TODO in EventHandler::handleGestureEvent() to add proper hit testing. I think once this is done it'll be possible to go back and do this.
Comment 6 Darin Fisher (:fishd, Google) 2011-08-31 14:57:21 PDT
Comment on attachment 104578 [details]
Patch

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

>>> Source/WebKit/chromium/src/WebViewImpl.cpp:615
>>> +
>> 
>> I'm not an expert in plugins, but I would've expected the flow to go "if plugin has touch focus, send to plugin, and only send to webview if plugin didn't handle it". Sending the event to two targets looks weird to me. But again, I'm probably not the right person to look at this.
> 
> Unfortunately this is not possible right now. Currently ScrollableArea::handleGestureEvent() just swallows all events.
> 
> There's a TODO in EventHandler::handleGestureEvent() to add proper hit testing. I think once this is done it'll be possible to go back and do this.

I agree with Nico.  Dispatching the event to both targets seems kind of problematic.  I also wonder why the EventHandler wouldn't just forward the event to the plugin if that is what it hits.
Comment 7 Sailesh Agrawal 2011-08-31 15:06:23 PDT
Comment on attachment 104578 [details]
Patch

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

>>>> Source/WebKit/chromium/src/WebViewImpl.cpp:615
>>>> +
>>> 
>>> I'm not an expert in plugins, but I would've expected the flow to go "if plugin has touch focus, send to plugin, and only send to webview if plugin didn't handle it". Sending the event to two targets looks weird to me. But again, I'm probably not the right person to look at this.
>> 
>> Unfortunately this is not possible right now. Currently ScrollableArea::handleGestureEvent() just swallows all events.
>> 
>> There's a TODO in EventHandler::handleGestureEvent() to add proper hit testing. I think once this is done it'll be possible to go back and do this.
> 
> I agree with Nico.  Dispatching the event to both targets seems kind of problematic.  I also wonder why the EventHandler wouldn't just forward the event to the plugin if that is what it hits.

The EventHandler doesn't do hit testing so it can't tell if the gesture was for the plugin or not.

Safari does it the same way too so I think we can live with this for now.
Comment 8 John Abd-El-Malek 2011-08-31 16:05:54 PDT
btw this looks good to me
Comment 9 Darin Fisher (:fishd, Google) 2011-08-31 16:30:39 PDT
Comment on attachment 104578 [details]
Patch

OK, is there a bug about cleaning this up?
Comment 10 Sailesh Agrawal 2011-08-31 16:43:40 PDT
(In reply to comment #9)
> (From update of attachment 104578 [details])
> OK, is there a bug about cleaning this up?

I couldn't find one so I filed this bug:
https://bugs.webkit.org/show_bug.cgi?id=67338
Comment 11 WebKit Review Bot 2011-08-31 18:30:55 PDT
Comment on attachment 104578 [details]
Patch

Clearing flags on attachment: 104578

Committed r94261: <http://trac.webkit.org/changeset/94261>
Comment 12 WebKit Review Bot 2011-08-31 18:30:59 PDT
All reviewed patches have been landed.  Closing bug.