Bug 54297 - position fixed element does not render properly when dynamically updated via javascript
Summary: position fixed element does not render properly when dynamically updated via ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-11 11:14 PST by Darth
Modified: 2011-04-21 10:27 PDT (History)
9 users (show)

See Also:


Attachments
testcase (348 bytes, text/html)
2011-02-11 11:14 PST, Darth
no flags Details
Seeming Fix (615 bytes, patch)
2011-02-23 15:49 PST, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (631 bytes, patch)
2011-02-23 16:26 PST, Levi Weintraub
no flags Details | Formatted Diff | Diff
Third try (773 bytes, patch)
2011-02-24 17:24 PST, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (32.83 KB, patch)
2011-02-25 13:46 PST, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (57.54 KB, patch)
2011-03-02 16:48 PST, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (72.21 KB, patch)
2011-03-24 16:37 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (67.98 KB, patch)
2011-04-20 15:06 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darth 2011-02-11 11:14:55 PST
Created attachment 82147 [details]
testcase

Downstream bug - http://code.google.com/p/chromium/issues/detail?id=72553

In the attached test case, scroll down. If you don't see scroll bar, reduce the window size.
You will notice that the ticking fixed position div in the top left doesn't get updated properly.
This works fine under non webkit browsers. Scrolling back to the top shows the div being updated as normal.
Comment 1 Levi Weintraub 2011-02-23 14:20:42 PST
The repaint rect isn't properly being translated to view coordinates. If you scroll only a tiny bit down from the top, you can see part of the box being updated while the rest stays static.
Comment 2 Levi Weintraub 2011-02-23 15:49:07 PST
Created attachment 83567 [details]
Seeming Fix

I am not an expert at this code, but the attached patch causes the problem to go away. I'm running the pixel tests at the moment with it to see if I'm missing something... I'd love a second opinion :)
Comment 3 Simon Fraser (smfr) 2011-02-23 16:00:59 PST
Comment on attachment 83567 [details]
Seeming Fix

This doesn't do the right thing for fixed position inside a transformed element, which no longer behaves as fixed.
Comment 4 Levi Weintraub 2011-02-23 16:26:27 PST
Created attachment 83574 [details]
Patch

I believe this would properly address that case.  Unless you tell me otherwise I'll create a legitimate patch and test case.
Comment 5 Simon Fraser (smfr) 2011-02-23 16:28:20 PST
Comment on attachment 83574 [details]
Patch

No, any ancestor can have a transform.
Comment 6 Levi Weintraub 2011-02-24 11:33:03 PST
Thanks for the help! Is there a faster way of finding that this is the case than walking ancestors/layers?
Comment 7 Simon Fraser (smfr) 2011-02-24 13:59:55 PST
Ideally it would just fall out of walking the renderers during repaint (see the 'fixed' param to mapLocalToContainer).
Comment 8 Levi Weintraub 2011-02-24 17:24:25 PST
Created attachment 83753 [details]
Third try

Once again, appreciate the help! The problem only exists in the LayoutState fast path, so that's where I put the fix.

I tried it with a containing div with a transform and it worked as expected. Thoughts?
Comment 9 Simon Fraser (smfr) 2011-02-24 18:25:59 PST
Not sure about that fix yet, but your patch will also have to include a repaint test (see tests in fast/repaint).
Comment 10 Levi Weintraub 2011-02-25 13:46:08 PST
Created attachment 83870 [details]
Patch
Comment 11 Levi Weintraub 2011-03-01 16:39:26 PST
(In reply to comment #10)
> Created an attachment (id=83870) [details]
> Patch

I believe this is correct if you can review :)
Comment 12 Simon Fraser (smfr) 2011-03-01 16:50:27 PST
Comment on attachment 83870 [details]
Patch

Your testcase does not test a fixed position element inside a transformed (but not fixed) element.
Comment 13 Levi Weintraub 2011-03-02 16:48:48 PST
Created attachment 84486 [details]
Patch
Comment 14 Levi Weintraub 2011-03-02 16:52:02 PST
Interestingly enough, the bug wouldn't reproduce if I put all three of the types I wanted to test in the same document... Let me know if this handles all the cases you're interested in.
Comment 15 Levi Weintraub 2011-03-24 13:53:58 PDT
smfr: ping? :)
Comment 16 Benjamin Poulain 2011-03-24 15:29:02 PDT
Comment on attachment 84486 [details]
Patch

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

> LayoutTests/fast/repaint/transformed-contents-changed-after-scroll.html:27
> +      <div style="-webkit-transform: rotateX(360deg); position:fixed; top: 70px; left: 10px;">
> +          <div id="target1" style="width: 100px; position: fixed; top: 0px; left: 0px; border: 1px solid black; text-align: center;">FAIL</div>
> +      </div>
> +      <div style="-webkit-transform: rotateX(360deg); position:relative; top: 262px; left: 130px;">

For the test coverage, especially of repaint issue, rotateX(360) is not a very interesting transform.

