Bug 1577058 - part 2: Make nsFrameSelection::CommonPageMove() handle nsFrameSelection::ScrollSelectionIntoView() too r=smaug

Currently, `nsFrameSelection::CommonPageMove()` is called before every caller
calls `nsFrameSelection::ScrollSelectionIntoView()`.  However, when an editing
host has focus, the scroll target may be outside of it.  In such case, without
moving caret, user may want only to scroll the scrollable element.

Chrome behaves like so.  Chrome also can scroll outside scrollable element
of focused editing host.  However, it scrolls caret into view only when
caret is moved actually.  Therefore, it makes sense to follow this behavior.

This patch makes `nsFrameSelection::CommonPageMove()` also call
`nsFrameSelection::ScrollSelectionIntoView()`.  However, it newly takes
`SelectionIntoView` flag for making callers can choose the condition.  I.e.,
`ScrollSelectionIntoView()` should be called always, or only when selection
is actually changed, or shouldn't be called.

Differential Revision: https://phabricator.services.mozilla.com/D50178

--HG--
extra : moz-landing-system : lando

dom/html/nsTextEditorState.cpp

@@ -634,10 +634,18 @@ nsTextInputSelectionImpl::PageMove(bool aForward, bool aExtend) {
   if (mScrollFrame) {
     RefPtr<nsFrameSelection> frameSelection = mFrameSelection;
     nsIFrame* scrollFrame = do_QueryFrame(mScrollFrame);
-    frameSelection->CommonPageMove(aForward, aExtend, scrollFrame);
+    // We won't scroll parent scrollable element of mScrollFrame.  Therefore,
+    // this may be handled when mScrollFrame is completely outside of the view.
+    // In such case, user may be confused since they might have wanted to
+    // scroll a parent scrollable element.  For making clearer which element
+    // handles PageDown/PageUp, we should move selection into view even if
+    // selection is not changed.
+    return frameSelection->PageMove(aForward, aExtend, scrollFrame,
+                                    nsFrameSelection::SelectionIntoView::Yes);
   }
-  // After ScrollSelectionIntoView(), the pending notifications might be
-  // flushed and PresShell/PresContext/Frames may be dead. See bug 418470.
+  // Similarly, if there is no scrollable frame, we should move the editor
+  // frame into the view for making it clearer which element handles
+  // PageDown/PageUp.
   return ScrollSelectionIntoView(
       nsISelectionController::SELECTION_NORMAL,
       nsISelectionController::SELECTION_FOCUS_REGION,

layout/base/PresShell.cpp

@@ -2264,15 +2264,12 @@ PresShell::PageMove(bool aForward, bool aExtend) {
       return NS_OK;
     }
   }
+  // We may scroll parent scrollable element of current selection limiter.
+  // In such case, we don't want to scroll selection into view unless
+  // selection is changed.
   RefPtr<nsFrameSelection> frameSelection = mSelection;
-  frameSelection->CommonPageMove(aForward, aExtend, frame);
-  // After ScrollSelectionIntoView(), the pending notifications might be
-  // flushed and PresShell/PresContext/Frames may be dead. See bug 418470.
-  return ScrollSelectionIntoView(
-      nsISelectionController::SELECTION_NORMAL,
-      nsISelectionController::SELECTION_FOCUS_REGION,
-      nsISelectionController::SCROLL_SYNCHRONOUS |
-          nsISelectionController::SCROLL_FOR_CARET_MOVE);
+  return frameSelection->PageMove(
+      aForward, aExtend, frame, nsFrameSelection::SelectionIntoView::IfChanged);
 }
 
 NS_IMETHODIMP

layout/base/tests/test_scroll_per_page.html

