Bug 9272 - Left/Right borders/padding/margins are not always added correctly when rendering multiline inline boxes with bidi elements
Summary: Left/Right borders/padding/margins are not always added correctly when render...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 43302 (view as bug list)
Depends on:
Blocks:
 
Reported: 2006-06-02 15:02 PDT by Sam Weinig
Modified: 2011-03-30 05:08 PDT (History)
12 users (show)

See Also:


Attachments
test case 1 (2.70 KB, text/html)
2006-06-02 15:04 PDT, Sam Weinig
no flags Details
test case 2 (2.93 KB, text/html)
2006-06-02 15:05 PDT, Sam Weinig
no flags Details
test case 3 (2.80 KB, text/html)
2006-06-02 15:06 PDT, Sam Weinig
no flags Details
test case using hebrew (1.91 KB, text/html)
2006-06-02 15:06 PDT, Sam Weinig
no flags Details
Reduced test case (1.05 KB, text/html)
2011-03-27 02:41 PDT, Yael
no flags Details
Patch. (593.22 KB, patch)
2011-03-28 09:18 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch (590.75 KB, patch)
2011-03-28 09:22 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch. (590.65 KB, patch)
2011-03-28 09:50 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch (590.64 KB, patch)
2011-03-29 00:25 PDT, Yael
eric: review-
Details | Formatted Diff | Diff
Patch (591.10 KB, patch)
2011-03-29 09:34 PDT, Yael
no flags Details | Formatted Diff | Diff
Patch. (671.03 KB, patch)
2011-03-30 01:09 PDT, Yael
eric: review+
Details | Formatted Diff | Diff
Patch (671.08 KB, patch)
2011-03-30 02:18 PDT, Yael
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2006-06-02 15:02:27 PDT
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.
Comment 1 Sam Weinig 2006-06-02 15:02:54 PDT
This is a follow up bug for bug 8387.
Comment 2 Sam Weinig 2006-06-02 15:04:39 PDT
Created attachment 8672 [details]
test case 1
Comment 3 Sam Weinig 2006-06-02 15:05:09 PDT
Created attachment 8673 [details]
test case 2
Comment 4 Sam Weinig 2006-06-02 15:06:06 PDT
Created attachment 8674 [details]
test case 3
Comment 5 Sam Weinig 2006-06-02 15:06:47 PDT
Created attachment 8675 [details]
test case using hebrew
Comment 6 mitz 2008-01-19 14:25:30 PST
The same problem exists with background-image-position: left and right. See <https://bugzilla.mozilla.org/attachment.cgi?id=297097> (from Mozilla's 412093).
Comment 7 mitz 2010-08-02 08:56:13 PDT
*** Bug 43302 has been marked as a duplicate of this bug. ***
Comment 8 Ryosuke Niwa 2011-03-07 17:00:57 PST
The bug 47210 is related to this bug (possibly a dupe).
Comment 9 Eric Seidel (no email) 2011-03-27 02:35:00 PDT
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.
Comment 10 Yael 2011-03-27 02:41:24 PDT
Created attachment 87063 [details]
Reduced test case

A much reduced version of the first test case.
Comment 11 Yael 2011-03-28 09:18:55 PDT
Created attachment 87150 [details]
Patch.
Comment 12 Yael 2011-03-28 09:22:04 PDT
Created attachment 87151 [details]
Patch

Remove xcode project changes that snack in.
Comment 13 WebKit Review Bot 2011-03-28 09:24:54 PDT
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 14 Eric Seidel (no email) 2011-03-28 09:28:54 PDT
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
Comment 15 Yael 2011-03-28 09:50:29 PDT
Created attachment 87156 [details]
Patch.

Fix style error and add early return suggested by Eric.
Comment 16 Yael 2011-03-29 00:25:37 PDT
Created attachment 87274 [details]
Patch

Same patch, just rebased it in hope it will apply now.
Comment 17 Eric Seidel (no email) 2011-03-29 01:31:13 PDT
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! :)
Comment 18 Yael 2011-03-29 09:34:09 PDT
Created attachment 87347 [details]
Patch

Patch, address Eric's comments and fix the regression found in the previous patch (in existing layout tests).
Comment 19 Eric Seidel (no email) 2011-03-29 09:44:36 PDT
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?
Comment 20 Yael 2011-03-29 14:26:46 PDT
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.
Comment 21 Yael 2011-03-30 01:09:08 PDT
Created attachment 87481 [details]
Patch.

Updated the results for css2.1/t090501-c414-flt-ln-02-d.html , and address comment #19.
Comment 22 Eric Seidel (no email) 2011-03-30 01:11:23 PDT
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!
Comment 23 Yael 2011-03-30 02:18:23 PDT
Created attachment 87486 [details]
Patch

Resolve conflicts in the patch.
Comment 24 WebKit Commit Bot 2011-03-30 03:09:52 PDT
Comment on attachment 87486 [details]
Patch

Clearing flags on attachment: 87486

Committed r82419: <http://trac.webkit.org/changeset/82419>
Comment 25 WebKit Commit Bot 2011-03-30 03:10:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 WebKit Review Bot 2011-03-30 04:04:53 PDT
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
Comment 27 Yuta Kitamura 2011-03-30 04:59:03 PDT
This change caused a new pixel test failure (fast/box-shadow/basic-shadows.html). I will create another bug to track this.
Comment 28 Yuta Kitamura 2011-03-30 05:08:32 PDT
Filed bug 57442.