Bug 47237

<rdar://problem/8612041>

Created attachment 73118 [details] Patch

Attachment 73118 [details] did not build on qt: Build output: http://queues.webkit.org/results/5331023

Comment on attachment 73118 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=73118&action=review Found one mistake. > WebCore/rendering/RenderBlock.cpp:2709 > + if (!paintInfo || (style()->isHorizontalWritingMode() && physicalRect.y() < paintInfo->rect.bottom() && physicalRect.bottom() > paintInfo->rect.y() > + || (!style()->isHorizontalWritingMode() && physicalRect.x() < paintInfo->rect.right() && physicalRect.right() > paintInfo->rect.x()))) Redundant parentheses there. > WebCore/rendering/RenderBlock.h:121 > + IntRect logicalLeftSelectionGap(RenderBlock* rootBlock, const IntPoint& rootBlockPhysicalPosition, const IntSize& offsetFromRootBlock, > + RenderObject* selObj, int logicalLeft, int logicalTop, int logicalHeight, const PaintInfo* paintInfo); > + IntRect logicalRightSelectionGap(RenderBlock* rootBlock, const IntPoint& rootBlockPhysicalPosition, const IntSize& offsetFromRootBlock, > + RenderObject* selObj, int logicalRight, int logicalTop, int logicalHeight, const PaintInfo* paintInfo); Don’t add the argument name “paintInfo”. > WebCore/rendering/RenderBlock.h:541 > + GapRects selectionGaps(RenderBlock* rootBlock, const IntPoint& rootBlockPhysicalPosition, const IntSize& offsetFromRootBlock, > + int& lastLogicalTop, int& lastLogicalLeft, int& lastLogicalRight, const PaintInfo* paintInfo = 0); > + GapRects inlineSelectionGaps(RenderBlock* rootBlock, const IntPoint& rootBlockPhysicalPosition, const IntSize& offsetFromRootBlock, > + int& lastLogicalTop, int& lastLogicalLeft, int& lastLogicalRight, const PaintInfo* paintInfo); > + GapRects blockSelectionGaps(RenderBlock* rootBlock, const IntPoint& rootBlockPhysicalPosition, const IntSize& offsetFromRootBlock, > + int& lastLogicalTop, int& lastLogicalLeft, int& lastLogicalRight, const PaintInfo* paintInfo); > + IntRect blockSelectionGap(RenderBlock* rootBlock, const IntPoint& rootBlockPhysicalPosition, const IntSize& offsetFromRootBlock, > + int lastLogicalTop, int lastLogicalLeft, int lastLogicalRight, int logicalBottom, const PaintInfo* paintInfo); > int logicalLeftSelectionOffset(RenderBlock* rootBlock, int position); Ditto. > WebCore/rendering/RenderBlock.h:543 > + whitespace! > WebCore/rendering/RenderBox.cpp:3245 > + return style()->isHorizontalWritingMode() ? IntSize(offset.width(), logicalHeight() - offset.height()) : IntSize(logicalHeight() - offset.width(), offset.height()); Should be logicalWidth() - offset.width() in the second branch. > WebCore/rendering/RootInlineBox.cpp:254 > +GapRects RootInlineBox::lineSelectionGap(RenderBlock* rootBlock, const IntPoint& rootBlockPhysicalPosition, const IntSize& offsetFromRootBlock, > + int selTop, int selHeight, const PaintInfo* paintInfo) Can omit “paintInfo”.

(In reply to comment #4) > > WebCore/rendering/RootInlineBox.cpp:254 > > +GapRects RootInlineBox::lineSelectionGap(RenderBlock* rootBlock, const IntPoint& rootBlockPhysicalPosition, const IntSize& offsetFromRootBlock, > > + int selTop, int selHeight, const PaintInfo* paintInfo) > > Can omit “paintInfo”. Ignore this :)

Created attachment 73164 [details] Patch

This new patch addresses Dan's comments, but more importantly it fixes one failing layout test from the previous patch. See the second paragraph of the WebCore ChangeLog for the explanation. The only change is to RootInlineBox::selectionTop() (to now extend selection up to the top content edge of the block now that lineTop no longer always starts there) and changing RenderBlock::positionForPoint to just exactly match selection as far as what line it chooses when you're in between lines.

Fixed in r71465.

(In reply to comment #8) > Fixed in r71465. It made editing/selection/after-line-break.html fail on Qt bot, I filed a new bug on it: https://bugs.webkit.org/show_bug.cgi?id=49127

A while bunch of editing pixel tests seem to be failing after this patch. See: http://build.webkit.org/results/Chromium%20Win%20Release%20(Tests)/r71466%20(4928)/results.html http://build.webkit.org/results/Chromium%20Mac%20Release%20(Tests)/r71467%20(2905)/results.html http://build.webkit.org/results/Chromium%20Linux%20Release%20(Tests)/r71465%20(8023)/results.html

Hi, After this patch and changes made inside RootInlineBox::selectionTop(), I've noticed some problem on BBC site which I reported here: https://bugs.webkit.org/show_bug.cgi?id=65307 Could you please take a look on it? (In reply to comment #7) > This new patch addresses Dan's comments, but more importantly it fixes one failing layout test from the previous patch. See the second paragraph of the WebCore ChangeLog for the explanation. The only change is to RootInlineBox::selectionTop() (to now extend selection up to the top content edge of the block now that lineTop no longer always starts there) and changing RenderBlock::positionForPoint to just exactly match selection as far as what line it chooses when you're in between lines.