Bug 132879 – REGRESSION (WebKit2): Zooming to text field leaves it partially hidden by the form assistant

• Component: WebKit2
• Status: RESOLVED FIXED

[Enrica Casucci]

We need to have in WebKit2 the same logic we had in WebKit for iOS.
This tracks the work required to implement the heuristics for zooming based on the font size and the device type.

<rdar://problem/16318049>

[Enrica Casucci]

Created attachment 231397 [details]
Patch

[WebKit Commit Bot]

Attachment 231397 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:499:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:500:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:501:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:502:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:503:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:504:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:700:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:746:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:750:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:751:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentView.mm:482:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentView.mm:483:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentView.mm:484:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentView.mm:485:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentView.mm:486:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentView.mm:487:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 16 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Comment on attachment 231397 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231397&action=review

> Source/WebCore/ChangeLog:10
> +        position in the when using delegate scrolling.

in the when

> Source/WebCore/platform/ScrollView.cpp:500
> +    IntPoint newScrollPosition = !delegatesScrolling() ? adjustScrollPositionWithinRange(scrollPoint) : scrollPoint;

this is somewhat scary to me

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:87
> +@interface UIView (UIViewInternal)

are these supposed to be (Details), or do we have different guidance now? In any case I don't think UIViewInternal is the right category name.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:632
> +// focusedElementRect and selectionRect are both in document cooridnate.

*coordinates*

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:638
> +    const double WKWebViewStandardFontSize = 16;
> +    const double kMinimumHeightToShowContentAboveKeyboard = 106;
> +    const CFTimeInterval UIWebFormAnimationDuration = 0.25;
> +    const double CaretOffsetFromWindowEdge = 20;

There is a big bunch of style mismatching here.

> 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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:663
> +    CGRect unoscuredScrollViewRectInWebViewCoordinate = UIEdgeInsetsInsetRect([self bounds], _obscuredInsets);

Coordinate*s* in a bunch of places

typo "unoscured"

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:744
> +    WebCore::FloatPoint newCenter = CGPointMake((topLeft.x + unoscuredScrollViewRectInWebViewCoordinate.size.width / 2.0),

Why the inner parens?

[Benjamin Poulain]

Comment on attachment 231397 [details]
Patch

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

[Enrica Casucci]

> this is somewhat scary to me
I discussed this at length with Ben. We should not be clamping the scroll position on the WebProcess side where the information could be stale.

> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:87
> > +@interface UIView (UIViewInternal)
> 
> are these supposed to be (Details), or do we have different guidance now? In any case I don't think UIViewInternal is the right category name.
I followed what was donne on line 79 of the same file where the author used UIScrollViewInternal as category name.

> Coordinate*s* in a bunch of places

Forgive me, it is the italian taking over coordinate is the plural in italian :-). I'll fix them.

[Enrica Casucci]

(In reply to comment #4)
> (From update of attachment 231397 [details])
> 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?
In theory an app developer can use he WK2 view in a remotely hosted view...

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

I've fixed it in both places.
 
> >      _keyboardVerticalOverlap = [[UIPeripheralHost sharedInstance] getVerticalOverlapForView:self usingKeyboardInfo:keyboardInfo];
> 
> We should nuke this, generalize the new code for double tap to zoom.
I will replace its use with _inputViewBounds.size.height

> 
> > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1786
> > +    information.selectionRect = IntRect(m_lastInteractionLocation, IntSize(1, 1));
> 
> Hehehe
I know :-)

[Ahmad Saleem]

Landed here: https://github.com/WebKit/WebKit/commit/395ac0af89e548a5ef46420d96307a0ab0934017


and didn't seem to back out.