Bug 61339 - Can't scroll scaled page that has overflow:hidden on its root
Summary: Can't scroll scaled page that has overflow:hidden on its root
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: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-05-23 19:25 PDT by Sam Weinig
Modified: 2011-05-24 13:40 PDT (History)
4 users (show)

See Also:


Attachments
Patch (5.34 KB, patch)
2011-05-23 19:29 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.28 MB, application/zip)
2011-05-23 22:51 PDT, WebKit Review Bot
no flags Details
Patch (5.05 KB, patch)
2011-05-24 11:24 PDT, Sam Weinig
bdakin: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (1.37 MB, application/zip)
2011-05-24 11:43 PDT, WebKit Review Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2011-05-23 19:25:27 PDT
Can't scroll scaled page that has overflow:hidden on its root
Comment 1 Sam Weinig 2011-05-23 19:27:41 PDT
<rdar://problem/9029189>
Comment 2 Sam Weinig 2011-05-23 19:29:46 PDT
Created attachment 94548 [details]
Patch
Comment 3 WebKit Review Bot 2011-05-23 19:33:09 PDT
Attachment 94548 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/page/FrameView.cpp:523:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Source/WebCore/page/FrameView.cpp:542:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 WebKit Review Bot 2011-05-23 22:51:54 PDT
Comment on attachment 94548 [details]
Patch

Attachment 94548 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8658037

New failing tests:
fast/block/float/overhanging-tall-block.html
fast/backgrounds/size/contain-and-cover.html
http/tests/misc/acid2.html
fast/repaint/iframe-scroll-repaint.html
fast/overflow/overflow_hidden.html
fast/block/positioning/vertical-rl/fixed-positioning.html
fast/css/acid2.html
fast/block/positioning/rtl-fixed-positioning.html
svg/custom/image-rescale-scroll.html
fast/events/scroll-in-scaled-page-with-overflow-hidden.html
Comment 5 WebKit Review Bot 2011-05-23 22:51:58 PDT
Created attachment 94568 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 6 Darin Adler 2011-05-24 10:03:36 PDT
Comment on attachment 94548 [details]
Patch

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

I’m going to say r=me but this probably needs more refinement.

>> Source/WebCore/page/FrameView.cpp:523
>> +        case OHIDDEN: {
> 
> A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]

No need to add braces.

> Source/WebCore/page/FrameView.cpp:527
> +            if (m_frame->pageScaleFactor() != 1)
> +                hMode = ScrollbarAuto;
> +            else
> +                hMode = ScrollbarAlwaysOff;

I think you want to check if the scale factor is > 1. I don’t think that != is quite right.

Also, I think this logic is right for the top level frame, but not sure it’s helpful for nested frames.

> LayoutTests/ChangeLog:19
> +2011-05-23  Sam Weinig  <sam@webkit.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Need a short description and bug URL (OOPS!)
> +
> +        * fast/events/scroll-in-scaled-page-with-overflow-hidden-expected.txt: Added.
> +        * fast/events/scroll-in-scaled-page-with-overflow-hidden.html: Added.
> +
> +2011-05-23  Sam Weinig  <sam@webkit.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Can't scroll scaled page that has overflow:hidden on its root
> +        https://bugs.webkit.org/show_bug.cgi?id=61339
> +
> +        * fast/events/scroll-in-scaled-page-with-overflow-hidden-expected.txt: Added.
> +        * fast/events/scroll-in-scaled-page-with-overflow-hidden.html: Added.
> +

Double change log is not good.
Comment 7 Sam Weinig 2011-05-24 11:03:40 PDT
(In reply to comment #6)
> (From update of attachment 94548 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94548&action=review
> 
> I’m going to say r=me but this probably needs more refinement.

There are a couple tests failing, so I definitely need another round. I will address your feedback as well. Thanks for the review.
Comment 8 Sam Weinig 2011-05-24 11:14:33 PDT
 > There are a couple tests failing, so I definitely need another round. I will address your feedback as well. Thanks for the review.

The test failures were from setting hMode for the vertical case.  Silly me.
Comment 9 Sam Weinig 2011-05-24 11:24:49 PDT
Created attachment 94647 [details]
Patch
Comment 10 Anders Carlsson 2011-05-24 11:42:03 PDT
Comment on attachment 94647 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:519
> +    // To combat the inablity to scroll on a page with overflow:hidden on the root when scaled, disregard hidden when

inablity -> inability
Comment 11 WebKit Review Bot 2011-05-24 11:43:10 PDT
Comment on attachment 94647 [details]
Patch

Attachment 94647 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8730536

New failing tests:
fast/events/scroll-in-scaled-page-with-overflow-hidden.html
Comment 12 WebKit Review Bot 2011-05-24 11:43:15 PDT
Created attachment 94652 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 13 Sam Weinig 2011-05-24 12:23:22 PDT
Committed r87187: <http://trac.webkit.org/changeset/87187>
Comment 14 Adam Klein 2011-05-24 13:40:12 PDT
Note: this test fails on Chromium WebKit due to lack of eventSender.scalePageBy (already filed as https://bugs.webkit.org/show_bug.cgi?id=58013).  I'll add a line to test_expectations.txt for it.