Bug 20329 - CSS3 Box shadow property pushes outside the bounds of the page causing horizontal scrollbars
Summary: CSS3 Box shadow property pushes outside the bounds of the page causing horizo...
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 525.x (Safari 3.1)
Hardware: PC All
: P2 Normal
Assignee: Dave Hyatt
URL: http://www.multiblah.com/exps/css/box...
Depends on: 28625
Blocks: 27569
  Show dependency treegraph
Reported: 2008-08-08 07:13 PDT by Kevin Cannon
Modified: 2009-08-21 14:48 PDT (History)
7 users (show)

See Also:

Work in progress. Just saving what I've done so far. (127.00 KB, patch)
2009-08-14 22:48 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
New patch (still WIP) (129.38 KB, patch)
2009-08-15 17:27 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
All non-pixel tests pass except for multicol (130.22 KB, patch)
2009-08-15 22:33 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
Code changes (140.30 KB, patch)
2009-08-16 12:48 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
Layout test changes (1.88 MB, patch)
2009-08-16 12:49 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
Code Changes (143.22 KB, patch)
2009-08-16 12:53 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch (146.07 KB, patch)
2009-08-18 11:26 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
New patch that fixes a bug in addOverflowFromChild caught by fixing the propagation of float overflow. (145.55 KB, patch)
2009-08-18 11:58 PDT, 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 Kevin Cannon 2008-08-08 07:13:27 PDT
The new -webkit-box-shadow implementation of the CSS3 box-shadow property has a bug.

When you place a box-shadow on an item near the edge of the screen, the box-shadow itself is taking up space and causes horizontal scrollbars.

Suggest preventing the box-shadow take up space in situations where it pushes against boundaries of other items.
Comment 1 mitz 2008-08-08 13:18:05 PDT
That was actually done on purpose.
Comment 2 Kevin Cannon 2008-08-08 13:28:54 PDT
Interesting. Is that due to the W3C specs or for another reason?

I'm sure this behaviour is not what is desired by people.

Is there a workaround?
Comment 3 Dave Hyatt 2008-08-08 13:29:29 PDT
overflow:hidden on your body.

I do think shadow should be considered part of overflow, so don't view this as a bug.
Comment 4 Kevin Cannon 2008-08-08 13:50:56 PDT
Well, overflow: hidden is going to cause your whole body not to function as intended. You won't get any scrollbars at all then. 

I suppose you could use the semi-standard overflow-x: hidden, but still seems wrong to me. I think setting overflow-x like that could have all sorts of unintended consequences with browser width & fluid layouts. E.g. if you have overflow-x:hidden set, and also have fixed width site, then it'll hide scrollbars when it should really show them. I'd guess that this is just one issue that will crop up in a number of situations with containing boxes that we can't forsee. 

I'll defer to your expertise though, but I do contend that it's a bit counter intuitive, as shadows and glows etc... are effects added onto an element. People won't expect it to act in this way.
Comment 5 Dave Hyatt 2008-08-08 14:02:27 PDT
Except people use the shadows sometimes as a means of actually drawing.  One of the text shadow examples in the CSS spec basically uses the shadow to make the text visible.  If that were not considered overflow, then you would possibly cut off that text.

In other words, I don't think it's as simple as saying "this is always overflow" or "this is never overflow."  For now we've erred on the side of caution (make it overflow so the user can always see it).

Comment 6 Dave Hyatt 2009-08-04 13:22:06 PDT
https://bugs.webkit.org/show_bug.cgi?id=24751 might be related.
Comment 7 Dave Hyatt 2009-08-14 22:48:11 PDT
Created attachment 34888 [details]
Work in progress.  Just saving what I've done so far.
Comment 8 Dave Hyatt 2009-08-14 23:21:02 PDT
Some notes:

Multicol overflow broken.

Need to make sure the line boxes that don't render just don't contribute to overflow, since if they're not going to paint anything, they don't matter.  Will have to think about hit testing though.
Comment 9 Dave Hyatt 2009-08-15 17:27:02 PDT
Created attachment 34909 [details]
New patch (still WIP)

