# HG changeset patch # User Masayuki Nakano # Date 1539837773 0 # Node ID 02a07fe8780872236c65ddc30470209bef95b71b # Parent 1154c7b853c63620679307f47b41ea0c1fbf614e Bug 1482425 - PresShell::PageMove() should use different rules to look for a container element for aExtend value r=smaug PresShell::PageMove() climbs up to parent document when there is no scrollable parent in current document. However, if aExtend is true, it should expand Selection in the document itself. Therefore, it needs different rules to look for container of expanding Selection from scrollable element to scroll. Additionally, old rules (i.e., before the fix of bug 1369072 which caused this regression) were also buggy. It used parent scrollable element or root scrollable element simply. Therefore, if found scrollable element is ancestor of selection limiter, it didn't work as expected. This patch creates nsFrameSelection::GetFrameToPageSelect() to retrieve per-page selection container element with the following rules: - look for a scrollable element in selection limiter. - if there is no scrollable element, use selection limiter. - if there is no selection limiter, use the root frame. So, nsFrameSelection::CommonPageMove() should take nsIFrame rather than nsIScrollableFrame since container of per-page selection may be used in non-scrollable contenteditable element. If it's called with non-scrollable frame, it needs to compute the expanding range with the frame size. Differential Revision: https://phabricator.services.mozilla.com/D8954 diff --git a/dom/html/nsTextEditorState.cpp b/dom/html/nsTextEditorState.cpp --- a/dom/html/nsTextEditorState.cpp +++ b/dom/html/nsTextEditorState.cpp @@ -306,16 +306,17 @@ public: NS_IMETHOD PhysicalMove(int16_t aDirection, int16_t aAmount, bool aExtend) override; NS_IMETHOD CharacterMove(bool aForward, bool aExtend) override; NS_IMETHOD CharacterExtendForDelete() override; NS_IMETHOD CharacterExtendForBackspace() override; NS_IMETHOD WordMove(bool aForward, bool aExtend) override; NS_IMETHOD WordExtendForDelete(bool aForward) override; NS_IMETHOD LineMove(bool aForward, bool aExtend) override; NS_IMETHOD IntraLineMove(bool aForward, bool aExtend) override; + MOZ_CAN_RUN_SCRIPT NS_IMETHOD PageMove(bool aForward, bool aExtend) override; NS_IMETHOD CompleteScroll(bool aForward) override; NS_IMETHOD CompleteMove(bool aForward, bool aExtend) override; NS_IMETHOD ScrollPage(bool aForward) override; NS_IMETHOD ScrollLine(bool aForward) override; NS_IMETHOD ScrollCharacter(bool aRight) override; NS_IMETHOD SelectAll(void) override; NS_IMETHOD CheckVisibility(nsIDOMNode *node, int16_t startOffset, int16_t EndOffset, bool* _retval) override; @@ -642,20 +643,20 @@ nsTextInputSelectionImpl::IntraLineMove( } NS_IMETHODIMP nsTextInputSelectionImpl::PageMove(bool aForward, bool aExtend) { // expected behavior for PageMove is to scroll AND move the caret // and to remain relative position of the caret in view. see Bug 4302. - if (mScrollFrame) - { + if (mScrollFrame) { RefPtr frameSelection = mFrameSelection; - frameSelection->CommonPageMove(aForward, aExtend, mScrollFrame); + nsIFrame* scrollFrame = do_QueryFrame(mScrollFrame); + frameSelection->CommonPageMove(aForward, aExtend, scrollFrame); } // 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); } diff --git a/layout/base/PresShell.cpp b/layout/base/PresShell.cpp --- a/layout/base/PresShell.cpp +++ b/layout/base/PresShell.cpp @@ -2251,23 +2251,27 @@ PresShell::IntraLineMove(bool aForward, return frameSelection->IntraLineMove(aForward, aExtend); } NS_IMETHODIMP PresShell::PageMove(bool aForward, bool aExtend) { - nsIScrollableFrame *scrollableFrame = - GetScrollableFrameToScroll(nsIPresShell::eVertical); - if (!scrollableFrame) + nsIFrame* frame; + if (!aExtend) { + frame = do_QueryFrame(GetScrollableFrameToScroll(nsIPresShell::eVertical)); + } else { + frame = mSelection->GetFrameToPageSelect(); + } + if (!frame) { return NS_OK; - + } RefPtr frameSelection = mSelection; - frameSelection->CommonPageMove(aForward, aExtend, scrollableFrame); + 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); } diff --git a/layout/base/PresShell.h b/layout/base/PresShell.h --- a/layout/base/PresShell.h +++ b/layout/base/PresShell.h @@ -264,16 +264,17 @@ public: NS_IMETHOD PhysicalMove(int16_t aDirection, int16_t aAmount, bool aExtend) override; NS_IMETHOD CharacterMove(bool aForward, bool aExtend) override; NS_IMETHOD CharacterExtendForDelete() override; NS_IMETHOD CharacterExtendForBackspace() override; NS_IMETHOD WordMove(bool aForward, bool aExtend) override; NS_IMETHOD WordExtendForDelete(bool aForward) override; NS_IMETHOD LineMove(bool aForward, bool aExtend) override; NS_IMETHOD IntraLineMove(bool aForward, bool aExtend) override; + MOZ_CAN_RUN_SCRIPT NS_IMETHOD PageMove(bool aForward, bool aExtend) override; NS_IMETHOD ScrollPage(bool aForward) override; NS_IMETHOD ScrollLine(bool aForward) override; NS_IMETHOD ScrollCharacter(bool aRight) override; NS_IMETHOD CompleteScroll(bool aForward) override; NS_IMETHOD CompleteMove(bool aForward, bool aExtend) override; NS_IMETHOD SelectAll() override; NS_IMETHOD CheckVisibility(nsIDOMNode *node, int16_t startOffset, int16_t EndOffset, bool *_retval) override; diff --git a/layout/base/tests/mochitest.ini b/layout/base/tests/mochitest.ini --- a/layout/base/tests/mochitest.ini +++ b/layout/base/tests/mochitest.ini @@ -144,16 +144,18 @@ support-files = bug1226904.html [test_bug1246622.html] [test_bug1278021.html] [test_emulateMedium.html] [test_event_target_iframe_oop.html] skip-if = e10s # bug 1020135, nested oop iframes not supported support-files = bug921928_event_target_iframe_apps_oop.html [test_event_target_radius.html] skip-if = toolkit == 'android' # Bug 1355836 +[test_expanding_selection_per_page.html] +support-files = window_empty_document.html [test_frame_reconstruction_for_pseudo_elements.html] [test_frame_reconstruction_for_svg_transforms.html] [test_frame_reconstruction_scroll_restore.html] [test_getBoxQuads_convertPointRectQuad.html] support-files = file_getBoxQuads_convertPointRectQuad_frame1.html file_getBoxQuads_convertPointRectQuad_frame2.html [test_getClientRects_emptytext.html] diff --git a/layout/base/tests/test_expanding_selection_per_page.html b/layout/base/tests/test_expanding_selection_per_page.html new file mode 100644 --- /dev/null +++ b/layout/base/tests/test_expanding_selection_per_page.html @@ -0,0 +1,311 @@ + + + + Test for expanding selection per page + + + + + + +
+
+
diff --git a/layout/base/tests/window_empty_document.html b/layout/base/tests/window_empty_document.html
new file mode 100644
--- /dev/null
+++ b/layout/base/tests/window_empty_document.html
@@ -0,0 +1,7 @@
+
+
+
+Empty document to test something in new window
+
+
+
diff --git a/layout/generic/nsFrameSelection.cpp b/layout/generic/nsFrameSelection.cpp
--- a/layout/generic/nsFrameSelection.cpp
+++ b/layout/generic/nsFrameSelection.cpp
@@ -1741,66 +1741,140 @@ nsFrameSelection::GetFrameForNodeOffset(
   }
 
   // find the child frame containing the offset we want
   returnFrame->GetChildFrameContainingOffset(*aReturnOffset, aHint == CARET_ASSOCIATE_AFTER,
                                              &aOffset, &returnFrame);
   return returnFrame;
 }
 
