Bug 104369 – [GTK] Expose HitTestResult::scrollbar() condition in API

• Component: WebKit2
• Status: RESOLVED FIXED

[Eduardo Lima Mitev]

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.

[Eduardo Lima Mitev]

Adding dependency on bug 99591.

[Eduardo Lima Mitev]

Created attachment 178221 [details]
patch

[Carlos Garcia Campos]

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.

[Eduardo Lima Mitev]

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

[WebKit Review Bot]

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

[Carlos Garcia Campos]

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.

[Eduardo Lima Mitev]

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.

[Eduardo Lima Mitev]

Created attachment 178594 [details]
patch correcting the ChangeLog syntax

The ChangeLog syntax in previous patch was still wrong.

[Carlos Garcia Campos]

Comment on attachment 178594 [details]
patch correcting the ChangeLog syntax

Perfect, thanks!

[WebKit Review Bot]

Comment on attachment 178594 [details]
patch correcting the ChangeLog syntax

Clearing flags on attachment: 178594

Committed r137192: <http://trac.webkit.org/changeset/137192>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.