Bug 67162 – Applying a scale transform to an <img> doesn't repaint properly

• Component: CSS
• Status: RESOLVED WORKSFORME

[Matthew Delaney]

Created attachment 105538 [details]
testcase

This affects both Safari and Chrome on Lion - didn't test other OSes.

Attaching a test case that shows the issue. It appears that the dirty rect after the transform is applied is of the initial (smaller) size of the img before the scale.

[Radar WebKit Bug Importer]

<rdar://problem/10042548>

[Simon Fraser (smfr)]

Created attachment 105546 [details]
Standalone testcase

[Tom Zakrajsek]

Created attachment 120350 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 120350 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Test: fast/repaint/bugzilla-67162.html

This needs to say what you changed and why. Your change looks pretty mysterious to me.

[Tom Zakrajsek]

Created attachment 120351 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 120351 [details]
Patch

Still needs a better changelog with an explanation of the change (which doens't look right to me).

[Tom Zakrajsek]

Created attachment 120359 [details]
Patch

[Julien Chaffraix]

Comment on attachment 120359 [details]
Patch

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

It looks like any transform removal could potentially show the same badness. Am I mistaking or do you think it would be possible possible to add some tests with different transforms?

> Source/WebCore/rendering/LayoutRepainter.cpp:40
> +        m_oldBounds = oldBounds ? *oldBounds : IntRect(0, 0, 0, 0);

I second Simon. This code is used by everything (not only when a transform changes) and this looks like it could impact more than just your case.

> LayoutTests/ChangeLog:10
> +        * fast/repaint/bugzilla-67162-expected.png: Added.
> +        * fast/repaint/bugzilla-67162-expected.txt: Added.
> +        * fast/repaint/bugzilla-67162.html: Added.

Nit: we prefer naming that reflects what you test like transform-scale-up.html.

> LayoutTests/fast/repaint/bugzilla-67162.html:19
> +<img src="../repaint/resources/webkit-background.png" style="-webkit-transform: scale(0.25); -moz-transform: scale(0.25); -o-transform: scale(0.25)" id="img">

Looks like the |src| path could be shortened here. It could be resources/webkit-background.png. Also any reason you don't re-use resources/apple.jpg?

[Tom Zakrajsek]

(In reply to comment #8)
> (From update of attachment 120359 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120359&action=review
> 
> It looks like any transform removal could potentially show the same badness. Am I mistaking or do you think it would be possible possible to add some tests with different transforms?
I agree.  And, I can add tests for those too.
> 
> > Source/WebCore/rendering/LayoutRepainter.cpp:40
> > +        m_oldBounds = oldBounds ? *oldBounds : IntRect(0, 0, 0, 0);
> 
> I second Simon. This code is used by everything (not only when a transform changes) and this looks like it could impact more than just your case.
The problem appears to be that LayoutRepainter and RenderObject are each holding state for what they believe was the previously drawn rectangle (oldBounds), as interpreted by RenderObject::repaintAfterLayoutIfNeeded, and they are not in sync.  I'll continue to look for a fix.
> 
> > LayoutTests/ChangeLog:10
> > +        * fast/repaint/bugzilla-67162-expected.png: Added.
> > +        * fast/repaint/bugzilla-67162-expected.txt: Added.
> > +        * fast/repaint/bugzilla-67162.html: Added.
> 
> Nit: we prefer naming that reflects what you test like transform-scale-up.html.
OK.
> 
> > LayoutTests/fast/repaint/bugzilla-67162.html:19
> > +<img src="../repaint/resources/webkit-background.png" style="-webkit-transform: scale(0.25); -moz-transform: scale(0.25); -o-transform: scale(0.25)" id="img">
> 
> Looks like the |src| path could be shortened here. It could be resources/webkit-background.png. Also any reason you don't re-use resources/apple.jpg?
Yeah, the path was a mistake added because at first I accidentally had this test living in .../replace instead of .../repaint. And I hadn't seen the apple.jpg.  I can certainly use that.

[Simon Fraser (smfr)]

This no longer reproduces in Safari 8.