+nsIFrame*
+nsFrameSelection::GetFrameToPageSelect() const
+{
+  if (NS_WARN_IF(!mShell)) {
+    return nullptr;
+  }
+
+  nsIFrame* rootFrameToSelect;
+  if (mLimiter) {
+    rootFrameToSelect = mLimiter->GetPrimaryFrame();
+    if (NS_WARN_IF(!rootFrameToSelect)) {
+      return nullptr;
+    }
+  } else if (mAncestorLimiter) {
+    rootFrameToSelect = mAncestorLimiter->GetPrimaryFrame();
+    if (NS_WARN_IF(!rootFrameToSelect)) {
+      return nullptr;
+    }
+  } else {
+    rootFrameToSelect = mShell->GetRootScrollFrame();
+    if (NS_WARN_IF(!rootFrameToSelect)) {
+      return nullptr;
+    }
+  }
+
+  nsCOMPtr contentToSelect = mShell->GetContentForScrolling();
+  if (contentToSelect) {
+    // If there is selected content, look for nearest and vertical scrollable
+    // parent under the root frame.
+    for (nsIFrame* frame = contentToSelect->GetPrimaryFrame();
+         frame && frame != rootFrameToSelect;
+         frame = frame->GetParent()) {
+      nsIScrollableFrame* scrollableFrame = do_QueryFrame(frame);
+      if (!scrollableFrame) {
+        continue;
+      }
+      ScrollbarStyles scrollbarStyles = scrollableFrame->GetScrollbarStyles();
+      if (scrollbarStyles.mVertical == NS_STYLE_OVERFLOW_HIDDEN) {
+        continue;
+      }
+      uint32_t directions = scrollableFrame->GetPerceivedScrollingDirections();
+      if (directions & nsIScrollableFrame::VERTICAL) {
+        // If there is sub scrollable frame, let's use its page size to select.
+        return frame;
+      }
+    }
+  }
+  // Otherwise, i.e., there is no scrollable frame or only the root frame is
+  // scrollable, let's return the root frame because Shift + PageUp/PageDown
+  // should expand the selection in the root content even if it's not
+  // scrollable.
+  return rootFrameToSelect;
+}
+
 void
 nsFrameSelection::CommonPageMove(bool aForward,
                                  bool aExtend,
-                                 nsIScrollableFrame* aScrollableFrame)
+                                 nsIFrame* aFrame)
 {
+  MOZ_ASSERT(aFrame);
+
   // expected behavior for PageMove is to scroll AND move the caret
   // and remain relative position of the caret in view. see Bug 4302.
 
-  //get the frame from the scrollable view
-
-  nsIFrame* scrolledFrame = aScrollableFrame->GetScrolledFrame();
-  if (!scrolledFrame)
+  // Get the scrollable frame.  If aFrame is not scrollable, this is nullptr.
+  nsIScrollableFrame* scrollableFrame = aFrame->GetScrollTargetFrame();
+  // Get the scrolled frame.  If aFrame is not scrollable, this is aFrame
+  // itself.
+  nsIFrame* scrolledFrame =
+    scrollableFrame ? scrollableFrame->GetScrolledFrame() : aFrame;
+  if (!scrolledFrame) {
     return;
+  }
 
   // find out where the caret is.
-  // we should know mDesiredPos value of nsFrameSelection, but I havent seen that behavior in other windows applications yet.
+  // we should know mDesiredPos value of nsFrameSelection, but I havent seen
+  // that behavior in other windows applications yet.
   nsISelection* domSel = GetSelection(SelectionType::eNormal);
   if (!domSel) {
     return;
   }
 
   nsRect caretPos;
   nsIFrame* caretFrame = nsCaret::GetGeometry(domSel, &caretPos);
-  if (!caretFrame)
+  if (!caretFrame) {
     return;
-
-  //need to adjust caret jump by percentage scroll
-  nsSize scrollDelta = aScrollableFrame->GetPageScrollAmount();
-
-  if (aForward)
-    caretPos.y += scrollDelta.height;
-  else
-    caretPos.y -= scrollDelta.height;
+  }
+
+  if (scrollableFrame) {
+    // 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;
+    } else {
+      caretPos.y -= scrolledFrame->GetSize().height;
+    }
+  }
 
   caretPos += caretFrame->GetOffsetTo(scrolledFrame);
 
   // get a content at desired location
   nsPoint desiredPoint;
   desiredPoint.x = caretPos.x;
