Bug 25969 - REGRESSION (r42334): Unnecessary scroll bars remain after the document shrinks
Summary: REGRESSION (r42334): Unnecessary scroll bars remain after the document shrinks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Frames (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P1 Normal
Assignee: Dave Hyatt
URL:
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2009-05-22 14:48 PDT by mitz
Modified: 2009-05-26 11:17 PDT (History)
1 user (show)

See Also:


Attachments
Test case (379 bytes, text/html)
2009-05-22 20:26 PDT, mitz
no flags Details
Patch (56.59 KB, patch)
2009-05-26 08:46 PDT, Dave Hyatt
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2009-05-22 14:48:41 PDT
In the attached test case, the document initially requires both scroll bars, but then it shrinks down to a size that will allow it to fit in a window without scroll bars. The scroll bars should be removed, but they are not, because the document’s size is compared to the viewable size with the scrollbars still present, which is smaller.

This is a regression from <http://trac.webkit.org/changeset/42334>, which removed some of the logic that account for present scroll bars.
Comment 1 mitz 2009-05-22 14:49:04 PDT
<rdar://problem/6916276>
Comment 2 mitz 2009-05-22 20:26:34 PDT
Created attachment 30603 [details]
Test case
Comment 3 Dave Hyatt 2009-05-26 08:46:58 PDT
Created attachment 30670 [details]
Patch
Comment 4 Darin Adler 2009-05-26 09:04:44 PDT
Comment on attachment 30670 [details]
Patch

> +        Added two tests in fast/dynamic.

It's better to give the names of the tests.

> -- (void)_boundsChanged
> +- (void)_boundsChangedToNewSize:(NSSize)size

I think _boundsChangedTo: would be fine here rather than _boundsChangedToNewSize:.

> -    if (!NSEqualSizes(_private->lastLayoutSize, [self bounds].size)) {
> +     if (!NSEqualSizes(_private->lastLayoutSize, size)) {

Indented one extra space here.

> -            frame->view()->resize([self bounds].size.width, [self bounds].size.height);
> +            frame->view()->resize(size.width, size.height);

How about just resize(IntSize(size)) here instead? Has to be an explicit conversion because it's a conversion from floating point to integer, but nice to leave it as an object instead of breaking up into width/height.

> +- (void)setFrameSize:(NSSize)size
> +{
> +    [self _boundsChangedToNewSize:size];
> +    [super setFrameSize:size];
> +}

I'm not sure we need a separate _boundsChangedToNewSize: method. I'd just put that code into the setFrameSize: override and not have a separate method.

r=me
Comment 5 Dave Hyatt 2009-05-26 09:15:37 PDT
Comment on attachment 30670 [details]
Patch

Breaks mail. clearing flag.
Comment 6 Dave Hyatt 2009-05-26 11:10:58 PDT
Comment on attachment 30670 [details]
Patch

Putting back darin's r+, since the bug in Mail is there in TOT and is not caused by this patch.
Comment 7 Dave Hyatt 2009-05-26 11:17:49 PDT
Fixed in r44152.