Bug 104369 - [GTK] Expose HitTestResult::scrollbar() condition in API
: [GTK] Expose HitTestResult::scrollbar() condition in API
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit2
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Nobody
:
Depends on: 99591
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-07 07:32 PST by Eduardo Lima Mitev
Modified: 2012-12-10 11:43 PST (History)
6 users (show)

See Also:


Attachments
patch (9.69 KB, patch)
2012-12-07 08:14 PST, Eduardo Lima Mitev
cgarcia: review-
cgarcia: commit‑queue-
Details | Formatted Diff | Diff
previous patch with unit test (13.82 KB, patch)
2012-12-07 10:03 PST, Eduardo Lima Mitev
no flags Details | Formatted Diff | Diff
new patch correcting the ChangeLog (14.45 KB, patch)
2012-12-10 07:43 PST, Eduardo Lima Mitev
no flags Details | Formatted Diff | Diff
patch correcting the ChangeLog syntax (14.56 KB, patch)
2012-12-10 11:08 PST, Eduardo Lima Mitev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.