Scrolling with spacebar on a page with fixed header breaks reading flow
authorbenjamin@webkit.org <benjamin@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 28 Aug 2014 20:01:18 +0000 (20:01 +0000)
committerbenjamin@webkit.org <benjamin@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 28 Aug 2014 20:01:18 +0000 (20:01 +0000)
https://bugs.webkit.org/show_bug.cgi?id=135506

Patch by Benjamin Poulain <bpoulain@apple.com> on 2014-08-28
Reviewed by Simon Fraser.

Source/WebCore:

When scrolling by page, find the height of any bar that is obscuring the top or bottom of the page,
and substract that height from the step to scroll.

Tests: scrollbars/scrolling-backward-by-page-accounting-bottom-fixed-elements-on-keyboard-spacebar.html
       scrollbars/scrolling-backward-by-page-on-keyboard-spacebar.html
       scrollbars/scrolling-by-page-accounting-oversized-fixed-elements-on-keyboard-spacebar.html
       scrollbars/scrolling-by-page-accounting-top-fixed-elements-on-keyboard-spacebar.html
       scrollbars/scrolling-by-page-accounting-top-fixed-elements-with-negative-top-on-keyboard-spacebar.html
       scrollbars/scrolling-by-page-ignoring-hidden-fixed-elements-on-keyboard-spacebar.html
       scrollbars/scrolling-by-page-ignoring-transparent-fixed-elements-on-keyboard-spacebar.html
       scrollbars/scrolling-by-page-on-keyboard-spacebar.html

* WebCore.exp.in:
* page/FrameView.cpp:
(WebCore::FrameView::adjustScrollStepForFixedContent):
* page/FrameView.h:
* platform/ScrollableArea.cpp:
(WebCore::ScrollableArea::adjustScrollStepForFixedContent):
(WebCore::ScrollableArea::scroll):
* platform/ScrollableArea.h:

LayoutTests:

There was pretty much no test coverage for scrolling by page, add some tests.

* fast/events/scrollbar-double-click-expected.txt:
* platform/mac-wk1/TestExpectations:
* scrollbars/scrolling-backward-by-page-accounting-bottom-fixed-elements-on-keyboard-spacebar-expected.txt: Added.
* scrollbars/scrolling-backward-by-page-accounting-bottom-fixed-elements-on-keyboard-spacebar.html: Added.
* scrollbars/scrolling-backward-by-page-on-keyboard-spacebar-expected.txt: Added.
* scrollbars/scrolling-backward-by-page-on-keyboard-spacebar.html: Added.
* scrollbars/scrolling-by-page-accounting-oversized-fixed-elements-on-keyboard-spacebar-expected.txt: Added.
* scrollbars/scrolling-by-page-accounting-oversized-fixed-elements-on-keyboard-spacebar.html: Added.
* scrollbars/scrolling-by-page-accounting-top-fixed-elements-on-keyboard-spacebar-expected.txt: Added.
* scrollbars/scrolling-by-page-accounting-top-fixed-elements-on-keyboard-spacebar.html: Added.
* scrollbars/scrolling-by-page-accounting-top-fixed-elements-with-negative-top-on-keyboard-spacebar-expected.txt: Added.
* scrollbars/scrolling-by-page-accounting-top-fixed-elements-with-negative-top-on-keyboard-spacebar.html: Added.
* scrollbars/scrolling-by-page-ignoring-hidden-fixed-elements-on-keyboard-spacebar-expected.txt: Added.
* scrollbars/scrolling-by-page-ignoring-hidden-fixed-elements-on-keyboard-spacebar.html: Added.
* scrollbars/scrolling-by-page-ignoring-transparent-fixed-elements-on-keyboard-spacebar-expected.txt: Added.
* scrollbars/scrolling-by-page-ignoring-transparent-fixed-elements-on-keyboard-spacebar.html: Added.
* scrollbars/scrolling-by-page-on-keyboard-spacebar-expected.txt: Added.
* scrollbars/scrolling-by-page-on-keyboard-spacebar.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@173074 268f45cc-cd09-0410-ab3c-d52691b4dbfc

27 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/events/scrollbar-double-click-expected.txt
LayoutTests/platform/mac-wk1/TestExpectations
LayoutTests/scrollbars/scrolling-backward-by-page-accounting-bottom-fixed-elements-on-keyboard-spacebar-expected.txt [new file with mode: 0644]
LayoutTests/scrollbars/scrolling-backward-by-page-accounting-bottom-fixed-elements-on-keyboard-spacebar.html [new file with mode: 0644]
LayoutTests/scrollbars/scrolling-backward-by-page-on-keyboard-spacebar-expected.txt [new file with mode: 0644]
LayoutTests/scrollbars/scrolling-backward-by-page-on-keyboard-spacebar.html [new file with mode: 0644]
LayoutTests/scrollbars/scrolling-by-page-accounting-oversized-fixed-elements-on-keyboard-spacebar-expected.txt [new file with mode: 0644]
LayoutTests/scrollbars/scrolling-by-page-accounting-oversized-fixed-elements-on-keyboard-spacebar.html [new file with mode: 0644]
LayoutTests/scrollbars/scrolling-by-page-accounting-top-fixed-elements-on-keyboard-spacebar-expected.txt [new file with mode: 0644]
LayoutTests/scrollbars/scrolling-by-page-accounting-top-fixed-elements-on-keyboard-spacebar.html [new file with mode: 0644]
LayoutTests/scrollbars/scrolling-by-page-accounting-top-fixed-elements-with-negative-top-on-keyboard-spacebar-expected.txt [new file with mode: 0644]
LayoutTests/scrollbars/scrolling-by-page-accounting-top-fixed-elements-with-negative-top-on-keyboard-spacebar.html [new file with mode: 0644]
LayoutTests/scrollbars/scrolling-by-page-ignoring-hidden-fixed-elements-on-keyboard-spacebar-expected.txt [new file with mode: 0644]
LayoutTests/scrollbars/scrolling-by-page-ignoring-hidden-fixed-elements-on-keyboard-spacebar.html [new file with mode: 0644]
LayoutTests/scrollbars/scrolling-by-page-ignoring-transparent-fixed-elements-on-keyboard-spacebar-expected.txt [new file with mode: 0644]
LayoutTests/scrollbars/scrolling-by-page-ignoring-transparent-fixed-elements-on-keyboard-spacebar.html [new file with mode: 0644]
LayoutTests/scrollbars/scrolling-by-page-on-keyboard-spacebar-expected.txt [new file with mode: 0644]
LayoutTests/scrollbars/scrolling-by-page-on-keyboard-spacebar.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/WebCore.exp.in
Source/WebCore/page/FrameView.cpp
Source/WebCore/page/FrameView.h
Source/WebCore/platform/ScrollableArea.cpp
Source/WebCore/platform/ScrollableArea.h
Source/WebCore/platform/Scrollbar.h
Source/WebCore/platform/mock/ScrollbarThemeMock.h

