Bug 71230 - Clicking where the overlay scroll track would be should not scroll the page
Summary: Clicking where the overlay scroll track would be should not scroll the page
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Beth Dakin
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-10-31 12:09 PDT by Mark Mentovai
Modified: 2012-01-13 15:59 PST (History)
7 users (show)

See Also:


Attachments
Patch (6.55 KB, patch)
2012-01-12 16:03 PST, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch that fixes PDF as well (8.03 KB, patch)
2012-01-13 12:05 PST, Beth Dakin
no flags Details | Formatted Diff | Diff
Better patch that also fixed PDF (8.37 KB, patch)
2012-01-13 13:55 PST, Beth Dakin
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Mentovai 2011-10-31 12:09:16 PDT
The overlay scrollbar eats clicks on Lion, even when the scrollbar is not visible. The user will place the mouse over web content such as a link, which is all that is visible at a certain position, and click, expecting the web content to receive the click. Instead, the scrollbar picks up the click, either swallowing it or scrolling the page. When the click is swallowed (because it occurred over where the scroll thumb would be), the scrollbar thumb and track don’t even appear, so the user is left without any feedback as to why the click didn’t reach the web content.

I experience this bug in both Safari 5.1.1 (7534.51.22) and Chrome 17.0.921.3 canary on Mac OS X 10.7.2 11C74.

Steps to Reproduce:

0. Use Safari or Chrome on a system that uses overlay scrollbars: a Lion system with a gestureable pointing device such as a laptop trackpad is required.
1. Visit a web page that can scroll vertically, such as http://www.nytimes.com/
2. Resize the window so that the web page is vertically scrollable and there are links present in the web content area at the extreme right edge of the window.
3. Wait for the overlays to disappear, if shown. This should only take a couple of seconds with the mouse cursor outside of the scrollbar area.
4. Click on a web link at the extreme right edge of the window, within the rightmost several pixels.

Expect: to navigate to the link that was clicked. The mouse cursor will have changed to a finger pointing, indicating that a click would navigate to the link.

Observe: Although the mouse cursor changed to the finger, the click does not navigate. Instead, WebKit behaves as though the scrollbar was clicked, even though the scrollbar was not visible.

If the click occurred over where the scroll thumb would have been, no visible feedback is provided at all, because the page does not scroll at all upon a single click of a scroll thumb. (The thumb, is, however, draggable.)

If the click occurred in the area where the scroll track would be but not over the thumb, web content jumps one page in the appropriate direction, and the scroll thumb appears.
Comment 1 Radar WebKit Bug Importer 2011-10-31 16:37:18 PDT
<rdar://problem/10373860>
Comment 2 Beth Dakin 2012-01-12 15:55:25 PST
(In reply to comment #1)
> <rdar://problem/10373860>

That bug has been marked a duplicate of this one: <rdar://problem/9585424>
Comment 3 Beth Dakin 2012-01-12 16:03:12 PST
Created attachment 122329 [details]
Patch
Comment 4 Alexey Proskuryakov 2012-01-12 16:15:29 PST
Comment on attachment 122329 [details]
Patch

Do we need the same check in BuiltinPDFView?
Comment 5 Beth Dakin 2012-01-12 16:19:25 PST
(In reply to comment #4)
> (From update of attachment 122329 [details])
> Do we need the same check in BuiltinPDFView?

Oh, thank you for the reminder! We probably do. I will look into that now.
Comment 6 Beth Dakin 2012-01-12 16:27:49 PST
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 122329 [details] [details])
> > Do we need the same check in BuiltinPDFView?
> 
> Oh, thank you for the reminder! We probably do. I will look into that now.

Looks like PDFs don't have this bug, actually.
Comment 7 Beth Dakin 2012-01-12 16:33:27 PST
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (From update of attachment 122329 [details] [details] [details])
> > > Do we need the same check in BuiltinPDFView?
> > 
> > Oh, thank you for the reminder! We probably do. I will look into that now.
> 
> Looks like PDFs don't have this bug, actually.

Oh, I take it back! Alexey sent me a test case that does show the bug.
Comment 8 Beth Dakin 2012-01-12 17:18:59 PST
Comment on attachment 122329 [details]
Patch

I will post a new patch shortly that fixes PDF too. If anyone can suggest a different name for Scrollbar::isCurrentlyVisible(), I'd love to hear it. Scrollbar's parent class Widget already has a function called isVisible() that has a different meaning, so it seems wrong to re-use that.
Comment 9 Beth Dakin 2012-01-13 12:05:28 PST
Created attachment 122479 [details]
Patch that fixes PDF as well
Comment 10 Alexey Proskuryakov 2012-01-13 12:21:08 PST
Comment on attachment 122479 [details]
Patch that fixes PDF as well

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

> Source/WebCore/platform/Scrollbar.cpp:421
> +    if (!isClickable())
> +        return false;

If I'm reading it correctly, clicks in invisible scroll bars will not be handled by underlying PDF content. That's not a problem now, given that we don't support any links or forms in BuiltinPDFView, but we need to make sure that those work correctly eventually. That would mean that hit testing should return PluginView, not the Scrollbar.
Comment 11 Beth Dakin 2012-01-13 13:55:41 PST
Created attachment 122494 [details]
Better patch that also fixed PDF

You're right, Alexey. I think I got it this time!
Comment 12 Alexey Proskuryakov 2012-01-13 14:33:36 PST
Comment on attachment 122494 [details]
Better patch that also fixed PDF

The "clickable" concept could be somewhat confusing - is a visible, but disabled scrollbar clickable? Looks like the code uses both meanings in different places.

Then again, I'm not sure what it means for a scrollbar to be disabled.
Comment 13 Beth Dakin 2012-01-13 14:37:29 PST
Thanks for the review!

(In reply to comment #12)
> (From update of attachment 122494 [details])
> The "clickable" concept could be somewhat confusing - is a visible, but disabled scrollbar clickable? Looks like the code uses both meanings in different places.
> 
> Then again, I'm not sure what it means for a scrollbar to be disabled.

Hmm, this is a good point. I suppose isHitTestable would be more accurate, but that is not great either because it sounds a bit clumsy. I will think about it a bit more before I check in.
Comment 14 Beth Dakin 2012-01-13 15:59:13 PST
Committed with http://trac.webkit.org/changeset/105006

And I changed the name of isClickable() to shouldParticipateInHitTesting() as Alexey suggested on irc.