Bug 25459 – Regression(?): tabs in not-left alignment erroneously overflows

• Component: Layout and Rendering
• Status: REOPENED

[Xiaomei Ji]

Test version:
Safari 4 public beta (528.16): OK
with nightly webkit build r42662: FAIL

Steps:
1. open the attached txt file, copy the whole text
2. open the attached HTML file, paste the text into the text area

Results:
The first Hebrew character is not displayed inside the textarea.

[Xiaomei Ji]

Created attachment 29867 [details]
txt file to copy text from

[Xiaomei Ji]

Created attachment 29868 [details]
HTML file to past text into

[Xiaomei Ji]

In Chrome bug:
http://code.google.com/p/chromium/issues/detail?id=11022

[Yair Yogev]

if you paste and then rsize that textarea to the right by dragging the corner of it, you can "choose" how much you want the string to be covered. (you can paste and then resize or resize and then paste, same results, and the string is covered differently for every textarea width)

for example try this text area instead
http://chromium.googlecode.com/issues/attachment?aid=5947434597506434990&name=test3.html
here the whole first word (not character) will be covered.

if instead you paste the string in that textarea *after* writing the word
hello (anything between 3-8 characters will work in this specific case)
the string will appear behind the "hello"

see screenshot
http://img17.imageshack.us/img17/7143/chrometextoverlaps.jpg

[Ryosuke Niwa]

Mn.. I can't reproduce this bug on TOT.  All "פורסם" is displayed on the textarea.  Maybe it has been fixed?  Xiaomei, can you verify it on TOT?

[Yair Yogev]

Still reproducible using Chromium ToT with WebKit 70105 under XP.

Make sure you use Ctrl+A to copy the whole text from the txt file (maybe something slipped out?)

[Xiaomei Ji]

Created attachment 71303 [details]
chrome result in Windows vista

second progame.
test in Chrome 8.0.560.0 (63221) (Webkit r70136) in Windows Vista.

[Ryosuke Niwa]