@@ -183,6 +183,53 @@ async function doTests(aWindow) {
   ok(container.scrollTop >= previousScrollTop - container.clientHeight,
      `${description} The document should not be scrolled up too much (got: ${container.scrollTop}, previous position: ${previousScrollTop}, scroll height: ${container.clientHeight})`);
 
+  // Shouldn't scroll to caret position after pagedown scrolls editing host.
+  body.innerHTML = '<div id="editor" contenteditable style="height: 300px; overflow: auto;"><div style="height: 1500px;">abc<br>def<br></div></div>';
+  editor = doc.getElementById("editor");
+  container = editor;
+  editor.focus();
+
+  description = "PageDown in scrollable editing host";
+  previousScrollTop = container.scrollTop;
+  await doPageDown();
+  ok(container.scrollTop > previousScrollTop,
+     `${description} #1: Should be scrolled down (got: ${container.scrollTop}, previous position: ${previousScrollTop})`);
+  previousScrollTop = container.scrollTop;
+  await doPageDown();
+  ok(container.scrollTop > previousScrollTop,
+     `${description} #2: should be scrolled down (got:${container.scrollTop}, previous position: ${previousScrollTop})`);
+  previousScrollTop = container.scrollTop;
+  await doPageDown();
+  ok(container.scrollTop > previousScrollTop,
+     `${description} #3: should be scrolled down (got:${container.scrollTop}, previous position: ${previousScrollTop})`);
+  await doPageUp();
+  ok(container.scrollTop < 300,
+     `PageUp in scrollable editing host after scrolled down 3 pages: should be scrolled up to show caret (got:${container.scrollTop}`);
+
+  // Shouldn't scroll to caret position after pagedown scrolls outside of editing host.
+  body.innerHTML = '<div id="editor" contenteditable style="height: 1500px">abc<br>def<br></div>';
+  editor = doc.getElementById("editor");
+  container = doc.documentElement;
+  editor.focus();
+  selection.collapse(editor.firstChild);
+
+  description = "PageDown in too high non-scrollable editing host";
+  previousScrollTop = container.scrollTop;
+  await doPageDown();
+  ok(container.scrollTop > previousScrollTop,
+     `${description} #1: Should be scrolled down (got: ${container.scrollTop}, previous position: ${previousScrollTop})`);
+  previousScrollTop = container.scrollTop;
+  await doPageDown();
+  ok(container.scrollTop > previousScrollTop,
+     `${description} #2: should be scrolled down (got:${container.scrollTop}, previous position: ${previousScrollTop})`);
+  previousScrollTop = container.scrollTop;
+  await doPageDown();
+  ok(container.scrollTop > previousScrollTop,
+     `${description} #3: should be scrolled down (got:${container.scrollTop}, previous position: ${previousScrollTop})`);
+  await doPageUp();
+  ok(container.scrollTop < 300,
+     `PageUp in too high non-scrollable editing host after scrolled down 3 pages: should be scrolled up to show caret (got:${container.scrollTop}`);
+
   aWindow.close();
   SimpleTest.finish();
 }

layout/generic/nsFrameSelection.cpp

@@ -1636,8 +1636,9 @@ nsIFrame* nsFrameSelection::GetFrameToPageSelect() const {
   return rootFrameToSelect;
 }
 