index 9acc2b67b12129745c1153ad2fe801c60ee93ed2..a16893f37db75dedbbe81f0e9d484d63ee4f48d9 100644 (file)
@@ -1,3 +1,31 @@
+2014-08-28  Benjamin Poulain  <bpoulain@apple.com>
+
+        Scrolling with spacebar on a page with fixed header breaks reading flow
+        https://bugs.webkit.org/show_bug.cgi?id=135506
+
+        Reviewed by Simon Fraser.
+
+        There was pretty much no test coverage for scrolling by page, add some tests.
+
+        * fast/events/scrollbar-double-click-expected.txt:
+        * platform/mac-wk1/TestExpectations:
+        * scrollbars/scrolling-backward-by-page-accounting-bottom-fixed-elements-on-keyboard-spacebar-expected.txt: Added.
+        * scrollbars/scrolling-backward-by-page-accounting-bottom-fixed-elements-on-keyboard-spacebar.html: Added.
+        * scrollbars/scrolling-backward-by-page-on-keyboard-spacebar-expected.txt: Added.
+        * scrollbars/scrolling-backward-by-page-on-keyboard-spacebar.html: Added.
+        * scrollbars/scrolling-by-page-accounting-oversized-fixed-elements-on-keyboard-spacebar-expected.txt: Added.
+        * scrollbars/scrolling-by-page-accounting-oversized-fixed-elements-on-keyboard-spacebar.html: Added.
+        * scrollbars/scrolling-by-page-accounting-top-fixed-elements-on-keyboard-spacebar-expected.txt: Added.
+        * scrollbars/scrolling-by-page-accounting-top-fixed-elements-on-keyboard-spacebar.html: Added.
+        * scrollbars/scrolling-by-page-accounting-top-fixed-elements-with-negative-top-on-keyboard-spacebar-expected.txt: Added.
+        * scrollbars/scrolling-by-page-accounting-top-fixed-elements-with-negative-top-on-keyboard-spacebar.html: Added.
+        * scrollbars/scrolling-by-page-ignoring-hidden-fixed-elements-on-keyboard-spacebar-expected.txt: Added.
+        * scrollbars/scrolling-by-page-ignoring-hidden-fixed-elements-on-keyboard-spacebar.html: Added.
+        * scrollbars/scrolling-by-page-ignoring-transparent-fixed-elements-on-keyboard-spacebar-expected.txt: Added.
+        * scrollbars/scrolling-by-page-ignoring-transparent-fixed-elements-on-keyboard-spacebar.html: Added.
+        * scrollbars/scrolling-by-page-on-keyboard-spacebar-expected.txt: Added.
+        * scrollbars/scrolling-by-page-on-keyboard-spacebar.html: Added.
+
 2014-08-27  Filip Pizlo  <fpizlo@apple.com>
 
         FTL should be able to do polymorphic call inlining
index 51e0acadedacddf3841666da3a323bd59534903f..7a1ac5a6fae367bc3602fbaff9b6ac8b97dd8222 100644 (file)
@@ -1 +1 @@
-Scroll offset is 700
+Scroll offset is 720
index 44888b9f4d5bb363b6e76ed8c50dd4df11b1bea5..87811cd0f1a5ab337ffb2340aed5942ae2c3c5f8 100644 (file)
@@ -54,5 +54,15 @@ plugins/snapshotting
 [ MountainLion Mavericks ] platform/mac/fast/scrolling/scroll-select-bottom-test.html [ Skip ]
 [ MountainLion Mavericks ] platform/mac-wk1/fast/backgrounds/top-content-inset-fixed-attachment.html [ Skip ]
 
+# WK1 uses the native scrollview for scrolling by page.
+scrollbars/scrolling-backward-by-page-accounting-bottom-fixed-elements-on-keyboard-spacebar.html
+scrollbars/scrolling-backward-by-page-on-keyboard-spacebar.html
+scrollbars/scrolling-by-page-accounting-oversized-fixed-elements-on-keyboard-spacebar.html
+scrollbars/scrolling-by-page-accounting-top-fixed-elements-on-keyboard-spacebar.html
+scrollbars/scrolling-by-page-accounting-top-fixed-elements-with-negative-top-on-keyboard-spacebar.html
+scrollbars/scrolling-by-page-ignoring-hidden-fixed-elements-on-keyboard-spacebar.html
+scrollbars/scrolling-by-page-ignoring-transparent-fixed-elements-on-keyboard-spacebar.html
+scrollbars/scrolling-by-page-on-keyboard-spacebar.html
+
 ### END OF (2) Failures without bug reports
 ########################################
