position fixed element does not render properly when dynamically updated via javascript

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Darth]

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.

[Levi Weintraub]

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.

[Levi Weintraub]

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 :)

[Simon Fraser (smfr)]

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.

[Levi Weintraub]

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.

[Simon Fraser (smfr)]

Comment on attachment 83574 [details]
Patch

No, any ancestor can have a transform.

[Levi Weintraub]

Thanks for the help! Is there a faster way of finding that this is the case than walking ancestors/layers?

[Simon Fraser (smfr)]

Ideally it would just fall out of walking the renderers during repaint (see the 'fixed' param to mapLocalToContainer).

[Levi Weintraub]

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?

[Simon Fraser (smfr)]

Not sure about that fix yet, but your patch will also have to include a repaint test (see tests in fast/repaint).

[Levi Weintraub]

Created attachment 83870 [details]
Patch

[Levi Weintraub]

(In reply to comment #10)
> Created an attachment (id=83870) [details]
> Patch

I believe this is correct if you can review :)

[Simon Fraser (smfr)]

Comment on attachment 83870 [details]
Patch

Your testcase does not test a fixed position element inside a transformed (but not fixed) element.

[Levi Weintraub]

Created attachment 84486 [details]
Patch

[Levi Weintraub]

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.

[Levi Weintraub]

smfr: ping? :)

[Benjamin Poulain]

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.

[Levi Weintraub]

Created attachment 86856 [details]
Patch

[Simon Fraser (smfr)]

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

[Levi Weintraub]

This has miraculously started working properly in Safari with ToT WebKit, but is still broken in Chrome...

[Levi Weintraub]

Created attachment 90423 [details]
Patch

[Simon Fraser (smfr)]

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?

[Levi Weintraub]

(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

[Levi Weintraub]

Comment on attachment 90423 [details]
Patch

Clearing flags on attachment: 90423

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