Bug 176451 - scrollIntoView and scrolling to anchor inside iframe don't scroll content to proper position
Summary: scrollIntoView and scrolling to anchor inside iframe don't scroll content to ...
Status: RESOLVED DUPLICATE of bug 186956
Alias: None
Product: WebKit
Classification: Unclassified
Component: Frames (show other bugs)
Version: Safari 10
Hardware: iPhone / iPad iOS 10.3
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-09-06 06:06 PDT by barry@ecwid.com
Modified: 2018-12-03 01:19 PST (History)
5 users (show)

See Also:


Attachments
page for reproduce (542 bytes, text/html)
2017-09-06 06:06 PDT, barry@ecwid.com
no flags Details
iframe for reproduce (635 bytes, text/html)
2017-09-06 06:06 PDT, barry@ecwid.com
no flags Details
Patch for testing (2.69 KB, patch)
2018-02-13 07:37 PST, Frédéric Wang (:fredw)
ews: commit-queue-
Details | Formatted Diff | Diff
Testcase (starts at y=0) (1.00 KB, text/html)
2018-02-13 08:46 PST, Frédéric Wang (:fredw)
no flags Details
Testcase (starts at y=200) (1.00 KB, text/html)
2018-02-13 08:46 PST, Frédéric Wang (:fredw)
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.20 MB, application/zip)
2018-02-13 09:05 PST, Build Bot
no flags Details
Patch for testing (3.71 KB, patch)
2018-02-13 10:40 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch for EWS (5.32 KB, patch)
2018-02-15 08:07 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (8.59 KB, patch)
2018-04-04 02:41 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (9.01 KB, patch)
2018-09-05 07:10 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description barry@ecwid.com 2017-09-06 06:06:15 PDT
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.
Comment 1 barry@ecwid.com 2017-09-06 06:06:56 PDT
Created attachment 320013 [details]
iframe for reproduce
Comment 2 Simon Fraser (smfr) 2017-09-06 14:45:46 PDT
Please test in the iOS 11 beta.
Comment 3 barry@ecwid.com 2017-09-07 00:05:15 PDT
Yes, this reproduce in the iOS 11 beta.
tested on iPhone
Comment 4 Radar WebKit Bug Importer 2017-09-07 11:10:44 PDT
<rdar://problem/34311545>
Comment 5 Simon Fraser (smfr) 2017-12-04 14:44:51 PST
This works correctly now, unless you've zoomed in, so I'll fix that.
Comment 6 Simon Fraser (smfr) 2017-12-04 14:49:59 PST
Actually we still seem to over-scroll in landscape on iPad.
Comment 7 Simon Fraser (smfr) 2017-12-04 15:18:59 PST
Ah, it's wrong if the document is not scrolled to the top when you initiate the scroll.
Comment 8 Simon Fraser (smfr) 2017-12-04 15:27:39 PST
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.
Comment 9 Simon Fraser (smfr) 2017-12-04 17:43:23 PST
I'm going to have to review all the sites that call delegatesScrolling() in UIWebView and WKWebView when fixing this.
Comment 10 Frédéric Wang (:fredw) 2018-02-13 07:37:48 PST
Created attachment 333689 [details]
Patch for testing
Comment 11 Frédéric Wang (:fredw) 2018-02-13 08:46:19 PST
Created attachment 333691 [details]
Testcase (starts at y=0)
Comment 12 Frédéric Wang (:fredw) 2018-02-13 08:46:39 PST
Created attachment 333692 [details]
Testcase (starts at y=200)
Comment 13 Build Bot 2018-02-13 09:05:36 PST
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
Comment 14 Build Bot 2018-02-13 09:05:37 PST
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
Comment 15 Frédéric Wang (:fredw) 2018-02-13 10:40:54 PST
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.
Comment 16 Frédéric Wang (:fredw) 2018-02-15 08:07:23 PST
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.
Comment 17 Frédéric Wang (:fredw) 2018-04-04 02:41:40 PDT
Created attachment 337154 [details]
Patch
Comment 18 Simon Fraser (smfr) 2018-04-09 07:00:06 PDT
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 19 Frédéric Wang (:fredw) 2018-04-09 08:06:01 PDT
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 20 Frédéric Wang (:fredw) 2018-04-11 03:50:55 PDT
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.
Comment 21 Frédéric Wang (:fredw) 2018-09-05 07:10:29 PDT
Created attachment 348915 [details]
Patch

Rebasing...
Comment 22 Frédéric Wang (:fredw) 2018-10-22 09:36:16 PDT
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 ***