Bug 36652 – Fixed, transformed elements are not repainted while scrolling or update

• Component: Layout and Rendering
• Status: RESOLVED INVALID

[Benjamin Poulain]

Fixed, transformed elements are not repainted correctly because the update rect we sent to ChromeClient does not correspond to the area where the element really is.

It seems there are two patches causing this issue:
-31283: update the way fixed, transformed element are handled, but did not update RenderBox::computeRectForRepaint() accordingly.
-33150: paintingRootRect() is used to find the repaint rect. This method does not (and should not) take into account the transformation of the current element.

[Benjamin Poulain]

Created attachment 51725 [details]
Patch

I could not make an layouttest because:
1) We don't have pixel tests in Qt
1) Apple's port don't use FrameView.

Any idea is welcome on how to test the repaint rect after scrolling.

[Simon Fraser (smfr)]

Does the bug reproduce on Mac?

[Benjamin Poulain]

(In reply to comment #2)
> Does the bug reproduce on Mac?

The change in RenderBox::computeRectForRepaint() is a problem for Mac. I just found a way to reproduce it:
-scroll the page
-update the transformed-fixed block
-enjoy :)

So I guess I have now a way to do the pixel test :)

[Benjamin Poulain]

Created attachment 51743 [details]
Test case for Safari

A test page to test the problem easily on Safari trunk:
-scroll the page
-open the inspector
-change the color of the transformed rect
-> the color does not change

[Benjamin Poulain]

Created attachment 51750 [details]
Test case for Safari

Test case with Javascript, the one I plan to use for the pixel test.

[Benjamin Poulain]

Created attachment 51757 [details]
Patch

Same patch with a pixel test for Mac.

[Benjamin Poulain]

