Bug 86239 - REGRESSION (r114170): Scroll areas in nested frames improperly placed when tiled drawing is enabled
Summary: REGRESSION (r114170): Scroll areas in nested frames improperly placed when ti...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Vicki Pfau
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-05-11 11:54 PDT by Vicki Pfau
Modified: 2012-05-11 19:01 PDT (History)
4 users (show)

See Also:


Attachments
Patch (5.11 KB, patch)
2012-05-11 11:57 PDT, Vicki Pfau
no flags Details | Formatted Diff | Diff
Patch (1.51 KB, patch)
2012-05-11 15:17 PDT, Vicki Pfau
no flags Details | Formatted Diff | Diff
Patch (5.35 KB, patch)
2012-05-11 15:57 PDT, Vicki Pfau
simon.fraser: review-
Details | Formatted Diff | Diff
Patch (5.46 KB, patch)
2012-05-11 16:36 PDT, Vicki Pfau
no flags Details | Formatted Diff | Diff
Patch (7.25 KB, patch)
2012-05-11 18:13 PDT, Vicki Pfau
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vicki Pfau 2012-05-11 11:54:41 PDT
When tiled drawing is enabled, scroll areas in nested frames (e.g. iframes) are placed as though the frame is located at 0,0. This causes wheel scrolling to be broken when the frames are not at 0,0.

<rdar://problem/11338017>
Comment 1 Vicki Pfau 2012-05-11 11:57:39 PDT
Created attachment 141462 [details]
Patch
Comment 2 Vicki Pfau 2012-05-11 15:17:54 PDT
Created attachment 141507 [details]
Patch

This one should apply against ToT
Comment 3 WebKit Review Bot 2012-05-11 15:22:59 PDT
Attachment 141507 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog']" exit_code: 1
Source/WebCore/ChangeLog:11:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:12:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Vicki Pfau 2012-05-11 15:57:06 PDT
Created attachment 141519 [details]
Patch
Comment 5 Simon Fraser (smfr) 2012-05-11 16:04:41 PDT
Comment on attachment 141519 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Scroll areas in nested frames improperly placed when tiled drawing is enabled
> +        https://bugs.webkit.org/show_bug.cgi?id=86239
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Iteratively add positions of parent frames to placement of nested frame scroll areas.
> +
> +        Manual tests: ManualTests/scrollable-positioned-frame.html

This needs more explanation of what broke it and why this fix helps.

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:124
> +            for (Frame* nestedFrame = frame; owner; nestedFrame = owner->document()->frame(), owner = nestedFrame->ownerElement())
> +            if (owner->renderPart())
> +                offset.moveBy(owner->renderPart()->widget()->frameRect().location());

Weird indentation here. You want { } around the two lines.
Comment 6 Vicki Pfau 2012-05-11 16:36:45 PDT
Created attachment 141526 [details]
Patch
Comment 7 Darin Adler 2012-05-11 16:43:59 PDT
Comment on attachment 141526 [details]
Patch

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

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:125
> +            IntPoint offset;
> +            HTMLFrameOwnerElement* owner = frame->ownerElement();
> +            for (Frame* nestedFrame = frame; owner; nestedFrame = owner->document()->frame(), owner = nestedFrame->ownerElement()) {
> +                if (owner->renderPart())
> +                    offset.moveBy(owner->renderPart()->widget()->frameRect().location());
> +            }

Do we have an existing coordinate conversion function that could be used instead of this loop? The loop is OK with me, but I am a bit surprised to see it.

What guarantee do we have that widget() is non-zero?
Comment 8 Vicki Pfau 2012-05-11 18:13:57 PDT
Created attachment 141539 [details]
Patch
Comment 9 Anders Carlsson 2012-05-11 18:20:57 PDT
Comment on attachment 141539 [details]
Patch

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

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:110
> +static Region computeNonFastScrollableRegion(Frame* frame, const IntPoint& parentCorner = IntPoint())

Instead of "parentCorner" I'd call this frameLocation, since corner could be any of the four frame corners :)

Also, don't use a default argument here. It's better to just pass IntPoint() at the call site.
Comment 10 Vicki Pfau 2012-05-11 19:01:23 PDT
Committed r116832: <http://trac.webkit.org/changeset/116832>