diff --git a/LayoutTests/scrollbars/scrolling-backward-by-page-accounting-bottom-fixed-elements-on-keyboard-spacebar-expected.txt b/LayoutTests/scrollbars/scrolling-backward-by-page-accounting-bottom-fixed-elements-on-keyboard-spacebar-expected.txt
new file mode 100644 (file)
index 0000000..ea76f9d
--- /dev/null
@@ -0,0 +1,10 @@
+Test that scrolling backward by page excludes the area taken by fixed element covering the full width.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS Scrolled to 520
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/scrollbars/scrolling-backward-by-page-accounting-bottom-fixed-elements-on-keyboard-spacebar.html b/LayoutTests/scrollbars/scrolling-backward-by-page-accounting-bottom-fixed-elements-on-keyboard-spacebar.html
new file mode 100644 (file)
index 0000000..aca7302
--- /dev/null
@@ -0,0 +1,76 @@
+<html>
+    <head>
+        <style>
+            body {
+                height: 10000px;
+            }
+            #bottom-bar {
+                position: fixed;
+                height: 120px;
+                width: 100%;
+                background-color: green;
+                bottom: 0;
+                left: 0;
+            }
+            /* This fixed element does not cover the entire width and should be ignored. */
+            #ignored-right {
+                position: fixed;
+                width: 20px;
+                height: 5000px;
+                background-color: red;
+                right: 0;
+                top: 0;
+            }
+            /* This fixed element does not cover an edge and should be ignored. */
+            #ignored-center {
+                position: fixed;
+                width: 100%;
+                height: 150px;
+                background-color: red;
+                left: 0;
+                top: 50%;
+            }
+        </style>
+        <script src="../resources/js-test-pre.js"></script>
+    </head>
+    <body>
+        <div id="bottom-bar"></div>
+        <div id="ignored-right"></div>
+        <div id="ignored-center"></div>
+        <script>
+            description("Test that scrolling backward by page excludes the area taken by fixed element covering the full width.");
+
+            jsTestIsAsync = true;
+
+            var failTimeoutId;
+            function test() {
+                if (window.eventSender) {
+                    // Force the first layout to avoid the suppressed scrollbar cases.
+                    scratch = document.documentElement.offsetWidth;
+                    scrollBy(0, 1000);
+
+                    // Avoid special cases for being "onload".
+                    setTimeout(function() {
+                        eventSender.keyDown(' ', ['shiftKey']);
+                    } , 0);
+
+                    failTimeoutId = setTimeout(function() {
+                        testFailed("The scrollview failed to scroll in response to the event.");
+                        debug("window.scrollY = " + window.scrollY + " excepted value around " + (1000 - (window.innerHeight - 120)));
+                        finishJSTest();
+                    }, 1000);
+                }
+            }
+
+            window.addEventListener("scroll", function() {
+                if (window.scrollY == 1000 - (window.innerHeight - 120)) {
+                    testPassed("Scrolled to " + window.scrollY);
+                    clearTimeout(failTimeoutId);
+                    finishJSTest();
+                }
+            })
+            window.addEventListener("load", test);
+        </script>
+        <script src="../resources/js-test-post.js"></script>
+    </body>
+</html>
diff --git a/LayoutTests/scrollbars/scrolling-backward-by-page-on-keyboard-spacebar-expected.txt b/LayoutTests/scrollbars/scrolling-backward-by-page-on-keyboard-spacebar-expected.txt
new file mode 100644 (file)
index 0000000..9bc6bea
--- /dev/null
@@ -0,0 +1,10 @@
+Test scrolling backward with page granularity by using the space bar.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS Scrolled to 440
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/scrollbars/scrolling-backward-by-page-on-keyboard-spacebar.html b/LayoutTests/scrollbars/scrolling-backward-by-page-on-keyboard-spacebar.html
new file mode 100644 (file)
index 0000000..ac43438
--- /dev/null
@@ -0,0 +1,47 @@
+<html>
+    <head>
+        <style>
+            body {
+                height: 10000px;
+            }
+        </style>
+        <script src="../resources/js-test-pre.js"></script>
+    </head>
+    <body>
+        <script>
+            description("Test scrolling backward with page granularity by using the space bar.");
+
+            jsTestIsAsync = true;
+
+            var failTimeoutId;
+            function test() {
+                if (window.eventSender) {
+                    // Force the first layout to avoid the suppressed scrollbar cases.
+                    scratch = document.documentElement.offsetWidth;
+                    scrollBy(0, 1000);
+
+                    // Avoid special cases for being "onload".
+                    setTimeout(function() {
+                        eventSender.keyDown(' ', ['shiftKey']);
+                    } , 0);
+
+                    failTimeoutId = setTimeout(function() {
+                        testFailed("The scrollview failed to scroll in response to the event.");
+                        debug("window.scrollY = " + window.scrollY + " excepted value around " + (1000 - (window.innerHeight - 40)));
+                        finishJSTest();
+                    }, 1000);
+                }
+            }
+
+            window.addEventListener("scroll", function() {
+                if (window.scrollY == 1000 - (window.innerHeight - 40)) {
+                    testPassed("Scrolled to " + window.scrollY);
+                    clearTimeout(failTimeoutId);
+                    finishJSTest();
+                }
+            })
+            window.addEventListener("load", test);
+        </script>
+        <script src="../resources/js-test-post.js"></script>
+    </body>
+</html>
\ No newline at end of file
diff --git a/LayoutTests/scrollbars/scrolling-by-page-accounting-oversized-fixed-elements-on-keyboard-spacebar-expected.txt b/LayoutTests/scrollbars/scrolling-by-page-accounting-oversized-fixed-elements-on-keyboard-spacebar-expected.txt
new file mode 100644 (file)
index 0000000..6300f25
--- /dev/null
@@ -0,0 +1,10 @@
+Test scrolling with page granularity by using the space bar excludes the height of fixed element covering the full page width. In this case, the cumulative size of both bars is larger than the threshold, and only 120px are removed from the 240px fixed height.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS Scrolled to 480
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/scrollbars/scrolling-by-page-accounting-oversized-fixed-elements-on-keyboard-spacebar.html b/LayoutTests/scrollbars/scrolling-by-page-accounting-oversized-fixed-elements-on-keyboard-spacebar.html
new file mode 100644 (file)
index 0000000..f06af6b
--- /dev/null
@@ -0,0 +1,84 @@
+<html>
+    <head>
+        <style>
+            body {
+                height: 10000px;
+            }
+            #top-bar {
+                position: fixed;
+                width: 100%;
+                height: 120px;
+                background-color: green;
+                top: 0;
+                left: 0;
+            }
+            #bottom-bar {
+                position: fixed;
+                width: 100%;
+                height: 120px;
+                background-color: green;
+                bottom: 0;
+                left: 0;
+            }
+            /* This fixed element does not cover the entire width and should be ignored. */
+            #ignored-left {
+                position: fixed;
+                width: 20px;
+                height: 5000px;
+                background-color: red;
+                left: 0;
+                top: 0;
+            }
+            /* This fixed element does not cover an edge and should be ignored. */
+            #ignored-center {
+                position: fixed;
+                width: 100%;
+                height: 150px;
+                background-color: red;
+                left: 0;
+                top: 50%;
+            }
+        </style>
+        <script src="../resources/js-test-pre.js"></script>
+    </head>
+    <body>
+        <div id="top-bar"></div>
+        <div id="bottom-bar"></div>
+        <div id="ignored-left"></div>
+        <div id="ignored-center"></div>
+        <script>
+            description("Test scrolling with page granularity by using the space bar excludes the height of fixed element covering the full page width. In this case, the cumulative size of both bars is larger than the threshold, and only 120px are removed from the 240px fixed height.");
+
+            jsTestIsAsync = true;
+
+            var failTimeoutId;
+            function test() {
+                if (window.eventSender) {
+                    // Force the first layout to avoid the suppressed scrollbar cases.
+                    scratch = document.documentElement.offsetWidth;
+
+                    // Avoid special cases for being "onload".
+                    setTimeout(function() {
+                        eventSender.keyDown(' ');
+                    } , 0);
+
+                    failTimeoutId = setTimeout(function() {
+                        testFailed("The scrollview failed to scroll in response to the event.");
+                        debug("window.scrollY = " + window.scrollY + " excepted value around " + (window.innerHeight - 120));
+                        finishJSTest();
+                    }, 1000);
+                }
+            }
+
+            window.addEventListener("scroll", function() {
+                if (window.scrollY == window.innerHeight - 120) {
+                    testPassed("Scrolled to " + window.scrollY);
+                    clearTimeout(failTimeoutId);
+                    finishJSTest();
+                }
+            })
+            window.addEventListener("load", test);
+        </script>
+        <script src="../resources/js-test-post.js"></script>
+    </body>
+</html>
\ No newline at end of file
diff --git a/LayoutTests/scrollbars/scrolling-by-page-accounting-top-fixed-elements-on-keyboard-spacebar-expected.txt b/LayoutTests/scrollbars/scrolling-by-page-accounting-top-fixed-elements-on-keyboard-spacebar-expected.txt
new file mode 100644 (file)
index 0000000..794ddd2
--- /dev/null
@@ -0,0 +1,10 @@
+Test scrolling with page granularity by using the space bar excludes the height of fixed element covering the full page width.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS Scrolled to 480
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/scrollbars/scrolling-by-page-accounting-top-fixed-elements-on-keyboard-spacebar.html b/LayoutTests/scrollbars/scrolling-by-page-accounting-top-fixed-elements-on-keyboard-spacebar.html
new file mode 100644 (file)
index 0000000..12b911a
--- /dev/null
@@ -0,0 +1,75 @@
+<html>
+    <head>
+        <style>
+            body {
+                height: 10000px;
+            }
+            #top-bar {
+                position: fixed;
+                width: 100%;
+                height: 120px;
+                background-color: green;
+                top: 0;
+                left: 0;
+            }
+            /* This fixed element does not cover the entire width and should be ignored. */
+            #ignored-left {
+                position: fixed;
+                width: 20px;
+                height: 5000px;
+                background-color: red;
+                left: 0;
+                top: 0;
+            }
+            /* This fixed element does not cover an edge and should be ignored. */
+            #ignored-center {
+                position: fixed;
+                width: 100%;
+                height: 150px;
+                background-color: red;
+                left: 0;
+                top: 50%;
+            }
+        </style>
+        <script src="../resources/js-test-pre.js"></script>
+    </head>
+    <body>
+        <div id="top-bar"></div>
+        <div id="ignored-left"></div>
+        <div id="ignored-center"></div>
+        <script>
+            description("Test scrolling with page granularity by using the space bar excludes the height of fixed element covering the full page width.");
+
+            jsTestIsAsync = true;
+
+            var failTimeoutId;
+            function test() {
+                if (window.eventSender) {
+                    // Force the first layout to avoid the suppressed scrollbar cases.
+                    scratch = document.documentElement.offsetWidth;
+
+                    // Avoid special cases for being "onload".
+                    setTimeout(function() {
+                        eventSender.keyDown(' ');
+                    } , 0);
+
+                    failTimeoutId = setTimeout(function() {
+                        testFailed("The scrollview failed to scroll in response to the event.");
+                        debug("window.scrollY = " + window.scrollY + " excepted value around " + (window.innerHeight - 120));
+                        finishJSTest();
+                    }, 1000);
+                }
+            }
+
+            window.addEventListener("scroll", function() {
+                if (window.scrollY == window.innerHeight - 120) {
+                    testPassed("Scrolled to " + window.scrollY);
+                    clearTimeout(failTimeoutId);
+                    finishJSTest();
+                }
+            })
+            window.addEventListener("load", test);
+        </script>
+        <script src="../resources/js-test-post.js"></script>
+    </body>
+</html>
\ No newline at end of file
diff --git a/LayoutTests/scrollbars/scrolling-by-page-accounting-top-fixed-elements-with-negative-top-on-keyboard-spacebar-expected.txt b/LayoutTests/scrollbars/scrolling-by-page-accounting-top-fixed-elements-with-negative-top-on-keyboard-spacebar-expected.txt
new file mode 100644 (file)
index 0000000..da2862c
--- /dev/null
@@ -0,0 +1,10 @@
+Test scrolling with page granularity by using the space bar excludes the height of fixed element covering the full page width. In this case, the top fixed element starts above the page, but the overlap should still be accounted for.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS Scrolled to 510
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/scrollbars/scrolling-by-page-accounting-top-fixed-elements-with-negative-top-on-keyboard-spacebar.html b/LayoutTests/scrollbars/scrolling-by-page-accounting-top-fixed-elements-with-negative-top-on-keyboard-spacebar.html
new file mode 100644 (file)
index 0000000..6aa1796
--- /dev/null
@@ -0,0 +1,76 @@
+<html>
+    <head>
+        <style>
+            body {
+                height: 10000px;
+            }
+            #top-bar {
+                position: fixed;
+                width: 100%;
+                height: 200px;
+                background-color: green;
+                top: -150px;
+                left: 0;
+            }
+            /* This fixed element does not cover the entire width and should be ignored. */
+            #ignored-left {
+                position: fixed;
+                width: 20px;
+                height: 5000px;
+                background-color: red;
+                left: 0;
+                top: 0;
+            }
+            /* This fixed element does not cover an edge and should be ignored. */
+            #ignored-center {
+                position: fixed;
+                width: 100%;
+                height: 150px;
+                background-color: red;
+                left: 0;
+                top: 50%;
+            }
+        </style>
+        <script src="../resources/js-test-pre.js"></script>
+    </head>
+    <body>
+        <div id="top-bar"></div>
+        <div id="ignored-left"></div>
+        <div id="ignored-center"></div>
+        <script>
+            description("Test scrolling with page granularity by using the space bar excludes the height of fixed element covering the full page width. In this case, the top fixed element starts above the page, but the overlap should still be accounted for.");
+
+            jsTestIsAsync = true;
+
+            var maxOverlapBetweenPages = 40;
+            var failTimeoutId;
+            function test() {
+                if (window.eventSender) {
+                    // Force the first layout to avoid the suppressed scrollbar cases.
+                    scratch = document.documentElement.offsetWidth;
+
+                    // Avoid special cases for being "onload".
+                    setTimeout(function() {
+                        eventSender.keyDown(' ');
+                    } , 0);
+
+                    failTimeoutId = setTimeout(function() {
+                        testFailed("The scrollview failed to scroll in response to the event.");
+                        debug("window.scrollY = " + window.scrollY + " excepted value around " + (window.innerHeight - 50 - maxOverlapBetweenPages));
+                        finishJSTest();
+                    }, 1000);
+                }
+            }
+
+            window.addEventListener("scroll", function() {
+                if (window.scrollY == window.innerHeight - 50 - maxOverlapBetweenPages) {
+                    testPassed("Scrolled to " + window.scrollY);
+                    clearTimeout(failTimeoutId);
+                    finishJSTest();
+                }
+            })
+            window.addEventListener("load", test);
+        </script>
+        <script src="../resources/js-test-post.js"></script>
+    </body>
+</html>
\ No newline at end of file
diff --git a/LayoutTests/scrollbars/scrolling-by-page-ignoring-hidden-fixed-elements-on-keyboard-spacebar-expected.txt b/LayoutTests/scrollbars/scrolling-by-page-ignoring-hidden-fixed-elements-on-keyboard-spacebar-expected.txt
new file mode 100644 (file)
index 0000000..325948e
--- /dev/null
@@ -0,0 +1,10 @@
+Test scrolling with page granularity by using the space bar. The fixed elements should be ignored because they are hidden.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS Scrolled to 560
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/scrollbars/scrolling-by-page-ignoring-hidden-fixed-elements-on-keyboard-spacebar.html b/LayoutTests/scrollbars/scrolling-by-page-ignoring-hidden-fixed-elements-on-keyboard-spacebar.html
new file mode 100644 (file)
index 0000000..17423d0
--- /dev/null
@@ -0,0 +1,66 @@
+<html>
+    <head>
+        <style>
+            body {
+                height: 10000px;
+            }
+            #top-bar {
+                position: fixed;
+                width: 100%;
+                height: 200px;
+                background-color: green;
+                top: 0;
+                left: 0;
+                visibility: hidden;
+            }
+            #bottom-bar {
+                position: fixed;
+                width: 100%;
+                height: 200px;
+                background-color: green;
+                bottom: 0;
+                left: 0;
+                visibility: hidden;
+            }
+        </style>
+        <script src="../resources/js-test-pre.js"></script>
+    </head>
+    <body>
+        <div id="top-bar"></div>
+        <div id="bottom-bar"></div>
+        <script>
+            description("Test scrolling with page granularity by using the space bar. The fixed elements should be ignored because they are hidden.");
+
+            jsTestIsAsync = true;
+
+            var failTimeoutId;
+            function test() {
+                if (window.eventSender) {
+                    // Force the first layout to avoid the suppressed scrollbar cases.
+                    scratch = document.documentElement.offsetWidth;
+
+                    // Avoid special cases for being "onload".
+                    setTimeout(function() {
+                        eventSender.keyDown(' ');
+                    } , 0);
+
+                    failTimeoutId = setTimeout(function() {
+                        testFailed("The scrollview failed to scroll in response to the event.");
+                        debug("window.scrollY = " + window.scrollY + " excepted value around " + (window.innerHeight - 40));
+                        finishJSTest();
+                    }, 1000);
+                }
+            }
+
+            window.addEventListener("scroll", function() {
+                if (window.scrollY == window.innerHeight - 40) {
+                    testPassed("Scrolled to " + window.scrollY);
+                    clearTimeout(failTimeoutId);
+                    finishJSTest();
+                }
+            })
+            window.addEventListener("load", test);
+        </script>
+        <script src="../resources/js-test-post.js"></script>
+    </body>
+</html>
\ No newline at end of file
diff --git a/LayoutTests/scrollbars/scrolling-by-page-ignoring-transparent-fixed-elements-on-keyboard-spacebar-expected.txt b/LayoutTests/scrollbars/scrolling-by-page-ignoring-transparent-fixed-elements-on-keyboard-spacebar-expected.txt
new file mode 100644 (file)
index 0000000..db41c28
--- /dev/null
@@ -0,0 +1,10 @@
+Test scrolling with page granularity by using the space bar. The fixed elements should be ignored because they are transparent.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS Scrolled to 560
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/scrollbars/scrolling-by-page-ignoring-transparent-fixed-elements-on-keyboard-spacebar.html b/LayoutTests/scrollbars/scrolling-by-page-ignoring-transparent-fixed-elements-on-keyboard-spacebar.html
new file mode 100644 (file)
index 0000000..8371639
--- /dev/null
@@ -0,0 +1,66 @@
+<html>
+    <head>
+        <style>
+            body {
+                height: 10000px;
+            }
+            #top-bar {
+                position: fixed;
+                width: 100%;
+                height: 200px;
+                background-color: green;
+                top: 0;
+                left: 0;
+                opacity: 0;
+            }
+            #bottom-bar {
+                position: fixed;
+                width: 100%;
+                height: 200px;
+                background-color: green;
+                bottom: 0;
+                left: 0;
+                opacity: 0;
+            }
+        </style>
+        <script src="../resources/js-test-pre.js"></script>
+    </head>
+    <body>
+        <div id="top-bar"></div>
+        <div id="bottom-bar"></div>
+        <script>
+            description("Test scrolling with page granularity by using the space bar. The fixed elements should be ignored because they are transparent.");
+
+            jsTestIsAsync = true;
+
+            var failTimeoutId;
+            function test() {
+                if (window.eventSender) {
+                    // Force the first layout to avoid the suppressed scrollbar cases.
+                    scratch = document.documentElement.offsetWidth;
+
+                    // Avoid special cases for being "onload".
+                    setTimeout(function() {
+                        eventSender.keyDown(' ');
+                    } , 0);
+
+                    failTimeoutId = setTimeout(function() {
+                        testFailed("The scrollview failed to scroll in response to the event.");
+                        debug("window.scrollY = " + window.scrollY + " excepted value around " + (window.innerHeight - 40));
+                        finishJSTest();
+                    }, 1000);
+                }
+            }
+
+            window.addEventListener("scroll", function() {
+                if (window.scrollY == window.innerHeight - 40) {
+                    testPassed("Scrolled to " + window.scrollY);
+                    clearTimeout(failTimeoutId);
+                    finishJSTest();
+                }
+            })
+            window.addEventListener("load", test);
+        </script>
+        <script src="../resources/js-test-post.js"></script>
+    </body>
+</html>
\ No newline at end of file
diff --git a/LayoutTests/scrollbars/scrolling-by-page-on-keyboard-spacebar-expected.txt b/LayoutTests/scrollbars/scrolling-by-page-on-keyboard-spacebar-expected.txt
new file mode 100644 (file)
index 0000000..0706b6f
--- /dev/null
@@ -0,0 +1,10 @@
+Test scrolling with page granularity by using the space bar.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS Scrolled to 560
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/scrollbars/scrolling-by-page-on-keyboard-spacebar.html b/LayoutTests/scrollbars/scrolling-by-page-on-keyboard-spacebar.html
new file mode 100644 (file)
index 0000000..940d947
--- /dev/null
@@ -0,0 +1,46 @@
+<html>
+    <head>
+        <style>
+            body {
+                height: 10000px;
+            }
+        </style>
+        <script src="../resources/js-test-pre.js"></script>
+    </head>
+    <body>
+        <script>
+            description("Test scrolling with page granularity by using the space bar.");
+
+            jsTestIsAsync = true;
+
+            var failTimeoutId;
+            function test() {
+                if (window.eventSender) {
+                    // Force the first layout to avoid the suppressed scrollbar cases.
+                    scratch = document.documentElement.offsetWidth;
+
+                    // Avoid special cases for being "onload".
+                    setTimeout(function() {
+                        eventSender.keyDown(' ');
+                    } , 0);
+
+                    failTimeoutId = setTimeout(function() {
+                        testFailed("The scrollview failed to scroll in response to the event.");
+                        debug("window.scrollY = " + window.scrollY + " excepted value around " + (window.innerHeight - 40));
+                        finishJSTest();
+                    }, 1000);
+                }
+            }
+
+            window.addEventListener("scroll", function() {
+                if (window.scrollY == window.innerHeight - 40) {
+                    testPassed("Scrolled to " + window.scrollY);
+                    clearTimeout(failTimeoutId);
+                    finishJSTest();
+                }
+            })
+            window.addEventListener("load", test);
+        </script>
+        <script src="../resources/js-test-post.js"></script>
+    </body>
+</html>
\ No newline at end of file
index 61abb4455df6b3fef6f02eb98ce647afe1143b2d..ef28b2d8bd3d76c5eee4ac949c1694d656b24f3a 100644 (file)
@@ -1,3 +1,31 @@
+2014-08-28  Benjamin Poulain  <bpoulain@apple.com>
+
+        Scrolling with spacebar on a page with fixed header breaks reading flow
+        https://bugs.webkit.org/show_bug.cgi?id=135506
+
+        Reviewed by Simon Fraser.
+
+        When scrolling by page, find the height of any bar that is obscuring the top or bottom of the page,
+        and substract that height from the step to scroll.
+
+        Tests: scrollbars/scrolling-backward-by-page-accounting-bottom-fixed-elements-on-keyboard-spacebar.html
+               scrollbars/scrolling-backward-by-page-on-keyboard-spacebar.html
+               scrollbars/scrolling-by-page-accounting-oversized-fixed-elements-on-keyboard-spacebar.html
+               scrollbars/scrolling-by-page-accounting-top-fixed-elements-on-keyboard-spacebar.html
+               scrollbars/scrolling-by-page-accounting-top-fixed-elements-with-negative-top-on-keyboard-spacebar.html
+               scrollbars/scrolling-by-page-ignoring-hidden-fixed-elements-on-keyboard-spacebar.html
+               scrollbars/scrolling-by-page-ignoring-transparent-fixed-elements-on-keyboard-spacebar.html
+               scrollbars/scrolling-by-page-on-keyboard-spacebar.html
+
+        * WebCore.exp.in:
+        * page/FrameView.cpp:
+        (WebCore::FrameView::adjustScrollStepForFixedContent):
+        * page/FrameView.h:
+        * platform/ScrollableArea.cpp:
+        (WebCore::ScrollableArea::adjustScrollStepForFixedContent):
+        (WebCore::ScrollableArea::scroll):
+        * platform/ScrollableArea.h:
+
 2014-08-28  Zalan Bujtas  <zalan@apple.com>
 
         Subpixel layout: Remove unused pixel snapping functions.
