Bug 9272 – Left/Right borders/padding/margins are not always added correctly when rendering multiline inline boxes with bidi elements

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Sam Weinig]

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.

[Sam Weinig]

This is a follow up bug for bug 8387.

[Sam Weinig]

Created attachment 8672 [details]
test case 1

[Sam Weinig]

Created attachment 8673 [details]
test case 2

[Sam Weinig]

Created attachment 8674 [details]
test case 3

[Sam Weinig]

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

[Ryosuke Niwa]

The bug 47210 is related to this bug (possibly a dupe).

[Eric Seidel (no email)]

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.

[Yael]

Created attachment 87063 [details]
Reduced test case

A much reduced version of the first test case.

[Yael]

Created attachment 87150 [details]
Patch.

[Yael]

Created attachment 87151 [details]
Patch

Remove xcode project changes that snack in.

[WebKit Review Bot]

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.

[Eric Seidel (no email)]

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

[Yael]

Created attachment 87156 [details]
Patch.

Fix style error and add early return suggested by Eric.

[Yael]

Created attachment 87274 [details]
Patch

Same patch, just rebased it in hope it will apply now.

[Eric Seidel (no email)]

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! :)

[Yael]

Created attachment 87347 [details]
Patch

Patch, address Eric's comments and fix the regression found in the previous patch (in existing layout tests).

[Eric Seidel (no email)]

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?

[Yael]

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.

[Yael]

Created attachment 87481 [details]
Patch.

Updated the results for css2.1/t090501-c414-flt-ln-02-d.html , and address comment #19.

[Eric Seidel (no email)]

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!

[Yael]

Created attachment 87486 [details]
Patch

Resolve conflicts in the patch.

[WebKit Commit Bot]

Comment on attachment 87486 [details]
Patch

Clearing flags on attachment: 87486

Committed r82419: <http://trac.webkit.org/changeset/82419>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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

[Yuta Kitamura]

This change caused a new pixel test failure (fast/box-shadow/basic-shadows.html). I will create another bug to track this.

[Yuta Kitamura]

Filed bug 57442.