Bug 85231 - Fixed position objects that are removed from the DOM don't kick off fixed position recalculation
Summary: Fixed position objects that are removed from the DOM don't kick off fixed pos...
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: Beth Dakin
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-04-30 14:15 PDT by Beth Dakin
Modified: 2012-05-01 08:08 PDT (History)
1 user (show)

See Also:


Attachments
Patch (2.15 KB, patch)
2012-04-30 14:22 PDT, Beth Dakin
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2012-04-30 14:15:22 PDT
Fixed position objects that are removed from the DOM don't kick off fixed position recalculation. It is currently a limitation of tiled scrolling that we can't go into tiled scrolling mode when there are fixed position objects. But because of this bug, some pages will be stuck in non-tiled scrolling mode even though the fixed position objects have been destroyed.

<rdar://problem/11297916>

Patch forthcoming.
Comment 1 Beth Dakin 2012-04-30 14:22:58 PDT
Created attachment 139516 [details]
Patch
Comment 2 Darin Adler 2012-04-30 15:37:26 PDT
Comment on attachment 139516 [details]
Patch

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

Is there a way to write a regression test for this? Normally we would not want to land a code change without a test.

> Source/WebCore/rendering/RenderBox.cpp:259
> +    if (frameView && styleToUse) {

I’d suggest putting the styleToUse if before even fetching the frameView. I’d write this:

    if (styleToUse) {
        if (RenderView* view = this->view()) {
            if (FrameView* frameView = view->frameView()) {

> Source/WebCore/rendering/RenderBox.cpp:260
> +        // If this renderer is owning renderer for the frameview's custom scrollbars,

I think it should be “frame view” or “frameView”, not “frameview”.
Comment 3 Beth Dakin 2012-05-01 07:53:27 PDT
(In reply to comment #2)
> (From update of attachment 139516 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139516&action=review
> 
> Is there a way to write a regression test for this? Normally we would not want to land a code change without a test.
> 

We would need the ability via WebKitTestRunner to detect whether a page is in tiled scrolling mode, and I don't think we currently have the ability to do that. I will talk to Anders about it when he gets in. 

> > Source/WebCore/rendering/RenderBox.cpp:259
> > +    if (frameView && styleToUse) {
> 
> I’d suggest putting the styleToUse if before even fetching the frameView. I’d write this:
> 
>     if (styleToUse) {
>         if (RenderView* view = this->view()) {
>             if (FrameView* frameView = view->frameView()) {
> 

Good call, will fix.

> > Source/WebCore/rendering/RenderBox.cpp:260
> > +        // If this renderer is owning renderer for the frameview's custom scrollbars,
> 
> I think it should be “frame view” or “frameView”, not “frameview”.

I think I like FrameView, which is not actually one of your suggestions, but I suspect you would like it too?
Comment 4 Beth Dakin 2012-05-01 08:08:59 PDT
I will still talk to Anders about a test for this, but in the meantime: http://trac.webkit.org/changeset/115725