This patch gets the inline box model in really good shape.  It makes the boxes with no text children shrinking quirk much more sane in that the boxes are now still positioned correctly on the baseline now and have their full height.   They just get clamped for the purposes of overflow calculations.

We should probably consider making inline boxes with borders, backgrounds or box-shadows turn off the quirk.  I put a fixme in the code to that effect.

What is still broken:
Some text field issues
Compositing hw accel issues
Comment 10 Dave Hyatt 2009-08-15 22:33:04 PDT
Created attachment 34921 [details]
All non-pixel tests pass except for multicol

Closer now.  Fixed some layout overflow propagation bugs (it shouldn't propagate across overflow != visible).  Only multicol is broken.

I have yet to run pixel tests to exercise visual overflow, so that's a remaining can of worms.

Layout test results not included to keep patch size down, since this isn't for review yet.
Comment 11 Dave Hyatt 2009-08-16 12:48:49 PDT
Created attachment 34931 [details]
Code changes
Comment 12 Dave Hyatt 2009-08-16 12:49:26 PDT
Created attachment 34932 [details]
Layout test changes
Comment 13 Dave Hyatt 2009-08-16 12:53:13 PDT
Created attachment 34933 [details]
Code Changes
Comment 14 mitz 2009-08-18 10:20:03 PDT
Comment on attachment 34933 [details]
Code Changes

You caught a few substantial comments while I was reviewing the patch. Here are my comments:

@@ -14561,6 +14563,7 @@
 				BCEA4820097D93020094C9E4 /* RenderBlock.cpp */,
 				BCEA4821097D93020094C9E4 /* RenderBlock.h */,
 				BCEA4813097D93020094C9E4 /* RenderBlockLineLayout.cpp */,
+				BCFA930710333193007B25D1 /* RenderOverflow.h */,
 				BCEA4822097D93020094C9E4 /* RenderBox.cpp */,
 				BCEA4823097D93020094C9E4 /* RenderBox.h */,
 				BC96DB450F3A882200573CB3 /* RenderBoxModelObject.cpp */,
I think this means that you didn’t sort the file list in the Xcode project.

-            // If letter-spacing is negative, we should factor that into right overflow. (Even in RTL, letter-spacing is
+            // If letter-spacing is negative, we should factor that into right visual and layout overflow. (Even in RTL, letter-spacing is
             // applied to the right, so this is not an issue with left overflow.
             int letterSpacing = min(0, (int)rt->style(m_firstLine)->font().letterSpacing());
+            rightLayoutOverflow = max(xPos + text->width() - letterSpacing, rightLayoutOverflow);

The comment says “factor that into right visual and layout overflow” but you’re only adding it explicitly to layout overflow. The code seems correct to me, but the comment is confusing.

+            leftVisualOverflow = min(xPos + visualOverflowLeft, leftVisualOverflow);
+            rightVisualOverflow = max(xPos + text->width() + visualOverflowRight, rightVisualOverflow);

It’s very confusing that you kept the visualOverflowLeft and visualOverflowRight variables’ names when you added leftVisualOverflow and rightVisualOverflow. Can you rename the former?

+void RenderBlock::addOverflowFromBlockChildren()
+    IntRect borderBox = borderBoxRect();

I think borderBox is unused.

@@ -3161,12 +3030,12 @@ int RenderBlock::addOverhangingFloats(Re
             r->m_shouldPaint = true;
         if (r->m_shouldPaint && !r->m_renderer->hasSelfPaintingLayer()) {
-            IntRect floatOverflowRect = r->m_renderer->overflowRect(false);
+            IntRect floatOverflowRect = r->m_renderer->combinedOverflowRect();
             floatOverflowRect.move(r->m_left + r->m_renderer->marginLeft(), r->m_top + r->m_renderer->marginTop());
-    child->addVisualOverflow(floatsOverflowRect);
+    child->addLayoutOverflow(floatsOverflowRect);
     return lowestFloatBottom;

I don’t understand this. Don’t you need to collect visual overflow from floats into the child’s visual overflow? This is a recurring theme (see RenderSlider later), so maybe I’m missing something.

Index: WebCore/rendering/RenderBox.cpp
--- WebCore/rendering/RenderBox.cpp	(revision 47306)
+++ WebCore/rendering/RenderBox.cpp	(working copy)
@@ -91,7 +91,7 @@ void RenderBox::destroy()
     if (style() && (style()->height().isPercent() || style()->minHeight().isPercent() || style()->maxHeight().isPercent()))

Extra whitespace.

+    void adjustOverflowForHeightChange(int height);

The parameter probably doesn’t need to be named here.

-        int top = min(curr->root()->topOverflow(), curr->root()->selectionTop()) - renderer->maximalOutlineSize(info.phase);
-        int bottom = curr->root()->bottomOverflow() + renderer->maximalOutlineSize(info.phase);
+        IntRect boxRect(curr->combinedOverflowRect());

I think boxRect is unused.

+} // namespace WebCorre

Typo you noticed.

Index: WebCore/rendering/RenderSlider.cpp
--- WebCore/rendering/RenderSlider.cpp	(revision 47306)
+++ WebCore/rendering/RenderSlider.cpp	(working copy)
@@ -355,17 +355,12 @@ void RenderSlider::layout()
-        IntRect thumbOverflowRect = thumb->overflowRect();
+        IntRect thumbOverflowRect = thumb->combinedOverflowRect();
         thumbOverflowRect.move(thumb->x(), thumb->y());
-    // FIXME: m_overflowWidth and m_overflowHeight should be renamed
-    // m_overflowRight and m_overflowBottom.
-    m_overflowLeft = overflowRect.x();
-    m_overflowTop = overflowRect.y();
-    m_overflowWidth = overflowRect.right();
-    m_overflowHeight = overflowRect.bottom();
+    addLayoutOverflow(overflowRect);

Isn’t this potentially adding the thumb’s visual overflow to the slider’s layout overflow?

-void RootInlineBox::destroy(RenderArena* arena)
-    if (m_overflow)
-        m_overflow->destroy(arena);
-    detachEllipsisBox(arena);
-    InlineFlowBox::destroy(arena);

What happened to detachEllipsisBox()? Aren’t we going to leak all the dots now?

Index: WebCore/rendering/RootInlineBox.h
--- WebCore/rendering/RootInlineBox.h	(revision 47306)
+++ WebCore/rendering/RootInlineBox.h	(working copy)
@@ -38,31 +38,33 @@ class RootInlineBox : public InlineFlowB
     RootInlineBox(RenderObject* obj)
         : InlineFlowBox(obj)
-        , m_overflow(0)
         , m_lineBreakObj(0)
         , m_lineBreakPos(0)
+        , m_lineTop(0)
+        , m_lineBottom(0)
+        , m_floats(0)

You don’t need to initialize an OwnPtr to 0.
Comment 15 Dave Hyatt 2009-08-18 11:26:09 PDT
Created attachment 35055 [details]

Patch incorporating all of mitzpettel's feedback along with some additional fixes to reflections.
Comment 16 Dave Hyatt 2009-08-18 11:58:49 PDT
Created attachment 35059 [details]
New patch that fixes a bug in addOverflowFromChild caught by fixing the propagation of float overflow.
Comment 17 Dave Hyatt 2009-08-18 12:16:01 PDT
Fixed in r47440.
Comment 18 Dimitri Glazkov (Google) 2009-08-21 13:08:32 PDT
Landing introduced a pretty ghastly regression: The focus ring now snakes around overflowing objects in contenteditable areas. Take a look at this for instance on a ToT:


Part of the reason this wasn't caught because we don't run pixel tests on build bots.
Comment 19 mitz 2009-08-21 13:26:22 PDT
Reopening a bug is not the appropriate way to track side-effects of fixing the original bug. Please file a new bug if you think the new behavior is incorrect.
Comment 20 Eric Seidel (no email) 2009-08-21 14:48:48 PDT
I'm not sure we have very clear policy on any of this. :)  But I agree with mitz, we should open a new bug.  Closing this one.