index 3cc64ebd070c95c46f62d8198f570ccc675b6ce1..42a465daf5c27272a9f73e2fcd2cf09ff797d170 100644 (file)
@@ -430,6 +430,7 @@ __ZN7WebCore14ScrollableArea24setScrollbarOverlayStyleENS_21ScrollbarOverlayStyl
 __ZN7WebCore14ScrollableArea27notifyScrollPositionChangedERKNS_8IntPointE
 __ZN7WebCore14ScrollableArea28setScrollOffsetFromInternalsERKNS_8IntPointE
 __ZN7WebCore14ScrollableArea30scrollToOffsetWithoutAnimationERKNS_10FloatPointE
+__ZN7WebCore14ScrollableArea31adjustScrollStepForFixedContentEfNS_20ScrollbarOrientationENS_17ScrollGranularityE
 __ZN7WebCore14ScrollableArea34constrainScrollPositionForOverhangERKNS_10LayoutRectERKNS_10LayoutSizeERKNS_11LayoutPointES9_ii
 __ZN7WebCore14ScrollableArea6scrollENS_15ScrollDirectionENS_17ScrollGranularityEf
 __ZN7WebCore14ScrollableAreaC2Ev
index 339233d8d2d2193c773ce2b935fe35fef7cd6400..4ae1348575513137219dce457b39cab203c9e759 100644 (file)
@@ -3219,6 +3219,45 @@ void FrameView::scrollTo(const IntSize& newOffset)
     frame().loader().client().didChangeScrollOffset();
 }
 
