| Summary: | [Transforms] WebView background is not painted correctly if the document has a transform | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||||
| Component: | Layout and Rendering | Assignee: | Simon Fraser (smfr) <simon.fraser> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | hyatt | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Mac | ||||||||||
| OS: | OS X 10.5 | ||||||||||
| Attachments: |
|
||||||||||
Created attachment 26152 [details]
Testcase
Created attachment 26156 [details]
Patch, testcase, changelog
Comment on attachment 26156 [details]
Patch, testcase, changelog
Where did the null check of view() go? Removing that could lead to crashes. If it was redundant, then it's OK to remove it, but the change log should explain that.
Oops that was a mistake. New patch coming. Created attachment 26158 [details]
Fixed patch - restore the !view() check
Comment on attachment 26158 [details] Fixed patch - restore the !view() check > + // If painting will entirely fill the view, no need to fill the background Comments in sentence style should end in a period. > + if (elt || > + (firstChild() && (firstChild()->style()->visibility() == VISIBLE && !firstChild()->style()->hasTransform())) || > + !view()) > return; You have an extra set of parentheses in there that I don't think add clarity. You don't need the ones after "firstChild() &&". Our typical format for long if statements like this is to start lines with operators, and also (maybe not a good format) to indent an extra level so the expression doesn't line up with the return statement. if (elt || (firstChild() && firstChild()->style()->visibility() == VISIBLE && !firstChild()->style()->hasTransform()) || !view()) return; But I think the best way to clean this up is with a helper function. static inline bool isVisibleWithNoTransform(RenderObject* object) { return object && object->style()->visibility() == VISIBLE && ! object >style()->hasTransform(); } [...] if (elt || isVisibleWithNoTransform(firstChild()) || !view()) return; [...] I think it's worth it to keep this code readable. > - // This code typically only executes if the root element's visibility has been set to hidden. > + // This code typically only executes if the root element's visibility has been set to hidden, > + // or there is a transform on the <html> Comments in sentence style should end in a period. r=me -- please consider my refinement suggestions. Committing to http://svn.webkit.org/repository/webkit/trunk ... M LayoutTests/ChangeLog A LayoutTests/fast/transforms/transformed-document-element.html A LayoutTests/platform/mac/fast/transforms/transformed-document-element-expected.checksum A LayoutTests/platform/mac/fast/transforms/transformed-document-element-expected.png A LayoutTests/platform/mac/fast/transforms/transformed-document-element-expected.txt M WebCore/ChangeLog M WebCore/rendering/RenderView.cpp Committed r39413 |
If the document has a transform, via something like: html { -webkit-transform: translateY(100px) rotate(20deg); } then the exposed area of the viewport that is not covered by the document is unpainted, and full of pixel crap.