33851 2010-01-19 11:33:23 -0800 ScrollView::platformVisibleContentRect returns incorrectly sized rectangle when includeScrollbars == true 2010-03-17 17:06:26 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Mac OS X 10.6 RESOLVED FIXED PlatformOnly P2 Normal --- 1 dbates webkit-unassigned bdakin darin hyatt mitz simon.fraser oldest_to_newest 182341 0 dbates 2010-01-19 11:33:23 -0800 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). 182348 1 dbates 2010-01-19 11:37:24 -0800 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> 182353 2 46931 dbates 2010-01-19 11:45:34 -0800 Created attachment 46931 Patch No test included, since I cannot think of a way to test this change using either a manual or DRT test. 182385 3 darin 2010-01-19 12:43:56 -0800 (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? 182391 4 dbates 2010-01-19 12:56:49 -0800 (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]. 182398 5 46931 darin 2010-01-19 13:02:40 -0800 Comment on attachment 46931 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. 182438 6 46931 dbates 2010-01-19 13:54:43 -0800 Comment on attachment 46931 Patch Clearing flags on attachment: 46931 Committed r53490: <http://trac.webkit.org/changeset/53490> 182439 7 dbates 2010-01-19 13:54:56 -0800 All reviewed patches have been landed. Closing bug. 201039 8 simon.fraser 2010-03-17 16:48:10 -0700 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. 201043 9 darin 2010-03-17 16:55:16 -0700 Darn, that's not good! Can we just roll this out? 201053 10 dbates 2010-03-17 17:06:26 -0700 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? 46931 2010-01-19 11:45:34 -0800 2010-01-19 13:54:43 -0800 Patch Bug33851_1.patch text/plain 2402 dbates SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA1MzM4OSkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMzEgQEAKKzIwMTAtMDEtMTkgIERhbmllbCBCYXRlcyAgPGRiYXRlc0ByaW0uY29t PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIGh0dHBz Oi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0zMzg1MQorCisgICAgICAgIEZpeGVz IGFuIGlzc3VlIGluIHRoZSBBcHBsZSBNYWMgcG9ydCB3aGVyZSBTY3JvbGxWaWV3OjpwbGF0Zm9y bVZpc2libGVDb250ZW50UmVjdAorICAgICAgICByZXR1cm5zIHRoZSByZWN0YW5nbGUgb2YgdGhl IGRvY3VtZW50IHdpdGhpbiB0aGUgY29udGVudCB2aWV3IG9mCisgICAgICAgIHRoZSBzY3JvbGwg dmlldyAoaS5lLiB0aGUgcmVjdGFuZ2xlIG5vdCBpbmNsdWRpbmcgc2Nyb2xsYmFycykgd2hlbgor ICAgICAgICB0aGUgcGFyYW1ldGVyIGluY2x1ZGVTY3JvbGxiYXJzID09IHRydWUKKworICAgICAg ICBDdXJyZW50bHksIHRoaXMgYmVoYXZpb3IgY29udHJhZGljdHMgYm90aCB0aGUgY29tbWVudCBp biBTY3JvbGxWaWV3LmgKKyAgICAgICAgZm9yIG1ldGhvZCB2aXNpYmxlQ29udGVudFJlY3QgYXMg d2VsbCBhcyB0aGUgYmVoYXZpb3IgaW4KKyAgICAgICAgU2Nyb2xsVmlldzo6dmlzaWJsZUNvbnRl bnRSZWN0KCkgZm9yIGEgcGxhdGZvcm0taW5kZXBlbmRlbnQgc2Nyb2xsIHZpZXcuCisKKyAgICAg ICAgSW5zdGVhZCwgaXQgc2hvdWxkIHJldHVybiB0aGUgcmVjdGFuZ2xlIHdob3NlIGRpbWVuc2lv bnMgaW5jbHVkZQorICAgICAgICB0aGUgc2Nyb2xsYmFycy4KKworICAgICAgICBBbHNvLCByZW1v dmVzIHNvbWUgZXh0cmEgd2hpdGVzcGFjZSBhdCB0aGUgZW5kIG9mIHRoZSBsaW5lcy4KKworICAg ICAgICBObyB0ZXN0cyBpbmNsdWRlZCBiZWNhdXNlIHdlIGNhbm5vdCB0ZXN0IHRoaXMgdXNpbmcg ZWl0aGVyIERSVAorICAgICAgICBvciBhIG1hbnVhbCB0ZXN0LgorCisgICAgICAgICogcGxhdGZv cm0vbWFjL1Njcm9sbFZpZXdNYWMubW06CisgICAgICAgIChXZWJDb3JlOjpTY3JvbGxWaWV3Ojpw bGF0Zm9ybVZpc2libGVDb250ZW50UmVjdCk6IElmIGluY2x1ZGVTY3JvbGxiYXJzID09IHRydWUK KyAgICAgICAgdGhlbiByZXR1cm4gdGhlIHJlY3RhbmdsZSB3aG9zZSBkaW1lbnNpb25zIGFyZSB0 aGF0IG9mCisgICAgICAgIHRoZSBmcmFtZSAoaS5lLiAtW05TU2Nyb2xsVmlldyBmcmFtZV0pLgor CiAyMDEwLTAxLTE3ICBPbGl2ZXIgSHVudCAgPG9saXZlckBhcHBsZS5jb20+CiAKICAgICAgICAg UmV2aWV3ZWQgYnkgRGFuIEJlcm5zdGVpbi4KSW5kZXg6IFdlYkNvcmUvcGxhdGZvcm0vbWFjL1Nj cm9sbFZpZXdNYWMubW0KPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9wbGF0Zm9ybS9tYWMvU2Nyb2xs Vmlld01hYy5tbQkocmV2aXNpb24gNTMzMTkpCisrKyBXZWJDb3JlL3BsYXRmb3JtL21hYy9TY3Jv bGxWaWV3TWFjLm1tCSh3b3JraW5nIGNvcHkpCkBAIC0xMDcsMTMgKzEwNywxMiBAQCBib29sIFNj cm9sbFZpZXc6OnBsYXRmb3JtQ2FuQmxpdE9uU2Nyb2xsCiAKIEludFJlY3QgU2Nyb2xsVmlldzo6 cGxhdGZvcm1WaXNpYmxlQ29udGVudFJlY3QoYm9vbCBpbmNsdWRlU2Nyb2xsYmFycykgY29uc3QK IHsKLSAgICBCRUdJTl9CTE9DS19PQkpDX0VYQ0VQVElPTlM7IAotICAgIGlmIChpbmNsdWRlU2Ny b2xsYmFycykgewotICAgICAgICBpZiAoTlNWaWV3KiBkb2N1bWVudFZpZXcgPSB0aGlzLT5kb2N1 bWVudFZpZXcoKSkKLSAgICAgICAgICAgIHJldHVybiBlbmNsb3NpbmdJbnRSZWN0KFtkb2N1bWVu dFZpZXcgdmlzaWJsZVJlY3RdKTsKLSAgICB9Ci0gICAgcmV0dXJuIGVuY2xvc2luZ0ludFJlY3Qo W3Njcm9sbFZpZXcoKSBkb2N1bWVudFZpc2libGVSZWN0XSk7IAotICAgIEVORF9CTE9DS19PQkpD X0VYQ0VQVElPTlM7IAorICAgIEJFR0lOX0JMT0NLX09CSkNfRVhDRVBUSU9OUzsKKyAgICBJbnRS ZWN0IHJlc3VsdCA9IGVuY2xvc2luZ0ludFJlY3QoW3Njcm9sbFZpZXcoKSBkb2N1bWVudFZpc2li bGVSZWN0XSk7CisgICAgaWYgKGluY2x1ZGVTY3JvbGxiYXJzKQorICAgICAgICByZXN1bHQuc2V0 U2l6ZShJbnRTaXplKFtzY3JvbGxWaWV3KCkgZnJhbWVdLnNpemUpKTsKKyAgICByZXR1cm4gcmVz dWx0OworICAgIEVORF9CTE9DS19PQkpDX0VYQ0VQVElPTlM7CiAgICAgcmV0dXJuIEludFJlY3Qo KTsKIH0KIAo=