Bug 9774

Summary: exeter.edu renders improperly
Product: WebKit Reporter: Will Stoutin <willstoutin>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: carlamichele, emacemac7, sam
Priority: P2 Keywords: HasReduction
Version: 420+   
Hardware: Mac   
OS: OS X 10.4   
URL: http://www.exeter.edu
Description Flags
reduced test case
Slightly more reduced test case
Patch, including layout test and change log
mitz: review-
updated patch
mitz: review-
patch with new ChangeLog
mitz: review-
screenshot of rendering error none

Description Will Stoutin 2006-07-06 20:48:11 PDT
On the homepage "Lion's Eye" logo covers up much text. Click on "Current Students" near the top of the page and the header text "Useful" appears on the wrong side and higher up on the page than it should be. Some pages (including current students) have links for "larger text" and "print this page" which should be in the right hand corner of the white area but appear to the left in webkit. Problems exist in Safari, Omniweb, Shiira, but all pages render fine in Camino and Firefox.
Comment 1 Sam Weinig 2006-07-07 06:35:32 PDT
Created attachment 9246 [details]
reduced test case

Adding a reduced test case to illustrate the difference between ToT and Firefox.
Comment 2 jonathanjohnsson 2006-07-09 04:11:26 PDT
I see this too.

Reporter, you should file one bug report for each issue you see, unless you are certain they all have the same root cause. Since Sam Weinig already added a test case for the wrong logo placement, I think it would be good to only track that bug here, and that you should file new bugs for the other issues.
Comment 3 jonathanjohnsson 2006-07-09 04:14:32 PDT
Created attachment 9291 [details]
Slightly more reduced test case

I reduced Sam's reduction slightly, without removing the issue.
Comment 4 mitz 2006-07-11 11:25:14 PDT
Created attachment 9373 [details]
Patch, including layout test and change log
Comment 5 David Kilzer (:ddkilzer) 2006-07-11 11:30:25 PDT
Is this a regression from production Safari?  If so, it needs to be P1 and have a NeedsRadar keyword added.
Comment 6 Sam Weinig 2006-07-11 14:14:37 PDT
This issue is present in the current production safari (for 10.4.7, 419.3), so this is not a regression.  Leaving it at P2.
Comment 7 mitz 2006-11-13 00:14:37 PST
Comment on attachment 9373 [details]
Patch, including layout test and change log

This patch is actually wrong. You should pass nonAnonymousContainingBlock()->contentHeight() to calcValue() instead of containingBlockHeight().
Comment 8 Sam Weinig 2006-11-13 10:55:06 PST
Created attachment 11504 [details]
updated patch

This updates mitz's patch based on his comments.
Comment 9 mitz 2006-11-13 11:12:07 PST
Comment on attachment 11504 [details]
updated patch

r- due to some ChangeLog issues I discussed with Sam on IRC.
Comment 10 Sam Weinig 2006-11-13 14:28:44 PST
Created attachment 11511 [details]
patch with new ChangeLog

Updated patch.  Someone should really test this before landing as my computer is rather on the fritz and the results have been rather inconsistent.
Comment 11 mitz 2006-11-13 22:23:54 PST
(In reply to comment #10)
> Someone should really test this before landing as my computer
> is rather on the fritz and the results have been rather inconsistent. 

I applied the patch to my tree and verified that there are no layout test regressions (and that the new test passes).
Comment 12 Dave Hyatt 2006-11-14 01:43:47 PST
I'm not so sure about this.  Why should anonymous blocks be skipped?
Comment 13 mitz 2006-11-14 03:33:42 PST
(In reply to comment #12)
> I'm not so sure about this.  Why should anonymous blocks be skipped?

We talked about it on IRC. The spec doesn't say anything about it, but Firefox and Opera do it even in strict mode. However, the patch is incomplete since it addresses percentage 'top' and 'bottom' but not percentage 'height' (the latter skips anonymous blocks in quirks mode, but only because it skips all block with height: auto in that mode); starting from the nonAnonymousContainingBlock in calcPercentageHeight should solve that. The layout test should cover percentage height.
Comment 14 mitz 2006-11-14 03:35:52 PST
Comment on attachment 11511 [details]
patch with new ChangeLog

r- due to the above
Comment 15 Carla Hufstedler 2007-05-18 21:38:15 PDT
Created attachment 14619 [details]
screenshot of rendering error

showing the error reported as of build 420+
Comment 16 Carla Hufstedler 2007-05-18 21:50:48 PDT
I'm not so certain this is a WebKit bug, per se, since I just validated the webpage in question, and it failed with 140 XHTML errors (http://validator.w3.org/check?uri=http%3A//www.exeter.edu/). There are also several CSS errors (http://jigsaw.w3.org/css-validator/validator?uri=http%3A//www.exeter.edu/), including the repeated substitution of "font-name" for "font-family".
Comment 17 mitz 2007-06-01 08:37:18 PDT
(In reply to comment #13)
> The spec doesn't say anything about it

Actually, I think the spec is pretty clear: "if the element's position is 'relative' or 'static', the containing block is formed by the content edge of the nearest *block-level*, table cell or inline-block ancestor box" (<http://www.w3.org/TR/2006/WD-CSS21-20061106/visudet.html#containing-block-details>).

Generating an anonymous block box doesn't make you a block-level element.

I think containingBlock() should just be changed to not return anonymous blocks in this case. The problem is that some callers expect the current behavior, so I think containingBlock() could have an allowAnonymousBlocks parameter that defaults to false, and callers that need to see anonymous boxes could be changed to pass true for that parameter.

A slightly less risky approach for now would be to make the default 'true' and just patch the few callers that we know that need to skip anonymous blocks (currently relativePositionOffsetY and calcPercentageHeight).