Bug 47237 – Make selection work with vertical text.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dave Hyatt]

Make selection work with vertical text.

[Adele Peterson]

<rdar://problem/8612041>

[Dave Hyatt]

Created attachment 73118 [details]
Patch

[Early Warning System Bot]

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

[Dave Hyatt]

Created attachment 73164 [details]
Patch

[Dave Hyatt]

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.

[Dave Hyatt]

Fixed in r71465.

[Csaba Osztrogonác]

(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

[Ryosuke Niwa]

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

[Kamil Blank]

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.