Bug 33851 - ScrollView::platformVisibleContentRect returns incorrectly sized rectangle when includeScrollbars == true
Summary: ScrollView::platformVisibleContentRect returns incorrectly sized rectangle wh...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.6
: P2 Normal
Assignee: Nobody
URL:
Keywords: PlatformOnly
Depends on:
Blocks:
 
Reported: 2010-01-19 11:33 PST by Daniel Bates
Modified: 2010-03-17 17:06 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.35 KB, patch)
2010-01-19 11:45 PST, Daniel Bates
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2010-01-19 11:33:23 PST
For the Apple Mac port, the method ScrollView::platformVisibleContentRect returns an incorrectly sized rectangle when includeScrollbars == true.

Moreover, the dimensions of this rectangle disagree with both the comment in file ScrollView.h <http://trac.webkit.org/browser/trunk/WebCore/platform/ScrollView.h?rev=53259#L112> and the rectangle that would be returned by the method ScrollView::visibleContentRect <http://trac.webkit.org/browser/trunk/WebCore/platform/ScrollView.cpp?rev=53259#L182> had the widget not been platform-specific (i.e. platformWidget() == false).
Comment 1 Daniel Bates 2010-01-19 11:37:24 PST
From my understanding of the hierarchy of a NSScrollView (*), when includeScrollbars == true, ScrollView::platformVisibleContentRect returns the rectangle of the document within the content view of the scroll view. And this rectangle has the same size as the rectangle returned by -[NSScrollView documentVisibleRect] (i.e. the rectangle returned when includeScrollbars == false).

(*) See subsection Components of a Scroll View of section How Scroll Views Work of the Scroll View Programming Guide for Cocoa <http://developer.apple.com/mac/library/documentation/Cocoa/Conceptual/NSScrollViewGuide/Articles/Introduction.html#//apple_ref/doc/uid/TP40003460>
Comment 2 Daniel Bates 2010-01-19 11:45:34 PST
Created attachment 46931 [details]
Patch

No test included, since I cannot think of a way to test this change using either a manual or DRT test.
Comment 3 Darin Adler 2010-01-19 12:43:56 PST
(In reply to comment #2)
> No test included, since I cannot think of a way to test this change using
> either a manual or DRT test.

If this has no effect on browser behavior, how did you discover the problem?
Comment 4 Daniel Bates 2010-01-19 12:56:49 PST
(In reply to comment #3)
> (In reply to comment #2)
> > No test included, since I cannot think of a way to test this change using
> > either a manual or DRT test.
> 
> If this has no effect on browser behavior, how did you discover the problem?

In looking into another bug involving scrolling I noticed that the result of ScrollView::visibleContentRect was a rectangle whose dimensions did not include scrollbar even when I passed the parameter includeScrollbars == true. Upon inspection and from my understanding of the API of NSScrollView, NSClipView, NSView, and the description in Scroll View Programming Guide for Cocoa, it seems that [[NSScrollView documentView] visibleRect] == [[NSScrollView documentVisibleRect].
Comment 5 Darin Adler 2010-01-19 13:02:40 PST
Comment on attachment 46931 [details]
Patch

rs=me

Seems safe to do this since it currently has no effect. Later it might come in handy to have a correct implementation.
Comment 6 Daniel Bates 2010-01-19 13:54:43 PST
Comment on attachment 46931 [details]
Patch

Clearing flags on attachment: 46931

Committed r53490: <http://trac.webkit.org/changeset/53490>
Comment 7 Daniel Bates 2010-01-19 13:54:56 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Simon Fraser (smfr) 2010-03-17 16:48:10 PDT
This broke autoscrolling in Mail on Mac. -documentVisibleRect is not equivalent to -visibleRect, because the latter takes all enclosing views into account. This is required to make drag-scrolling work.
Comment 9 Darin Adler 2010-03-17 16:55:16 PDT
Darn, that's not good! Can we just roll this out?
Comment 10 Daniel Bates 2010-03-17 17:06:26 PDT
Spoke to Simon Fraser on IRC about this. There is still an issue with the returned dimensions. Simon mentioned that he will look to fix the issue while preventing the regression. I am also looking into this issue. If needed, feel free to rollout this patch.

(In reply to comment #9)
> Darn, that's not good! Can we just roll this out?