Bug 176451

Created attachment 320013 [details] iframe for reproduce

Please test in the iOS 11 beta.

Yes, this reproduce in the iOS 11 beta. tested on iPhone

<rdar://problem/34311545>

This works correctly now, unless you've zoomed in, so I'll fix that.

Actually we still seem to over-scroll in landscape on iPad.

Ah, it's wrong if the document is not scrolled to the top when you initiate the scroll.

The issue here is that FrameView::convertToContainingView() doesn't do the mapping of contents to view when delegatesScrolling() is true, which it is for WKWebView.

I'm going to have to review all the sites that call delegatesScrolling() in UIWebView and WKWebView when fixing this.

Created attachment 333689 [details] Patch for testing

Created attachment 333691 [details] Testcase (starts at y=0)

Created attachment 333692 [details] Testcase (starts at y=200)

Comment on attachment 333689 [details] Patch for testing Attachment 333689 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/6483444 New failing tests: fast/scrolling/scrollIntoView-iframe.html

Created attachment 333694 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6

Created attachment 333703 [details] Patch for testing The following trace seems relevant: WebCore::ScrollView::contentsToContainingViewContents(WebCore::IntRect) const WebCore::RenderLayer::scrollRectToVisible(WebCore::SelectionRevealMode, WebCore::LayoutRect const&, bool, WebCore::ScrollAlignment const&, WebCore::ScrollAlignment const&) WebCore::RenderLayer::scrollRectToVisible(WebCore::SelectionRevealMode, WebCore::LayoutRect const&, bool, WebCore::ScrollAlignment const&, WebCore::ScrollAlignment const&) WebCore::RenderObject::scrollRectToVisible(WebCore::SelectionRevealMode, WebCore::LayoutRect const&, bool, WebCore::ScrollAlignment const&, WebCore::ScrollAlignment const&) WebCore::Element::scrollIntoView(bool) where contentsToContainingViewContents is called in RenderLayer::scrollRectToVisible here: if (frameView.safeToPropagateScrollToParent()) { ... // Convert the rect into the coordinate space of the parent frame's document. newRect = frameView.contentsToContainingViewContents(enclosingIntRect(newRect)); For the test case with initial scroll y=200 and target y=2000, the newRect becomes at y=2200, which is then wrongly used for the call to frameView.setScrollPosition. I'm attaching a hacky patch that fixes the issue for the test cases I attached, as well as the link provided in comment 0. The issue seems similar (using or not using viewToContents/contentsToView to take into account scroll offset) but different from the issue I had in bug 176451.

Created attachment 333901 [details] Patch for EWS This uses the function introduced in bug 182785. Apparently ScrollView::contentsToContainingViewContents(IntRect rect) is only used in the one place mentioned in comment 15.

Created attachment 337154 [details] Patch

Comment on attachment 337154 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337154&action=review > Source/WebCore/page/FrameView.h:665 > + bool coordinateChangeShouldIgnoreScrollPosition() const final; Why are you doing it this way, and not making use of a fixed windowToContents()?

Comment on attachment 337154 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337154&action=review >> Source/WebCore/page/FrameView.h:665 >> + bool coordinateChangeShouldIgnoreScrollPosition() const final; > > Why are you doing it this way, and not making use of a fixed windowToContents()? I need to check, but I think windowToContents() is not involved here? And I'm keeping this coordinateChangeShouldIgnoreScrollPosition (which probably needs a better name) because we still need support for WK1 IIUC.

Comment on attachment 337154 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337154&action=review >>> Source/WebCore/page/FrameView.h:665 >>> + bool coordinateChangeShouldIgnoreScrollPosition() const final; >> >> Why are you doing it this way, and not making use of a fixed windowToContents()? > > I need to check, but I think windowToContents() is not involved here? And I'm keeping this coordinateChangeShouldIgnoreScrollPosition (which probably needs a better name) because we still need support for WK1 IIUC. I just checked again and indeed windowToContents is not involved in the trace I mentioned in comment 15, so that won't help here. However I noticed that with two nested Frames we arrive at FrameView::convertFromRendererToContainingView FrameView::convertToContainingView(IntRect) WebCore::ScrollView::contentsToContainingViewContents(IntRect) where !delegateScrolling() is again used to skip a contentsToView call on the parent frame. So it's possible that we should also take that into account.

Created attachment 348915 [details] Patch Rebasing...

This is essentially fixed by bug 186956 via a likely more impactful change. However trying with WebKit 1, the test case still passes. *** This bug has been marked as a duplicate of bug 186956 ***