[webkit-reviews] review granted: [Bug 134466] REGRESSION (WK2): Weird selection behavior on autos.yahoo.com, en.wikipedia.org : [Attachment 234117] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Enrica Casucci
<enrica at apple.com>'s request for review:
Bug 134466: REGRESSION (WK2): Weird selection behavior on autos.yahoo.com,
en.wikipedia.org
https://bugs.webkit.org/show_bug.cgi?id=134466

Attachment 234117: Patch
https://bugs.webkit.org/attachment.cgi?id=234117&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=234117&action=review


> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:760
> +    // Make sure the block is selectable.

Probably not useful.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:764
> +    if (renderer->childrenInline() && (renderer->isRenderBlock() &&
toRenderBlock(renderer)->inlineElementContinuation() == nil) &&
!renderer->isTable()) {

This "== nil" probably comes from UIKit, you can update this to the WebKit
style.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:770
> +    // If all we could find is a block whose height is veriy close to the
height

Typo: "veriy"

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:772
> +    const float factor = .97;

Maybe find a better name for this?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:774
> +    boundingRectInScrollViewCoordinates.scale(m_page->pageScaleFactor());

This is unlikely correct. The absoluteBoundingBoxRect and unobscuredContentRect
should be in the same coordinate system already.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:776
> +    if (boundingRectInScrollViewCoordinates.height() >
m_page->mainFrame().view()->unobscuredContentRect().height() * factor)

You may want to use the exposedContentRect() here to make the behavior more
predictable. Otherwise the behavior would change depending if the bars are in
or out.

It probably does not matter much.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1258
> -	       newRange = Range::create(newRange->startContainer()->document(),
newRange->startPosition(), currentRange->endPosition());
> +	       newRange = Range::create(newRange->startContainer()->document(),
newRange->endPosition(), currentRange->endPosition());
>	   else
> -	       newRange = Range::create(newRange->startContainer()->document(),
currentRange->startPosition(), newRange->endPosition());
> +	       newRange = Range::create(newRange->startContainer()->document(),
currentRange->startPosition(), newRange->startPosition());

Can you add an explanation for this fix in the ChangeLog?