Bug 104369

Summary: [GTK] Expose HitTestResult::scrollbar() condition in API
Product: WebKit Reporter: Eduardo Lima Mitev <elima>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia, gustavo, gyuyoung.kim, mrobinson, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 99591    
Bug Blocks:    
Attachments:
Description Flags
patch
cgarcia: review-, cgarcia: commit-queue-
previous patch with unit test
none
new patch correcting the ChangeLog
none
patch correcting the ChangeLog syntax none

Description Eduardo Lima Mitev 2012-12-07 07:32:33 PST
Specific UX behaviors require discriminating hits over elements' scrollbars, specially the WebView's main scrollbars. For example, any application behavior implemented on middle button, could break scrolling if the hit occurs over any scrollbars in the page.
Comment 1 Eduardo Lima Mitev 2012-12-07 07:40:17 PST
Adding dependency on bug 99591.
Comment 2 Eduardo Lima Mitev 2012-12-07 08:14:44 PST
Created attachment 178221 [details]
patch
Comment 3 Carlos Garcia Campos 2012-12-07 08:29:47 PST
Comment on attachment 178221 [details]
patch

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

Patch looks great, thanks! Setting r- just because it doesn't include unit tests. You can modify WebKitWebView/mouse-target test to check it. Use a form control containing scrollbars, because main scrollbars won't work due to bug #99591.

> Source/WebKit2/Shared/WebHitTestResult.h:55
> +        bool isOverScrollbar;

I would use isScrollbar.

> Source/WebKit2/Shared/WebHitTestResult.h:87
> +            , isOverScrollbar(hitTestResult.scrollbar())

Ditto.
Comment 4 Eduardo Lima Mitev 2012-12-07 10:03:39 PST
Created attachment 178234 [details]
previous patch with unit test

This is the previous patch updated to include unit test, and to replace WebHitTestResult::isOverScrollbar by isScrollbar as suggested.

Thanks for reviewing
Comment 5 WebKit Review Bot 2012-12-07 10:06:43 PST
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 6 Carlos Garcia Campos 2012-12-10 06:31:37 PST
Comment on attachment 178234 [details]
previous patch with unit test

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

> Source/WebKit2/ChangeLog:18
> +        - Adds a new bool 'isScrollbar' to WebHitTestResult to
> +          carry the scrollbar() condition from Web process to UI process.
> +        - Adds a new flag WEBKIT_HIT_TEST_RESULT_CONTEXT_SCROLLBAR to
> +          WebKitHitTestResult context enum.
> +        - Adds the corresponding accessor context_is_scrollbar() to
> +          WebKitHitTestResult.
> +        - Adds the corresponding unit test cases to
> +          '/webkit2/WebKitWebView/mouse-target'.

Please, move these below adding per function explanations instead. Sorry, that I didn't notice this in the previous patch.
Comment 7 Eduardo Lima Mitev 2012-12-10 07:43:10 PST
Created attachment 178550 [details]
new patch correcting the ChangeLog

>> Source/WebKit2/ChangeLog:18
>> +        - Adds a new bool 'isScrollbar' to WebHitTestResult to
>> +          carry the scrollbar() condition from Web process to UI process.
>> +        - Adds a new flag WEBKIT_HIT_TEST_RESULT_CONTEXT_SCROLLBAR to
>> +          WebKitHitTestResult context enum.
>> +        - Adds the corresponding accessor context_is_scrollbar() to
>> +          WebKitHitTestResult.
>> +        - Adds the corresponding unit test cases to
>> +          '/webkit2/WebKitWebView/mouse-target'.

> Please, move these below adding per function explanations instead. Sorry,
> that I didn't notice this in the previous patch.

New patch fixing this.
Comment 8 Eduardo Lima Mitev 2012-12-10 11:08:35 PST
Created attachment 178594 [details]
patch correcting the ChangeLog syntax

The ChangeLog syntax in previous patch was still wrong.
Comment 9 Carlos Garcia Campos 2012-12-10 11:22:54 PST
Comment on attachment 178594 [details]
patch correcting the ChangeLog syntax

Perfect, thanks!
Comment 10 WebKit Review Bot 2012-12-10 11:43:19 PST
Comment on attachment 178594 [details]
patch correcting the ChangeLog syntax

Clearing flags on attachment: 178594

Committed r137192: <http://trac.webkit.org/changeset/137192>
Comment 11 WebKit Review Bot 2012-12-10 11:43:24 PST
All reviewed patches have been landed.  Closing bug.