Bug 20329 – CSS3 Box shadow property pushes outside the bounds of the page causing horizontal scrollbars

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Kevin Cannon]

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.

That was actually done on purpose.

[Kevin Cannon]

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?

[Dave Hyatt]

overflow:hidden on your body.

I do think shadow should be considered part of overflow, so don't view this as a bug.

[Kevin Cannon]

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.

[Dave Hyatt]

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).

[Dave Hyatt]

https://bugs.webkit.org/show_bug.cgi?id=24751 might be related.

[Dave Hyatt]

Created attachment 34888 [details]
Work in progress.  Just saving what I've done so far.

[Dave Hyatt]

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.

[Dave Hyatt]

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:
Columns
Marquees
Some text field issues
Compositing hw accel issues

[Dave Hyatt]

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.

[Dave Hyatt]

Created attachment 34931 [details]
Code changes

[Dave Hyatt]

Created attachment 34932 [details]
Layout test changes

[Dave Hyatt]

Created attachment 34933 [details]
Code Changes

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());
             floatsOverflowRect.unite(floatOverflowRect);
         }
     }
-    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()))
         RenderBlock::removePercentHeightDescendant(this);
-
+    

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()
 
         statePusher.pop();
 
-        IntRect thumbOverflowRect = thumb->overflowRect();
+        IntRect thumbOverflowRect = thumb->combinedOverflowRect();
         thumbOverflowRect.move(thumb->x(), thumb->y());
         overflowRect.unite(thumbOverflowRect);
     }
 
-    // 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
 public:
     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.

[Dave Hyatt]

Created attachment 35055 [details]
Patch

Patch incorporating all of mitzpettel's feedback along with some additional fixes to reflections.

[Dave Hyatt]

Created attachment 35059 [details]
New patch that fixes a bug in addOverflowFromChild caught by fixing the propagation of float overflow.

[Dave Hyatt]

Fixed in r47440.

[Dimitri Glazkov (Google)]

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:

http://trac.webkit.org/export/47638/trunk/LayoutTests/editing/pasteboard/styled-element-markup.html

Part of the reason this wasn't caught because we don't run pixel tests on build bots.

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.

[Eric Seidel (no email)]

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.