Bug 13576 - REGRESSION (r20980): Vertical alignment was working, now doesn't always work.
Summary: REGRESSION (r20980): Vertical alignment was working, now doesn't always work.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 523.x (Safari 3)
Hardware: All OS X 10.4
: P1 Major
Assignee: Dave Hyatt
URL:
Keywords: HasReduction, InRadar, Regression
Depends on:
Blocks:
 
Reported: 2007-05-03 10:15 PDT by Tom Brown
Modified: 2007-05-10 00:54 PDT (History)
3 users (show)

See Also:


Attachments
HTML containing problem example (1.56 KB, text/html)
2007-05-03 10:18 PDT, Tom Brown
no flags Details
Image showing broken and unbroken rendering (76.39 KB, image/png)
2007-05-03 10:19 PDT, Tom Brown
no flags Details
Test case that illustrates the problem without using any first lines. (349 bytes, text/html)
2007-05-10 00:14 PDT, Dave Hyatt
no flags Details
Make sure RenderText doesn't call verticalPositionHint on an inline block. (476 bytes, patch)
2007-05-10 00:47 PDT, Dave Hyatt
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.