Bug 20329

That was actually done on purpose.

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?

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

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.

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

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

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

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.

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

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.

Created attachment 34931 [details] Code changes

Created attachment 34932 [details] Layout test changes

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.

Created attachment 35055 [details] Patch Patch incorporating all of mitzpettel's feedback along with some additional fixes to reflections.

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

Fixed in r47440.

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.

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.