(In reply to comment #7)
> Created an attachment (id=71303) [details]
> chrome result in Windows vista
> 
> second progame.
> test in Chrome 8.0.560.0 (63221) (Webkit r70136) in Windows Vista.

It seems like this only reproduces on Windows Chromium.  I'm not even sure if this is a WebKit bug.  Is there a good reason to believe this is a bug in WebKit?

[Xiaomei Ji]

(In reply to comment #8)
> (In reply to comment #7)
> > Created an attachment (id=71303) [details] [details]
> > chrome result in Windows vista
> > 
> > second progame.
> > test in Chrome 8.0.560.0 (63221) (Webkit r70136) in Windows Vista.
> 
> It seems like this only reproduces on Windows Chromium.  I'm not even sure if this is a WebKit bug.  Is there a good reason to believe this is a bug in WebKit?

The operation should be taken care by WebKit, not Chromium.
It might be a WebKit platform-dependent bug.

Safari (4.0.5) in Windows behaves the same as Chromium (8.0.552.5 dev).
('div' in the attachment is from my local build, which might not be accurate. paste the string to 'div' works fine for both Safari and Chromium dev).

[Ryosuke Niwa]

Created attachment 71462 [details]
demo

Open the attached document, and you see the first few letters overflowing from the div even though there are enough space to put the entire text node.

Isn’t this bug simply about tabs in right-to-left flows?

[Xiaomei Ji]

Created attachment 72773 [details]
test case

It is a webkit bug. Safari and Chromium in Mac overflows wrongly using the attached test case. 

Dan is correct that it is the tabs in right-to-left overflow problem.

In the following line in RenderBlock::computeInlineDirectionPositionsForLine()

            r->m_box->setLogicalWidth(rt->width(r->m_start, r->m_stop - r->m_start, totalLogicalWidth, firstLine, &fallbackFonts, &glyphOverflow) + hyphenWidth);

'totalLogicalWidth' is not the x-axis position for a BidiRun in RTL, base on which, the width of 'tab' is computed wrongly in WidthIterator::advance().

[Xiaomei Ji]

Created attachment 73013 [details]
patch w/ layout test

[Ryosuke Niwa]

Comment on attachment 73013 [details]
patch w/ layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=73013&action=review

> LayoutTests/fast/dom/tab-in-right-alignment-expected.txt:23
> +314
> +308
> +303
> +297
> +288
> +279
> +271
> +263
> +243
> +187
> +195
> +203
> +211
> +219
> +227
> +235
> +179
> +171
> +163
> +131
> +139
> +147

Instead of printing these coordinates out, can we compare it with some expected value?  It's really hard to tell whether the test is passing or not from this output.  Also, I suspect that this test fails on other platforms because of font differences.

[Xiaomei Ji]

Created attachment 73026 [details]
patch w/ layout test

changed to only compare the x-pos of the 2 right most characters. And should be no need to rebaseline for other platforms.

[Ryosuke Niwa]

Comment on attachment 73026 [details]
patch w/ layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=73026&action=review

> LayoutTests/fast/dom/tab-in-right-alignment.html:25
> +    }

Maybe print log("PASS") here so that it's easy to tell that the test is passing.  It's also a good idea to hide the contents of div in DRT to avoid unnecessary flakiness etc... i.e. do:
if (window.layoutTestController)
  div.innerHTML = ""; // or div.style.display = "none";

[Xiaomei Ji]

Created attachment 73122 [details]
patch w/ layout test

updated per rniwa's feedback and skip the test in QT since QT does not support textInputController.

[Dave Hyatt]

Comment on attachment 73122 [details]
patch w/ layout test

I think this idea is fine for RTL, but the patch (unless I'm mistaken) will break LTR.  In the case where the logicalLeft of the root box in LTR doesn't happen to match the left content edge of the containing block (e.g., because of a left float), then this will make the tabs behave differently on the narrower LTR lines.

[Dave Hyatt]

Are the tabs really variable on each RTL line?  I would have thought you'd go from the right edge so that you'd have some consistency.

[Dave Hyatt]

xji pointed out to me that there's an inconsistency here with how we measure, since computeInlineDirectionPositionsForLine doesn't include the actual offset from the block edge in its measurement.  For the rendering to be consistent it has to do the same thing.

It seems to me like this is right, then... we should maybe mention that textPos() is trying to be consistent with computeInlineDirectionPositionsForLine in the implementation of textPos().

It also seems like you don't need to subtract out the borders and padding any longer if you're subtracting from the root?

[Xiaomei Ji]

Created attachment 73282 [details]
patch w/ layout test

updated code and test per dhyatt's feedback.

[Ryosuke Niwa]

Comment on attachment 73282 [details]
patch w/ layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=73282&action=review

> WebCore/platform/graphics/TextRun.h:123
> +    int m_xpos;  

There's extraneous space after m_xpos.  Please remove it.

[Dave Hyatt]

Comment on attachment 73282 [details]
patch w/ layout test

r=me

[Xiaomei Ji]

Committed r72847: <http://trac.webkit.org/changeset/72847>

[Yair Yogev]

i don't think this issue is fixed :/
is passes the new test case, but fails the original "2 file copy+paste procedure" test case.
re-open?

[Xiaomei Ji]

Created attachment 75829 [details]
test case for textarea

works for 'div', but not for 'textarea'.
reopened.

[Xiaomei Ji]

Created attachment 76462 [details]
test case for Mac port

Previous fix brings the consistency between RenderBlock::computeInlineDirectionPositionsForLine() and Font::drawSimpleText(). And it fixed the problem when the line can fit in the containing block (when containing block's available width is greater then the total logical width of the line).

But there are 2 other problems:

1. In the case of writing direction is RTL, wide line should overflow to the left. So, we need to add logicalLeft adjustment when text alignment is LEFT or RIGHT in RenderBlock::computeInlineDirectionPositionsForLine(). For example:

        case RIGHT:
        case WEBKIT_RIGHT:
            if (style()->isLeftToRightDirection()) {
                ......
            } else {
                if (totalLogicalWidth > availableLogicalWidth && trailingSpaceRun) {
                    trailingSpaceRun->m_box->setLogicalWidth(max(0, trailingSpaceRun->m_box->logicalWidth() - totalLogicalWidth + availableLogicalWidth));
                    totalLogicalWidth -= trailingSpaceRun->m_box->logicalWidth();
+                  logicalLeft +=  availableLogicalWidth - totalLogicalWidth;
                } else
                    logicalLeft += availableLogicalWidth - totalLogicalWidth;
            }
            break;


2. TAB width computation is not consistent between RenderBlock::findNextLineBreak() and RenderBlock::computeInlineDirectionPositionsForLine()/ Font::drawSimpleText().

Given the following example: "ABCDE: \topqrst uvwx" in RTL block, where capital letter stands for Hebrew letter.
Let's assume the logical width of "ABCDE: " is 62, logical width of "opqrst" is 86, tab width is 48, and the width of block is 183.

The character width computation in RenderBlock::findNextLineBreak() follows logical order. So, the width of "\t" will be: "48 - mod(62, 48)" = 34,
and "ABCDE: \topqrst" can fit in one line.

After line break is found, bidi run will be generated on the line. Then, logical width and logical left are computed for each InlineBox in 
RenderBlock::computeInlineDirectionPositionsForLine().

The logical width computation in RenderBlock::computeInlineDirectionPositionsForLine() (and Font::drawSimpleText()) follows visual order between text runs and logical order inside one text run. The 1st visual text run in the above line is "opqrst", and the 2nd is "ABCDE: \t". So, the width of "\t" will be 
"48 - mod(86 + 62, 48)" = 44. Same when drawing text.

In the end, the above line overflowed left (although it should fit in).

I am thinking to compute the logical width of each InlineBox in RenderBlock::computeInlineDirectionPositionsForLine() following *logical* order of text runs.
And besides logical left in InlineBox, we will need a new data member: logical offset (offset of the box in logical order), which will replace textPos() as the x position of text run and is used in "\t" width computation.

[Xiaomei Ji]

reopened since the fix is not completed.

[Eric Seidel (no email)]

Comment on attachment 73282 [details]
patch w/ layout test

Cleared David Hyatt's review+ from obsolete attachment 73282 [details] so that this bug does not appear in http://webkit.org/pending-commit.