[webkit-reviews] review granted: [Bug 132879] REGRESSION (WebKit2): Zooming to text field leaves it partially hidden by the form assistant : [Attachment 231397] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue May 13 14:09:29 PDT 2014
Benjamin Poulain <benjamin at webkit.org> has granted Enrica Casucci
<enrica at apple.com>'s request for review:
Bug 132879: REGRESSION (WebKit2): Zooming to text field leaves it partially
hidden by the form assistant
https://bugs.webkit.org/show_bug.cgi?id=132879
Attachment 231397: Patch
https://bugs.webkit.org/attachment.cgi?id=231397&action=review
------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=231397&action=review
> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:92
> +- (BOOL)_isHostedInAnotherProcess;
I hope we don't actually need to support this?
>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:632
>> +// focusedElementRect and selectionRect are both in document cooridnate.
>
> *coordinates*
Let's rename them instead of having a comment:
focusedElementRectInDocumentCoordinates && selectionRectInDocumentCoordinates.
> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:633
> +- (void)_zoomToFocusRect:(WebCore::FloatRect)focusedElementRect
selectionRect:(WebCore::FloatRect)selectionRect fontSize:(float)fontSize
minimumScale:(double)minimumScale maximumScale:(double)maximumScale
allowUserScaling:(BOOL)allowUserScaling forceScroll:(BOOL)forceScroll
Let's rename allowUserScaling, it is not only user scaling.
> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:641
> + double scale = (allowUserScaling) ?
std::min(std::max(WKWebViewStandardFontSize / fontSize, minimumScale),
maximumScale) : contentZoomScale(self);
No need for parenthesis on allowUserScaling.
> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:643
> + CGFloat documentWidth = [_contentView bounds].size.width;
> + if (documentWidth)
It seems unlikely we end up in here without a documentWidth.
If that happen, that means there is a race between this call and a document
tear down. The right solution in that case is not to check the documentWidth,
but having a requestID that you invalidate.
> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:648
> + UIScrollView *scrollView = _scrollView.get();
Let's use _scrollView everywhere for clarity.
> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:653
> + focusedElementRectInNewScale.moveBy([_contentView frame].origin);
> + focusedElementRectInNewScale.scale(scale);
Looks like I put those two lines in the wrong order.
The scale should go first, then moveBy.
>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:655
>> + // Find first parent view with a view controller -> that controller's
root controller -> top-most full-screen view controller within root
controller's hierarchy.
>
> what-but-not-why comment
I agree with Tim, this should say that you want to find the part of the view
visible on screen.
> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:668
> + CGSize visibleSize = visibleScrollViewBoundsInWebViewCoordinate.size;
> + CGFloat visibleOffsetFromTop = 0;
I would put the new line above this pair instead of bellow.
The branch bellow is the one computing those values, it seems they should be in
the same paragraph.
> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:695
> + // Don't bother scrolling if the entire node is already visible,
whether or not we got a selectionRect.
> + if (CGRectContainsRect(currentlyVisibleRegionInWebViewCoordinates,
focusedElementRectInNewScale))
> + return;
> +
> + // Don't bother scrolling if we have a valid selectionRect and it is
already visible.
> + if (selectionRectIsNotNull &&
CGRectContainsRect(currentlyVisibleRegionInWebViewCoordinates, selectionRect))
> + return;
This does not look correct to me.
The first branch is comparing the focused element rect in *new* scale with the
current visible region. You should instead compare the current visible rect.
Instead of focusedElementRectInNewScale, you could use
convertRect:FromView:_contentView
The second branch comparing selectionRect in document coordinate with the
visible region in webView coordinate, that does not make sense.
> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:702
> + CGFloat horizontalSpaceInWebViewCoordinates = MAX((visibleSize.width -
focusedElementRectInNewScale.width()) / 2.0, 0.0);
> + CGFloat verticalSpaceInWebViewCoordinates = MAX((visibleSize.height -
focusedElementRectInNewScale.height()) / 2.0, 0.0);
Let's use std::max() for consistency.
> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:713
> + selectionRectInNewScale.moveBy([_contentView frame].origin);
> + selectionRectInNewScale.scale(scale);
I made the same mistake here, the origin is scaled.
> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:720
> + documentBoundsInNewScale.moveBy([_contentView frame].origin);
> + documentBoundsInNewScale.scale(scale);
Ditto.
> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:747
> + newCenter.scale(1 / scale, 1 / scale);
I would have a comment explaining this, that is incredibly confusing API.
> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:940
> _keyboardVerticalOverlap = [[UIPeripheralHost sharedInstance]
getVerticalOverlapForView:self usingKeyboardInfo:keyboardInfo];
We should nuke this, generalize the new code for double tap to zoom.
> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1786
> + information.selectionRect = IntRect(m_lastInteractionLocation,
IntSize(1, 1));
Hehehe
More information about the webkit-reviews
mailing list