Bug 105014 - [Qt] Horizontal scrollbars events are offseted making them difficult to use
Summary: [Qt] Horizontal scrollbars events are offseted making them difficult to use
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Major
Assignee: Michael Brüning
URL:
Keywords: Qt
: 105484 (view as bug list)
Depends on: 106219
Blocks: 103747 105484
  Show dependency treegraph
 
Reported: 2012-12-14 05:15 PST by Jocelyn Turcotte
Modified: 2013-01-07 08:14 PST (History)
5 users (show)

See Also:


Attachments
Patch (2.03 KB, patch)
2013-01-03 10:19 PST, Michael Brüning
no flags Details | Formatted Diff | Diff
Patch (1.71 KB, patch)
2013-01-04 05:22 PST, Michael Brüning
no flags Details | Formatted Diff | Diff
Patch (8.43 KB, patch)
2013-01-04 08:34 PST, Michael Brüning
no flags Details | Formatted Diff | Diff
Patch (1.77 KB, patch)
2013-01-07 05:34 PST, Michael Brüning
allan.jensen: review+
Details | Formatted Diff | Diff
Patch (1.93 KB, patch)
2013-01-07 06:07 PST, Michael Brüning
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jocelyn Turcotte 2012-12-14 05:15:16 PST
This isn't a critical bug, but it is quite embarassing.

To reproduce
- Open QtTestBrowser
- Open a big image URL and click to zoom and show scrollbars (e.g. http://www.fantom-xp.com/wallpapers/23/White_Horse_-_best_computer_backgrounds.jpg )
- Try to interract with the horizontal scrollbar at the bottom

What I see is that it happens that I click on the scroll bar but it reacts as if I clicked in the gutter, or the other way around.
The offset seems to change with the scroll position too.

I've seen this happening with all horizontal scrollbars (i.e. for scrollable divs/frames as well). Vertical scrollbars are fine.
Comment 1 Jocelyn Turcotte 2012-12-14 08:22:26 PST
I investigated a bit further and by trying to see when the slider gets highlighted when hovering over the scrollbar, it seems like the horizontal hit testing is reversed.

To highlight or click the slider I need to try hovering/clicking at its (scrollBarWidth - sliderX) position. e.g. to click on the right of the scroll bar if the slider is currently on the left.
Comment 2 Michael Brüning 2012-12-17 02:12:58 PST
Investigating further.
Comment 3 Michael Brüning 2013-01-03 10:12:58 PST
This is problem is caused by the layout direction being set to Qt::LayoutDirectionAuto by default in QStyleFacadeOption / the fact that for the scrolling hit test, the layout direction does not get overwritten due to the lack of a widget to poll for it's direction. This leads QStyle::visualRect to assume that the scrollbar is a RightToLeft layout direction component as it only checks for LeftToRight. 

This isn't a problem when painting the scrollbar as then, there is a widget present which is queried for the direction, but that is not the case when doing the hit test.
Comment 4 Michael Brüning 2013-01-03 10:19:39 PST
Created attachment 181185 [details]
Patch
Comment 5 Michael Brüning 2013-01-04 02:55:01 PST
*** Bug 105484 has been marked as a duplicate of this bug. ***
Comment 6 Simon Hausmann 2013-01-04 03:59:02 PST
Comment on attachment 181185 [details]
Patch

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

Good catch! I think however the fix is slightly different.

> Source/WebCore/ChangeLog:12
> +        hte rectangle for the hit test.

hte -> the

> Source/WebCore/platform/qt/ScrollbarThemeQStyle.cpp:158
> +    opt.direction = qApp->layoutDirection();

It seems like a bug that QStyle behaves that way, but ok, what we do isn't quite right either.

We _should_ be picking up the layout direction from the _widget_ unless specified by the RenderStyle. Therefore I think
the correct fix is to change initGenericStyleOption in QStyleFacadeImp.cpp to do the

    option->direction = facadeOption.direction;

initialization only if facadeOption.direction is not Auto. If it is auto, then we should
keep what we have in option->direction, which is (commonly) initialized from the widget's
layout direction, which comes from the QGuiApplication.
Comment 7 Michael Brüning 2013-01-04 05:22:34 PST
Created attachment 181291 [details]
Patch
Comment 8 Michael Brüning 2013-01-04 08:34:39 PST
Created attachment 181305 [details]
Patch
Comment 9 Michael Brüning 2013-01-07 02:10:44 PST
Committed r138933: <http://trac.webkit.org/changeset/138933>
Comment 10 Simon Hausmann 2013-01-07 04:55:04 PST
Comment on attachment 181305 [details]
Patch

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

> Source/WebKit/qt/WidgetSupport/QStyleFacadeImp.cpp:81
> +    if (option->direction == Qt::LayoutDirectionAuto)
> +        option->direction = facadeOption.direction;

Hang on, this doesn't seem right.

option->direction is initialized from the widget. But _if_ WebCore wants to override it, it sets facadeOption.direction to LTR or RTL. If WebCore does _not_ determine the direction (as it is the case in this very bug!), then it's auto and only then we should _not_ take facadeOption.directon. IOW I think the code should read

if (facadeOption.direction != Auto)
    option->direction = facadeOption.direction;

This way we avoid Auto as value in option->direction.
Comment 11 Michael Brüning 2013-01-07 05:27:15 PST
As per Simon's comment, the earlier fix was working in the test case, but not valid. Uploading a new patch soon.
Comment 12 Michael Brüning 2013-01-07 05:34:35 PST
Created attachment 181496 [details]
Patch
Comment 13 Michael Brüning 2013-01-07 06:07:37 PST
Created attachment 181498 [details]
Patch
Comment 14 Allan Sandfeld Jensen 2013-01-07 06:41:03 PST
Comment on attachment 181496 [details]
Patch

After having discussed it further, this has to be correct way.
Comment 15 Michael Brüning 2013-01-07 08:01:54 PST
Comment on attachment 181496 [details]
Patch

Obsoleting patch as it has been landed from bug 106219
Comment 16 Michael Brüning 2013-01-07 08:02:13 PST
Fixed in 106219.
Comment 17 Simon Hausmann 2013-01-07 08:09:16 PST
(In reply to comment #16)
> Fixed in 106219.

That looks like an unrelated change from last year? :) (see http://trac.webkit.org/changeset/106219 )
Comment 18 Michael Brüning 2013-01-07 08:14:44 PST
(In reply to comment #17)
> (In reply to comment #16)
> > Fixed in 106219.
> 
> That looks like an unrelated change from last year? :) (see http://trac.webkit.org/changeset/106219 )

Sorry, I meant bug 106219 that I put as a blocker to this one :) Changeset from there is http://trac.webkit.org/changeset/138946