|
Description
Mehmet
2011-05-23 19:03:39 PDT
Created attachment 117372 [details]
patch proposed for "element not fully repainted after application and removal of transform"
This patch is for returning the proper layout style difference enum for the changed Transform property that is StyleDifferenceLayout from the RenderStyle::diff function when ever styles are changed, and additionally the opacity is also looked for
Comment on attachment 117372 [details]
patch proposed for "element not fully repainted after application and removal of transform"
r- for no test case. Did you check that this also does not undo the main optimization here, which is that transform changes with accelerated compositing do not cause layout changes?
Comment on attachment 117372 [details] patch proposed for "element not fully repainted after application and removal of transform" View in context: https://bugs.webkit.org/attachment.cgi?id=117372&action=review More comments (Simon beat me on the test cases). This bug (and the fix) is very similar to bug 73161. > Source/WebCore/ChangeLog:10 > + You need at least 2 test cases here (either new or existing) as you are fixing real bugs: one for opacity and one for transform. > Source/WebCore/rendering/style/RenderStyle.cpp:350 > + StyleDifference diffToReturn = StyleDifferenceEqual; Looking at your code, this is unneeded as you can return directly what you need. However it looks like you are fixing the AC branch in both cases - see below - and thus could use this variable to change where we actually are not storing the difference which would make a more readable code. > Source/WebCore/rendering/style/RenderStyle.cpp:658 > + diffToReturn = StyleDifferenceRepaintLayer; Simon made a point on bug 73161 about whether opacity ever worked in this case: looking at the code it looks like it is specific to AC - please confirm or refute that - and due to the following (bad?) #else: if (rareNonInheritedData->opacity != other->rareNonInheritedData->opacity) { #if USE(ACCELERATED_COMPOSITING) changedContextSensitiveProperties |= ContextSensitivePropertyOpacity; // Don't return; keep looking for another change. #else return StyleDifferenceRepaintLayer; #endif } > Source/WebCore/rendering/style/RenderStyle.cpp:668 > + return diffToReturn; Be careful with trailing whitespace. I recall seeing another bug recently about an image not being repainted when a transform is changed. I wonder if it's the same thing. Thanks for notifying the change in opacity
this patch is actually for the transformed element not repainted when the transform is removed,and not intended to solve the opacity of the element,
As I was debugging the code with rotate transform operation the code change for transform related code as
(rareNonInheritedData->m_transform.get() != other->rareNonInheritedData->m_transform.get()
&& *rareNonInheritedData->m_transform.get() != *other->rareNonInheritedData->m_transform.get()) {
#if USE(ACCELERATED_COMPOSITING)
changedContextSensitiveProperties|=ContextSensitivePropertyTransform;
in RenderStyle::diff(const RenderStyle* other, unsigned& changedContextSensitiveProperties) in renderstyle.cpp
seemed to be same as the opacity property hence tried to optimize the code for opacity,but which upon testing,the case for opacity is different and
the code
changedContextSensitiveProperties |= ContextSensitivePropertyOpacity;
could not be reached upon removal of the opacity property,
hence, reverting the code change for the opacity and retain only
the rotate transform property,along with test case is what suits here
this patch is not related to the bug
https://bugs.webkit.org/show_bug.cgi?id=73161
Absolute child is not repainted when parent opacity changes
as this patch is not intended to solve this bug
Thanks and Regards,
Vamshi krishna
Created attachment 118963 [details]
proposed patch along with the test case for the "Element not fully repainted after application and removal of transform"
supporting the test case along with the removal of opacity changes in Element not fully repainted after application and removal of transform, Adjusting the diff in adjustStyleDifference for the changed Transform property considering the StyleDifferenceEqual returned after setting up of the ContextSensitivePropertyTransform in the diff function, and preserving the old optimizations.
Comment on attachment 118963 [details] proposed patch along with the test case for the "Element not fully repainted after application and removal of transform" View in context: https://bugs.webkit.org/attachment.cgi?id=118963&action=review It looks like your patch is confusing our tools. Could you rebaseline the patch on WebKit trunk and use "Tools/Scripts/webkit-patch upload" (webkit-patch help upload is your friend) to submit the rebaselined patch. > Source/WebCore/rendering/RenderObject.cpp:1689 > + if (!hasLayer() || diff == StyleDifferenceEqual) I still think RenderStyle::diff could handle that more cleanly and I haven't seen a reply from you on that (see comment #3 about the #if AC). RenderStyle::diff could handle this as shown by the previous patch,but I dont want to disturb the homogeneity of the code,
and the handling part of the transform after applied and removed could be handled better in the above shown place.
and the part of the transform ie ContextSensitivePropertyTransform on contextSensitiveProperties is exclusively being handled in the above
RenderObject::adjustStyleDifference ie
if (contextSensitiveProperties & ContextSensitivePropertyTransform) {...
} this is the better place for adjustment for transform case.
As The # if AC part is being met with transform also, I had to come up with the previous patch which was similar to opacity,previously
and this is not related to simons shown bug on bugNo73161 ie absolute child is not repainted when parent opacity changes. I had a reply
on this in comment No5,
Created attachment 119602 [details] Attaching the test case for element not fully repainted after application and removal of transform Here is the test case attached for this bug from the link http://trac.webkit.org/wiki/Rebaseline came to know as the build bots are getting confused,the new test case needs to be committed and then rebaselining can be done as notified by Comment #7 From Julien Chaffraix can any one commit this and generate the expected pngs and txts respectively, Thanks, Vamshi krishna.N Comment on attachment 119602 [details] Attaching the test case for element not fully repainted after application and removal of transform View in context: https://bugs.webkit.org/attachment.cgi?id=119602&action=review The SVN property you set is confusing our tools (click on the EWS purple bubbles and you will see that). You need at least one expected result for several reasons (regardless of the platform you used to generate it): it gives the reviewers some confidence in what you are testing and for the port maintainer who will have to generate the result, it gives them a reference. r- as you should land the full change including this test case + one expected result. The test case looks fine though. > LayoutTests/fast/repaint/transform-rotate-remove.html:1 > + Unneeded empty line (not repeated after). > LayoutTests/fast/repaint/transform-rotate-remove.html:9 > + .A { Please use something meaningful: here it should be .rotated or something equivalent. > LayoutTests/fast/repaint/transform-rotate-remove.html:11 > + -moz-transform: rotate(50deg); Great that you are trying to make it work in Mozilla! Don't forget the unprefixed version that will be used when browsers drop the prefix: -transform: rotate(50deg); > LayoutTests/fast/repaint/transform-rotate-remove.html:18 > + <script src="resources/repaint.js" type="text/javascript"></script> > + > + <script type="text/javascript"> The 'type' attribute is not needed in both <script> tag. > LayoutTests/fast/repaint/transform-rotate-remove.html:25 > + document.getElementById("try").innerHTML = "Rotation Removed"; This text looks unneeded. If the output is wrong, you will see it in your pixel result. > LayoutTests/fast/repaint/transform-rotate-remove.html:36 > + }, 0); Do we really need the setTimeout logic? Sometimes tests do some operation at the end of the page through an inline <script> (in your case rotate()) and the repaint logic on the 'load' event as you are doing. > LayoutTests/fast/repaint/transform-rotate-remove.html:48 > + <p>Test for <a href="https://bugs.webkit.org/show_bug.cgi?id=61338">https://bugs.webkit.org/show_bug.cgi?id=61338</a>.Test that element after transform applied and removed will not clip the element.one should see the element fully painted</p> Using text makes the test platform-dependent and is not advised unless you need it for testing. Here it seems like you could skip it. See http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree > LayoutTests/fast/repaint/transform-rotate-remove.html:51 > +</br> > +</br> > +</br> Do we need all those <br/>? (not that normally they should be written this way even if it doesn't make a difference due to HTML5 fixing the syntax). > LayoutTests/fast/repaint/transform-rotate-remove.html:52 > + <div id='try' style="border:1px solid red; background-color:pink; height: 50px; width:200px">TEST</div> Red should usually be avoided in an expected output as it has the meaning 'failed'. (In reply to comment #10) > (From update of attachment 119602 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=119602&action=review > > The SVN property you set is confusing our tools (click on the EWS purple bubbles and you will see that). > yes, in each added file this was present in earlier patch,so I verified again to see svn:executable in newly added files ## -0,0 +1 ##, for this patch but this is present in RenderObject.cpp which is a fresh checkout. +* > You need at least one expected result for several reasons (regardless of the platform you used to generate it): it gives the reviewers some confidence in what you are testing and for the port maintainer who will have to generate the result, it gives them a reference. >yes!! > r- as you should land the full change including this test case + one expected result. The test case looks fine though. > I had provided the test case along with expected .txt and .png files > > LayoutTests/fast/repaint/transform-rotate-remove.html:1 > > + > removed this one > Unneeded empty line (not repeated after). > > > LayoutTests/fast/repaint/transform-rotate-remove.html:9 > > + .A { > > Please use something meaningful: here it should be .rotated or something equivalent. > yeah, A is replaced with Rotate > > LayoutTests/fast/repaint/transform-rotate-remove.html:11 > > + -moz-transform: rotate(50deg); > > Great that you are trying to make it work in Mozilla! Don't forget the unprefixed version that will be used when browsers drop the prefix: -transform: rotate(50deg); > this one could render properly in firefox, I had to remove this from the test case > > LayoutTests/fast/repaint/transform-rotate-remove.html:18 > > + <script src="resources/repaint.js" type="text/javascript"></script> > > + > > + <script type="text/javascript"> > > The 'type' attribute is not needed in both <script> tag. > removed the type > > LayoutTests/fast/repaint/transform-rotate-remove.html:25 > > + document.getElementById("try").innerHTML = "Rotation Removed"; > > This text looks unneeded. If the output is wrong, you will see it in your pixel result. > removed this one in the newly created patch > > LayoutTests/fast/repaint/transform-rotate-remove.html:36 > > + }, 0); > > Do we really need the setTimeout logic? Sometimes tests do some operation at the end of the page through an inline <script> (in your case rotate()) and the repaint logic on the 'load' event as you are doing. >this test has the div element which needs rotation on load and also after a small amount of time to see the element painted properly, > > LayoutTests/fast/repaint/transform-rotate-remove.html:48 > > + <p>Test for <a href="https://bugs.webkit.org/show_bug.cgi?id=61338">https://bugs.webkit.org/show_bug.cgi?id=61338</a>.Test that element after transform applied and removed will not clip the element.one should see the element fully painted</p> > > Using text makes the test platform-dependent and is not advised unless you need it for testing. Here it seems like you could skip it. See http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree > had skipped this in newly created patch > > LayoutTests/fast/repaint/transform-rotate-remove.html:51 > > +</br> > > +</br> > > +</br> > > Do we need all those <br/>? (not that normally they should be written this way even if it doesn't make a difference due to HTML5 fixing the syntax). > removed these, as are not required > > LayoutTests/fast/repaint/transform-rotate-remove.html:52 > > + <div id='try' style="border:1px solid red; background-color:pink; height: 50px; width:200px">TEST</div> > > Red should usually be avoided in an expected output as it has the meaning 'failed'. Red border is replaced by blue line uploading the new testcase,expected results with no svn properties set Created attachment 121086 [details]
patch for "ement not fully repainted after application and removal of transform " with testcase,expected results with no svn properties on added files
Now with the test case(reviewed after comments ),expected text and expected image files and patch with no svn:executable present in the added files,But the source file Source/WebCore/rendering/renderObject.cpp has the svn:executable property set as this is from the checkout from the repo
Comment on attachment 121086 [details] patch for "ement not fully repainted after application and removal of transform " with testcase,expected results with no svn properties on added files View in context: https://bugs.webkit.org/attachment.cgi?id=121086&action=review > Source/WebCore/ChangeLog:12 > + Adjusting the diff in adjustStyleDifference for the changed Transform property > + taking into account the StyleDifferenceEqual returned after setting up of the > + ContextSensitivePropertyTransform from the diff function. I can't really understand from this what the change is about. It's better to state what the original problem is, and how you fixed it. I'm not really convinced that the change is correct. > LayoutTests/fast/repaint/transform-rotate-remove.html:7 > + .Rotate { Class names should be adjectives, and lowercase, so 'rotated'. > LayoutTests/fast/repaint/transform-rotate-remove.html:18 > + rotate(); Odd indentation here. > LayoutTests/fast/repaint/transform-rotate-remove.html:26 > + if (window.layoutTestController) And here. > LayoutTests/fast/repaint/transform-rotate-remove.html:35 > + document.body.offsetTop; > + layoutTestController.display(); Why the offsetTop? display() should force a layout. Is there a reason this test isn't using the standard repaint test helper script? (In reply to comment #13) > (From update of attachment 121086 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=121086&action=review > > > Source/WebCore/ChangeLog:12 > > + Adjusting the diff in adjustStyleDifference for the changed Transform property > > + taking into account the StyleDifferenceEqual returned after setting up of the > > + ContextSensitivePropertyTransform from the diff function. > > I can't really understand from this what the change is about. It's better to state what the original problem is, and how you fixed it. I'm not really convinced that the change is correct. The problem is with div element having rotated using transform, and then made to its original state after removing the transform,the div element gets clipped off in the sides as can be observed by https://bugs.webkit.org/attachment.cgi?id=94544 RenderObject::setStyle is the one which gets called when the -webkit-transform: rotate(50deg) is toggled ie when this property is applied or removed. Inside this the RenderStyle::diff(style.get(), contextSensitiveProperties) is the one which sets the contextSensitiveProperties to 1 when the rotation is applied and also removed. If the RenderObject::adjustStyleDifference which takes the diff and when the diff equals StyleDiffEqual, contextSensitiveProperties set for transform operation, diff should be adjusted to StyleDifferenceLayout,as this is the only one which makes the div element properly repainted. > > LayoutTests/fast/repaint/transform-rotate-remove.html:7 > > + .Rotate { > > Class names should be adjectives, and lowercase, so 'rotated'. > using rotated seems to be the better name > > LayoutTests/fast/repaint/transform-rotate-remove.html:18 > > + rotate(); > > Odd indentation here. > > > LayoutTests/fast/repaint/transform-rotate-remove.html:26 > > + if (window.layoutTestController) > > And here. > > > LayoutTests/fast/repaint/transform-rotate-remove.html:35 > > + document.body.offsetTop; > > + layoutTestController.display(); > Indentations be corrected. > Why the offsetTop? display() should force a layout. > I have tested removing the offsetTop and retaining only the layoutTestController.display(); which also could generate proper results as expected > Is there a reason this test isn't using the standard repaint test helper script? I was looking to remove the dependency in external files for the test case, the script code which runs is the same. Created attachment 125619 [details]
Fix to repaint the element when its transform property changes
The patches solves the repaint issue when its transform property changes. Also I tried to address the review comments provided by Simon Fraser & Julien Chaffraix in the attached patch.
Comment on attachment 125619 [details] Fix to repaint the element when its transform property changes View in context: https://bugs.webkit.org/attachment.cgi?id=125619&action=review > Source/WebCore/ChangeLog:8 > + when the transform property of an element changes, It's required to recaliculate its layout. when -> When It's -> it's > Source/WebCore/ChangeLog:9 > + Though the change in transform property results in creation & destruction of renderlayer, & -> and > Source/WebCore/ChangeLog:10 > + the layout update is not taken care when renderlayer is destroyed. renderlayer -> RenderLayer > Source/WebCore/ChangeLog:11 > + This change needs a little more explanation. Transforms don't affect layout, so what layout-related data needs to be updated when a transform is removed? Comment on attachment 125619 [details] Fix to repaint the element when its transform property changes View in context: https://bugs.webkit.org/attachment.cgi?id=125619&action=review Your patch is confusing our review tool (we can't see the png baseline) and webkit-patch (all the EWS cannot apply the patch). Make sure you use webkit-patch upload for your next patch. > LayoutTests/fast/repaint/transform-rotate-remove.html:29 > + <div id='try' style="border:1px solid red; background-color:pink; height: 50px; width:200px;" onclick="rotate();">TRY IT OUT!</div> Avoid to use text as this makes the test platform dependant. See http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree#Pixeltesttips Created attachment 127808 [details]
Fix to repaint the element when its transform property changes
The patch contains the following updated from the previous patch.
1. updated changelog
2. modified the test case to ref-test to avoid pixel testing.
3. should resolve the style issue occured in the previous patch.
Comment on attachment 127808 [details] Fix to repaint the element when its transform property changes Attachment 127808 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11549250 New failing tests: fast/repaint/transform-rotate-and-remove.html Created attachment 129907 [details]
patch to repaint the element when its transform property changes
Comment on attachment 129907 [details] patch to repaint the element when its transform property changes Attachment 129907 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11800022 New failing tests: fast/repaint/transform-rotate-and-remove.html Comment on attachment 129907 [details] patch to repaint the element when its transform property changes View in context: https://bugs.webkit.org/attachment.cgi?id=129907&action=review Sorry, I meant to look at your previous patch in a more timely fashion. > Source/WebCore/rendering/RenderBoxModelObject.cpp:383 > + if (s_hadTransform) > + setNeedsLayoutAndPrefWidthsRecalc(); Yet another static variable... I think it's OK-ish (I am really set up on RenderStyle::diff for this bug as removing all transforms on an element should force layout as we may be back to normal flow - but that's likely more complicated and I don't have a good visibility on that). Simon may have more comments on that. > LayoutTests/fast/repaint/transform-rotate-and-remove.html:24 > + rotate(); > + setTimeout(function() { rotate(); if (window.layoutTestController) { layoutTestController.notifyDone(); } }, 1000); If you start with your element rotated, you can remove the setTimeout logic AFAICT. Btw, it's *not* an option to set a timeout to 1s. We have over 25,000 tests and if they all needed a second, we would never be able to run them. This is also the reason (I guess) that the Chromium EWS is turning red: tests that takes more than 30 seconds needs to be explicitly marked in test_expectations.txt or our test driver kills them. > LayoutTests/fast/repaint/transform-rotate-and-remove.html:30 > + <div id='try' style="border:1px solid red; background-color:pink; height: 50px; width:200px;" onclick="rotate();"></div> I really think you could just visually show what is wrong using something along those lines: <style> absolutePositioned { position: absolute; left: 100px; top: 100px; height: 50px; width: 200px; /* Need to account for other elements / padding / etc to be being relativePositioned */ } relativePositioned { position: relative; left: 100px; top: 100px; height: 50px; width: 200px; } </style> <div class="absolutePositioned" style="background: red"></div> <div id='try' class="relativePositioned" style="background: green;" onclick="rotate()"></div> <p>Bug 61338...</p> If you don't properly repaint, I would expect to see the first red div. > LayoutTests/platform/win/fast/repaint/transform-rotate-and-remove-expected.txt:13 > + text run at (71,0) width 447: ": Element not fully repainted after application and removal of transform" This should be a ref tests for 2 reasons: * the text makes the output platform-specific unfortunately. * the output is super easy to reproduce in another way (especially considering my previous suggestion) Also please include what you expect the test to look like. Comment on attachment 129907 [details]
patch to repaint the element when its transform property changes
r- given Julien's comments.
Created attachment 130065 [details]
patch to repaint the element when its transform property is removed
Modified patch based on review comments.
Hi Julien/Simon,
Unfortunately I could not use "positioned" style for this test due to following reasons.
1. z-index of an element can not be auto when the transform property is ON for an element.
2. z-index of an element is usually auto in normal scenarios like no transform.
As you know z-index effect is considered only for positioned elements,
the z-index auto value is being used in StyleDiff for a positioned element and on that basis StyleDiff returned can be StyleDifferenceRepaintLayer.
thus making the issue not reproducible when "positioned" style is applied.
Hence i could not make a test case such that Green div element getting placed on Red div element.
Except for this exception,I tried to incorporate all your suggestions turning it to ref test.
Thanks for your review comments, they are quite helpful in resubmitting the patch.
Comment on attachment 130065 [details] patch to repaint the element when its transform property is removed Attachment 130065 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11803885 New failing tests: fast/repaint/transform-rotate-and-remove.html Created attachment 130144 [details]
patch to repaint the element when its transform property is removed
Another attempt to pass chromium-linux
Comment on attachment 130065 [details]
patch to repaint the element when its transform property is removed
Clearing flags of this patch as a new version have been uploaded.
Comment on attachment 130144 [details]
patch to repaint the element when its transform property is removed
Hi,Can some one please commit this patch.
Comment on attachment 130144 [details] patch to repaint the element when its transform property is removed Clearing flags on attachment: 130144 Committed r109867: <http://trac.webkit.org/changeset/109867> All reviewed patches have been landed. Closing bug. |