Attachment 368647 Details for Bug 197211 – Fixed the test

[patch] Fixed the test

bug-197211-20190430203640.patch (text/plain), 13.58 KB, created by Ryosuke Niwa on 2019-04-30 20:36:41 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Ryosuke Niwa

Created: 2019-04-30 20:36:41 PDT

Size: 13.58 KB

patch

obsolete

>Subversion Revision: 244802
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index c1ca7a22d5a7c6189503ad8f29bb231dfbb5f54f..2245f14a2c0287efa6d8d9faa62d9fea1626e345 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,25 @@
>+2019-04-30  Ryosuke Niwa  <rniwa@webkit.org>
>+
>+        [iOS] Element::focus and Element::scrollIntoView do not clamp scroll positions
>+        https://bugs.webkit.org/show_bug.cgi?id=197211
>+
>+        Reviewed by Simon Fraser.
>+
>+        Fixed the bug that Element::focus and Element::scrollIntoView were not clamping scroll offsets,
>+        which causes scrollTop etc... to return a bogus negative scrolling offset.
>+
>+        Unfortunately, we can't just use FrameView's ScrollableArea::constrainScrollPosition since
>+        scrollRectToVisible relies on the visible rect being expanded by the content insets in order to scroll to
>+        a position within the content insets of UIScrollView; e.g. revealing the top of the page as the center.
>+        We manually expand minimumScrollPosition() and maximumScrollPosition() by the content insets instead.
>+
>+        Tests: fast/scrolling/ios/programmatic-scroll-via-focus-should-clamp-top.html
>+               fast/scrolling/ios/programmatic-scroll-via-scrollIntoView-inside-iframe-should-clamp-top.html
>+               fast/scrolling/ios/programmatic-scroll-via-scrollIntoView-should-clamp-top.html
>+
>+        * rendering/RenderLayer.cpp:
>+        (WebCore::RenderLayer::scrollRectToVisible):
>+
> 2019-04-30  Commit Queue  <commit-queue@webkit.org>
> 
>         Unreviewed, rolling out r244773.
>diff --git a/Source/WebCore/rendering/RenderLayer.cpp b/Source/WebCore/rendering/RenderLayer.cpp
>index 0485c29deba9a64255e3c6cfb65c2da7cebdce49..1d4f8761f2d87a19e824c77b247f9989da5c4cfc 100644
>--- a/Source/WebCore/rendering/RenderLayer.cpp
>+++ b/Source/WebCore/rendering/RenderLayer.cpp
>@@ -2586,18 +2586,28 @@ void RenderLayer::scrollRectToVisible(const LayoutRect& absoluteRect, bool insid
>             if (options.revealMode == SelectionRevealMode::RevealUpToMainFrame && frameView.frame().isMainFrame())
>                 return;
> 
>+            auto minScrollPosition = frameView.minimumScrollPosition();
>+            auto maxScrollPosition = frameView.maximumScrollPosition();
>+
> #if !PLATFORM(IOS_FAMILY)
>             LayoutRect viewRect = frameView.visibleContentRect();
> #else
>+            // FIXME: ContentInsets should be taken care of in UI process side.
>+            // To do that, getRectToExpose needs to return the additional scrolling to do beyond content rect.
>             LayoutRect viewRect = frameView.visualViewportRectExpandedByContentInsets();
>+
>+            auto contentInsets = page().contentInsets();
>+            minScrollPosition.move(-contentInsets.left(), -contentInsets.top());
>+            maxScrollPosition.move(contentInsets.right(), contentInsets.bottom());
> #endif
>             // Move the target rect into "scrollView contents" coordinates.
>             LayoutRect targetRect = absoluteRect;
>             targetRect.move(0, frameView.headerHeight());
> 
>             LayoutRect revealRect = getRectToExpose(viewRect, targetRect, insideFixed, options.alignX, options.alignY);
>-            
>-            frameView.setScrollPosition(roundedIntPoint(revealRect.location()));
>+            ScrollOffset clampedScrollPosition = roundedIntPoint(revealRect.location()).constrainedBetween(minScrollPosition, maxScrollPosition);
>+            frameView.setScrollPosition(clampedScrollPosition);
>+//            frameView.setScrollPosition(roundedIntPoint(revealRect.location()));
> 
>             // This is the outermost view of a web page, so after scrolling this view we
>             // scroll its container by calling Page::scrollRectIntoView.
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index c59c09195a739dc43c8dfe48c6271291d11c97d7..6dd640e4e075557b2a5fa4f8f7d94a3736ed941a 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,23 @@
>+2019-04-30  Ryosuke Niwa  <rniwa@webkit.org>
>+
>+        [iOS] Element::focus and Element::scrollIntoView do not clamp scroll positions
>+        https://bugs.webkit.org/show_bug.cgi?id=197211
>+
>+        Reviewed by Simon Fraser.
>+
>+        Added tests for scrolling via Element.prototype.focus and Element.prototype.scrollIntoView.
>+
>+        * fast/scrolling/ios/programmatic-scroll-via-focus-should-clamp-top-expected.txt: Added.
>+        * fast/scrolling/ios/programmatic-scroll-via-focus-should-clamp-top.html: Added.
>+        * fast/scrolling/ios/programmatic-scroll-via-scrollIntoView-inside-iframe-should-clamp-top-expected.txt: Added.
>+        * fast/scrolling/ios/programmatic-scroll-via-scrollIntoView-inside-iframe-should-clamp-top.html: Added.
>+        * fast/scrolling/ios/programmatic-scroll-via-scrollIntoView-should-clamp-top-expected.txt: Added.
>+        * fast/scrolling/ios/programmatic-scroll-via-scrollIntoView-should-clamp-top.html: Added.
>+        * platform/ios-wk2/fast/dom/focus-contenteditable-expected.txt: Rebaselined the output now that the scroll position
>+        is properly clamped.
>+        * platform/ios-wk2/imported/w3c/web-platform-tests/css/cssom-view/elementFromPoint-expected.txt: Rebaselined as one
>+        more test case is passing.
>+
> 2019-04-30  Commit Queue  <commit-queue@webkit.org>
> 
>         Unreviewed, rolling out r244774.
>diff --git a/LayoutTests/fast/scrolling/ios/programmatic-scroll-via-focus-should-clamp-top-expected.txt b/LayoutTests/fast/scrolling/ios/programmatic-scroll-via-focus-should-clamp-top-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..ea0cca9aed3a5b64ffa2675936c86c06d04f11a2
>--- /dev/null
>+++ b/LayoutTests/fast/scrolling/ios/programmatic-scroll-via-focus-should-clamp-top-expected.txt
>@@ -0,0 +1,13 @@
>+This tests programming scrolling via element.focus to an element position well above the viewport top
>+WebKit should clamp the scroll top to 0px.
>+
>+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
>+
>+
>+PASS document.documentElement.scrollTop is 0
>+PASS visualViewport.pageTop is 0
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+hi
>+
>diff --git a/LayoutTests/fast/scrolling/ios/programmatic-scroll-via-focus-should-clamp-top.html b/LayoutTests/fast/scrolling/ios/programmatic-scroll-via-focus-should-clamp-top.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..7afc15318bc9d6e3d0d646efc1dee99a8b4d12b0
>--- /dev/null
>+++ b/LayoutTests/fast/scrolling/ios/programmatic-scroll-via-focus-should-clamp-top.html
>@@ -0,0 +1,20 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<meta name="viewport" content="width=device-width, initial-scale=1">
>+</head>
>+<body>
>+<div style="position: absolute; top:-1000px;" id="hiddenEditor" contenteditable>hi<br></div>
>+<script src="../../../resources/js-test.js"></script>
>+<script>
>+
>+description(`This tests programming scrolling via element.focus to an element position well above the viewport top<br>
>+WebKit should clamp the scroll top to 0px.`);
>+
>+hiddenEditor.focus();
>+shouldBe('document.documentElement.scrollTop', '0');
>+shouldBe('visualViewport.pageTop', '0');
>+
>+</script>
>+</body>
>+</html>
>diff --git a/LayoutTests/fast/scrolling/ios/programmatic-scroll-via-scrollIntoView-inside-iframe-should-clamp-top-expected.txt b/LayoutTests/fast/scrolling/ios/programmatic-scroll-via-scrollIntoView-inside-iframe-should-clamp-top-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..1cf4206ca8b558b99ab318fe8a344402f24e20b9
>--- /dev/null
>+++ b/LayoutTests/fast/scrolling/ios/programmatic-scroll-via-scrollIntoView-inside-iframe-should-clamp-top-expected.txt
>@@ -0,0 +1,15 @@
>+This tests programming scrolling via element.scrollIntoView to an element inside an iframe position well above the viewport top
>+WebKit should clamp the scroll top to 0px.
>+
>+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
>+
>+
>+PASS document.documentElement.scrollTop is 0
>+PASS visualViewport.pageTop is 0
>+PASS iframe.contentDocument.documentElement.scrollTop is 0
>+PASS iframe.contentWindow.visualViewport.pageTop > 900 is true
>+PASS iframe.contentWindow.visualViewport.pageTop is target.offsetTop + target.offsetHeight - iframe.clientHeight
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+
>diff --git a/LayoutTests/fast/scrolling/ios/programmatic-scroll-via-scrollIntoView-inside-iframe-should-clamp-top.html b/LayoutTests/fast/scrolling/ios/programmatic-scroll-via-scrollIntoView-inside-iframe-should-clamp-top.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..97f66f8fb4094129fb599c9c83bf87aa41afb0f5
>--- /dev/null
>+++ b/LayoutTests/fast/scrolling/ios/programmatic-scroll-via-scrollIntoView-inside-iframe-should-clamp-top.html
>@@ -0,0 +1,33 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<meta name="viewport" content="width=device-width, initial-scale=1">
>+</head>
>+<body>
>+<script src="../../../resources/js-test.js"></script>
>+<script>
>+
>+if (window.internals)
>+    internals.settings.setAsyncFrameScrollingEnabled(true);
>+
>+description(`This tests programming scrolling via element.scrollIntoView to an element inside an iframe position well above the viewport top<br>
>+WebKit should clamp the scroll top to 0px.`);
>+
>+const iframe = document.createElement('iframe');
>+document.body.append(iframe);
>+iframe.style.position = 'absolute';
>+iframe.style.top = '-1000px';
>+iframe.style.height = '1050px';
>+iframe.contentDocument.body.innerHTML = '<div id="target" style="position: absolute; top:2000px; height: 20px;">some content</div>';
>+const target = iframe.contentWindow.target;
>+
>+target.scrollIntoView({block: 'end'});
>+shouldBe('document.documentElement.scrollTop', '0');
>+shouldBe('visualViewport.pageTop', '0');
>+shouldBe('iframe.contentDocument.documentElement.scrollTop', '0');
>+shouldBeTrue('iframe.contentWindow.visualViewport.pageTop > 900');
>+shouldBe('iframe.contentWindow.visualViewport.pageTop', 'target.offsetTop + target.offsetHeight - iframe.clientHeight');
>+
>+</script>
>+</body>
>+</html>
>diff --git a/LayoutTests/fast/scrolling/ios/programmatic-scroll-via-scrollIntoView-should-clamp-top-expected.txt b/LayoutTests/fast/scrolling/ios/programmatic-scroll-via-scrollIntoView-should-clamp-top-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..90f71b11bee0624137cd3c7854eecb196f9ba0ec
>--- /dev/null
>+++ b/LayoutTests/fast/scrolling/ios/programmatic-scroll-via-scrollIntoView-should-clamp-top-expected.txt
>@@ -0,0 +1,12 @@
>+This tests programming scrolling via element.scrollIntoView to an element position well above the viewport top
>+WebKit should clamp the scroll top to 0px.
>+
>+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
>+
>+
>+PASS document.documentElement.scrollTop is 0
>+PASS visualViewport.pageTop is 0
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+some content
>diff --git a/LayoutTests/fast/scrolling/ios/programmatic-scroll-via-scrollIntoView-should-clamp-top.html b/LayoutTests/fast/scrolling/ios/programmatic-scroll-via-scrollIntoView-should-clamp-top.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..1648f60eae0f99e571c148e25e13ef7bfc2e8999
>--- /dev/null
>+++ b/LayoutTests/fast/scrolling/ios/programmatic-scroll-via-scrollIntoView-should-clamp-top.html
>@@ -0,0 +1,20 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<meta name="viewport" content="width=device-width, initial-scale=1">
>+</head>
>+<body>
>+<div id="target" style="position: absolute; top:-1000px;">some content</div>
>+<script src="../../../resources/js-test.js"></script>
>+<script>
>+
>+description(`This tests programming scrolling via element.scrollIntoView to an element position well above the viewport top<br>
>+WebKit should clamp the scroll top to 0px.`);
>+
>+target.scrollIntoView();
>+shouldBe('document.documentElement.scrollTop', '0');
>+shouldBe('visualViewport.pageTop', '0');
>+
>+</script>
>+</body>
>+</html>
>diff --git a/LayoutTests/platform/ios-wk2/fast/dom/focus-contenteditable-expected.txt b/LayoutTests/platform/ios-wk2/fast/dom/focus-contenteditable-expected.txt
>index 0d36b4031637f0111fe86275741227ead57f8ad3..6a8699b4870778983c3f80e2b4ad060431a87d69 100644
>--- a/LayoutTests/platform/ios-wk2/fast/dom/focus-contenteditable-expected.txt
>+++ b/LayoutTests/platform/ios-wk2/fast/dom/focus-contenteditable-expected.txt
>@@ -18,4 +18,4 @@ layer at (0,0) size 800x902
>           text run at (0,0) width 78: "Test Passed."
>       RenderBlock {DIV} at (0,886) size 784x0
> caret: position 0 of child 0 {#text} of child 5 {DIV} of body
>-scrolled to 0,561
>+scrolled to 0,282
>diff --git a/LayoutTests/platform/ios-wk2/imported/w3c/web-platform-tests/css/cssom-view/elementFromPoint-expected.txt b/LayoutTests/platform/ios-wk2/imported/w3c/web-platform-tests/css/cssom-view/elementFromPoint-expected.txt
>index ad670104648c6703b7c008b6b4891ff5b694862f..b6e56682b9b84f8e7580d407bf6edc8e6154d7b1 100644
>--- a/LayoutTests/platform/ios-wk2/imported/w3c/web-platform-tests/css/cssom-view/elementFromPoint-expected.txt
>+++ b/LayoutTests/platform/ios-wk2/imported/w3c/web-platform-tests/css/cssom-view/elementFromPoint-expected.txt
>@@ -17,5 +17,5 @@ PASS no hit target at x,y
> PASS No viewport available 
> FAIL Image Maps assert_equals: Should have returned the area element expected Element node <area id="rectG" shape="rect" coords="0,0,90,100" href="#... but got Element node <html><head><title>cssom-view - elementFromPoint</title>
> ...
>-FAIL Fieldsets assert_equals: The fieldset should not cover up the div it doesn't even overlap expected Element node <div id="fieldset-div" class="size" style="position: abso... but got null
>+PASS Fieldsets 
>

Flags:

simon.fraser: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 197211: 368063 | 368068 | 368071 | 368080 | 368081 | 368088 | 368100 | 368108 | 368113 | 368647