Bug 105014 – [Qt] Horizontal scrollbars events are offseted making them difficult to use

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jocelyn Turcotte]

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.

[Jocelyn Turcotte]

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.

[Michael Brüning]

Investigating further.

[Michael Brüning]

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.

[Michael Brüning]

Created attachment 181185 [details]
Patch

[Michael Brüning]

*** Bug 105484 has been marked as a duplicate of this bug. ***

[Simon Hausmann]

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.

[Michael Brüning]

Created attachment 181291 [details]
Patch

[Michael Brüning]

Created attachment 181305 [details]
Patch

[Michael Brüning]

Committed r138933: <http://trac.webkit.org/changeset/138933>

[Simon Hausmann]

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.

[Michael Brüning]

As per Simon's comment, the earlier fix was working in the test case, but not valid. Uploading a new patch soon.

[Michael Brüning]

Created attachment 181496 [details]
Patch

[Michael Brüning]

Created attachment 181498 [details]
Patch

[Allan Sandfeld Jensen]

Comment on attachment 181496 [details]
Patch

After having discussed it further, this has to be correct way.

[Michael Brüning]

Comment on attachment 181496 [details]
Patch

Obsoleting patch as it has been landed from bug 106219

[Michael Brüning]

Fixed in 106219.

[Simon Hausmann]

(In reply to comment #16)
> Fixed in 106219.

That looks like an unrelated change from last year? :) (see http://trac.webkit.org/changeset/106219 )

[Michael Brüning]

(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