Bug 105014

Summary: [Qt] Horizontal scrollbars events are offseted making them difficult to use
Product: WebKit Reporter: Jocelyn Turcotte <jturcotte>
Component: New BugsAssignee: Michael Brüning <michael.bruning>
Status: RESOLVED FIXED    
Severity: Major CC: allan.jensen, hausmann, jturcotte, michael.bruning, pierre.rossi
Priority: P1 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 106219    
Bug Blocks: 103747, 105484    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
allan.jensen: review+
Patch none

Jocelyn Turcotte
Reported 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.
Attachments
Patch (2.03 KB, patch)
2013-01-03 10:19 PST, Michael Brüning 		no flags
DetailsFormatted DiffDiff
Patch (1.71 KB, patch)
2013-01-04 05:22 PST, Michael Brüning 		no flags
DetailsFormatted DiffDiff
Patch (8.43 KB, patch)
2013-01-04 08:34 PST, Michael Brüning 		no flags
DetailsFormatted DiffDiff
Patch (1.77 KB, patch)
2013-01-07 05:34 PST, Michael Brüning 		allan.jensen: review+
DetailsFormatted DiffDiff
Patch (1.93 KB, patch)
2013-01-07 06:07 PST, Michael Brüning 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Jocelyn Turcotte
Comment 1 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.
Michael Brüning
Comment 2 2012-12-17 02:12:58 PST
Investigating further.
Michael Brüning
Comment 3 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.
Michael Brüning
Comment 4 2013-01-03 10:19:39 PST
Created attachment 181185 [details] Patch
Michael Brüning
Comment 5 2013-01-04 02:55:01 PST
*** Bug 105484 has been marked as a duplicate of this bug. ***
Simon Hausmann
Comment 6 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.
Michael Brüning
Comment 7 2013-01-04 05:22:34 PST
Created attachment 181291 [details] Patch
Michael Brüning
Comment 8 2013-01-04 08:34:39 PST
Created attachment 181305 [details] Patch
Michael Brüning
Comment 9 2013-01-07 02:10:44 PST
Committed r138933: <http://trac.webkit.org/changeset/138933>
Simon Hausmann
Comment 10 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.
Michael Brüning
Comment 11 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.
Michael Brüning
Comment 12 2013-01-07 05:34:35 PST
Created attachment 181496 [details] Patch
Michael Brüning
Comment 13 2013-01-07 06:07:37 PST
Created attachment 181498 [details] Patch
Allan Sandfeld Jensen
Comment 14 2013-01-07 06:41:03 PST
Comment on attachment 181496 [details] Patch After having discussed it further, this has to be correct way.
Michael Brüning
Comment 15 2013-01-07 08:01:54 PST
Comment on attachment 181496 [details] Patch Obsoleting patch as it has been landed from bug 106219
Michael Brüning
Comment 16 2013-01-07 08:02:13 PST
Fixed in 106219.
Simon Hausmann
Comment 17 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 )
Michael Brüning
Comment 18 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
Note You need to log in before you can comment on or make changes to this bug.