Bug 86239 – REGRESSION (r114170): Scroll areas in nested frames improperly placed when tiled drawing is enabled

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Vicki Pfau]

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>

[Vicki Pfau]

Created attachment 141462 [details]
Patch

[Vicki Pfau]

Created attachment 141507 [details]
Patch

This one should apply against ToT

[WebKit Review Bot]

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.

[Vicki Pfau]

Created attachment 141519 [details]
Patch

[Simon Fraser (smfr)]

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.

[Vicki Pfau]

Created attachment 141526 [details]
Patch

[Darin Adler]

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?

[Vicki Pfau]

Created attachment 141539 [details]
Patch

[Anders Carlsson]

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.

[Vicki Pfau]

Committed r116832: <http://trac.webkit.org/changeset/116832>