Bug 50970 - REGRESSION: Floated text is not rendered after r73385
Summary: REGRESSION: Floated text is not rendered after r73385
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Dave Hyatt
URL:
Keywords:
Depends on:
Blocks: 49220
  Show dependency treegraph
 
Reported: 2010-12-13 14:07 PST by Mihai Parparita
Modified: 2010-12-14 15:02 PST (History)
3 users (show)

See Also:


Attachments
Test Case (333 bytes, text/html)
2010-12-14 13:10 PST, Dave Hyatt
no flags Details
Screenshot of testcase in IE9 in IE9 mode (19.35 KB, image/png)
2010-12-14 13:32 PST, Tony Gentilcore
no flags Details
Patch (45.40 KB, patch)
2010-12-14 14:39 PST, Dave Hyatt
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihai Parparita 2010-12-13 14:07:12 PST
Test case: http://persistent.info/webkit/test-cases/crbug-66564.html. Verified locally that building before http://trac.webkit.org/changeset/73385 results in the text being displayed, but not when building after.

Initially reported on the Chromium side as http://crbug.com/66564
Comment 1 James Robinson 2010-12-13 14:12:35 PST
I suspect this is related to bug 50963
Comment 2 James Robinson 2010-12-13 15:28:23 PST
(In reply to comment #1)
> I suspect this is related to bug 50963
Nope, it's something else.
Comment 3 James Robinson 2010-12-13 16:33:33 PST
We're inserting the float into the float list for the <body>, but the enclosing self render layer for the float is the layer for the relpositioned inline <div>.  The RenderLayer for the body doesn't paint the float (the enclosing self painting layers are different) and the RenderLayer for the relpos div doesn't paint the float because it's not in the floating object list for anything within the layer.
Comment 4 James Robinson 2010-12-13 17:41:00 PST
This fixes the issue:

--- a/WebCore/rendering/RenderBlock.cpp
+++ b/WebCore/rendering/RenderBlock.cpp
@@ -3580,8 +3580,7 @@ int RenderBlock::addOverhangingFloats(RenderBlock* child, int logicalLeftOffset,
                 }
                 m_floatingObjects->append(floatingObj);
             }
-        } else {
-            if (makeChildPaintOtherFloats && !r->m_shouldPaint && !r->m_renderer->hasSelfPaintingLayer() &&
+        } else if (makeChildPaintOtherFloats && !r->m_shouldPaint && !r->m_renderer->hasSelfPaintingLayer() &&
                 r->m_renderer->isDescendantOf(child) && r->m_renderer->enclosingLayer() == child->enclosingLayer()) {
                 // The float is not overhanging from this block, so if it is a descendant of the child, the child should
                 // paint it (the other case is that it is intruding into the child), unless it has its own layer or enclosing
@@ -3590,12 +3589,11 @@ int RenderBlock::addOverhangingFloats(RenderBlock* child, int logicalLeftOffset,
                 // it should paint.
                 r->m_shouldPaint = true;
             }
-            
-            // Since the float doesn't overhang, it didn't get put into our list.  We need to go ahead and add its overflow in to the
-            // child now.
-            if (r->m_isDescendant)
-                child->addOverflowFromChild(r->m_renderer, IntSize(r->left() + r->m_renderer->marginLeft(), r->top() + r->m_renderer->marginTop()));
         }
+        // Since the float doesn't overhang, it didn't get put into our list.  We need to go ahead and add its overflow in to the
+        // child now.
+        if (r->m_isDescendant)
+            child->addOverflowFromChild(r->m_renderer, IntSize(r->left() + r->m_renderer->marginLeft(), r->top() + r->m_renderer->marginTop()));
     }
     return lowestFloatLogicalBottom;


It's essentially reverting all the changes that 73385 made to addOverhangingFloats() except for the floatingObj->m_isDescendant = true line.  I think this is a case for Hyatt to decide what to do :)
Comment 5 Dave Hyatt 2010-12-14 13:09:17 PST
This is interesting.  Really the issue is just who exactly is supposed to paint that float?  Should the relatively positioned inline paint the float, or should the block paint the float?  The fact that the block paints it means the float is effectively ignoring the fact that it is inside the relative positioned span.

See the attached test case.
Comment 6 Dave Hyatt 2010-12-14 13:10:35 PST
Created attachment 76561 [details]
Test Case

In the attached test case, Firefox and WebKit disagree.  Firefox is inconsistent though, since it is applying the stacking order based off the float being inside the relative positioned div, but it's not applying the relative positioned offset to the div.  Opera agrees with WebKit.  Maybe someone could try IE9.
Comment 7 Tony Gentilcore 2010-12-14 13:32:57 PST
Created attachment 76564 [details]
Screenshot of testcase in IE9 in IE9 mode
Comment 8 Dave Hyatt 2010-12-14 14:39:58 PST
Created attachment 76572 [details]
Patch

For now just retain our behavior of having the blocks always paint floats.  I actually think IE9's rendering makes more sense than what we do, but changing to match that rendering is more involved.
Comment 9 Dave Hyatt 2010-12-14 15:02:05 PST
Fixed in r74063.