Attachment 348915 Details for Bug 176451 – Patch

[patch] Patch

0001-Bug-176451-scrollIntoView-and-scrolling-to-anchor-in.patch (text/plain), 9.01 KB, created by Frédéric Wang (:fredw) on 2018-09-05 07:10:29 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Frédéric Wang (:fredw)

Created: 2018-09-05 07:10:29 PDT

Size: 9.01 KB

patch

obsolete

>From 6d61da3565fe398801039ea30de53f11497f8f23 Mon Sep 17 00:00:00 2001
>From: Frederic Wang <fwang@igalia.com>
>Date: Wed, 9 May 2018 14:55:20 +0200
>Subject: [PATCH xserver] Bug 176451 - scrollIntoView and scrolling to anchor
> inside iframe don't scroll content to proper position
>
>---
> LayoutTests/ChangeLog                         | 13 +++++++
> .../scrollIntoView-iframe-expected.html       | 22 +++++++++++
> .../fast/scrolling/scrollIntoView-iframe.html | 35 +++++++++++++++++
> Source/WebCore/ChangeLog                      | 38 +++++++++++++++++++
> Source/WebCore/page/FrameView.cpp             |  5 +++
> Source/WebCore/page/FrameView.h               |  2 +
> Source/WebCore/platform/ScrollView.cpp        |  2 +
> Source/WebCore/platform/ScrollView.h          |  2 +
> 8 files changed, 119 insertions(+)
> create mode 100644 LayoutTests/fast/scrolling/scrollIntoView-iframe-expected.html
> create mode 100644 LayoutTests/fast/scrolling/scrollIntoView-iframe.html
>
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 2c5c3a54eef..a2d09f964ea 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,16 @@
>+2018-04-04  Frederic Wang  <fwang@igalia.com>
>+
>+        scrollIntoView and scrolling to anchor inside iframe don't scroll content to proper position
>+        https://bugs.webkit.org/show_bug.cgi?id=176451
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add a test to verify that Element::scrollIntoView properly works in subframes. Note that it
>+        exercises the same code as scrolling to anchor.
>+
>+        * fast/scrolling/scrollIntoView-iframe-expected.html: Added.
>+        * fast/scrolling/scrollIntoView-iframe.html: Added.
>+
> 2018-09-04  Frederic Wang  <fwang@igalia.com>
> 
>         Add basic support for ScrollIntoViewOptions
>diff --git a/LayoutTests/fast/scrolling/scrollIntoView-iframe-expected.html b/LayoutTests/fast/scrolling/scrollIntoView-iframe-expected.html
>new file mode 100644
>index 00000000000..988d2f68444
>--- /dev/null
>+++ b/LayoutTests/fast/scrolling/scrollIntoView-iframe-expected.html
>@@ -0,0 +1,22 @@
>+<!DOCTYPE html>
>+<html>
>+  <head>
>+    <title>scrollIntoView iframe</title>
>+    <meta name="viewport" content="width=device-width, initial-scale=1">
>+    <style>
>+        body {
>+            margin: 0;
>+            overflow: hidden;
>+        }
>+        #greenSquare {
>+            background: green;
>+            height: 200px;
>+            width: 200px;
>+        }
>+    </style>
>+  </head>
>+  <body>
>+    <div style="position: fixed">This test passes if the page is scrolled such that the green square at the top-left corner.</div>
>+    <div id="greenSquare"></div>
>+  </body>
>+</html>
>diff --git a/LayoutTests/fast/scrolling/scrollIntoView-iframe.html b/LayoutTests/fast/scrolling/scrollIntoView-iframe.html
>new file mode 100644
>index 00000000000..9b21288cb6e
>--- /dev/null
>+++ b/LayoutTests/fast/scrolling/scrollIntoView-iframe.html
>@@ -0,0 +1,35 @@
>+<!DOCTYPE html>
>+<html>
>+  <head>
>+    <title>scrollIntoView iframe</title>
>+    <meta name="viewport" content="width=device-width, initial-scale=1">
>+    <script>
>+        if (window.testRunner)
>+            testRunner.waitUntilDone();
>+        function runTest() {
>+            window.scrollTo(0, 200);
>+            window.frames[0].document.body.scrollIntoView();
>+            if (window.testRunner)
>+                testRunner.notifyDone();
>+        }
>+     </script>
>+     <style>
>+        body {
>+            margin: 0;
>+            overflow: hidden;
>+        }
>+        iframe {
>+            background: green;
>+            border: 0;
>+            height: 200px;
>+            width: 200px;
>+        }
>+     </style>
>+  </head>
>+  <body>
>+    <div style="position: fixed">This test passes if the page is scrolled such that the green square at the top-left corner.</div>
>+    <div style="height: 2000px;"></div>
>+    <iframe srcdoc="<body style='margin: 0;'></body>" onload="runTest()"></iframe>
>+    <div style="height: 2000px;"></div>
>+  </body>
>+</html>
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 225cb67172d..c3e7f84fbec 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,41 @@
>+2018-04-04  Frederic Wang  <fwang@igalia.com>
>+
>+        scrollIntoView and scrolling to anchor inside iframe don't scroll content to proper position
>+        https://bugs.webkit.org/show_bug.cgi?id=176451
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        RenderLayer::scrollRectToVisible incorrectly takes into account the scroll position of
>+        subframes when converting into the coordinate space of the parent frame's document, which is
>+        causing wrong behavior with Element::scrollIntoView or when scrolling to anchors.
>+        This is a general issue of helper functions from ScrollView/FrameView, where we conditionally
>+        call viewToContents and contentsToView to take into account the scroll position for
>+        coordinate space conversion. The condition is just "delegatesScrolling()" which is to handle
>+        WKWebView. This patch refines the condition to take into account the difference between main
>+        frame VS subframes and WK1 VS WK2. delegatesScrolling() and the helper functions from
>+        ScrollView/FrameView are used at several places of the code, so one has to be careful with
>+        the migration. For now, the new function coordinateChangeShouldIgnoreScrollPosition() is only
>+        introduced in ScrollView::contentsToContainingViewContents(IntRect) which is itself only used
>+        in RenderLayer::scrollRectToVisible so it is safe to modify it. Performing that change allows
>+        to fix the issue with Element::scrollIntoView and scrolling to anchors for subframes. It is
>+        expected that coordinateChangeShouldIgnoreScrollPosition() will be used in follow-up bugs to
>+        fix similar issues.
>+
>+        Test: fast/scrolling/scrollIntoView-iframe.html
>+
>+        * platform/ScrollView.h:
>+        (WebCore::ScrollView::coordinateChangeShouldIgnoreScrollPosition const): New function that
>+        does the same as delegatesScrolling() for ScrollView.
>+        * page/FrameView.h: Override coordinateChangeShouldIgnoreScrollPosition to make it more
>+        restrictive.
>+        * page/FrameView.cpp:
>+        (WebCore::FrameView::coordinateChangeShouldIgnoreScrollPosition const): For subframes (i.e.
>+        !frame().isMainFrame()) and WebKit2 (i.e. !platformWidget()) the scroll position should be
>+        taken into account in ScrollView's helper functions.
>+        * platform/ScrollView.cpp:
>+        (WebCore::ScrollView::contentsToContainingViewContents const): If the scroll position should
>+        be ignored, then return the result of convertToContainingView without further mapping.
>+
> 2018-09-04  Frederic Wang  <fwang@igalia.com>
> 
>         Add basic support for ScrollIntoViewOptions
>diff --git a/Source/WebCore/page/FrameView.cpp b/Source/WebCore/page/FrameView.cpp
>index 18c54774443..cd661403ff2 100644
>--- a/Source/WebCore/page/FrameView.cpp
>+++ b/Source/WebCore/page/FrameView.cpp
>@@ -1390,6 +1390,11 @@ void FrameView::adjustMediaTypeForPrinting(bool printing)
>     }
> }
> 
>+bool FrameView::coordinateChangeShouldIgnoreScrollPosition() const
>+{
>+    return delegatesScrolling() && (platformWidget() || frame().isMainFrame());
>+}
>+
> bool FrameView::useSlowRepaints(bool considerOverlap) const
> {
>     bool mustBeSlow = hasSlowRepaintObjects() || (platformWidget() && hasViewportConstrainedObjects());
>diff --git a/Source/WebCore/page/FrameView.h b/Source/WebCore/page/FrameView.h
>index f50cde6ae90..220ec98a6a5 100644
>--- a/Source/WebCore/page/FrameView.h
>+++ b/Source/WebCore/page/FrameView.h
>@@ -672,6 +672,8 @@ private:
> 
>     bool isFrameView() const final { return true; }
> 
>+    bool coordinateChangeShouldIgnoreScrollPosition() const final;
>+
>     friend class RenderWidget;
>     bool useSlowRepaints(bool considerOverlap = true) const;
>     bool useSlowRepaintsIfNotOverlapped() const;
>diff --git a/Source/WebCore/platform/ScrollView.cpp b/Source/WebCore/platform/ScrollView.cpp
>index b8adee4e4f3..5b416e330dd 100644
>--- a/Source/WebCore/platform/ScrollView.cpp
>+++ b/Source/WebCore/platform/ScrollView.cpp
>@@ -852,6 +852,8 @@ IntRect ScrollView::contentsToContainingViewContents(IntRect rect) const
>     
>     if (const ScrollView* parentScrollView = parent()) {
>         IntRect rectInContainingView = convertToContainingView(contentsToView(rect));
>+        if (parentScrollView->coordinateChangeShouldIgnoreScrollPosition())
>+            return rectInContainingView;
>         return parentScrollView->viewToContents(rectInContainingView);
>     }
> 
>diff --git a/Source/WebCore/platform/ScrollView.h b/Source/WebCore/platform/ScrollView.h
>index 83149810ba2..2d6f9d47940 100644
>--- a/Source/WebCore/platform/ScrollView.h
>+++ b/Source/WebCore/platform/ScrollView.h
>@@ -377,6 +377,8 @@ public:
> protected:
>     ScrollView();
> 
>+    virtual bool coordinateChangeShouldIgnoreScrollPosition() const { return delegatesScrolling(); }
>+
>     virtual void repaintContentRectangle(const IntRect&);
>     virtual void paintContents(GraphicsContext&, const IntRect& damageRect, SecurityOriginPaintPolicy = SecurityOriginPaintPolicy::AnyOrigin) = 0;
> 
>-- 
>2.18.0
>

Actions: View | Formatted Diff | Diff

Attachments on bug 176451: 320012 | 320013 | 333689 | 333691 | 333692 | 333694 | 333703 | 333901 | 337154 | 348915