Bug 13576

Summary: REGRESSION (r20980): Vertical alignment was working, now doesn't always work.
Product: WebKit Reporter: Tom Brown <tom>
Component: Layout and RenderingAssignee: Dave Hyatt <hyatt>
Status: RESOLVED FIXED    
Severity: Major CC: bdakin, hyatt, mitz
Priority: P1 Keywords: HasReduction, InRadar, Regression
Version: 523.x (Safari 3)   
Hardware: All   
OS: OS X 10.4   
Attachments:
Description Flags
HTML containing problem example
none
Image showing broken and unbroken rendering
none
Test case that illustrates the problem without using any first lines.
none
Make sure RenderText doesn't call verticalPositionHint on an inline block. oliver: review+

Description Tom Brown 2007-05-03 10:15:44 PDT
In certain circumstances, vertical alignment no longer works as it did before. The attached example stopped working between nightly 20961 and 20988.
Comment 1 Tom Brown 2007-05-03 10:18:25 PDT
Created attachment 14320 [details]
HTML containing problem example
Comment 2 Tom Brown 2007-05-03 10:19:50 PDT
Created attachment 14321 [details]
Image showing broken and unbroken rendering
Comment 3 mitz 2007-05-03 10:40:03 PDT
I'm pretty sure this regressed in <http://trac.webkit.org/projects/webkit/changeset/20980>, because adding a dummy :first-line rule at the beginning fixes the reduction.
Comment 4 Darin Adler 2007-05-04 22:20:21 PDT
<rdar://problem/5183697>
Comment 5 Beth Dakin 2007-05-09 23:04:49 PDT
Mitz is right. The following two lines were added to the top of RenderObject::verticalPositionHint() in that patch:

+if (firstLine) // We're only really a first-line style if the document actually uses first-line rules.
+       firstLine = document()->usesFirstLineRules();

And removing those lines fixes the bug. I hesitate to post a patch before consulting Hyatt, though, because I am confused about those lines...even though I reviewed the patch. /me feels sheepish!
Comment 6 Dave Hyatt 2007-05-09 23:28:59 PDT
Bug happens on line #2.

<div>
 Foo<br> <img src="http://i128.photobucket.com/albums/p168/p3dragonlady/FridayYet.gif" style="vertical-align:middle;outline:1px dotted red;width:9px;height:9px;">
  <div style="display:inline-block;vertical-align:middle;outline:1px dotted red;text-align:center;">
    Text Goes Here
  </div>
</div>
Comment 7 Dave Hyatt 2007-05-10 00:00:32 PDT
The duality of inline-block is the problem.  m_verticalPositionHint being cached is causing both the inside/outside to reference it, and of course it's not right for both.
Comment 8 Dave Hyatt 2007-05-10 00:14:38 PDT
Created attachment 14450 [details]
Test case that illustrates the problem without using any first lines.

This test case illustrates the bug.  This test case fails in Safari 2.0 and illustrates that this flaw is not a regression.  The cache itself is buggy, and so the regression stems from using the cache more.
Comment 9 Dave Hyatt 2007-05-10 00:47:48 PDT
Created attachment 14453 [details]
Make sure RenderText doesn't call verticalPositionHint on an inline block.

Don't call verticalPositionHint on an inline-block from inside the inline-block. This code path is actually the only way this could happen.
Comment 10 Dave Hyatt 2007-05-10 00:54:51 PDT
Fixed.