WebKit Bugzilla
Attachment 368647 Details for
Bug 197211
: [iOS] Element::focus and Element::scrollIntoView do not clamp scroll positions
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[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:
Fixed the test
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><!-- webkit-test-runner [ useFlexibleViewport=true ] --> >+<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><!-- webkit-test-runner [ useFlexibleViewport=true internal:AsyncFrameScrollingEnabled=true ] --> >+<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><!-- webkit-test-runner [ useFlexibleViewport=true ] --> >+<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 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Flags:
simon.fraser
:
review+
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 197211
:
368063
|
368068
|
368071
|
368080
|
368081
|
368088
|
368100
|
368108
|
368113
| 368647