-  desiredPoint.y = caretPos.y + caretPos.height/2;
+  desiredPoint.y = caretPos.y + caretPos.height / 2;
   nsIFrame::ContentOffsets offsets =
       scrolledFrame->GetContentOffsetsFromPoint(desiredPoint);
 
-  if (!offsets.content)
+  if (!offsets.content) {
     return;
-
-  // scroll one page
-  aScrollableFrame->ScrollBy(nsIntPoint(0, aForward ? 1 : -1),
-                             nsIScrollableFrame::PAGES,
-                             nsIScrollableFrame::SMOOTH);
+  }
+
+  // Scroll one page if necessary.
+  if (scrollableFrame) {
+    scrollableFrame->ScrollBy(nsIntPoint(0, aForward ? 1 : -1),
+                               nsIScrollableFrame::PAGES,
+                               nsIScrollableFrame::SMOOTH);
+  }
 
   // place the caret
   HandleClick(offsets.content, offsets.offset,
               offsets.offset, aExtend, false, CARET_ASSOCIATE_AFTER);
 }
 
 nsresult
 nsFrameSelection::PhysicalMove(int16_t aDirection, int16_t aAmount,
diff --git a/layout/generic/nsFrameSelection.h b/layout/generic/nsFrameSelection.h
--- a/layout/generic/nsFrameSelection.h
+++ b/layout/generic/nsFrameSelection.h
@@ -398,31 +398,41 @@ public:
    * @param aReturnOffset will contain offset into frame.
    */
   nsIFrame* GetFrameForNodeOffset(nsIContent*        aNode,
                                   int32_t            aOffset,
                                   CaretAssociateHint aHint,
                                   int32_t*           aReturnOffset) const;
 
   /**
+   * GetFrameToPageSelect() returns a frame which is ancestor limit of
+   * per-page selection.  The frame may not be scrollable.  E.g.,
+   * when selection ancestor limit is set to a frame of an editing host of
+   * contenteditable element and it's not scrollable.
+   */
+  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.
    *
    * @param aForward if true, scroll forward if not scroll backward
    * @param aExtend  if true, extend selection to the new point
-   * @param aScrollableFrame the frame to scroll
+   * @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().
    */
-  /*unsafe*/
+  MOZ_CAN_RUN_SCRIPT
   void CommonPageMove(bool aForward,
                       bool aExtend,
-                      nsIScrollableFrame* aScrollableFrame);
+                      nsIFrame* aFrame);
 
   void SetHint(CaretAssociateHint aHintRight) { mHint = aHintRight; }
   CaretAssociateHint GetHint() const { return mHint; }
 
   /**
    * SetCaretBidiLevel sets the caret bidi level.
    * @param aLevel the caret bidi level
    */