Bug 176451 – scrollIntoView and scrolling to anchor inside iframe don't scroll content to proper position

• Component: Frames
• Status: RESOLVED DUPLICATE

[[e-mail]]

Created attachment 320012 [details]
page for reproduce

"scrollIntoView" and anchor don't scroll content to proper position on iPad.
Desktop version work properly.

Steps to Reproduce:
1. Open https://lamps.ecwid.com/~barry/test_ios/ or files on attachment
2. Scroll down a little (~100px)
2. Tap on "Scroll to anchor" or "Scroll to this element" 
3. Browser scroll to footer

Expected Results:
Browser will scroll to proper position.

Observed Results:
Browser will scroll to footer

Version:
Google Chrome 60.0.3112.89
Safari Mobile 10.3.3
tested on iPad.

[[e-mail]]

Created attachment 320013 [details]
iframe for reproduce

[Simon Fraser (smfr)]

Please test in the iOS 11 beta.

[[e-mail]]

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

[Radar WebKit Bug Importer]

<rdar://problem/34311545>

[Simon Fraser (smfr)]

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

[Simon Fraser (smfr)]

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

[Simon Fraser (smfr)]

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

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

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

[Frédéric Wang (:fredw)]

Created attachment 333689 [details]
Patch for testing

[Frédéric Wang (:fredw)]

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

[Frédéric Wang (:fredw)]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

Created attachment 337154 [details]
Patch

[Simon Fraser (smfr)]

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()?

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

Created attachment 348915 [details]
Patch

Rebasing...

[Frédéric Wang (:fredw)]

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