Bug 40142

Summary: REGRESSION (r52839): Incomplete repaint of IMG with text-align:center
Product: WebKit Reporter: Cory Mintz <cory>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue, dbates, eae, eric, jamesr, leviw, mitz, ojan, pierre.lafayette, simon.fraser, tony, webkit.review.bot
Priority: P1 Keywords: HasReduction, Regression
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Correct vs Incorrect painting.
none
Reduction
none
Reduction (Revised)
none
Test cased reduced even further; showing example of text-align right hiding image completely
none
This should fix it.
none
Contains two new layout tests as well as the original patch by dhyatt
none
Revised patch with added layout tests
none
PNG for block-layout-inline-children-replaced.html test case
none
PNG for block-layout-inline-children-float-positioned.html test case
none
Patch
none
Patch
none
Patch none

Description Cory Mintz 2010-06-03 14:53:41 PDT
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.
Comment 1 Cory Mintz 2010-06-04 08:12:03 PDT
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.
Comment 2 Cory Mintz 2010-06-04 08:22:20 PDT
Created attachment 57881 [details]
Reduction (Revised)

Made variable names more clear.
Comment 3 James Robinson 2010-08-30 11:23:10 PDT
Looks (from quartz debug) that the repaint invalidation is the right size/shape but at the wrong offset.
Comment 4 Pierre-Antoine LaFayette 2010-08-30 12:34:24 PDT
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.
Comment 5 Cory Mintz 2010-08-30 12:47:07 PDT
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.
Comment 6 Pierre-Antoine LaFayette 2010-08-30 12:47:33 PDT
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.
Comment 7 Dave Hyatt 2010-09-01 15:23:56 PDT
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.
Comment 8 Pierre-Antoine LaFayette 2010-09-01 15:41:19 PDT
Will write up a new test and upload the patch soon. Thanks for your help!
Comment 9 Pierre-Antoine LaFayette 2010-09-02 14:28:38 PDT
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.
Comment 10 Pierre-Antoine LaFayette 2010-09-03 08:11:51 PDT
Created attachment 66494 [details]
Revised patch with added layout tests

Contains layout tests and the bug fix.
Comment 11 Pierre-Antoine LaFayette 2010-09-03 08:13:10 PDT
Created attachment 66496 [details]
PNG for block-layout-inline-children-replaced.html test case
Comment 12 Pierre-Antoine LaFayette 2010-09-03 08:14:13 PDT
Created attachment 66497 [details]
PNG for block-layout-inline-children-float-positioned.html test case
Comment 13 WebKit Review Bot 2010-09-03 12:40:01 PDT
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.
Comment 14 WebKit Review Bot 2010-09-03 12:40:31 PDT
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.
Comment 15 Pierre-Antoine LaFayette 2010-09-03 14:51:21 PDT
Created attachment 66548 [details]
Patch
Comment 16 WebKit Review Bot 2010-09-03 14:52:36 PDT
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.
Comment 17 James Robinson 2010-09-03 14:54:03 PDT
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.
Comment 18 Pierre-Antoine LaFayette 2010-09-03 15:06:34 PDT
Created attachment 66554 [details]
Patch
Comment 19 Darin Adler 2010-09-03 15:17:23 PDT
Comment on attachment 66554 [details]
Patch

The ChangeLog entry needs to include a link to this bug report. The patch is otherwise good.
Comment 20 Pierre-Antoine LaFayette 2010-09-03 15:30:59 PDT
Created attachment 66557 [details]
Patch
Comment 21 WebKit Commit Bot 2010-09-03 18:28:26 PDT
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
Comment 22 Eric Seidel (no email) 2010-09-06 16:53:09 PDT
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.
Comment 23 Eric Seidel (no email) 2010-09-13 21:23:03 PDT
Comment on attachment 66557 [details]
Patch

Sorry about the noise.  The CQ bot was sick: bug 45723.
Comment 24 WebKit Commit Bot 2010-09-13 22:49:37 PDT
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
Comment 25 Eric Seidel (no email) 2010-09-14 03:58:05 PDT
Comment on attachment 66557 [details]
Patch

I believe it's fixed for real this time.
Comment 26 WebKit Commit Bot 2010-09-14 06:25:30 PDT
Comment on attachment 66557 [details]
Patch

Clearing flags on attachment: 66557

Committed r67463: <http://trac.webkit.org/changeset/67463>
Comment 27 WebKit Commit Bot 2010-09-14 06:25:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 WebKit Review Bot 2010-09-14 08:30:29 PDT
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
Comment 29 Eric Seidel (no email) 2012-08-01 00:00:18 PDT
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?
Comment 30 Eric Seidel (no email) 2012-08-10 13:02:58 PDT
(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.
Comment 31 Eric Seidel (no email) 2012-08-10 13:06:12 PDT
Interesting.  Our current rendering matches FF, however FF does not jump the text between two positions when you resize the window like we do.