Bug 40142 – REGRESSION (r52839): Incomplete repaint of IMG with text-align:center

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Cory Mintz]

Created attachment 57817 [details]
Correct vs Incorrect painting.

We noticed IMGs in our web software were not repainting correctly in Chrome 5, but worked in Safari 4.0.4. We narrowed it down to revision 52839. I'm having trouble reducing it down, but I'm still working on it. Attached is a screen shot showing what it looks like before and after revision 52839.

I created a small video clip to demonstrate at http://albums.phanfare.com/isolated/F5944fka/1/4697732 
You will notice that if I click on an "album" the thumbnail grid is redrawn, but the images at the end of the row do repaint in their entirty. As soon as they are interacted with, the missing portions snap in.

[Cory Mintz]

Created attachment 57878 [details]
Reduction

Reduction attached.

The reduction shows an IMG inside of a DIV. The IMG has text-align:center. With the correct sequence of steps the region the IMG would be in if it wasn't centered is repainted, rather than where it actually this. This causes the IMG to be clipped in half.

Note: It's possible that the test case will work on first load and you will need to refresh the page.

[Cory Mintz]

Created attachment 57881 [details]
Reduction (Revised)

Made variable names more clear.

[James Robinson]

Looks (from quartz debug) that the repaint invalidation is the right size/shape but at the wrong offset.

[Pierre-Antoine LaFayette]

If text-align:right is used the image is completely hidden when you refresh, this indicates that the text-align property is not affecting the offset of the dirty rect at all.

[Cory Mintz]

Also, it is not limited to text-align.

This came up because we were using this guys method to vertically center an image: http://www.brunildo.org/test/img_center.html

This will cause the image to get cut vertically as well so you just see the top-left quadrant.

[Pierre-Antoine LaFayette]

Created attachment 65941 [details]
Test cased reduced even further; showing example of text-align right hiding image completely

On Chrome 7.0.503.0, loading this file always hides the image completely. Selecting the area around the region makes the image reappear as seen in the demo video.

[Dave Hyatt]

Created attachment 66290 [details]
This should fix it.

This should fix the issue.  James. R's fix didn't really deal with the inline replaced element case.  This just generalizes to all inline content.

[Pierre-Antoine LaFayette]

Will write up a new test and upload the patch soon. Thanks for your help!

[Pierre-Antoine LaFayette]

Created attachment 66412 [details]
Contains two new layout tests as well as the original patch by dhyatt

Adding new layout tests to dhyatt's patch.

[Pierre-Antoine LaFayette]

Created attachment 66494 [details]
Revised patch with added layout tests

Contains layout tests and the bug fix.

[Pierre-Antoine LaFayette]

Created attachment 66496 [details]
PNG for block-layout-inline-children-replaced.html test case

[Pierre-Antoine LaFayette]

Created attachment 66497 [details]
PNG for block-layout-inline-children-float-positioned.html test case

[WebKit Review Bot]

Attachment 66496 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 66497 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pierre-Antoine LaFayette]

Created attachment 66548 [details]
Patch

[WebKit Review Bot]

Attachment 66548 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
LayoutTests/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
LayoutTests/ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Robinson]

LayoutTests/ChangeLog has some indentation issues. WebCore/ChangeLog needs to include a one-line description and link to a bug before the full description.

Otherwise, this looks great.

[Pierre-Antoine LaFayette]

Created attachment 66554 [details]
Patch

[Darin Adler]

Comment on attachment 66554 [details]
Patch

The ChangeLog entry needs to include a link to this bug report. The patch is otherwise good.

[Pierre-Antoine LaFayette]

Created attachment 66557 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 66557 [details]
Patch

Rejecting patch 66557 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Darin Adler', u'--force']" exit_code: 255
Last 500 characters of output:
ositioned.html
patching file LayoutTests/fast/repaint/block-layout-inline-children-replaced.html
patching file LayoutTests/platform/mac/fast/repaint/block-layout-inline-children-float-positioned-expected.checksum
literal is only avaliable with the XS version at /Library/Perl/5.8.8/Compress/Zlib.pm line 9
BEGIN failed--compilation aborted at /Library/Perl/5.8.8/Compress/Zlib.pm line 9.
Compilation failed in require at /Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/VCSUtils.pm line 1670.

Full output: http://queues.webkit.org/results/3917084

[Eric Seidel (no email)]

There is a problem with the perl install on the cq machine, which we've not been able to figure out yet.  It has trouble with git binary patches as a result.  I'll see if I can troubleshoot it again.

[Eric Seidel (no email)]

Comment on attachment 66557 [details]
Patch

Sorry about the noise.  The CQ bot was sick: bug 45723.

[WebKit Commit Bot]

Comment on attachment 66557 [details]
Patch

Rejecting patch 66557 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Darin Adler', u'--force']" exit_code: 255
Last 500 characters of output:
ositioned.html
patching file LayoutTests/fast/repaint/block-layout-inline-children-replaced.html
patching file LayoutTests/platform/mac/fast/repaint/block-layout-inline-children-float-positioned-expected.checksum
literal is only avaliable with the XS version at /Library/Perl/5.8.8/Compress/Zlib.pm line 9
BEGIN failed--compilation aborted at /Library/Perl/5.8.8/Compress/Zlib.pm line 9.
Compilation failed in require at /Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/VCSUtils.pm line 1670.

Full output: http://queues.webkit.org/results/4004006

[Eric Seidel (no email)]

Comment on attachment 66557 [details]
Patch

I believe it's fixed for real this time.

[WebKit Commit Bot]

Comment on attachment 66557 [details]
Patch

Clearing flags on attachment: 66557

Committed r67463: <http://trac.webkit.org/changeset/67463>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/67463 might have broken GTK Linux 32-bit Debug
The following changes are on the blame list:
http://trac.webkit.org/changeset/67464
http://trac.webkit.org/changeset/67463

[Eric Seidel (no email)]

fast/repaint/block-layout-inline-children-float-positioned.html no longer renders quite like it did when you landed it.  (Note how the text overlaps now:
http://trac.webkit.org/browser/trunk/LayoutTests/platform/mac/fast/repaint/block-layout-inline-children-float-positioned-expected.png

It looks like the results changed in:
http://trac.webkit.org/changeset/96005

Which doesn't make a whole lot of sense to me.

Are we sure our rendering is still correct?

[Eric Seidel (no email)]

(In reply to comment #29)
> fast/repaint/block-layout-inline-children-float-positioned.html no longer renders quite like it did when you landed it.  (Note how the text overlaps now:
> http://trac.webkit.org/browser/trunk/LayoutTests/platform/mac/fast/repaint/block-layout-inline-children-float-positioned-expected.png
> 
> It looks like the results changed in:
> http://trac.webkit.org/changeset/96005
> 
> Which doesn't make a whole lot of sense to me.
> 
> Are we sure our rendering is still correct?

The rendering is clearly wrong.  When you resize the window the text jumps from between teh left and the middle.  Someone was recently working on align: center iirc.

[Eric Seidel (no email)]

Interesting.  Our current rendering matches FF, however FF does not jump the text between two positions when you resize the window like we do.