Bug 104369 - [GTK] Expose HitTestResult::scrollbar() condition in API
: [GTK] Expose HitTestResult::scrollbar() condition in API
Status: RESOLVED FIXED
: WebKit
WebKit2
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 99591
:
  Show dependency treegraph
 
Reported: 2012-12-07 07:32 PST by
Modified: 2012-12-10 11:43 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2012-12-07 07:40:17 PST -------
Adding dependency on bug 99591.
------- Comment #2 From 2012-12-07 08:14:44 PST -------
Created an attachment (id=178221) [details]
patch
------- Comment #3 From 2012-12-07 08:29:47 PST -------
(From update of attachment 178221 [details])
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 From 2012-12-07 10:03:39 PST -------
Created an attachment (id=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 From 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 From 2012-12-10 06:31:37 PST -------
(From update of attachment 178234 [details])
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 From 2012-12-10 07:43:10 PST -------
Created an attachment (id=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 From 2012-12-10 11:08:35 PST -------
Created an attachment (id=178594) [details]
patch correcting the ChangeLog syntax

The ChangeLog syntax in previous patch was still wrong.
------- Comment #9 From 2012-12-10 11:22:54 PST -------
(From update of attachment 178594 [details])
Perfect, thanks!
------- Comment #10 From 2012-12-10 11:43:19 PST -------
(From update of attachment 178594 [details])
Clearing flags on attachment: 178594

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