Bug 54929 – REGRESSION: Text selection broken for text with line-height applied

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Patrick Quinn-Graham]

Starting a selection at the beginning of the contenteditable area and going past the end of the text results in the text selection being cancelled.

However, just to make this bug hard to reproduce, it only applies above a certain font size, in the attached test case (reduction.html) it only applies above 36px.

[Patrick Quinn-Graham]

Created attachment 83261 [details]
Test case

[Patrick Quinn-Graham]

Worth noting is that this does not occur in Safari 5.0.3, but does in the latest webkit (r78794) and chromium (75286).

[Ryosuke Niwa]

This one seems to be a regression in complex text path.

[Ryosuke Niwa]

(In reply to comment #3)
> This one seems to be a regression in complex text path.

Maybe not.  The selection is actually cancelled so this might be a hit-testing issue than rendering issue.

[Levi Weintraub]

I believe this is a hit testing bug, as you implied.

The selection isn't cancelled, but instead extended the opposite direction as if you'd dragged your mouse to the line above. You can see this if you start your selection in the middle of the content instead of the beginning.

[Levi Weintraub]

I messed around a bit here and it comes down to RenderBlock returning the wrong height.  In the success case, a valid height is given to calculating the boundsRect when hit testing the background in RenderBlock::nodeAtPoint, but in the fail case, a height that is much lower than the block's true height is used instead. When the hit test fails to be in the background for the RenderBlock, selection is placed at the end of the previous content, which gives us the result we're seeing.

I'll continue to investigate.

[Levi Weintraub]

Found the offending logic in RenderBlock::positionForPointWithInlineChildren (trac won't look at files as big as RenderBlock.cpp, so no linky). The correct InlineBox is determined, but a point inside that box is supposed to be passed in, but is calculated using the logicalTop, which can be negative when the text exceeds the size of the line. The result is positionForPoint on that box returns a value above it.

I believe using the InlineBox's root's selectionTop() is the correct fix for the problem.

[Levi Weintraub]

Created attachment 85560 [details]
Patch

[Levi Weintraub]

Can I get a review?

[Ryosuke Niwa]

Comment on attachment 85560 [details]
Patch

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

I can't review this patch since I'm not familiar with rendering engine but I'll comment on some nits in the tests.

> LayoutTests/editing/selection/selecting-text-with-height-exceeding-line-height.html:3
> +<div id="description">This tests selection of text boxes that exceed their line height. To test this manually, start dragging in the text below and drag right (not down) past the end of the content. The selection should contain the end of the content.</div>

I think you should check condition in script (just dump window.getSelection().toString()) so that you can print PASS/FAIL.  That way, you don't even have to include those editing delegate dumps.

> LayoutTests/editing/selection/selecting-text-with-height-exceeding-line-height.html:10
> +function runTest() {
> +    if (!window.layoutTestController)
> +        return;

I don't think you need to put your test in this function.  You can just do:

if (window.layoutTestController) {
...
}

> LayoutTests/editing/selection/selecting-text-with-height-exceeding-line-height.html:13
> +    window.layoutTestController.dumpEditingCallbacks();
> +    window.layoutTestController.dumpAsText();

You don't need "window."

[Justin Garcia]

-        IntPoint point(pointInLogicalContents.x(), closestBox->logicalTop());
+        IntPoint point(pointInLogicalContents.x(), closestBox->isText() ? closestBox->root()->selectionTop() : closestBox->logicalHeight());

Why not pass selectionTop() in all cases?

RenderReplaced::positionForPoint(const IntPoint& point)

assumes that anything above selectionTop() is above it also, for example. Not sure about the others...

[Levi Weintraub]

(In reply to comment #11)
> -        IntPoint point(pointInLogicalContents.x(), closestBox->logicalTop());
> +        IntPoint point(pointInLogicalContents.x(), closestBox->isText() ? closestBox->root()->selectionTop() : closestBox->logicalHeight());
> 
> Why not pass selectionTop() in all cases?
> 
> RenderReplaced::positionForPoint(const IntPoint& point)
> 
> assumes that anything above selectionTop() is above it also, for example. Not sure about the others...

I agree, it's weird, and that was originally the patch I tried. However, this broke for input elements, which would behave exactly like the attached test case.

[Ojan Vafai]

Comment on attachment 85560 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        When an InlineBox has a line-height smaller than its actual height it results in a negative logicalTop for
> +        associated InlineTextBoxes. This negative value won't necessarily result in a point the RenderText 

Is ever having a negative logicalTop a bug? Shouldn't logicalTop always be non-negative? I don't actually know. Hyatt?

> Source/WebCore/rendering/RenderBlock.cpp:4077
> +        IntPoint point(pointInLogicalContents.x(), closestBox->isText() ? closestBox->root()->selectionTop() : closestBox->logicalHeight());

The selectionTop of the rootlinebox is not necessarily inside the closestBox, is it? As in, what if the closestBox is on the second line?

[Justin Garcia]

<rdar://problem/9347763>

[Levi Weintraub]

Comment on attachment 85560 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        associated InlineTextBoxes. This negative value won't necessarily result in a point the RenderText 
> 
> Is ever having a negative logicalTop a bug? Shouldn't logicalTop always be non-negative? I don't actually know. Hyatt?

Perhaps this is the bug, I don't know. It does render correctly.

>> Source/WebCore/rendering/RenderBlock.cpp:4077
>> +        IntPoint point(pointInLogicalContents.x(), closestBox->isText() ? closestBox->root()->selectionTop() : closestBox->logicalHeight());
> 
> The selectionTop of the rootlinebox is not necessarily inside the closestBox, is it? As in, what if the closestBox is on the second line?

The RootInlineBox always owns just one line, so it'll always contain its InlineBox children.

[Levi Weintraub]

(In reply to comment #15)
> > Is ever having a negative logicalTop a bug? Shouldn't logicalTop always be non-negative? I don't actually know. Hyatt?

It'd be really great to have an answer to this question so we could fix this ugly bug :)

[Dave Hyatt]

It's not a bug that logicalTop can be negative.  It's a bit confusing to me why you would have to special case text though.  Other boxes can have negative logicalTop values also.  If you need a point that is inside selectionTop and selectionBottom maybe just clamp to it with max/min rather than special casing with isText?

[Ojan Vafai]

Comment on attachment 85560 [details]
Patch

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

>>> Source/WebCore/rendering/RenderBlock.cpp:4077
>>> +        IntPoint point(pointInLogicalContents.x(), closestBox->isText() ? closestBox->root()->selectionTop() : closestBox->logicalHeight());
>> 
>> The selectionTop of the rootlinebox is not necessarily inside the closestBox, is it? As in, what if the closestBox is on the second line?
> 
> The RootInlineBox always owns just one line, so it'll always contain its InlineBox children.

Heh. Whoops. Wrapping was a bad example. A better example is the following:
<span style="border:2px solid lawngreen">
    <span style="border:1px solid blue;font-size:20em">foo</span>
    <span style="border:1px solid red;">bar</span>
</span>

If you click to the right of foo, but above bar, don't we pass the wrong value, e.g., we could pass a value that's below the bottom of the box.

Also, why change logicalTop to logicalHeight in the non-isText case?

Sorry, my knowledge of the RenderTree and LineBox trees isn't good enough to know the answers to these off the top of my head.

[Ryosuke Niwa]

*** Bug 60336 has been marked as a duplicate of this bug. ***

[Ryosuke Niwa]

I might have encountered this bug while writing a patch for the bug 59435.  Will investigate later.

[Ryosuke Niwa]

Hit testing is editing.

[Ryosuke Niwa]

Created attachment 94178 [details]
fixes the bug

[Eric Seidel (no email)]

I wonder how related this is to bug 55481.

[Ryosuke Niwa]

Created attachment 95709 [details]
test case 2

(In reply to comment #23)
> I wonder how related this is to bug 55481.

This patch doesn't fix the bug 55481 but it fixes a similar case where a selectionTop/lineTop is affected by padding of surrounding lines.  Also patches are compatible in that merging both patches don't break tests added by the patches except... that the test included in the patch for the bug 55481 doesn't work on any port/platform but whatever platform the author used because the test relies on font metrics and text layout :(

[Ryosuke Niwa]

I would really like this patch be landed.  I'm really tired of not being able to select text properly on many websites.

[Simon Fraser (smfr)]

Comment on attachment 94178 [details]
fixes the bug

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

> LayoutTests/editing/selection/hit-test-on-text-with-line-height.html:38
> +        document.writeln('FAIL - selection was not collapsed');
> +    else if (selection.baseNode != textNode)
> +        document.writeln('FAIL - baseNode was not "' + textNode.textContent + '"');
> +    else if (selection.baseOffset != expectedOffset)
> +        document.writeln('FAIL - caret was at ' + selection.baseOffset + ' but expected to be at ' + expectedOffset);
> +    else
> +        document.writeln('PASS');

Please don't use document.writeln(). Append to innerText or innerHTML instead.

This whole test could probably be a script test.

[Ryosuke Niwa]

(In reply to comment #26)
> (From update of attachment 94178 [details])
> Please don't use document.writeln(). Append to innerText or innerHTML instead.

Why?

> This whole test could probably be a script test.

Will do.

[Simon Fraser (smfr)]

(In reply to comment #27)
> (In reply to comment #26)
> > (From update of attachment 94178 [details] [details])
> > Please don't use document.writeln(). Append to innerText or innerHTML instead.
> 
> Why?

http://stackoverflow.com/questions/802854/why-is-document-write-considered-a-bad-practice

> > This whole test could probably be a script test.
> 
> Will do.

Cool, thanks.

[Ryosuke Niwa]

Created attachment 95713 [details]
converted the test to a script test

[Ryosuke Niwa]

Committed r87936: <http://trac.webkit.org/changeset/87936>

[Ryosuke Niwa]

Thanks for the review!

[Ademar Reis]

Revision r87936 cherry-picked into qtwebkit-2.2 with commit 3104ebf <http://gitorious.org/webkit/qtwebkit/commit/3104ebf>