Translate can be more useful (sometimes showing the repaint rect is not translate). Rotation of a small number can be interesting but it usually create larger update region which can hide problems.
Comment 17 Levi Weintraub 2011-03-24 16:37:13 PDT
Created attachment 86856 [details]
Patch
Comment 18 Simon Fraser (smfr) 2011-03-27 09:39:37 PDT
Comment on attachment 86856 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:1403
> +            if (style()->position() == FixedPosition) {
> +                bool containerSkipped;
> +                fixed = true;
> +                RenderObject* o = container(repaintContainer, &containerSkipped);
> +                if (o)
> +                    o->computeRectForRepaint(repaintContainer, rect, fixed);
> +            }

Maybe we should instead disable layout state for fixed position elements?

> LayoutTests/fast/repaint/transformed-contents-changed-after-scroll.html:1
> +<html>

It's best to avoid scrollbars in pixel results if you can. You can say overflow:hidden and still scroll programmatically from JS
Comment 19 Levi Weintraub 2011-04-20 09:03:51 PDT
This has miraculously started working properly in Safari with ToT WebKit, but is still broken in Chrome...
Comment 20 Levi Weintraub 2011-04-20 15:06:48 PDT
Created attachment 90423 [details]
Patch
Comment 21 Simon Fraser (smfr) 2011-04-20 15:33:25 PDT
Comment on attachment 90423 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:1436
> +        // LayoutState is only valid for root-relative, non-fixed position repainting
> +        if (v->layoutStateEnabled() && !repaintContainer && style()->position() != FixedPosition) {

This feels like a bandaid. Why does repainting in fixedpos elements normally work, but just not in this one case?
Comment 22 Levi Weintraub 2011-04-20 16:17:55 PDT
(In reply to comment #21)
> This feels like a bandaid. Why does repainting in fixedpos elements normally work, but just not in this one case?

It works fine when the fixed-position container re-layouts its children

(In reply to comment #21)
> (From update of attachment 90423 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90423&action=review
> 
> > Source/WebCore/rendering/RenderBox.cpp:1436
> > +        // LayoutState is only valid for root-relative, non-fixed position repainting
> > +        if (v->layoutStateEnabled() && !repaintContainer && style()->position() != FixedPosition) {
> 
> This feels like a bandaid. Why does repainting in fixedpos elements normally work, but just not in this one case?

When the layer positions are updated, it paints correctly. When a RenderView that corresponds to the fixed content is being layed out, it calculates the incorrect repaint rect. Here are stack traces for the working and broken cases:

Working
#0	0x1020a064c in WebCore::RenderBox::computeRectForRepaint at RenderBox.cpp:1436
#1	0x1020a0f0c in WebCore::RenderBox::clippedOverflowRectForRepaint at RenderBox.cpp:1420
#2	0x1020eb6a0 in WebCore::RenderLayer::updateLayerPositions at RenderLayer.cpp:335
#3	0x1020eb95c in WebCore::RenderLayer::updateLayerPositions at RenderLayer.cpp:373
#4	0x101a48aac in WebCore::FrameView::layout at FrameView.cpp:977
#5	0x101a4940a in WebCore::FrameView::layoutTimerFired at FrameView.cpp:1657
#6	0x101a4c951 in WebCore::Timer<WebCore::FrameView>::fired at Timer.h:100


Broken
#0	0x1020a064c in WebCore::RenderBox::computeRectForRepaint at RenderBox.cpp:1436
#1	0x102123b47 in WebCore::RenderObject::repaintRectangle at RenderObject.cpp:1207
#2	0x10206f6d3 in WebCore::RenderBlock::layoutBlock at RenderBlock.cpp:1309
#3	0x102066e82 in WebCore::RenderBlock::layout at RenderBlock.cpp:1122
#4	0x102092913 in WebCore::RenderObject::layoutIfNeeded at RenderObject.h:522
#5	0x102065bc7 in WebCore::RenderBlock::layoutPositionedObjects at RenderBlock.cpp:2177
#6	0x10206bc77 in WebCore::RenderBlock::simplifiedLayout at RenderBlock.cpp:2127
#7	0x10206eb80 in WebCore::RenderBlock::layoutBlock at RenderBlock.cpp:1137
#8	0x102066e82 in WebCore::RenderBlock::layout at RenderBlock.cpp:1122
#9	0x1021d287b in WebCore::RenderView::layout at RenderView.cpp:130
#10	0x101a48948 in WebCore::FrameView::layout at FrameView.cpp:951
#11	0x101a4940a in WebCore::FrameView::layoutTimerFired at FrameView.cpp:1657
#12	0x101a4c951 in WebCore::Timer<WebCore::FrameView>::fired at Timer.h:100
Comment 23 Levi Weintraub 2011-04-21 10:24:23 PDT
Comment on attachment 90423 [details]
Patch

Clearing flags on attachment: 90423

Committed r84516: <http://trac.webkit.org/changeset/84516>