+float FrameView::adjustScrollStepForFixedContent(float step, ScrollbarOrientation orientation, ScrollGranularity granularity)
+{
+    if (granularity != ScrollByPage || orientation == HorizontalScrollbar)
+        return step;
+
+    TrackedRendererListHashSet* positionedObjects = nullptr;
+    if (RenderView* root = m_frame->contentRenderer()) {
+        if (!root->hasPositionedObjects())
+            return step;
+        positionedObjects = root->positionedObjects();
+    }
+
+    FloatRect unobscuredContentRect = this->unobscuredContentRect();
+    float topObscuredArea = 0;
+    float bottomObscuredArea = 0;
+    for (const auto& positionedObject : *positionedObjects) {
+        const RenderStyle& style = positionedObject->style();
+        if (style.position() != FixedPosition || style.visibility() == HIDDEN || !style.opacity())
+            continue;
+
+        FloatQuad contentQuad = positionedObject->absoluteContentQuad();
+        if (!contentQuad.isRectilinear())
+            continue;
+
+        FloatRect contentBoundingBox = contentQuad.boundingBox();
+        FloatRect fixedRectInView = intersection(unobscuredContentRect, contentBoundingBox);
+
+        if (fixedRectInView.width() < unobscuredContentRect.width())
+            continue;
+
+        if (fixedRectInView.y() == unobscuredContentRect.y())
+            topObscuredArea = std::max(topObscuredArea, fixedRectInView.height());
+        else if (fixedRectInView.maxY() == unobscuredContentRect.maxY())
+            bottomObscuredArea = std::max(bottomObscuredArea, fixedRectInView.height());
+    }
+
+    return Scrollbar::pageStep(unobscuredContentRect.height(), unobscuredContentRect.height() - topObscuredArea - bottomObscuredArea);
+}
+
 void FrameView::invalidateScrollbarRect(Scrollbar* scrollbar, const IntRect& rect)
 {
     // Add in our offset within the FrameView.
index ac9198d4f2e0770f00c5466966e99e6c905c647e..4488085c0628e16ed32c152da5ef86d3b6564785 100644 (file)
@@ -509,6 +509,8 @@ public:
     virtual void updateSnapOffsets() override;
 #endif
 
+    virtual float adjustScrollStepForFixedContent(float step, ScrollbarOrientation, ScrollGranularity) override;
+
 protected:
     virtual bool scrollContentsFastPath(const IntSize& scrollDelta, const IntRect& rectToScroll, const IntRect& clipRect) override;
     virtual void scrollContentsSlowPath(const IntRect& updateRect) override;
index de27df1c4f96a782a4f698fff289453c3dbb77f6..442106993ff899b8262b3deaf877425524cfff76 100644 (file)
@@ -87,6 +87,11 @@ void ScrollableArea::setScrollOrigin(const IntPoint& origin)
     }
 }
 