-void nsFrameSelection::CommonPageMove(bool aForward, bool aExtend,
-                                      nsIFrame* aFrame) {
+nsresult nsFrameSelection::PageMove(bool aForward, bool aExtend,
+                                    nsIFrame* aFrame,
+                                    SelectionIntoView aSelectionIntoView) {
   MOZ_ASSERT(aFrame);
 
   // expected behavior for PageMove is to scroll AND move the caret
@@ -1650,21 +1651,21 @@ void nsFrameSelection::CommonPageMove(bool aForward, bool aExtend,
   nsIFrame* scrolledFrame =
       scrollableFrame ? scrollableFrame->GetScrolledFrame() : aFrame;
   if (!scrolledFrame) {
-    return;
+    return NS_OK;
   }
 
   // find out where the caret is.
   // we should know mDesiredPos value of nsFrameSelection, but I havent seen
   // that behavior in other windows applications yet.
-  Selection* domSel = GetSelection(SelectionType::eNormal);
-  if (!domSel) {
-    return;
+  RefPtr<Selection> selection = GetSelection(SelectionType::eNormal);
+  if (!selection) {
+    return NS_OK;
   }
 
   nsRect caretPos;
-  nsIFrame* caretFrame = nsCaret::GetGeometry(domSel, &caretPos);
+  nsIFrame* caretFrame = nsCaret::GetGeometry(selection, &caretPos);
   if (!caretFrame) {
-    return;
+    return NS_OK;
   }
 
   // If the scrolled frame is outside of current selection limiter,
@@ -1673,13 +1674,18 @@ void nsFrameSelection::CommonPageMove(bool aForward, bool aExtend,
   if (!IsValidSelectionPoint(this, scrolledFrame->GetContent())) {
     frameToClick = GetFrameToPageSelect();
     if (NS_WARN_IF(!frameToClick)) {
-      return;
+      return NS_OK;
     }
   }
 
   if (scrollableFrame) {
     // If there is a scrollable frame, adjust pseudo-click position with page
     // scroll amount.
+    // XXX This may scroll more than one page if ScrollSelectionIntoView is
+    //     called later because caret may not fully visible.  E.g., if
+    //     clicking line will be visible only half height with scrolling
+    //     the frame, ScrollSelectionIntoView additionally scrolls to show
+    //     the caret entirely.
     if (aForward) {
       caretPos.y += scrollableFrame->GetPageScrollAmount().height;
     } else {
@@ -1704,18 +1710,55 @@ void nsFrameSelection::CommonPageMove(bool aForward, bool aExtend,
       frameToClick->GetContentOffsetsFromPoint(desiredPoint);
 
   if (!offsets.content) {
-    return;
+    // XXX Do we need to handle ScrollSelectionIntoView in this case?
+    return NS_OK;
   }
 
-  // Scroll one page if necessary.
+  // First, place the caret.
+  bool selectionChanged;
+  {
+    // We don't want any script to run until we check whether selection is
+    // modified by HandleClick.
+    SelectionBatcher ensureNoSelectionChangeNotifications(selection);
+
+    RangeBoundary oldAnchor = selection->AnchorRef();
+    RangeBoundary oldFocus = selection->FocusRef();
+
+    HandleClick(offsets.content, offsets.offset, offsets.offset, aExtend, false,
+                CARET_ASSOCIATE_AFTER);
+
+    selectionChanged = selection->AnchorRef() != oldAnchor ||
+                       selection->FocusRef() != oldFocus;
+  }
+
+  bool doScrollSelectionIntoView = !(
+      aSelectionIntoView == SelectionIntoView::IfChanged && !selectionChanged);
+
+  // Then, scroll the given frame one page.
   if (scrollableFrame) {
+    // If we'll call ScrollSelectionIntoView later and selection wasn't
+    // changed and we scroll outside of selection limiter, we shouldn't use
+    // smooth scroll here because nsIScrollableFrame uses normal runnable,
+    // but ScrollSelectionIntoView uses early runner and it cancels the
+    // pending smooth scroll.  Therefore, if we used smooth scroll in such
+    // case, ScrollSelectionIntoView would scroll to show caret instead of
+    // page scroll of an element outside selection limiter.
+    ScrollMode scrollMode = doScrollSelectionIntoView && !selectionChanged &&
+                                    scrolledFrame != frameToClick
+                                ? ScrollMode::Instant
+                                : ScrollMode::Smooth;
     scrollableFrame->ScrollBy(nsIntPoint(0, aForward ? 1 : -1),
-                              nsIScrollableFrame::PAGES, ScrollMode::Smooth);
+                              nsIScrollableFrame::PAGES, scrollMode);
   }
 
-  // place the caret
-  HandleClick(offsets.content, offsets.offset, offsets.offset, aExtend, false,
-              CARET_ASSOCIATE_AFTER);
+  // Finally, scroll selection into view if requested.
+  if (!doScrollSelectionIntoView) {
+    return NS_OK;
+  }
+  return ScrollSelectionIntoView(
+      SelectionType::eNormal, nsISelectionController::SELECTION_FOCUS_REGION,
+      nsISelectionController::SCROLL_SYNCHRONOUS |
+          nsISelectionController::SCROLL_FOR_CARET_MOVE);
 }
 
 nsresult nsFrameSelection::PhysicalMove(int16_t aDirection, int16_t aAmount,

layout/generic/nsFrameSelection.h

@@ -472,21 +472,25 @@ class nsFrameSelection final {
   nsIFrame* GetFrameToPageSelect() const;
 
   /**
-   * Scrolling then moving caret placement code in common to text areas and
-   * content areas should be located in the implementer
-   * This method will accept the following parameters and perform the scroll
-   * and caret movement.  It remains for the caller to call the final
-   * ScrollCaretIntoView if that called wants to be sure the caret is always
-   * visible.
+   * This method moves caret (if aExtend is false) or expands selection (if
+   * aExtend is true).  Then, scrolls aFrame one page.  Finally, this may
+   * call ScrollSelectionIntoView() for making focus of selection visible
+   * but depending on aSelectionIntoView value.
    *
    * @param aForward if true, scroll forward if not scroll backward
    * @param aExtend  if true, extend selection to the new point
    * @param aFrame   the frame to scroll or container of per-page selection.
    *                 if aExtend is true and selection may have ancestor limit,
    *                 should set result of GetFrameToPageSelect().
+   * @param aSelectionIntoView
+   *                 If IfChanged, this makes selection into view only when
+   *                 selection is modified by the call.
+   *                 If Yes, this makes selection into view always.
    */
-  MOZ_CAN_RUN_SCRIPT
-  void CommonPageMove(bool aForward, bool aExtend, nsIFrame* aFrame);
+  enum class SelectionIntoView { IfChanged, Yes };
+  MOZ_CAN_RUN_SCRIPT nsresult PageMove(bool aForward, bool aExtend,
+                                       nsIFrame* aFrame,
+                                       SelectionIntoView aSelectionIntoView);
 
   void SetHint(CaretAssociateHint aHintRight) { mHint = aHintRight; }
   CaretAssociateHint GetHint() const { return mHint; }