When rendering multiline inline boxes with bidi elements, the left and right borders, padding, and margins are not always added correctly on the first and last lines.
This is a follow up bug for bug 8387.
Created attachment 8672 [details] test case 1
Created attachment 8673 [details] test case 2
Created attachment 8674 [details] test case 3
Created attachment 8675 [details] test case using hebrew
The same problem exists with background-image-position: left and right. See <https://bugzilla.mozilla.org/attachment.cgi?id=297097> (from Mozilla's 412093).
*** Bug 43302 has been marked as a duplicate of this bug. ***
The bug 47210 is related to this bug (possibly a dupe).
These test cases are very useful, thank you. However to best understand the bug, I think we need to reduce one of these down to a single span.
Created attachment 87063 [details] Reduced test case A much reduced version of the first test case.
Created attachment 87150 [details] Patch.
Created attachment 87151 [details] Patch Remove xcode project changes that snack in.
Attachment 87151 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/rendering/InlineFlowBox.cpp:250: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Source/WebCore/rendering/InlineFlowBox.cpp:254: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Total errors found: 2 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 87150 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=87150&action=review > Source/WebCore/rendering/InlineFlowBox.cpp:245 > + RenderObject* current = logicallyLastRunRenderer->parent(); > + while (current && !current->isRenderBlock()) { > + if (current == renderer()) > + break; > + current = current->parent(); > + } isn't this jsut containingBlock()? If so, it seems "current" is th wrong name. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:580 > + if (BidiRun* run = resolver.logicallyLastRun()) { I would have early returned instead of makign a block
Created attachment 87156 [details] Patch. Fix style error and add early return suggested by Eric.
Created attachment 87274 [details] Patch Same patch, just rebased it in hope it will apply now.
Comment on attachment 87274 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87274&action=review It's hard to tell if this is right because the code isn't self-documenting yet. I think we need another round of cleanup before we can land this. See possible suggestinos below. > Source/WebCore/rendering/InlineFlowBox.cpp:237 > if (!lineBoxList->lastLineBox()->isConstructed()) { I think once we land this patch we should land a cleanup patch which turns this whole big if block into a helper function which returns includeRightEdge and includeLeftEdge as reference variables. Then it's easy to early return instead of making these nested ifs. > Source/WebCore/rendering/InlineFlowBox.cpp:245 > + RenderObject* object = logicallyLastRunRenderer->parent(); > + while (object && !object->isRenderBlock()) { > + if (object == renderer()) > + break; > + object = object->parent(); > + } I guess this is "isAncestorWithoutInterveningBlock"? maybe? There is an isAncestorOf() function already. I guess you don't really need the isRenderBlock() check, except for speed. I think we should still consider breaking this out into a well named fucntion since it's not clear what it does. We could explainit in a comment, but it's betetr to make nicely named functions than add comments. > Source/WebCore/rendering/InlineFlowBox.cpp:248 > + bool shouldIncludeEdge = lastLine || object != renderer(); Yeah, this is just confusing. You should put the object != render check in a local bool, or better yet, just put this all in a nicely named helper. Also we don't need to do the walk unless we get into this if anyway. Should includeEdge is not the right name for this bool either. The whole decision you're making is about including eht edge. This specific bool is only part of that check. Or? If we convert this to be a helper fucntion with early return, than it's easy to check if (!shouldIncludeEdge) and return early! :)
Created attachment 87347 [details] Patch Patch, address Eric's comments and fix the regression found in the previous patch (in existing layout tests).
Comment on attachment 87347 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87347&action=review > Source/WebCore/rendering/InlineFlowBox.cpp:270 > + if (endObject && endObject->isText()) { > + if (ltr) { > + if (!nextLineBox() > + && (((lastLine || !isAnsectorAndWithinBlock(renderer(), logicallyLastRunRenderer->parent())) && !inlineFlow->continuation()))) > + includeRightEdge = true; > + } else { > + if ((!prevLineBox() || prevLineBox()->isConstructed()) > + && (((lastLine || !isAnsectorAndWithinBlock(renderer(), logicallyLastRunRenderer->parent())) && !inlineFlow->continuation()))) > + includeLeftEdge = true; > + } > } else { > - if ((!prevLineBox() || prevLineBox()->isConstructed()) && > - ((lastLine && !inlineFlow->continuation()) || prevOnLineExists() || onEndChain(endObject))) > - includeLeftEdge = true; > - } > + if (ltr) { > + if (!nextLineBox() > + && ((lastLine || onEndChain(logicallyLastRunRenderer)) && !inlineFlow->continuation())) > + includeRightEdge = true; > + } else { > + if ((!prevLineBox() || prevLineBox()->isConstructed()) > + && ((lastLine || onEndChain(logicallyLastRunRenderer)) && !inlineFlow->continuation())) > + includeLeftEdge = true; > + } > + } seems if we inverted this if most of hte duplicate code would go away. it seems endObject && endObject->isText() controls whether we check onEndChain(logicallyLastRunRenderer) or !isAnsectorAndWithinBlock(renderer(), logicallyLastRunRenderer->parent()) Which we could do once and cache in a local variable. Something like: bool isLastObjectOnLine = (endObject && endObject->isText()) ? !isAnsectorAndWithinBlock(renderer(), logicallyLastRunRenderer->parent())) ? onEndChain(logicallyLastRunRenderer); Right?
The last patch I submitted today is causing a different result in the test css2.1/t090501-c414-flt-ln-02-d.html. At first I thought that I introduced regression, but looking closer at the change, it is adding a right border to the spans B, C, and D. The span A in this test already has a right border in the expected results. Can someone please tell me why is it that the spans B, C and D in the above test should not have a right border, or perhaps the expected result wrong? thanks.
Created attachment 87481 [details] Patch. Updated the results for css2.1/t090501-c414-flt-ln-02-d.html , and address comment #19.
Comment on attachment 87481 [details] Patch. Fantastic! I think an ideal follow-up pach would include splitting the includeLeftEdge/includeRightEdge calculation out of determineSpacingForFlowBoxes into a new function (determineEdges?) which can use early return. I think the code would end up even simpler then. This looks great though!
Created attachment 87486 [details] Patch Resolve conflicts in the patch.
Comment on attachment 87486 [details] Patch Clearing flags on attachment: 87486 Committed r82419: <http://trac.webkit.org/changeset/82419>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/82419 might have broken Qt Linux Release The following tests are not passing: css1/box_properties/padding_right_inline.html css2.1/t0805-c5519-ibrdr-r-00-a.html css2.1/t090501-c414-flt-ln-00-d.html css2.1/t090501-c414-flt-ln-01-d-g.html css2.1/t090501-c414-flt-ln-02-d.html css2.1/t090501-c414-flt-ln-03-d.html
This change caused a new pixel test failure (fast/box-shadow/basic-shadows.html). I will create another bug to track this.
Filed bug 57442.