Bug 54929 - REGRESSION: Text selection broken for text with line-height applied
Summary: REGRESSION: Text selection broken for text with line-height applied
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
: 60336 (view as bug list)
Depends on:
Blocks: 59435
  Show dependency treegraph
 
Reported: 2011-02-21 19:52 PST by Patrick Quinn-Graham
Modified: 2011-06-03 09:14 PDT (History)
12 users (show)

See Also:


Attachments
Test case (1.19 KB, text/html)
2011-02-21 20:00 PST, Patrick Quinn-Graham
no flags Details
Patch (5.73 KB, patch)
2011-03-11 18:07 PST, Levi Weintraub
no flags Details | Formatted Diff | Diff
fixes the bug (8.01 KB, patch)
2011-05-19 22:54 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
test case 2 (231 bytes, text/html)
2011-06-01 20:19 PDT, Ryosuke Niwa
no flags Details
converted the test to a script test (8.06 KB, patch)
2011-06-01 20:46 PDT, Ryosuke Niwa
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick Quinn-Graham 2011-02-21 19:52:22 PST
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.
Comment 1 Patrick Quinn-Graham 2011-02-21 20:00:32 PST
Created attachment 83261 [details]
Test case
Comment 2 Patrick Quinn-Graham 2011-02-21 20:03:46 PST
Worth noting is that this does not occur in Safari 5.0.3, but does in the latest webkit (r78794) and chromium (75286).
Comment 3 Ryosuke Niwa 2011-03-09 11:49:02 PST
This one seems to be a regression in complex text path.
Comment 4 Ryosuke Niwa 2011-03-09 11:52:04 PST
(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.
Comment 5 Levi Weintraub 2011-03-09 13:45:43 PST
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.
Comment 6 Levi Weintraub 2011-03-10 16:41:18 PST
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.
Comment 7 Levi Weintraub 2011-03-11 14:29:19 PST
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.
Comment 8 Levi Weintraub 2011-03-11 18:07:40 PST
Created attachment 85560 [details]
Patch
Comment 9 Levi Weintraub 2011-03-17 11:57:52 PDT
Can I get a review?
Comment 10 Ryosuke Niwa 2011-03-17 12:08:02 PDT
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."
Comment 11 Justin Garcia 2011-03-17 13:04:28 PDT
-        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...
Comment 12 Levi Weintraub 2011-03-17 13:20:36 PDT
(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.
Comment 13 Ojan Vafai 2011-04-26 16:17:18 PDT
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?
Comment 14 Justin Garcia 2011-04-27 15:09:48 PDT
<rdar://problem/9347763>
Comment 15 Levi Weintraub 2011-04-27 15:20:13 PDT
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.
Comment 16 Levi Weintraub 2011-04-29 12:01:30 PDT
(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 :)
Comment 17 Dave Hyatt 2011-04-29 15:07:07 PDT
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?
Comment 18 Ojan Vafai 2011-04-29 15:15:16 PDT
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.
Comment 19 Ryosuke Niwa 2011-05-06 08:54:29 PDT
*** Bug 60336 has been marked as a duplicate of this bug. ***
Comment 20 Ryosuke Niwa 2011-05-06 08:56:10 PDT
I might have encountered this bug while writing a patch for the bug 59435.  Will investigate later.
Comment 21 Ryosuke Niwa 2011-05-19 22:25:27 PDT
Hit testing is editing.
Comment 22 Ryosuke Niwa 2011-05-19 22:54:50 PDT
Created attachment 94178 [details]
fixes the bug
Comment 23 Eric Seidel 2011-06-01 12:54:01 PDT
I wonder how related this is to bug 55481.
Comment 24 Ryosuke Niwa 2011-06-01 20:19:17 PDT
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 :(
Comment 25 Ryosuke Niwa 2011-06-01 20:25:44 PDT
I would really like this patch be landed.  I'm really tired of not being able to select text properly on many websites.
Comment 26 Simon Fraser (smfr) 2011-06-01 20:29:49 PDT
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.
Comment 27 Ryosuke Niwa 2011-06-01 20:34:21 PDT
(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.
Comment 28 Simon Fraser (smfr) 2011-06-01 20:37:44 PDT
(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.
Comment 29 Ryosuke Niwa 2011-06-01 20:46:27 PDT
Created attachment 95713 [details]
converted the test to a script test
Comment 30 Ryosuke Niwa 2011-06-02 12:05:58 PDT
Committed r87936: <http://trac.webkit.org/changeset/87936>
Comment 31 Ryosuke Niwa 2011-06-02 12:09:13 PDT
Thanks for the review!
Comment 32 Ademar Reis 2011-06-03 09:14:15 PDT
Revision r87936 cherry-picked into qtwebkit-2.2 with commit 3104ebf <http://gitorious.org/webkit/qtwebkit/commit/3104ebf>