(In reply to comment #3)
> (In reply to comment #2)
> > Does the bug reproduce on Mac?
> 
> The change in RenderBox::computeRectForRepaint() is a problem for Mac.

I meant the change in RenderBox::computeRectForRepaint() also fix a problem for Mac...

[Simon Fraser (smfr)]

I plan to review this but I'll have to think about it for a bit. Please don't r+ it yet.

[Simon Fraser (smfr)]

Comment on attachment 51757 [details]
Patch

> From 99851c34f49d4e81dc4fff9f21d37b30688a260a Mon Sep 17 00:00:00 2001
> From: Benjamin Poulain <[e-mail]>
> Date: Fri, 26 Mar 2010 18:24:34 +0100
> Subject: [PATCH] Patch
> 
> ---
>  LayoutTests/ChangeLog                              |   12 ++++++++
>  .../fixed-transformed-update-after-scroll.html     |   28 ++++++++++++++++++++
>  ...ansformed-update-after-scroll-expected.checksum |    1 +
>  ...ed-transformed-update-after-scroll-expected.png |  Bin 0 -> 18398 bytes
>  ...ed-transformed-update-after-scroll-expected.txt |   10 +++++++
>  WebCore/ChangeLog                                  |   27 +++++++++++++++++++
>  WebCore/page/FrameView.cpp                         |    3 +-
>  WebCore/rendering/RenderBox.cpp                    |    1 -
>  WebCore/rendering/RenderObject.cpp                 |    8 +++++
>  WebCore/rendering/RenderObject.h                   |    4 ++-
>  10 files changed, 90 insertions(+), 4 deletions(-)
>  create mode 100644 LayoutTests/fast/repaint/fixed-transformed-update-after-scroll.html
>  create mode 100644 LayoutTests/platform/mac/fast/repaint/fixed-transformed-update-after-scroll-expected.checksum
>  create mode 100644 LayoutTests/platform/mac/fast/repaint/fixed-transformed-update-after-scroll-expected.png
>  create mode 100644 LayoutTests/platform/mac/fast/repaint/fixed-transformed-update-after-scroll-expected.txt
> 
> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
> index b4bdacc..051db0f 100644
> --- a/LayoutTests/ChangeLog
> +++ b/LayoutTests/ChangeLog
> @@ -1,3 +1,15 @@
> [phone]  Benjamin Poulain  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Fixed, transformed elements are not repainted while scrolling or update
> +        https://bugs.webkit.org/show_bug.cgi?id=36652
> +
> +        * fast/repaint/fixed-transformed-update-after-scroll.html: Added.
> +        * platform/mac/fast/repaint/fixed-transformed-update-after-scroll-expected.checksum: Added.
> +        * platform/mac/fast/repaint/fixed-transformed-update-after-scroll-expected.png: Added.
> +        * platform/mac/fast/repaint/fixed-transformed-update-after-scroll-expected.txt: Added.
> +
>  2010-03-26  Yael Aharon  <[e-mail]>
>  
>          Reviewed by Antti Koivisto.
> diff --git a/LayoutTests/fast/repaint/fixed-transformed-update-after-scroll.html b/LayoutTests/fast/repaint/fixed-transformed-update-after-scroll.html
> new file mode 100644
> index 0000000..78709cb
> --- /dev/null
> +++ b/LayoutTests/fast/repaint/fixed-transformed-update-after-scroll.html
> @@ -0,0 +1,28 @@
> +<html>
> +</head>
> +    <script type="text/javascript">
> +        function scrollAndRepaint()
> +        {
> +            scrollBy(0, 100);
> +            if (window.layoutTestController) {
> +                document.body.offsetTop;
> +                layoutTestController.display();
> +                repaintTest();
> +            } else {
> +                setTimeout(repaintTest, 100);
> +            }
> +       }
> +       function repaintTest()
> +       {
> +           document.getElementById('transformed').style.backgroundColor = "blue";
> +       }
> +    </script>
> +</head>
> +<body onload="scrollAndRepaint()">
> +
> +<body style="height: 5000px;">
> + <div style="width: 100px; height: 100px; background-color: red; position: fixed; top: 60px; left: 60px;"></div>
> + <div id="transformed" style="width: 100px; height: 100px; background-color: green; position: fixed; top: 70px; left: 170px; -webkit-transform:
> +rotate(30deg);"></div>
> +</body>
> +</html>
> diff --git a/LayoutTests/platform/mac/fast/repaint/fixed-transformed-update-after-scroll-expected.checksum b/LayoutTests/platform/mac/fast/repaint/fixed-transformed-update-after-scroll-expected.checksum
> new file mode 100644
> index 0000000..527f090
> --- /dev/null
> +++ b/LayoutTests/platform/mac/fast/repaint/fixed-transformed-update-after-scroll-expected.checksum
> @@ -0,0 +1 @@
> +08b405fcf3bf7631cd6a1d4fb9fa6e6b
> \ No newline at end of file
> diff --git a/LayoutTests/platform/mac/fast/repaint/fixed-transformed-update-after-scroll-expected.png b/LayoutTests/platform/mac/fast/repaint/fixed-transformed-update-after-scroll-expected.png
> new file mode 100644
> index 0000000000000000000000000000000000000000..944fb098d46505bf1ddfa360e440a42c5180ee09
> GIT binary patch
> literal 18398
> zcmeHv`9GB3`~R41Sz=Nl*^3uhqHNiwtflCcLSYCY`<AhcWl{`cXrb&<sq9PEv1Ke-
> zqZo{>v9Dv_m(MLT<@Nsl1>eUbzv0~HzRq=?*YjG=x$fW_+8T^>C+R>S5aSh1)teyD
> zAs!G2Y(Ps5{3XWykrVLC>;n~*8&_0Rcy72jT0XG54+42mJh|0s%W~=1j<mzaZ+x_O
> zZI9%7MDSg?lV+{<LwXxxX3K!*E>MfGIDe(nL3(Od;PX4wFriJjUu`CtX4jv~YtBOa
> z>XW-_|MaK*W0uS+I^XPYt>E2cWsC1(zYNjG(R~2K=+b^+*`IQlvF_#@$OAjXWt%M&
> zDE!UgOBc@E#!qXXroI&5y6!#J<;8?F)T{R1gy+IDV%iI0XdZ7v2Q^~4g|msQ&UAN<
> zf>8<s$wGB*v;#w33)5&hZBLJ?&rD|(v3K|oP$|(Czw1T1&1b$t12kn{EIZ#+io=Dz
> zz;V(%aImbcXD-u;cwgEyn`V|Ar@e9{qAY&NS39p?=?m`{diD%u^`fwZ<)<CWgnK{V
> zj$#89Pq9fYt(hJ#Jo#}=>dP?`=IerQZaH_0G0wlr+}2yT`p79lw!z|_zw2F19o#jC
> zwUQtz9|0Uo76GpJ?@v!Oi&g)RKB8I=IP2UIPKJ{VF^rlwnAIrny~@4;4OBQOaUs`5
> znLmx1<GHuw!_zjUZf2@Em%OEW6$}4;+o!Pe51;c3mCv-#wl3`mXMNQf@my~-%I?xz
> zeUT9xQ?f89Uo-N$_#_wZd;O5>br|C15BKN#8d2$<UYMBalgiWj(GsJNq|jK`?9F*m
> zlb21=LQjv?4BcLw)O!5zu7u1$qMe_-B0;QG>~p7UMsTQ%UTn10wvFcI>?h3g(c9}f
> zu;uwJt7fb@K}bTtJ!FhGIJ!n`{#ln+8a=_(zK%BjWuQ>IedNaJ4pY+@%(A$#vY|(r
> zx*x;xzc1LE+HVD~`h5|$xDa1_#Bl?SuiIiBUJV&b{0A&<H(39=#9r5#$eeutor7y8
> zn6;wKXRg4(HpN$Vi_$<L?L@{+sYD(O#VPk7FRpQ+SKp(en&oZ=i6lNRnP@P(*>nT1
> za`Ptc4DP<xiM-dh%_iKtUVrCwIF&4Sc@>*__5SmXta(WGcJ|Hn_4J$FzS<`{k<lBD
> zm6&a{N{`TB5QqnKMfK7x4~m(-!!Imu`zX(uz;#-c&Lw?7`Jvb-!fX|=eazS21WSlX
> zJgQ}ra&D9Wixna193V2PVgH&xl@Q33h&`z&Aa+=yHYF&Dmo}IhOLgg9#JyLq6B2ee
> z%O+)XzcZ+;Uc=3NDoIS0%9`v*gA<9a8cZg$h636O^Vs-%yik7-1(=eSnFs1$38oUO
> z*jnf+cC|`3DwcGg?t<jy<;{qvzJG7#;^KnFVX>;4a;sl`^hL56OCi7$<VTZ^*GJ@7
> zHpdtgIv872);md@KmWbFq_T}$)y0td1O?WFhxCfQ2Tq7aAo-$qvUAL8Lb}R*yg4pQ
> zB9{DcR1plqme3Q;Gc^0s><yhfQ4fvGTGuKp^IJ)w=a)P5j7Sx+ZllEV=iS-=AQ@P<
> zQ%z^4>cI8oZU~LvLitxJQ7z<u$}lV}pg1jlnW$aZa3i-+4b&YQV*lqAw7qYk@QvGn
> zPp7gflrqa!dR8{jn0idvH%u|tX=21@_nDrqB4_jAW!}ErP2<9v81kC;-m}R(ZYd+_
> zVb_!Yv8?)bi|;iZ9pc7q>c}lID~%(HOWzF2ydG}GgK|UbISwj7INKgy(1r1Il`oC_
> z$!{g+VtHEpLhBiw<_3N0$h55Ti$~;!Biy+uERw&h@c&24Njdi$qhLwak2F?6U&Q=}
> zOJdJ_Wq!u`!OB2xJSm`;s&Rn#;EMz!e)6ekuquoWxXYlUcuz+=3?S4s%bWc3MRxwE
> z5NetXmP>2nR+6+^YV~z%-5C#$-W)41qUVVZks^ZwSZ(OhaV9R~@o`I)67~oMQ&w%t
> z2(|g1*MAg()J~%3-kuVB?1toDZC+(pD6zfV<nQ*V^MJZVG?SFw+=!)fZOVt94U9a)
> zmw?(!H98-~<ggrg73IpYad3TtA>8D#(ysS;p2c?vfj^2Ew4^HO4(pZB$!Tcq`=}>}
> zk0dT$UPcT&i2!7l(XypEpo39*i|p}7>Y0mGkJrCdAre!KMgk5mes&pYaPQlOfhJC;
> zAKt_M{vr%sKwDhw`Z0>-ejNpC^P?zH`Bd|<Vb%0I#ZpJ^7Rwzm<aVyVG-Wo^^I1Ys
> zv5#IL)TXNCz~Y##DJ2m`KFupd(;=KW52hMWFPle3zU$_gdGWbl%LuWJ^(laP8yopm
> zzc1?G#hX_LGUnKH2&bB^<5dpemO#01Ax%`KsrJ5d{hUPESp<M9>GPb;wl=xNJLY@>
> zg3`60@zOOvq?8|I(R{VN9QWw6tgZJSqxb;hIaL2rU*@N|7K@{
> zQe|es@AWS5zAYX{n01H_mDm<yOPXVw{cr8c&>YyvZRUEA%ksnKMa7sw=SIVf@Q;>d
> z_M0uQUfZ{Pu{l&9BJg-gOf?Y|pYsr31wfL?+|sg^q2AYH^YHIYL{3s=4k)n~7S&9S
> zZiL@n|Ay|8&O7g$?`f58tzB`qyUG7rR#U0zasrKH&Q3(vN_8W?)WVbb-m>3A(19I^
> zgSPlLHEGvgno5nWjKq0eIM01n;LTSqd(c<rcJD-dY+in6x=pnPtZ4RRM9m$1?15Dr
> zv%ahWTb1a3iw=%&9vhYFphgQs43`vNIBf-u(Rh?UU}gO^8bruq`YkkI(t_U}N5pM+
> zwLL}~=}2F($d%(fi<_YuXTK@wYyo|3826~-ioqXM^JF_A1+0gkb@L6ZNUA#;$}38)
> z7Q9~SGKWA{2Ir6mgn2@jf6r|8$0uoI{sVI{=cWwFxTgCrSF*LM?taMJ{zbt1qq|g+
> zWJ=p&C+jgedBx`}3Ot`@6>qgENGK-8ly6#DLfs8L4w4)nWlUoOd|Qyet|h+oxjKic
> zBw~2J<a}OCA-JB;f9C1|Tsa^yo44U%b<-`QkpZrWt}7|tiQcOtM=30165$76EOOO!
> zRv}HHM%Xb2AG>Vb*&Jxh`*z(yL!Pl+K9@a0?J)j7M$gll-ta9_OMWPFY7~(c-)yJJ
> zp&mnYQ7qIdO$4<|t_2*>LJi$;x`vosRU~m?$WET6y~+Oq(bbSuHKuTeAZ8H%_&<EZ
> z@~X7LLFsY!>m_T)!q`|ZCjMz32d<Fvx|d^sr_iu-JYeS=faML>jL$48#mwlsQoOzM
> znKiC(YRlXK0=;<H_Q1dsg5TZ+7cW~pR~K05pbR(88ZAW(i1z7}v>v1>W`GJtLj~OP
> z(VOGB>Mc*NS*}@E;C*B%EDE3ekE$FB_^UOZU+Bq>tV&t`*6fu(wPohu4Dk}Ycm06w
> z36wFR$^xU$)Zv_(L{Qlb;LFZ)c_O_EUs3!I2fc-g-?5<n%JVF%|D%4X6}0C;WO)i5
> zj}Ke_(ceV12RlTIhL|h*&;y-Pn*hx`(UdEj4R<2Sf3&Si``P@*t$(4EG;sTAZnfA|
> zHTyGw{hIre=c7VT^48yeDOUSm0*r&QhzRrywqe7z9G^qorO`@`0h-iku_sHM9cttM
> zj|~0O8pMkcO(&b*joFXa54PP$$8DcGl<TDHeLz+x7VyjieYr#JwPA7VT|Sw&10$pn
> z!;aqU?-WCHvmL<SEe~wZifBY(|4j3v;R(IwuLdNLYK%9YnX&g=^|>{0U|y)HBlz`&
> zm@`%v7dFuS=^A+s($jRj&2V{lou{-sijsZ@#%3EJ74klne#6AiTPuw{<dKi<j*xzO
> zLFhEjRqmZ4MmPIJnw{SPG43Mcmg~<5$}SlD#@Sn4j$w}&^>LVbq`6pN+)I`AJR9bM
> zWb&O0(DH2>ksW06Wp?hm2hr7SY};5X^lZN(80Hr#eEOGH84`{_Lmi_K8=mJUBvyI8
> zZ1sBcdHE{7)aCN|&gJ8XAJ_;0Aw9upA3JR8jvaPPNrp(z{o5t=%tf-MSan(NaOgI3
> zvqKkr0>j+n{hCC<ngF^?#lhTbP^{;envv5bYykJE`Jy^C#3tUmyG?+!6lfUOIOdh1
> zj<hZ>%eoD?E>nNexSSX@E7)fR*u*GhW`z==M!f&|5I=Wt(f4@17Fm8m$<)eLiy(P|
> z4Df4lmzI>mS)t(vcb>B=^$^_~X?aRTG5py<m9{1v^YO&#oR8bt4{fn&u;b3tIS@G&
> zN%%}h7(D+8$?ngsfL-%A*_C46wgrYUa5V=xbfy@OvATwW+5svZXDOP_Mah)sI99{@
> zeTm)>9fTRyXO?AtM4Wh^CYDT$Lt@{Iwj?B2rAy}8&dL?8T0$kAhV3*9X?XzBKq?`}
> zl$FgIqskdKJvO;_A<hGv>I&;$A?#O0NW6yhqV)R8SFJ9NTAKCkfYG-<33pHo;`9Hp
> zSg7)j{@5DMFRb2H@kHM60|TPB309&`b@-#9OmFrp6MQsz9Wmf$W$<S$_|=U}o_4$D
> z-nQ}>Y$WcHo7Mo{;kgvji^?)KbvzESu!F1cbj51H+DmV24{;Cd4y~Obc^K1Ue{2zv
> znEclJKBIiBUwMOH?PSyjm+D$F)5Fh(Om|IrJNpK@F0I~2*6lu54qofQohVvVa^I}j
> z4fSX;=hK$uCB(V%&HAqTOi!OB?NsFlx(iiiZ-P12^A{0E?<ukSSS=<q^06z#%q%93
> z*il>DqA{$9HX7`)<%g~0zEhkYQy%a}{Uq-CVsGn<&g5{0k(Qa=3oi4E?v?NUBr_$^
> z78uUJwJsMOw>_Rn0fBmn?VM(|N?>=cIj+SNxf|eJC(k$QQHOGE4Ff{?3A~dkSsEAG
> z{8VG9fBAQxN19tDI7$^-&_1Zv@NIc2`D@D_c7UUej?NbpGDZ!?*m~ijduJRVi91WC
> zco#P^^bR9O#V|7ydflCN-gJ6IR}PJh@~Or%y#6oP*zFn0Duq&2?eKc<V_A)l&1VOx
> zDQx|O3~X7xmi~O1#Jg-Ls6d$I*Vo~gofMg=w)9xW(#x8Qec^p9{9sLVQ=$|-V)Q&D
> zam&IJX}6dO539Edx9ho8?juSjjw827zRmlmXXaw=lBM)pM*e1aaRUBbUn_%V;~@Gj
> zm8B)0w&KI}Ppwmz7J{cdeKh-#XLo0izrimImYa&y+Tp)e-iXedxu$vkP&=m8-kAA8
> zf59&U&=-op`51ZpT!{7^c?r?+zKboJJwE?LT*S|BiR?eJd60+&)4XfaX^3Sm)mKDs
> z9o5!W-Rp#U{lcZP8R<JLA5;c4ltW~v(tmzh-dgl-Jt8-cMQMq1W|mJdlA-4awx6?p
> zf3at-ZTehCU}W%0tv<VS4BB3H!uPDd+tJRB!*RY|*baRrt8VTcC#-Xw8fI$*vhMbf
> zo;<@iWdRWtEOy<Z!TL?D>hsGB`Dxeq*)@BX+A>_d^W2mtd=><eS1HTq!%*-F@1oGM
> zpMm`h%{g`ITO%ebEgw(Ka!qmW&voNvWjF?Zo7kd1^O<{HiRk;1HK}zLH9BRC^Si(e
> z7X{ag(K*^2m0cemb$9aFRbd<Lso3m84QynfHv7a0XHD2g6DB#xb3tP-##{+6Pl_zW
> z_q9Ea;AB^i@#}2x>u-qNX<F*D)&PB+bPP7r9W>_-T^sY`QVjLmT}62h$P<Kgg4cFd
> zHL`ZSuI;P(>1!y9r2B=hZ&tU^{nK^Vp3Ayl5VCmRt@Qk73O&zt+OOa97S8xT?H)KZ
> zGHqx&)rtD9jbBx&-m>2b4OHIDLz#yMd-P55<#LgAmXr^r4nD36Q7-ni#AU$ow#q%-
> z5iqvy9CJ&YNd1bap}CEXuz;W-RYZHhyRwnv^;QmuDZ@s5&^znxk|{pe(i@cgfE>Yb
> z$5JSKoQ=$7HHXuU6*=^`t!<4yKd{}hXbdkJEfH|KkZRIgP-U*sBs~UezQCjy+kKG*
> zpH*>DFVm=-o;TFCwx3d6pGU{gYsOZiaMydvGw`E^@<jqKQ}EgbZnYqYoE9VBvDULk
> zs2oXn%Gn&BUG7)IIA6xy&#Z4!ShmXiML{a97-aGOFdsX#z;WP&e4#^#lx-(A<oMYp
> zf01n`Swc1)X6^|@&h=cl7VNuAtGBwsW;bLV!9v!FX#|)ZdZe@_MApOJl}-;v({8jX
> zfyeYrDZBIm9RIz^-!AciB{Z{CxKoFSdN|ejIgclkf_=DNcI6xGa9_#}K|Jui{GkcW
> z{`-U(M|9B;Cx8D&XFsPPb$dhr$#gsDEkJilErr7|*I6!i4%=rgWa@kV>$PapiI2ry
> zTkI*muWR?J=V)P95pQRvu`Tu6jS9J!py4NYPkf&dE_=B8c;aFls<cm(a6a*4vY8ZV
> z+D6_8))xbWh?ma2k>8z%lkw);KMF>xxX)jw?dCdbDP0%Xoi;SLj2|ip;ygvejc`i8
> z`}tEDtxt`Mn_evCRl4D(y6Ey-xivqnrz&>k3CT=h9$0?91;S<=dk#72iMq;TbxHEE
> zok0a(XO>}iDX7ec`De?hd+GOPySkWH2?_lV=RIc=3#T8_b>0l~A7Xe{Iob>n_np7t
> zo?(!>vqR*cb&2ku3Cb!fFvvT}x1ZYzGuPjUEW|a-8;uC*aY)-)Tggj1^mdiE+V}Nk
> zxeORlM$qk;{V+1?UlvY04cpAkcn$H{p7PG)A$sbZm%nIOirUx@N|5i40R;fbr^E4?
> zCHlUzk2G)zdu|*ZU->S2MO*P?>6ar(N!&Z<2&=y0gta05oe`5GgHT^`8qyUC4Ga@=
> za+_K&J<PMHhT!5rjwy;00y%2m$DI)!wlxX$_!hv&wY!FD6f<mBk!K@wPa+sxujGl4
> z`vf-GGQ-v>Nq;I6#}S!)&k>4-_+axPsHrS*TxQgMxB$3f$_TGIU%RudQydEgrBP1~
> z+jSpx9(@g|<Kl=!Vt+e`xBehWP2)P)2_(Eip0KqT{ca-CVQHb!JJq6ZcL!p!J*gS}
> zW^v!_Dw*kNc~lf+OA0)Y*JFWD$$Y+IF|-f7yW&_D`onJ(&JP5HefTnR#YW_bKPdND
> zNVvoWu%~GCOZ_9);pzqa>p#|%x4-WZh3w`u3KNz^b`;sifEC{S=VI**S|fxr7C)eL
> z%$Pk^duvz_B96F+jVGR(hx^G$-xqqk@23saDJU%1=cUT>!Kn!+b9^@2xxTk~fp<46
> zAZUaIR($&=)^D5jIN2ZX0K#uNjzz1Sp8Kk!KHMm2fNb@ZCd|UoYXQorwRtoRJ}QNj
> z*A5Bs_&*&h(!1T!bWK&s?b}Ut)4Fg}VVNi)96lYt5S1uOCZZ>Fv^?}La|gcBVU{x>
> zti~7e`#A97BMkL<iG8t<udgA$ZJrlc-Q^e4=kvk(#>bsI@2KF*gfB+n9uI%!MSI;+
> zg`G7c=Pc7&P*8VtI57}5_)a{mXf!67XnCA-3tutQCwi6`^Ag^gOE#4i0UrP9>oo<D
> z%teEbP^%cL(<OJW3od%{uJ=YDeCxdjI3inFejC;X01975qQ=$1;phBDAjLS9D4iRA
> zle*qIx0H#PpVKKYQcbgwpT}PWU8=`)V=#p~Y;9|dX^zKN$qAX|D@CASLV0{);S;Ms
> z3^qR?#~BpBR!~x3VO>2|^fPeJC%DGyREy5(`|Sn9WgOIY-IBdN9rF8oVVVu7!qkV;
> z2LyKXP{9^fDEc3gABIxx#o{C4XxZ5!6qAp2(seo6ee9pZ0^^yP!ZZ5}2E7MD*Q%8<
> z^X`V~8iToweA*X<2$z^biWVB$CJGZsB&D?{eppJ%v7_~d7>9nN4-(A`w@kmkN2xGz
> zTC*I{-j|vo8NmMIfHzv+tZNI={7Q4kGC!-S@xyVh+SngI#+MDzB!fT1!VEpq)S5+q
> zczE$NU`z>T|D{D#HgAnOs&9P}XDEUM3GQ=D;7urqz2RQl$8|8#&rh|*-$32OwBr*B
> zSLU!Zb%SDmHQ9c^YIcg(Z(l-UMu%#|#k!K-Qwz^MVpZ+kSvh3?rHb@c-V@BwKo2Lk
> z48gfceU0J1ERhs(Cp=z#IE<1M(nHI@V9hdf<)bgh7M1$9MkB{RX^TMzr}OwbvrH<(
> z-AQ<=)dQr$Cv&2|B<yaX5DUfkm#BPa$M_QF9=BzwP?2mKd6<@mW^@d9=oqlw{5u<2
> zG+q8GtSqlyp=4KkQAUO&UMT?}R8r?>oi>n@0>}3y&m@J5ULCxm6H>HV#=6WRPck5L
> zVZeYUD#N|*5WV!VRIE?Ej&CG_r#jUzqf#Q|j7E`y0z2?HnFT;ND!R;WyyTtArqEn_
> zwEEs+$p&_e8M>v&b`hzM;Uj!X3(PdUn9mJXUe<-ACX{e1FR_pyWe3QW!`ag&GW{l$
> zuYYHyZMqaTU@&;yF~p#3qZV&Z(#7O)X6Pj6W^%g1uKjvb(+&6QsV9V-z8r?2eV6$B
> zEa0TsruSy!CbS97u_U6_>ikZ{?@Ufyr@tn>+v?IHC$nAV_+Og+AUsg6NGNXQ{3JHf
> zL_w=C#14U1Y*L)6)FEwV4)ESucWaHbY=mZC4J%qNaoL>+=8#d3d)V1uk@u36dGPqJ
> z$!AZ4^<nJu4f<l<GH#~9lr0M92~mYcZvx~PNao-U0H}*`V*L1v*5I!E2TL0nbuuUR
> ztj0=l>d#VX-2ik5-YXLUk0o-?C#;HSeLu<R5AAn{dwGFq+%?IHqXd@9-q0p83~+*_
> zl|~-Xt1PuLYx^!XYpWpvk07~E+9p<JpdPelbZxMDzG#iW8Y-2VOaaug>Nxt(RZ^cK
> zoeHpV07PIH=#o!Y%&S-Tz<I^`;ax|^n}c$#!Lln|v0FW?<b5>;76F8~SLrZHFy`pm
> zf6v7J==7&NA}>OCdY;<NDd^0;N!4lt;4S6l+<8>ap@HSD4GpYF@haj$;xGb)!2s6$
> zw0JU_c>UM3ZW!_Cl$9uTo^aF;&=Pk-d#fref7J3b(%3IWP}b@Lvkl~MHZ+tUTMerS
> zsbji7;AS%q9*bj<nAlZ2!8EonUy>Qc?)Z)Q!Dulbt83V=^HKGm0i)qsEkY&kuKzI_
> z=p71*x9RYXj52Xw!SbVZ`u69<YO-LQ&IPfr!Wx}E?brQ|0ec_fg2Q(`%u#|JZSH7@
> z%3+qB$w#I1V(mm~Z$f@)Bq`q?bn`BzeFerT9!9MD`BG0AcAwi>Rh63gw+Hb9wrcy|
> z#aOk&h_m70GIbwAH^SnBfOxj-ZlQz6W~vp*l$`)iSFNaI-c~wR!(nclelql~jW|JR
> zHP{0a`>JCH2i^WPaL^x^?+{^eD3)a{)kAaZy29kB`{Fh&?X?Rs;_?TK9;gYQ1>8(l
> z+(2J?W%#QQ-8Y^$9^V(C^c{oqg~M6$a@e%gemNxM2w<&Ix_(Xga$yH{n!5KAS-Ig0
> zomps)8QZB3w_lRvGX<=xYD3Rw$*}JDeAc}GTBwKFR>hij$B7i!)<mjC@2GSN?8y#^
> zN~(Ey{28kMdFsjSdsc~=wZ_@m3nSc#jgrW6-)fNG7)R-9vsaYdQr#zY5i}5UD9m<z
> zE|Jbe>({c|Gp{$VX+B&=yHq(M%(22U5*#DEKlbhX+${iuHyG>I;G^|LB1)&wp?<+M
> zwXL<d+KNF))?5`+xW@}%BiH_!RQhFkY+?Yoi)vG1`W2FjFAkq|Mqn1>A%*1+rS79|
> zCF|-!#)^WwHN#%F{BlFcf0>z~>i1sM=A)lWaOn7XVQW}56jqBNXZ6D$B=g_xJfFz!
> zj4KVd9(zbC*|XgR=C=-dpAgRJc0(0rH5cZnx;2^Ask?ek->aS%VwE7*r1(RIWVp8g
> zE&j_{EHS;xNo1Q3)=@=le22ix7uin8UOsbwt*Ay+{X=yLDeQdu6!3MDDz|ks{MMuS
> zcNUfT0e^wol9`Xn1;a~pv3Uu%3$AC7y#F#SEzk1PWzE|qW@^N47NATKiQ^llYKxKt
> zvv0Kz{Ei(T19Q^et}~KEDlm?75>c{%d%>JZzn=|8d#GYnxG#1|pNijSwhx9-&`0kr
> zjrSX+y=MrCJ3<5?uBaDuHH?Ds5HCl`9Lgp~G}J)o#c&gs0~BgxeEA7q==ovWlc?pX
> z=&iZi+;ad&EvSP|?5n`Pk_XVirOAscP)faw5Rb1~Uad^)GXvqTUa@JOzs<R%R~RC@
> zz4Ym-p2)BL<H^1TY`A2V?Mfxp5oHBJ>#hQ!(ImWx=)<(-A;tYuDl52tEXROMC;@;2
> zrWT!EJH#rxvxZu1nR*h*AI|3CYL2T^B(l%HNPTL2hmJ&#fwur+X-BqY)EoxtXPy^3
> zWh&U`0Z*0=D=QknVO9<Mj{*d9*k<Q!lKU^=Z``U>c0V@K*_A}qnGx#Ik;W%h@(-W5
> zLE*IF;M?bXTx*p+#34iSI&T6|rq;&^0~LO!AODsauK50>hpZOWeRH$@a52~NI(7;s
> zbIZ7$eSdfo7+|GGmokg7G>ZQa(oDkEhLpo)r?X?uI2dg{NL4%KIUH7q|GkT(41kLc
> zi7Efd-ij(3eg>FS@T&x_5C>6PK?is#ih+l$ap(i&@pl|XD=XRx&fTRquG1*3RV52A
> z4+i|gla*o`tE$rS$K**^TMK4q8Dfh&VSm$n-|Rt1y-EgD1#QRSQ?A#rg39cX6vIVB
> zpUx`J!YeA5jdsX)0Z^E=Ub1ioM*xeE`|(-s7?{c10Ei=0F%#Dz95P4LqN)3XF~;-7
> zV@ELTjAZOM0P$Dxm6Ikvcp`8Cab7mWjfhlbMKc>4Oj--gk^U*zka5Z+$#ehQ3$MHp
> zOPLXsqK@qIEpT<jj4ebk?#73<FU@vc%=44_tPZza$+CL_$WdVCo;p#4@8eTiEk>ac
> zrwJ>C+~T3%b<jo4_O)pO9V9VQ1cBJ6|J8&u1ro^8ej#|h0fi{p_#h51QX#gOq`-p6
> z<dz~1jNM`TWC{hR0YP4R)gDL82SYq5RzHY&CT};qOzH97CY}7&UI9Rs+IFiEO6lN@
> zN3B*U6SPeB0xz0&ER<;@w1RDZszT|Tz^OQLA=4C4U)3%5bYq@sjlhW2GI2hYum6a6
> z>SwvOus=zd<li@o8=-*jY1B+}y#gG&rVA>5gx~E9H8~ZT`haoqnU2ry>qPcZuHVI`
> zSYR+7l?Dd}e_6s|-oukO%(?PT2N^8{obs~9zP1uuzsDItlE4WH=#652-FXJ=nbwc?
> zNrzAMW1bMtiCV=UkL3N6h6(_JzEW2)QP&%M%;Lo)v6Z46QXh0uZ1q{HvEt(E#y<KP
> z@{yy12#<en7xn~r^GpOQVoZt^;3wyWuaL8P)3|$4!y2AHJy+_;0AiB{Chk&K^;3tL
> z4%PO@r<U(5qk<IE5pqC^sM_y^`#o?knPk&YORT*-2;|cMh*5rPK|q!&q<d4z4q#%D
> z?@@YWX4nMsi)nPs0JFj4%4%+|d4Q)eKN%r3+!z%1Y+Yt*+)?Ks`RO{QZBnMnb3+pf
> z5~9p_T|)Ifl|Nw>)4$BSSLYlC?L@<4@v1=Cv9IM5If*ba1?{q3*ixI~MUET=>=p$t
> zJG*DbKwTiJfOXyI86!v3%Byg7QtoA714#Z+jT2o7rkr&~qr?%s1QqTO*~t%7TZ5MF
> z#kexQgstJ&?yop9mU#ik6lluqmJCDIDT)z*fO#PqX^We!sNSO~iJaYvRV@<pK!pG<
> zd6Dl<+xxh3vIAlj2XUUSf#`UAAp~HDxVfEoHwUm)_T-xh46gx?U~%Y;{4(7sXH_hX
> z60>u^C!~(M;#^Ril-%a@lNnoa(!`bL04Gy@j&1{*q%&B)8n%~#^>6m0s+owJX5x+}
> zYhO_KJ_m7=E|OnoX1fV2Q<Fx$5V#UECAaWqK|yp%O+3Wl^JFcT1{eGMBi3Gz(%)(M
> zy*=ypbX&^sLM~77!!~<R^x{uFex$4n*Xw3=rgsDBDOnr$fWS7;=kXF6%I~z)2I1cI
> zz*;GDI7gSND$F4vf(fWV5|+<-fzPUEHF{l3f+2$xP6Ndf{>+k3utP6N$V?FhY<Xpk
> z@gY&?;>6qJ*~FILH-uiWI??HQy8bS!(=vYmlCqEP<Ht$4C_o)a7{IXljY3IsQGh8w
> z0fFNuPOm3q<kK?$1JK0_85|$*PZEvU03OgL2=?>OQw>1$H`4rBlLQZ7Vj!$|+mozM
> z%BF$18o2<Z27kr*rL(`<`HLL@2!Ac>4}<*WoxiO7*Gc?!p?jCDkbeV=zv0*4;Ph|k
> zzL#(Ojg|k#%70_!zd8D^^26T>#@~Vp0EEBgroY9wzop8*h1_4s;@`UR->Uonk0k?B
> z5lZ*j-eL{T^<sQyMk!|rkupLXd!W-9sYdTs^+wsL_s6C}5h&N%OnDFQB<4?2ws1RX
> z%=Wd_8)(PWbCWmxQ!(Iyn&yCwzrCuMRo3>!?RPExe$K!05#?x5*p`C4JO>L4%gjY4
> zNjbUhmPc;*5+BM4DhNK9{05*7<BN=?<29bEhqjCMl^0(JP2BpQzFoiUdT#eT&c4lA
> zQ=sn{%IEJUe6XKSM3cu#&eGD-eI~XlMZQM30-s~9!J+OF<gmFigEw!EkpdbkKYAYQ
> z!hJ+3>10t~@3hyauJ}dWVG4YOO9W`7O}97)wAY`2o;Yns?vmOQ)cr6pxcg~ytV|&V
> zaH-!$pZRN2q!zsAwhV_$J#SEc6;{Zgl&y?>%N58cfwt?j`k8iCQquN4$2`4&wm8H?
> zt&oS0B0wO@=X>7;phhDyU}YD9#>R_Qmw_#*Ke#1Shlhpqich(HyEM~O_ZbJn2SzaP
> zv0q%sPiOdN3YUcr<B%yxbnQINO7Zso9L;YpO#!AqKK@0OI&$w`Jq5p&j0^Q?ovKM#
> z-`LI&9nbd3{sX(W*IUogH*vc!K1cGETi)qKrq0sK7CU;0``7yR=gfuzgFJ)M5PiLT
> z{=IP#!~_2<imcpX58vuFew>hM{V?@G^KLRo(gbToKEDHIaL_Y#q~O=7+n7iDVQ>TP
> zoAf;1BRP==w!VfAxC5;myt|&b8*J2SQo_zcVNw3H^-sZ`wl^IrlhRf(R9L>8u`={7
> z8a8<>-Iwj4AYFdQxaM)o3neK=K<iSerP9LT34NI7{nZXG+w(l$PHTVQ>|fb@xG8bF
> zK8OkC-hJPDOGFu7I5M+)88m^LOE@5w8a-mQbRZ1)yuj>yDFgSS%9Yu{OOL(Jtsk7q
> zj4E7Vp&mGlPBC+LFJR>Jl<(icFRX9_%|jJ|s|S|Auy=#zpR9lb+W!Web}rd^=o~N@
> zC~(|~T)6lr@}>Z9gT*cr|0`MS@p^gKNCv#n9^aX{NLZZh#1z}RxJWKsUP4^g(WwJE
> zHG1^93_!(qVDh9(8+(#`6VJBk__X)Qf^@^eOh%4%<NjqEM|f4kxn~3Sp<fLbFH)tM
> zw0j@eJ8IyXE5i#M>B{aeIE(|w$s1A_(qaBy)x<Oj46cWIa*}Ul>{V5LDOi(N|GTs|
> zRAQymxoa)(^6xuh#PFA_@nchS)lBHbor%Q$ArH&{ZKtjQ&e*Y#NO<j}$wFtT2A8MB
> z?)#N8kPH7&j{URYU*dE{Lt7k{#Md$C^?UrvR<y4xX!p7f$S3xD;w6IDuc2qF0{X)D
> z;Ir3~KU0TLs=pV++3EAS?k=c1p!(qV1Wxds^E+|c&vmIyqfl89*w`Lh=!*L(3JXW!
> zg5L+;%skoF$T*6xp)IAaBX$nCofnW%5=!$EKL|;lY+9rWlcF20e^IBSp%tK?d5?qi
> zbD46KCUjP2?+YBCdA&E43P;SS-+s?MD0&5zq-j%A)AWW9>*bQcNRwf23JcaqnFFvA
> zQB9&hUfgmAjkv#7uMc<L7#_ALRYXVQT}&t}cuQ6evJn%K;S|moZfu7y5MB5>1~mO4
> mxPaE*9sNMG*wf+iUGDooJ8w&s*nS29KUdVWRWmP}KKeg9Y{3fv
> 
> literal 0
> HcmV?d00001
> 
> diff --git a/LayoutTests/platform/mac/fast/repaint/fixed-transformed-update-after-scroll-expected.txt b/LayoutTests/platform/mac/fast/repaint/fixed-transformed-update-after-scroll-expected.txt
> new file mode 100644
> index 0000000..f089252
> --- /dev/null
> +++ b/LayoutTests/platform/mac/fast/repaint/fixed-transformed-update-after-scroll-expected.txt
> @@ -0,0 +1,10 @@
> +layer at (0,0) size 785x5016
> +  RenderView at (0,0) size 785x600
> +layer at (0,0) size 785x5016
> +  RenderBlock {HTML} at (0,0) size 785x5016
> +    RenderBody {BODY} at (8,8) size 769x5000
> +layer at (60,160) size 100x100
> +  RenderBlock (positioned) {DIV} at (60,60) size 100x100 [bgcolor=#FF0000]
> +layer at (170,170) size 100x100
> +  RenderBlock (positioned) {DIV} at (170,70) size 100x100 [bgcolor=#0000FF]
> +scrolled to 0,100
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 685dd1b..ad7221a 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,30 @@
> [phone]  Benjamin Poulain  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Fixed, transformed elements are not repainted while scrolling or update
> +        https://bugs.webkit.org/show_bug.cgi?id=36652
> +
> +        FrameViewWebCore::scrollContentsFastPath() now use the new
> +        function RenderObject::clippedOverflowRectIncludingDescendants() so
> +        the returned rect take into account the transformations of the current
> +        and child elements.
> +
> +        RenderBox::computeRectForRepaint() is updated to take into account
> +        if an element is fixed whe computing the repaint rect,
> +        even if there is a transformation.
> +
> +
> +        Test: fast/repaint/fixed-transformed-update-after-scroll.html
> +
> +        * page/FrameView.cpp:
> +        (WebCore::FrameView::scrollContentsFastPath):
> +        * rendering/RenderBox.cpp:
> +        (WebCore::RenderBox::computeRectForRepaint):
> +        * rendering/RenderObject.cpp:
> +        (WebCore::RenderObject::clippedOverflowRectIncludingDescendants):
> +        * rendering/RenderObject.h:
> +
>  2010-03-26  Yael Aharon  <[e-mail]>
>  
>          Reviewed by Antti Koivisto.
> diff --git a/WebCore/page/FrameView.cpp b/WebCore/page/FrameView.cpp
> index 32127e3..d911a00 100644
> --- a/WebCore/page/FrameView.cpp
> +++ b/WebCore/page/FrameView.cpp
> @@ -881,8 +881,7 @@ bool FrameView::scrollContentsFastPath(const IntSize& scrollDelta, const IntRect
>          RenderBox* renderBox = *it;
>          if (renderBox->style()->position() != FixedPosition)
>              continue;
> -        IntRect topLevelRect;
> -        IntRect updateRect = renderBox->paintingRootRect(topLevelRect);
> +        IntRect updateRect = renderBox->clippedOverflowRectIncludingDescendants(renderBox->containerForRepaint());
>          updateRect.move(-scrollX(), -scrollY());
>          updateRect.intersect(rectToScroll);
>          if (!updateRect.isEmpty()) {


> diff --git a/WebCore/rendering/RenderBox.cpp b/WebCore/rendering/RenderBox.cpp
> index 1c0e837..de9d779 100644
> --- a/WebCore/rendering/RenderBox.cpp
> +++ b/WebCore/rendering/RenderBox.cpp
> @@ -1195,7 +1195,6 @@ void RenderBox::computeRectForRepaint(RenderBoxModelObject* repaintContainer, In
>      // We are now in our parent container's coordinate space.  Apply our transform to obtain a bounding box
>      // in the parent's coordinate space that encloses us.
>      if (layer() && layer()->transform()) {
> -        fixed = false;

This method should have similar code to mapLocalToContainer(), but now that I look at that code, it seems wrong, and I'm not convinced that this change makes computeRectForRepaint() do the right thing in call cases. It seems that it may break a repaint of a fixed pos element inside a transformed element; please test that.

mapAbsoluteToLocalPoint()'s looks right (it's going down, not up the hierarchy).


> +IntRect RenderObject::clippedOverflowRectIncludingDescendants(RenderBoxModelObject* repaintContainer)
> +{
> +    IntRect repaintRect = clippedOverflowRectForRepaint(repaintContainer);
> +    for (RenderObject* child = firstChild(); child; child = child->nextSibling())
> +        repaintRect.unite(child->clippedOverflowRectIncludingDescendants(repaintContainer));
> +    return repaintRect;
> +}

It's sad that you have to add this, but I guess you need it for the fast scrolling code.

[Simon Fraser (smfr)]

I filed bug 36686 to look into mapLocalToContainer. I think it's mostly correct, but assumes that 'fixed' has been set to 'true' when called directly on a fixed element.

computeRectForRepaint() should have similar code.

[Benjamin Poulain]

Created attachment 51913 [details]
First half of the patch, fix FrameView

I split the patch in two, so the part that does not depends on transformations can already be reviewed.

The other half need to be fixed with 36686.

[Benjamin Poulain]

(In reply to comment #11)
> Created an attachment (id=51913) [details]
> First half of the patch, fix FrameView
> 
> I split the patch in two, so the part that does not depends on transformations
> can already be reviewed.
> 
> The other half need to be fixed with 36686.

For info, I did not submitted a test on this half because a pixel test is the only way to reproduce the issue, but:
-the pixel tests are not run for Qt
-the patch cannot be reproduced on Mac because the port does not use FrameView.

[James Robinson]

Does this handle outlines correctly?  They're handled separately from overflow.

It seems really unfortunate that clippedOverflowRectIncludingDescendants() is an O(N^2) pass through all the fixed position's descendants in the fast path scrolling code.  I'm worried that this might be slower than just going through the slowpath if a large part of the DOM is inside a position:fixed element.

[Benjamin Poulain]

(In reply to comment #13)
> Does this handle outlines correctly?  They're handled separately from overflow.

I think it should, we already use clippedOverflowRect() to compute repaint rects elsewhere in Webkit.
I can test that on Tuesday though.

> It seems really unfortunate that clippedOverflowRectIncludingDescendants() is
> an O(N^2) pass through all the fixed position's descendants in the fast path
> scrolling code.  I'm worried that this might be slower than just going through
> the slowpath if a large part of the DOM is inside a position:fixed element.

O(N^2)?
I would say N*M where N is the number of fixed element but capped to 5, and M the number of child.
It is marginally slower than paintingRootRect() which almost do the same thing.

If you think this will be a performance problem, I can have a look at using the repaint rect of RenderLayer. If https://bugs.webkit.org/show_bug.cgi?id=36783 is correct, the repaintrect of the layers becomes meaningful for fixed elements.

[James Robinson]

> > It seems really unfortunate that clippedOverflowRectIncludingDescendants() is
> > an O(N^2) pass through all the fixed position's descendants in the fast path
> > scrolling code.  I'm worried that this might be slower than just going through
> > the slowpath if a large part of the DOM is inside a position:fixed element.
> 
> O(N^2)?
> I would say N*M where N is the number of fixed element but capped to 5, and M
> the number of child.

RenderObject::clippedOverflowRectForRepaint() does a recursive walk up its containers, so calling it on an element that is D levels deep in the render tree takes O(D) time.  Thus, calling it on an element and all its descendants takes O(N*M) time where N is the number of elements and M is the average number of containers, and in the worst case N =~ M.

> It is marginally slower than paintingRootRect() which almost do the same thing.
> 
> If you think this will be a performance problem, I can have a look at using the
> repaint rect of RenderLayer. If https://bugs.webkit.org/show_bug.cgi?id=36783
> is correct, the repaintrect of the layers becomes meaningful for fixed
> elements.

[Dave Hyatt]

repaintIncludingDescendants on RenderLayer is kind of what you want here (rather than crawling the whole render tree, which is way slower).  I don't know if you need to get the rects out to do your heuristics with them, but in terms of repainting, conceptually you just want to repaint the layer tree.

[Dave Hyatt]

Comment on attachment 51913 [details]
First half of the patch, fix FrameView

Minusing the patch since a render tree grovel is too slow.

[Benjamin Poulain]

Closing. This is not relevant anymore.

[Daniel Bates]

Comment on attachment 51913 [details]
First half of the patch, fix FrameView

Clearing commit-queue flag to get this out of the commit-queue.