Visual viewports: carets and selection UI are incorrectly positioned when editing fixed elements

• Component: WebKit2
• Status: RESOLVED FIXED

[Wenson Hsieh]

1. Enable the new visual viewports for form assisted elements (inputs, etc.)
2. Go to whsieh.github.io/examples/fixed-inputs
3. Scroll somewhere in the middle and interact with the bottom input

Observed: text caret is somewhere high above where it should be.

[Wenson Hsieh]

<rdar://problem/29602382>

[Wenson Hsieh]

Created attachment 296943 [details]
Patch

[Wenson Hsieh]

Created attachment 297059 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1279:  Missing space around : in range-based for statement  [whitespace/colon] [4]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1282:  Missing space around : in range-based for statement  [whitespace/colon] [4]
Total errors found: 2 in 37 files


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

[Wenson Hsieh]

Created attachment 297062 [details]
Fix Mac builds

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1279:  Missing space around : in range-based for statement  [whitespace/colon] [4]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1282:  Missing space around : in range-based for statement  [whitespace/colon] [4]
Total errors found: 2 in 37 files


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

[Wenson Hsieh]

Created attachment 297064 [details]
Attempt to fix OpenSource iOS builds

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1279:  Missing space around : in range-based for statement  [whitespace/colon] [4]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1282:  Missing space around : in range-based for statement  [whitespace/colon] [4]
Total errors found: 2 in 38 files


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

[Wenson Hsieh]

Created attachment 297097 [details]
Fix OpenSource iOS builds

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1279:  Missing space around : in range-based for statement  [whitespace/colon] [4]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1282:  Missing space around : in range-based for statement  [whitespace/colon] [4]
Total errors found: 2 in 38 files


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

[Simon Fraser (smfr)]

Comment on attachment 297097 [details]
Fix OpenSource iOS builds

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:4782
> +- (void)_doAfterWebPageIsInStableState:(dispatch_block_t)updateBlock
> +{
> +    _doAfterWebPageIsInStableStateCallback = [updateBlock copy];
> +    _page->requestWebPageIsInStableState();
> +}

I don't like this single callback approach; if someone else calls _doAfterWebPageIsInStableState: it will just clobber the previous callback. I think you should build this on top of _doAfterNextPresentationUpdate:.

> Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:-230
> -    if (m_pageClient.isAssistingNode())
> -        return documentRect;
> -

This will conflict with what I landed.

> Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:1068
> +    m_process->send(Messages::WebPage::RequestIsInStableState(), m_pageID);

I don't think you should be using a new message for this. I think we should just include stable-ness in RemoteLayerTreeTransaction. WebPage stores m_isInStableState so you can just get it from there.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:157
> +    bool insideFixedPosition;
> +    if (auto* startNode = selection.start().containerNode()) {
> +        if (auto* renderer = startNode->renderer()) {
> +            renderer->localToContainerPoint({ }, nullptr, UseTransforms, &insideFixedPosition);
> +            if (insideFixedPosition)
> +                return true;
> +        }
> +    }
> +    if (auto* endNode = selection.end().containerNode()) {
> +        if (auto* renderer = endNode->renderer()) {
> +            renderer->localToContainerPoint({ }, nullptr, UseTransforms, &insideFixedPosition);
> +            if (insideFixedPosition)
> +                return true;
> +        }
> +    }
> +    return false;

We will have already called localToContainer... on a bunch of rects in the selection to compute them. We should be able to avoid calling localToContainerPoint twice again (it's somewhat expensive).

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:3113
> +            frameView.frame().selection().setCaretRectNeedsUpdate();

I'm a bit worried about marking it as needing update every time. Can we only do this if we know the caret is inside fixed position?

[Wenson Hsieh]

Comment on attachment 297097 [details]
Fix OpenSource iOS builds

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

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:4782
>> +}
> 
> I don't like this single callback approach; if someone else calls _doAfterWebPageIsInStableState: it will just clobber the previous callback. I think you should build this on top of _doAfterNextPresentationUpdate:.

Sounds good -- changed to hook into _doAfterNextPresentationUpdate

>> Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:-230
>> -
> 
> This will conflict with what I landed.

I discarded my changes here

>> Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:1068
>> +    m_process->send(Messages::WebPage::RequestIsInStableState(), m_pageID);
> 
> I don't think you should be using a new message for this. I think we should just include stable-ness in RemoteLayerTreeTransaction. WebPage stores m_isInStableState so you can just get it from there.

This makes sense. Changed to go through layer tree commits.

>> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:157
>> +    return false;
> 
> We will have already called localToContainer... on a bunch of rects in the selection to compute them. We should be able to avoid calling localToContainerPoint twice again (it's somewhat expensive).

Good catch! VisibleSelection::absoluteCaretBounds takes an outParam (insideFixed) with this information -- changed to use this information instead.

>> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:3113
>> +            frameView.frame().selection().setCaretRectNeedsUpdate();
> 
> I'm a bit worried about marking it as needing update every time. Can we only do this if we know the caret is inside fixed position?

Yes, this will work.

[Wenson Hsieh]

Created attachment 297294 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1280:  Missing space around : in range-based for statement  [whitespace/colon] [4]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1283:  Missing space around : in range-based for statement  [whitespace/colon] [4]
Total errors found: 2 in 33 files


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

[Simon Fraser (smfr)]

Comment on attachment 297294 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:1852
> +    if (oldRect != layoutViewportRect())

We need to be careful the this doesn't cause continual layouts while scrolling. Please check before landing.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:275
> +    BOOL _webPageIsInStableState;

I don't think you need this. I think you can use WePageProxy's m_lastVisibleContentRectUpdate.inStableState().

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1302
> +    _webPageIsInStableState = layerTreeTransaction.isInStableState();

Ah, if you're setting it based on the round-trip from the web process, then you should name it lastCommitHadStableState or something. Is it important that it's only set after a round-trip?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:4781
> +            [self _doAfterWebPageIsInStableState:updateBlockCopy];

This is a bit gross. Maybe you should store your own list of callbacks, and just use _doAfterNextPresentationUpdate: to process the list when the state is stable.

Also I don't think the "web page is stable" communicates clearly enough that this is about being stable after a round-trip from the web process. You could write it as doAfterNextStablePresentationUpdate.

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.h:182
> +    BOOL _isInStableState;

Ugh, this is a third instance of the same (or related state). Can we avoid this one?

> Tools/TestRunnerShared/UIScriptContext/Bindings/UIScriptController.idl:30
> +    void doAfterWebPageIsInStableState(object callback);

I would call this doAfterStablePresentationUpdate() here too.

If you never transition between unstable and stable does it fire? Does it trigger a presentation update if the state doesn't change?

> LayoutTests/fast/visual-viewport/ios/absolute-caret-after-scroll.html:19
> +            background-image: -webkit-gradient(linear, left top, left bottom, color-stop(0, #888888), color-stop(1, #FFFFFF));

linear-gradient()

> LayoutTests/fast/visual-viewport/ios/absolute-caret-after-scroll.html:33
> +                uiController.doAfterWebPageIsInStableState(function() {

Couldn't you just listen for the keyboard animation callback, or does that fire before state becomes stable?

> LayoutTests/fast/visual-viewport/ios/absolute-caret-after-scroll.html:47
> +            uiController.doAfterWebPageIsInStableState(function() {

I wouldn't expect immediateScrollToOffset to ever trigger an unstable state.

> LayoutTests/fast/visual-viewport/ios/absolute-selection-after-scroll.html:40
> +                uiController.doAfterWebPageIsInStableState(function() {

Does this fire later than the zoomingDidEnd callback?

[Build Bot]

Comment on attachment 297294 [details]
Patch

Attachment 297294 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2734236

New failing tests:
fast/visual-viewport/ios/fixed-caret-position-after-scroll.html
fast/visual-viewport/ios/absolute-caret-after-scroll.html

[Build Bot]

Created attachment 297299 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Wenson Hsieh]

Comment on attachment 297294 [details]
Patch

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

>> Source/WebCore/page/FrameView.cpp:1852
>> +    if (oldRect != layoutViewportRect())
> 
> We need to be careful the this doesn't cause continual layouts while scrolling. Please check before landing.

Yes, I verified this -- while scrolling, this only triggers a layout at the very end, when we are in stable state.

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:275
>> +    BOOL _webPageIsInStableState;
> 
> I don't think you need this. I think you can use WePageProxy's m_lastVisibleContentRectUpdate.inStableState().

I refactored this so I no longer need this variable.

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1302
>> +    _webPageIsInStableState = layerTreeTransaction.isInStableState();
> 
> Ah, if you're setting it based on the round-trip from the web process, then you should name it lastCommitHadStableState or something. Is it important that it's only set after a round-trip?

I removed this instance variable. I don't think it's necessary to wait for a round trip -- waiting until the next layer tree update where the WP has indicated that it is in stable state should be enough, because when the WP enters stable state, it will immediately send across the caret/selection rect as an EditorState update if it needs to do so (i.e. it is inside fixed position).

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:4781
>> +            [self _doAfterWebPageIsInStableState:updateBlockCopy];
> 
> This is a bit gross. Maybe you should store your own list of callbacks, and just use _doAfterNextPresentationUpdate: to process the list when the state is stable.
> 
> Also I don't think the "web page is stable" communicates clearly enough that this is about being stable after a round-trip from the web process. You could write it as doAfterNextStablePresentationUpdate.

Done. Refactored this to use an array of callbacks which are fired when receiving the next layer tree commit for which isInStableState() = true. This also means we don't need state in WKWebView keeping track of the last known stable state value.

>> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.h:182
>> +    BOOL _isInStableState;
> 
> Ugh, this is a third instance of the same (or related state). Can we avoid this one?

Yes, we can! Changed to just plumb the WebPageProxy's m_lastVisibleContentRectUpdate.inStableState() through.

>> Tools/TestRunnerShared/UIScriptContext/Bindings/UIScriptController.idl:30
>> +    void doAfterWebPageIsInStableState(object callback);
> 
> I would call this doAfterStablePresentationUpdate() here too.
> 
> If you never transition between unstable and stable does it fire? Does it trigger a presentation update if the state doesn't change?

Renamed. This fires as soon as the UI process gets a layer tree transaction indicating that it is in stable state, so a transition from non-stable state to stable state is not necessary. Additionally, if we haven't received a stable layer tree commit, keep bugging the web process until we do.

>> LayoutTests/fast/visual-viewport/ios/absolute-caret-after-scroll.html:19
>> +            background-image: -webkit-gradient(linear, left top, left bottom, color-stop(0, #888888), color-stop(1, #FFFFFF));
> 
> linear-gradient()

Changed to use linear-gradient.

>> LayoutTests/fast/visual-viewport/ios/absolute-caret-after-scroll.html:33
>> +                uiController.doAfterWebPageIsInStableState(function() {
> 
> Couldn't you just listen for the keyboard animation callback, or does that fire before state becomes stable?

That fires before state becomes stable.

>> LayoutTests/fast/visual-viewport/ios/absolute-caret-after-scroll.html:47
>> +            uiController.doAfterWebPageIsInStableState(function() {
> 
> I wouldn't expect immediateScrollToOffset to ever trigger an unstable state.

Right, it does not. I manually set uiController.stableStateOverride = false above, before performing the immediate scroll. I suppose what I should really do is move that down to right before I call uiController.immediateScrollToOffset, and then make sure I only fire the callback when the UI process and web process agree on stable state.

>> LayoutTests/fast/visual-viewport/ios/absolute-selection-after-scroll.html:40
>> +                uiController.doAfterWebPageIsInStableState(function() {
> 
> Does this fire later than the zoomingDidEnd callback?

We're long-pressing to select text here; I don't think we expect to zoom in.

But in the case of tapping on the input to show the keyboard, if we zoom to the input, we will trigger a visible content rect update and then -[WKWebView scrollViewDidEndZooming:withView:atScale:] will be called, which triggers the zoomingDidEnd in WKTR. A short while later, the WP will then come back to us with a layer tree commit indicating that the WP is in stable state, at which point we then fire the doAfterWebPageIsInStableState callback, so the doAfterWebPageIsInStableState callback will be fired after zooming is done.

[Wenson Hsieh]

Created attachment 297334 [details]
Address review feedback

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1280:  Missing space around : in range-based for statement  [whitespace/colon] [4]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1283:  Missing space around : in range-based for statement  [whitespace/colon] [4]
Total errors found: 2 in 35 files


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

[Simon Fraser (smfr)]

Comment on attachment 297334 [details]
Address review feedback

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1303
> +            action();
> +            Block_release(action);

You're double-retaining the block. You should just Block_release it after you added it to the NSArray.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:4786
> +    if (_stableStatePresentationUpdateCallbacks) {
> +        [_stableStatePresentationUpdateCallbacks addObject:Block_copy(updateBlock)];
> +        return;
> +    }
> +
> +    _stableStatePresentationUpdateCallbacks = adoptNS([[NSMutableArray alloc] initWithObjects:Block_copy(updateBlock), nil]);
> +    [self _firePresentationUpdateForPendingStableStatePresentationCallbacks];

Refactor to Block_release after adding to the array.

[Wenson Hsieh]

Comment on attachment 297334 [details]
Address review feedback

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

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1303
>> +            Block_release(action);
> 
> You're double-retaining the block. You should just Block_release it after you added it to the NSArray.

Good catch! Fixed by releasing after adding to _stableStatePresentationUpdateCallbacks

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:4786
>> +    [self _firePresentationUpdateForPendingStableStatePresentationCallbacks];
> 
> Refactor to Block_release after adding to the array.

Done. Thanks!

[Wenson Hsieh]

Created attachment 297337 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 297337 [details]
Patch for landing

Clearing flags on attachment: 297337

Committed r209931: <http://trac.webkit.org/changeset/209931>

[Build Bot]

Comment on attachment 297334 [details]
Address review feedback

Attachment 297334 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2738739

New failing tests:
http/tests/fetch/fetching-same-resource-with-diffferent-options.html

[Build Bot]

Created attachment 297343 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5