Bug 1498816 - part 1: Make nsFrameSelection::CommonPageMove() emulate click in current selection limiter r=smaug
authorMasayuki Nakano <masayuki@d-toybox.com>
Thu, 29 Nov 2018 06:16:15 +0000
changeset 507897 0a0c7f90e571e351952d2b292641187ee70815ca
parent 507896 3ea2a2c78bf4454226971e87036de12fafeadc4c
child 507898 de0f2f6fd5b3b2784355b64243ce846a328b5883
push id1905
push userffxbld-merge
push dateMon, 21 Jan 2019 12:33:13 +0000
treeherdermozilla-release@c2fca1944d8c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1498816
milestone65.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1498816 - part 1: Make nsFrameSelection::CommonPageMove() emulate click in current selection limiter r=smaug nsFrameSelection::CommonPageMove() is called only by nsTextInputSelectionImpl::PageMove() and PresShell::PageMove(). So, this is the only implementation of (Shift+) PageDown and (Shift+) PageUp. This scrolls down/up the specific frame. However, this allows to scroll outside of selection limiter, for example, even when an editing host is focused, its parent scrollable element may be scrolled. This is same behavior as Blink so that we should keep this behavior. However, it also emulates to click same position after scroll and this behavior is different from Blink. At this time, it does not check selection limiter and then, nsFrameSelection::HandleClick() may reset selection limiter the scrolled frame is a parent frame of the limiter. Therefore, this patch makes it check if the scrolled frame is a parent of the limiter, and if so, use result of GetFrameToPageSelect() to emulate a click instead. The result won't be a parent of the limiter because it is used when handling Shift + PageDown and Shift + PageUp which are always handled in the limiter. Differential Revision: https://phabricator.services.mozilla.com/D13202
layout/base/tests/test_scroll_per_page.html
layout/generic/nsFrameSelection.cpp
--- a/layout/base/tests/test_scroll_per_page.html
+++ b/layout/base/tests/test_scroll_per_page.html
@@ -99,43 +99,33 @@ async function doTests(aWindow) {
   editor.focus();
 
   let description = "PageDown in non-scrollable editing host: ";
   let previousScrollTop = container.scrollTop;
   await doPageDown();
   ok(container.scrollTop > previousScrollTop,
      `${description}the document should be scrolled down even if user presses PageDown in the editing host got: ${container.scrollTop}, previous position: ${previousScrollTop}`);
   let range = selection.getRangeAt(0);
-  todo_is(range.startContainer, editor.firstChild.nextSibling.nextSibling,
-          `${description}selection start shouldn't be moved to outside of the editing host (got: ${getNodeDescription(range.startContainer)})`);
+  is(range.startContainer, editor.firstChild.nextSibling.nextSibling,
+     `${description}selection start shouldn't be moved to outside of the editing host (got: ${getNodeDescription(range.startContainer)})`);
   ok(range.collapsed, description + "selection should be collapsed");
-  if (kUseKeyboardEvent) {
-    todo_is(doc.activeElement, editor,
-            description + "the editing host should keep having focus");
-  } else {
-    is(doc.activeElement, editor,
-       description + "the editing host should keep having focus");
-  }
+  is(doc.activeElement, editor,
+     description + "the editing host should keep having focus");
 
   description = "PageUp in non-scrollable editing host: ";
   previousScrollTop = container.scrollTop;
   await doPageUp();
   ok(container.scrollTop < previousScrollTop,
      `${description}the document should be scrolled up even if user presses PageDown in the editing host got: ${container.scrollTop}, previous position: ${previousScrollTop}`);
   range = selection.getRangeAt(0);
-  todo_is(range.startContainer, editor.firstChild,
-          `${description}selection start shouldn't be moved to outside of the editing host (got: ${getNodeDescription(range.startContainer)})`);
+  is(range.startContainer, editor.firstChild,
+     `${description}selection start shouldn't be moved to outside of the editing host (got: ${getNodeDescription(range.startContainer)})`);
   ok(range.collapsed, description + "selection should be collapsed");
-  if (kUseKeyboardEvent) {
-    todo_is(doc.activeElement, editor,
-            description + "the editing host should keep having focus");
-  } else {
-    is(doc.activeElement, editor,
-       description + "the editing host should keep having focus");
-  }
+  is(doc.activeElement, editor,
+     description + "the editing host should keep having focus");
 
   body.innerHTML = '<div id="largeDiv" style="height: 1500px;">' +
     	             "<p>previous line of the editor.</p>" +
                    '<div id="editor" contenteditable style="mergin-top 500px; height: 5em; overflow: auto;">' +
                    '<div id="innerDiv" style="height: 10em;">' +
                    "Here is first line<br>" +
                    "Here is second line" +
                    "</div>" +
--- a/layout/generic/nsFrameSelection.cpp
+++ b/layout/generic/nsFrameSelection.cpp
@@ -1796,41 +1796,51 @@ nsFrameSelection::CommonPageMove(bool aF
   }
 
   nsRect caretPos;
   nsIFrame* caretFrame = nsCaret::GetGeometry(domSel, &caretPos);
   if (!caretFrame) {
     return;
   }
 
-  if (scrollableFrame) {
+  // If the scrolled frame is outside of current selection limiter,
+  // we need to scroll the frame but keep moving selection in the limiter.
+  nsIFrame* frameToClick = scrolledFrame;
+  if (!IsValidSelectionPoint(this, scrolledFrame->GetContent())) {
+    frameToClick = GetFrameToPageSelect();
+    if (NS_WARN_IF(!frameToClick)) {
+      return;
+    }
+  }
+
+  if (scrollableFrame && scrolledFrame == frameToClick) {
     // If aFrame is scrollable, adjust pseudo-click position with page scroll
     // amount.
     if (aForward) {
       caretPos.y += scrollableFrame->GetPageScrollAmount().height;
     } else {
       caretPos.y -= scrollableFrame->GetPageScrollAmount().height;
     }
   } else {
     // Otherwise, adjust pseudo-click position with the frame size.
     if (aForward) {
-      caretPos.y += scrolledFrame->GetSize().height;
+      caretPos.y += frameToClick->GetSize().height;
     } else {
-      caretPos.y -= scrolledFrame->GetSize().height;
+      caretPos.y -= frameToClick->GetSize().height;
     }
   }
 
-  caretPos += caretFrame->GetOffsetTo(scrolledFrame);
+  caretPos += caretFrame->GetOffsetTo(frameToClick);
 
   // get a content at desired location
   nsPoint desiredPoint;
   desiredPoint.x = caretPos.x;
   desiredPoint.y = caretPos.y + caretPos.height / 2;
   nsIFrame::ContentOffsets offsets =
-      scrolledFrame->GetContentOffsetsFromPoint(desiredPoint);
+      frameToClick->GetContentOffsetsFromPoint(desiredPoint);
 
   if (!offsets.content) {
     return;
   }
 
   // Scroll one page if necessary.
   if (scrollableFrame) {
     scrollableFrame->ScrollBy(nsIntPoint(0, aForward ? 1 : -1),