Created attachment 222557 [details] Test case The failing test case: <b style="border:solid;"> <i style=" margin: 0px -8px 0px 0px; padding: 0px 1px 0px 0px;" > </i> </b> The error message: ASSERTION FAILED: x2 >= x1 WebKit/Source/WebCore/rendering/RenderObject.cpp(920) : void WebCore::RenderObject::drawLineForBoxSide(WebCore::GraphicsContext*, int, int, int, int, WebCore::BoxSide, WebCore::Color, WebCore::EBorderStyle, int, int, bool) Segmentation fault (core dumped) The backtrace: #1 0x00007ffff12ed160 in WebCore::RenderObject::drawLineForBoxSide (this=0x7eab60, graphicsContext=0xa2d420, x1=8, y1=5, x2=7, y2=8, side=WebCore::BSTop, color=..., borderStyle=WebCore::SOLID, adjacentWidth1=0, adjacentWidth2=0, antialias=false) at /home/martin/Data/WebKit/Source/WebCore/rendering/RenderObject.cpp:920 #2 0x00007ffff1207ea8 in WebCore::RenderBoxModelObject::paintOneBorderSide (this=0x7eab60, graphicsContext=0xa2d420, style=0x8a23a0, outerBorder=..., innerBorder=..., sideRect=..., side=WebCore::BSTop, adjacentSide1=WebCore::BSLeft, adjacentSide2=WebCore::BSRight, edges=0x7fffffffb130, path=0x0, bleedAvoidance=WebCore::BackgroundBleedNone, includeLogicalLeftEdge=true, includeLogicalRightEdge=true, antialias=false, overrideColor=0x0) at /home/martin/Data/WebKit/Source/WebCore/rendering/RenderBoxModelObject.cpp:1704 #3 0x00007ffff1208227 in WebCore::RenderBoxModelObject::paintBorderSides (this=0x7eab60, graphicsContext=0xa2d420, style=0x8a23a0, outerBorder=..., innerBorder=..., innerBorderAdjustment=..., edges=0x7fffffffb130, edgeSet=15, bleedAvoidance=WebCore::BackgroundBleedNone, includeLogicalLeftEdge=true, includeLogicalRightEdge=true, antialias=false, overrideColor=0x0) at /home/martin/Data/WebKit/Source/WebCore/rendering/RenderBoxModelObject.cpp:1745 #4 0x00007ffff1209905 in WebCore::RenderBoxModelObject::paintBorder (this=0x7eab60, info=..., rect=..., style=0x8a23a0, bleedAvoidance=WebCore::BackgroundBleedNone, includeLogicalLeftEdge=true, includeLogicalRightEdge=true) at /home/martin/Data/WebKit/Source/WebCore/rendering/RenderBoxModelObject.cpp:1976 #5 0x00007ffff1166252 in WebCore::InlineFlowBox::paintBoxDecorations (this=0x88de30, paintInfo=..., paintOffset=...) at /home/martin/Data/WebKit/Source/WebCore/rendering/InlineFlowBox.cpp:1334 #6 0x00007ffff1164ae6 in WebCore::InlineFlowBox::paint (this=0x88de30, paintInfo=..., paintOffset=..., lineTop=..., lineBottom=...) at /home/martin/Data/WebKit/Source/WebCore/rendering/InlineFlowBox.cpp:1146 #7 0x00007ffff1164c3d in WebCore::InlineFlowBox::paint (this=0xa209d0, paintInfo=..., paintOffset=..., lineTop=..., lineBottom=...) at /home/martin/Data/WebKit/Source/WebCore/rendering/InlineFlowBox.cpp:1162 #8 0x00007ffff136103d in WebCore::RootInlineBox::paint (this=0xa209d0, paintInfo=..., paintOffset=..., lineTop=..., lineBottom=...) at /home/martin/Data/WebKit/Source/WebCore/rendering/RootInlineBox.cpp:210 #9 0x00007ffff12becd5 in WebCore::RenderLineBoxList::paint (this=0x8a2378, renderer=0x8a22d0, paintInfo=..., paintOffset=...) at /home/martin/Data/WebKit/Source/WebCore/rendering/RenderLineBoxList.cpp:265 #10 0x00007ffff11bf833 in WebCore::RenderBlockFlow::paintInlineChildren (this=0x8a22d0, paintInfo=..., paintOffset=...) at /home/martin/Data/WebKit/Source/WebCore/rendering/RenderBlockFlow.cpp:3216 #11 0x00007ffff118578a in WebCore::RenderBlock::paintContents (this=0x8a22d0, paintInfo=..., paintOffset=...) at /home/martin/Data/WebKit/Source/WebCore/rendering/RenderBlock.cpp:2374 #12 0x00007ffff118649f in WebCore::RenderBlock::paintObject (this=0x8a22d0, paintInfo=..., paintOffset=...) at /home/martin/Data/WebKit/Source/WebCore/rendering/RenderBlock.cpp:2510 #13 0x00007ffff1183f21 in WebCore::RenderBlock::paint (this=0x8a22d0, paintInfo=..., paintOffset=...) at /home/martin/Data/WebKit/Source/WebCore/rendering/RenderBlock.cpp:2187 #14 0x00007ffff1185cc9 in WebCore::RenderBlock::paintChild (this=0x8a20d0, child=..., paintInfo=..., paintOffset=..., paintInfoForChild=..., usePrintRect=false) at /home/martin/Data/WebKit/Source/WebCore/rendering/RenderBlock.cpp:2424 #15 0x00007ffff11858c9 in WebCore::RenderBlock::paintChildren (this=0x8a20d0, paintInfo=..., paintOffset=..., paintInfoForChild=..., usePrintRect=false) at /home/martin/Data/WebKit/Source/WebCore/rendering/RenderBlock.cpp:2394 #16 0x00007ffff118586c in WebCore::RenderBlock::paintContents (this=0x8a20d0, paintInfo=..., paintOffset=...) at /home/martin/Data/WebKit/Source/WebCore/rendering/RenderBlock.cpp:2387 #17 0x00007ffff118649f in WebCore::RenderBlock::paintObject (this=0x8a20d0, paintInfo=..., paintOffset=...) at /home/martin/Data/WebKit/Source/WebCore/rendering/RenderBlock.cpp:2510 #18 0x00007ffff1183f21 in WebCore::RenderBlock::paint (this=0x8a20d0, paintInfo=..., paintOffset=...) at /home/martin/Data/WebKit/Source/WebCore/rendering/RenderBlock.cpp:2187 #19 0x00007ffff128a625 in WebCore::RenderLayer::paintForegroundForFragmentsWithPhase (this=0x8a21a0, phase=WebCore::PaintPhaseForeground, layerFragments=..., context=0xa2d420, localPaintingInfo=..., paintBehavior=0, subtreePaintRootForRenderer=0x0) at /home/martin/Data/WebKit/Source/WebCore/rendering/RenderLayer.cpp:4477 #20 0x00007ffff128a236 in WebCore::RenderLayer::paintForegroundForFragments (this=0x8a21a0, layerFragments=..., context=0xa2d420, transparencyLayerContext=0xa2d420, transparencyPaintDirtyRect=..., haveTransparency=false, localPaintingInfo=..., paintBehavior=0, subtreePaintRootForRenderer=0x0, selectionOnly=false, forceBlackText=false) at /home/martin/Data/WebKit/Source/WebCore/rendering/RenderLayer.cpp:4441 #21 0x00007ffff1288b32 in WebCore::RenderLayer::paintLayerContents (this=0x8a21a0, context=0xa2d420, paintingInfo=..., paintFlags=224) at /home/martin/Data/WebKit/Source/WebCore/rendering/RenderLayer.cpp:4162 #22 0x00007ffff1287bca in WebCore::RenderLayer::paintLayerContentsAndReflection (this=0x8a21a0, context=0xa2d420, paintingInfo=..., paintFlags=224) at /home/martin/Data/WebKit/Source/WebCore/rendering/RenderLayer.cpp:3858 #23 0x00007ffff1287a92 in WebCore::RenderLayer::paintLayer (this=0x8a21a0, context=0xa2d420, paintingInfo=..., paintFlags=224) at /home/martin/Data/WebKit/Source/WebCore/rendering/RenderLayer.cpp:3839 #24 0x00007ffff12891bb in WebCore::RenderLayer::paintList (this=0x8a2e00, list=0x8a4d90, context=0xa2d420, paintingInfo=..., paintFlags=224) at /home/martin/Data/WebKit/Source/WebCore/rendering/RenderLayer.cpp:4255 #25 0x00007ffff1288bf5 in WebCore::RenderLayer::paintLayerContents (this=0x8a2e00, context=0xa2d420, paintingInfo=..., paintFlags=224) at /home/martin/Data/WebKit/Source/WebCore/rendering/RenderLayer.cpp:4173 #26 0x00007ffff1287bca in WebCore::RenderLayer::paintLayerContentsAndReflection (this=0x8a2e00, context=0xa2d420, paintingInfo=..., paintFlags=0) at /home/martin/Data/WebKit/Source/WebCore/rendering/RenderLayer.cpp:3858 #27 0x00007ffff1287a92 in WebCore::RenderLayer::paintLayer (this=0x8a2e00, context=0xa2d420, paintingInfo=..., paintFlags=0) at /home/martin/Data/WebKit/Source/WebCore/rendering/RenderLayer.cpp:3839 #28 0x00007ffff1286c4c in WebCore::RenderLayer::paint (this=0x8a2e00, context=0xa2d420, damageRect=..., paintBehavior=0, subtreePaintRoot=0x0, region=0x0, paintFlags=0) at /home/martin/Data/WebKit/Source/WebCore/rendering/RenderLayer.cpp:3623 #29 0x00007ffff0ee30e4 in WebCore::FrameView::paintContents (this=0x787220, p=0xa2d420, rect=...) at /home/martin/Data/WebKit/Source/WebCore/page/FrameView.cpp:3497 #30 0x00007ffff0f8b403 in WebCore::ScrollView::paint (this=0x787220, context=0xa2d420, rect=...) at /home/martin/Data/WebKit/Source/WebCore/platform/ScrollView.cpp:1162 #31 0x00007ffff7b4ca05 in ewk_paint_context_paint (context=0x942400, view=0x787220, area=0xa21678) at /home/martin/Data/WebKit/Source/WebKit/efl/ewk/ewk_paint_context.cpp:179 #32 0x00007ffff7b6e0a5 in ewk_view_paint (priv=0x6ee240, context=0x942400, area=0xa21678) at /home/martin/Data/WebKit/Source/WebKit/efl/ewk/ewk_view.cpp:3019 #33 0x00007ffff7b5629f in _ewk_view_smart_repaints_process (smartData=0x6af2d0) at /home/martin/Data/WebKit/Source/WebKit/efl/ewk/ewk_view.cpp:1210 #34 0x00007ffff7b56643 in _ewk_view_smart_calculate (ewkView=0x92e6b0) at /home/martin/Data/WebKit/Source/WebKit/efl/ewk/ewk_view.cpp:1281 #35 0x00007ffff6969124 in evas_call_smarts_calculate (e=0x720c00) at evas_object_smart.c:838 #36 0x00007ffff69926a7 in evas_render_updates_internal (e=0x720c00, make_updates=make_updates@entry=1 '\001', do_draw=do_draw@entry=1 '\001') at evas_render.c:1255 #37 0x00007ffff6994fd9 in evas_render_updates (e=<optimized out>) at evas_render.c:1708 #38 0x00007ffff734adb4 in _ecore_evas_x_render (ee=0x7dc000) at ecore_evas_x.c:288 #39 0x00007ffff7347c31 in _ecore_evas_idle_enter (data=<optimized out>) at ecore_evas.c:59 #40 0x00007ffff756fef9 in _ecore_call_task_cb (data=<optimized out>, func=<optimized out>) at ecore_private.h:267 #41 _ecore_idle_enterer_call () at ecore_idle_enterer.c:168 #42 0x00007ffff75716ab in _ecore_main_loop_iterate_internal (once_only=once_only@entry=0) at ecore_main.c:1848 #43 0x00007ffff7571d57 in ecore_main_loop_begin () at ecore_main.c:956 #44 0x00000000004068e7 in main (argc=2, argv=0x7fffffffde48) at /home/martin/Data/WebKit/Tools/EWebLauncher/main.c:1008
Created attachment 225650 [details] Proposed patch
Can somebody take a look at this?
Comment on attachment 225650 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=225650&action=review > LayoutTests/fast/css/handling-negative-margins.html:12 > + ASSERTION FAILED: x2 >= x1 in WebCore::RenderObject::drawLineForBoxSide</p> It’s really strange to put this text into the test case. In the future it will be really confusing. There may not even be a function of that name there in future. A test that simply “passes if it does not crash” is not really so great. I presume that the assertion from before led to incorrect behavior of some sort. I would prefer a test that demonstrates the behavior is correct (also good if it would have triggered the assertion before). > Source/WebCore/ChangeLog:19 > + * css/CSSParser.cpp: > + (WebCore::CSSParser::parse4Values): Why is this listed? I don’t see a change to this function in the patch. > Source/WebCore/rendering/InlineBox.h:211 > + void setLogicalWidth(float w) > + { > + if (w < 0) { > + m_logicalWidth = -w; > + m_topLeft.setX(m_topLeft.x() - m_logicalWidth); > + } else > + m_logicalWidth = w; > + } This code looks OK to me, but I am not sure it’s good to put it inside the setter function itself. The other surrounding functions are simple setters and it seems a bit unusual to do this like this. I think the logic perhaps belongs in a slightly higher level, but really that’s a question for someone who has more knowledge/ownership of the structure of the code to build the line box tree. Also, as a matter of style, I prefer to put larger multi-line inline function definitions like this one outside the class.
(In reply to comment #3) > (From update of attachment 225650 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=225650&action=review > > A test that simply “passes if it does not crash” is not really so great. What basically happens is that the smaller we set the right margin, the shorter the top and bottom borderlines get, which are represented by rectangles. The length (or logical width) of a rectangle can even go under zero, so that the rectangle 'turns a somersault'. It means that the former right side of this 'rectangle-borderline' will be the left side now, and its former upper-left point becomes the upper-right point. My patch relocates this point to make it the upper-left point again and turns the negative length into positive. Same thing applies to the left margin. The output is a thin orange column (the content box) and a thick black column (the fake borders) that is slipped to the left and then the right side of the content. The black column actually consists of a top, a bottom, a left and a right 'rectangle-borderline', but they are overlapped. I don't think that a prettier test can be made to demonstrate this issue, but this is the correct behavior here. > This code looks OK to me, but I am not sure it’s good to put it inside the setter function itself. The function setLogicalWidth is used over 20 different files throughout the project, and the width of any InlineBox object (including its descendant classes) can only be set by using this setter function, so I don't think this issue can be handled at a higher level. I made a small change to the InlineBox constructor, so that the width can not be set to a negative value here either.
Created attachment 227966 [details] Proposed patch
Comment on attachment 227966 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=227966&action=review > LayoutTests/fast/css/padding-margin-overlap.html:5 > + if (window.testRunner) > + testRunner.dumpAsText(); I still don’t understand why this is a “passes if it does not crash” test rather than, say, a reference test.
Comment on attachment 227966 [details] Proposed patch I should read the spec first, but it might be that for cases like this the inlinebox's width should just be 0 and we don't draw borders at all as opposed to resetting the topleft so that border painting passes. -just checked; FF does not paint borders either.
Created attachment 228846 [details] Proposed patch I have read the related part of the standard, but it does not define the correct behavior in this situation. I also checked three browsers and they handled the issue in slightly different ways. - Firefox and Opera: displays content but no borders - Chrome: displays content and positive borders I believe that Firefox and Opera handle the issue properly by consistently not displaying borders nor shadows if either the length or the width of the box is negative.
Comment on attachment 228846 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=228846&action=review > LayoutTests/ChangeLog:8 > + Test #1: Negative right margin + positive right padding > + Test #2: Negative left margin + positive left padding Tests only covert vertical. Should cover horizontal too. > LayoutTests/ChangeLog:12 > + * fast/css/padding-margin-overlap-expected.txt: Added. Please make this a reference test instead of a render-tree-dumping test. > Source/WebCore/platform/graphics/LayoutRect.h:83 > + bool hasPositiveDimensions() const; Please don’t add this. It’s the same as !isEmpty. If you look at IntSize::isEmpty and LayoutSize::isEmpty you will find that they are checking for positive dimensions, not just non-zero. > Source/WebCore/rendering/RenderBoxModelObject.cpp:1731 > + if (!outerBorder.rect().hasPositiveDimensions()) > + return; Should be: if (outerBorder.rect().isEmpty()) return;
Created attachment 229434 [details] Proposed patch Thank you for the review again! I have extended the test to upper and bottom cases. However, it seems like we can not make the assert fail by simply manipulating the top and bottom padding and margin values. I created a reference test by setting the border color to white in cases where the border should not be displayed. I tried to set the border style to 'none' or 'hidden' instead, but it made the content box shift by a few pixel (the width of the border).
Comment on attachment 229434 [details] Proposed patch Clearing flags on attachment: 229434 Committed r167351: <http://trac.webkit.org/changeset/167351>
All reviewed patches have been landed. Closing bug.
Bug 131988 is extending this functionality.