+float ScrollableArea::adjustScrollStepForFixedContent(float step, ScrollbarOrientation, ScrollGranularity)
+{
+    return step;
+}
+
 bool ScrollableArea::scroll(ScrollDirection direction, ScrollGranularity granularity, float multiplier)
 {
     ScrollbarOrientation orientation;
@@ -122,6 +127,7 @@ bool ScrollableArea::scroll(ScrollDirection direction, ScrollGranularity granula
     if (direction == ScrollUp || direction == ScrollLeft)
         multiplier = -multiplier;
 
+    step = adjustScrollStepForFixedContent(step, orientation, granularity);
     return scrollAnimator()->scroll(orientation, granularity, step, multiplier);
 }
 
index 907b01264176c17d2891a9e69252342d9f98657c..938d622d53cc2003f837527a23b725a9444f829b 100644 (file)
@@ -261,6 +261,7 @@ protected:
     void setScrollOrigin(const IntPoint&);
     void resetScrollOriginChanged() { m_scrollOriginChanged = false; }
 
+    virtual float adjustScrollStepForFixedContent(float step, ScrollbarOrientation, ScrollGranularity);
     virtual void invalidateScrollbarRect(Scrollbar*, const IntRect&) = 0;
     virtual void invalidateScrollCornerRect(const IntRect&) = 0;
 
index 3b7673644b501987e2097600ab7622a61996a52e..44e23f7b661770224c5b8f2dc4d24d7061fee1b7 100644 (file)
@@ -97,9 +97,10 @@ public:
     void offsetDidChange();
 
     static int pixelsPerLineStep() { return 40; }
-    static float minFractionToStepWhenPaging() { return 0.875f; }
+    static float minFractionToStepWhenPaging() { return 0.8; }
     WEBCORE_EXPORT static int maxOverlapBetweenPages();
-    static int pageStep(int widthOrHeight) { return std::max(std::max<int>(lroundf(widthOrHeight * Scrollbar::minFractionToStepWhenPaging()), lroundf(widthOrHeight - Scrollbar::maxOverlapBetweenPages())), 1); }
+    static int pageStep(int viewWidthOrHeight, int contentWidthOrHeight) { return std::max(std::max<int>(lroundf(viewWidthOrHeight * Scrollbar::minFractionToStepWhenPaging()), lroundf(contentWidthOrHeight - Scrollbar::maxOverlapBetweenPages())), 1); }
+    static int pageStep(int viewWidthOrHeight) { return pageStep(viewWidthOrHeight, viewWidthOrHeight); }
     static float pageStepDelta(int widthOrHeight) { return std::max(std::max(static_cast<float>(widthOrHeight) * Scrollbar::minFractionToStepWhenPaging(), static_cast<float>(widthOrHeight) - Scrollbar::maxOverlapBetweenPages()), 1.0f); }
 
     void disconnectFromScrollableArea() { m_scrollableArea = 0; }
index 6333841ab796b9dc233f3f63eb800abfa7b0bb07..a500444d9486e84e812a91499b17807a9ed19020 100644 (file)
@@ -45,6 +45,7 @@ protected:
     
     virtual void paintTrackBackground(GraphicsContext*, ScrollbarThemeClient*, const IntRect&);
     virtual void paintThumb(GraphicsContext*, ScrollbarThemeClient*, const IntRect&);
+    virtual int maxOverlapBetweenPages() { return 40; }
     
 private:
     virtual bool isMockTheme() const { return true; }