Bug 54929

Created attachment 83261 [details] Test case

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

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

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

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.

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.

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.

Created attachment 85560 [details] Patch

Can I get a review?

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

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

(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 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?

<rdar://problem/9347763>

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.

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

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

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

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

Hit testing is editing.

Created attachment 94178 [details] fixes the bug

I wonder how related this is to bug 55481.

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 :(

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

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.

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

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

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

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

Thanks for the review!

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