From 707a9a407d1c757cb1bd80d4b2d90b9f08d2ec60 Mon Sep 17 00:00:00 2001
From: Daniel Holbert <dholbert@cs.stanford.edu>
Date: Fri, 28 Oct 2022 17:56:04 +0000
Subject: [PATCH] Bug 1797148: Simplify checkVisibility API (on
nsISelectionController and nsIFrame) into a single
nsTextFrame::HasVisibleText method. r=masayuki
Before this patch, we had two `checkVisibilty` methods on the
nsISelectionController interface, backed by several layers of implementation,
ultimately backed by a single function on nsTextFrame (which didn't actually
do anything meaningful with any of the parameters).
As it turns out, this API only had one caller, in HTMLEditUtils.cpp.
This patch converts that caller to directly query nsTextFrame (if the given
node's primary frame is indeed a nsTextFrame). The direct function-call is
renamed to HasVisibleText(), to be a bit clearer about it being text-specific
and also to avoid confusion with the (unrelated) recently-specified HTML
checkVisibility() API.
With these changes, we can remove the API from the nsISelectionController
interface and its implementations.
This patch also updates the HTMLEditUtils::IsInVisibleTextFrames documentation
(with s/all/any/) to reflect the reality of what the nsTextFrame impl actually
does.
Differential Revision: https://phabricator.services.mozilla.com/D160563
---
dom/base/nsISelectionController.idl | 10 -------
dom/html/TextControlState.cpp | 37 ------------------------
editor/libeditor/HTMLEditUtils.cpp | 35 +++++++----------------
editor/libeditor/HTMLEditUtils.h | 2 +-
layout/base/PresShell.cpp | 44 -----------------------------
layout/base/PresShell.h | 4 ---
layout/generic/nsIFrame.cpp | 5 ----
layout/generic/nsIFrame.h | 22 ---------------
layout/generic/nsTextFrame.cpp | 16 ++---------
layout/generic/nsTextFrame.h | 5 ++--
10 files changed, 16 insertions(+), 164 deletions(-)
diff --git a/dom/base/nsISelectionController.idl b/dom/base/nsISelectionController.idl
index d5eaa23812afd..2bda55e7f2b4a 100644
--- a/dom/base/nsISelectionController.idl
+++ b/dom/base/nsISelectionController.idl
@@ -276,16 +276,6 @@ interface nsISelectionController : nsISelectionDisplay
* @param aRight if true will scroll right. if not will scroll left.
*/
void scrollCharacter(in boolean right);
-
- /** CheckVisibility will return true if textnode and offsets are actually rendered
- * in the current precontext.
- * @param aNode textNode to test
- * @param aStartOffset offset in dom to first char of textnode to test
- * @param aEndOffset offset in dom to last char of textnode to test
- * @param aReturnBool boolean returned TRUE if visible FALSE if not
- */
- boolean checkVisibility(in Node node, in short startOffset, in short endOffset);
- [noscript,nostdcall] boolean checkVisibilityContent(in nsIContent node, in short startOffset, in short endOffset);
};
%{ C++
#define NS_ISELECTIONCONTROLLER_CID \
diff --git a/dom/html/TextControlState.cpp b/dom/html/TextControlState.cpp
index a3aaa19283ff9..a1523f2a60207 100644
--- a/dom/html/TextControlState.cpp
+++ b/dom/html/TextControlState.cpp
@@ -376,12 +376,6 @@ class TextInputSelectionController final : public nsSupportsWeakReference,
NS_IMETHOD ScrollPage(bool aForward) override;
NS_IMETHOD ScrollLine(bool aForward) override;
NS_IMETHOD ScrollCharacter(bool aRight) override;
- NS_IMETHOD CheckVisibility(nsINode* node, int16_t startOffset,
- int16_t EndOffset, bool* _retval) override;
- virtual nsresult CheckVisibilityContent(nsIContent* aNode,
- int16_t aStartOffset,
- int16_t aEndOffset,
- bool* aRetval) override;
void SelectionWillTakeFocus() override;
void SelectionWillLoseFocus() override;
@@ -781,37 +775,6 @@ void TextInputSelectionController::SelectionWillLoseFocus() {
}
}
-NS_IMETHODIMP
-TextInputSelectionController::CheckVisibility(nsINode* node,
- int16_t startOffset,
- int16_t EndOffset,
- bool* _retval) {
- if (!mPresShellWeak) {
- return NS_ERROR_NOT_INITIALIZED;
- }
- nsresult rv;
- nsCOMPtr<nsISelectionController> presShell =
- do_QueryReferent(mPresShellWeak, &rv);
- if (!presShell) {
- return NS_ERROR_FAILURE;
- }
- return presShell->CheckVisibility(node, startOffset, EndOffset, _retval);
-}
-
-nsresult TextInputSelectionController::CheckVisibilityContent(
- nsIContent* aNode, int16_t aStartOffset, int16_t aEndOffset,
- bool* aRetval) {
- if (!mPresShellWeak) {
- return NS_ERROR_NOT_INITIALIZED;
- }
- nsCOMPtr<nsISelectionController> presShell = do_QueryReferent(mPresShellWeak);
- if (NS_WARN_IF(!presShell)) {
- return NS_ERROR_FAILURE;
- }
- return presShell->CheckVisibilityContent(aNode, aStartOffset, aEndOffset,
- aRetval);
-}
-
/*****************************************************************************
* mozilla::TextInputListener
*****************************************************************************/
diff --git a/editor/libeditor/HTMLEditUtils.cpp b/editor/libeditor/HTMLEditUtils.cpp
index 3cd6bfe082131..64a6aae27b8b7 100644
--- a/editor/libeditor/HTMLEditUtils.cpp
+++ b/editor/libeditor/HTMLEditUtils.cpp
@@ -38,6 +38,7 @@
#include "nsString.h" // for nsAutoString
#include "nsStyledElement.h"
#include "nsTextFragment.h" // for nsTextFragment
+#include "nsTextFrame.h" // for nsTextFrame
namespace mozilla {
@@ -451,39 +452,23 @@ bool HTMLEditUtils::IsVisibleTextNode(const Text& aText) {
bool HTMLEditUtils::IsInVisibleTextFrames(nsPresContext* aPresContext,
const Text& aText) {
+ // TODO(dholbert): aPresContext is now unused; maybe we can remove it, here
+ // and in IsEmptyNode? We do use it as a signal (implicitly here,
+ // more-explicitly in IsEmptyNode) that we are in a "SafeToAskLayout" case...
+ // If/when we remove it, we should be sure we're not losing that signal of
+ // strictness, since this function here does absolutely need to query layout.
MOZ_ASSERT(aPresContext);
- nsIFrame* frame = aText.GetPrimaryFrame();
- if (!frame) {
- return false;
- }
-
- nsCOMPtr<nsISelectionController> selectionController;
- nsresult rv = frame->GetSelectionController(
- aPresContext, getter_AddRefs(selectionController));
- NS_WARNING_ASSERTION(NS_SUCCEEDED(rv),
- "nsIFrame::GetSelectionController() failed");
- if (NS_FAILED(rv) || !selectionController) {
+ if (!aText.TextDataLength()) {
return false;
}
- if (!aText.TextDataLength()) {
+ nsTextFrame* textFrame = do_QueryFrame(aText.GetPrimaryFrame());
+ if (!textFrame) {
return false;
}
- // Ask the selection controller for information about whether any of the
- // data in the node is really rendered. This is really something that
- // frames know about, but we aren't supposed to talk to frames. So we put
- // a call in the selection controller interface, since it's already in bed
- // with frames anyway. (This is a fix for bug 22227, and a partial fix for
- // bug 46209.)
- bool isVisible = false;
- rv = selectionController->CheckVisibilityContent(
- const_cast<Text*>(&aText), 0, aText.TextDataLength(), &isVisible);
- NS_WARNING_ASSERTION(
- NS_SUCCEEDED(rv),
- "nsISelectionController::CheckVisibilityContent() failed");
- return NS_SUCCEEDED(rv) && isVisible;
+ return textFrame->HasVisibleText();
}
Element* HTMLEditUtils::GetElementOfImmediateBlockBoundary(
diff --git a/editor/libeditor/HTMLEditUtils.h b/editor/libeditor/HTMLEditUtils.h
index f8337d0c69f0d..1420c3e0379c5 100644
--- a/editor/libeditor/HTMLEditUtils.h
+++ b/editor/libeditor/HTMLEditUtils.h
@@ -335,7 +335,7 @@ class HTMLEditUtils final {
static bool IsVisibleTextNode(const Text& aText);
/**
- * IsInVisibleTextFrames() returns true if all text in aText is in visible
+ * IsInVisibleTextFrames() returns true if any text in aText is in visible
* text frames. Callers have to guarantee that there is no pending reflow.
*/
static bool IsInVisibleTextFrames(nsPresContext* aPresContext,
diff --git a/layout/base/PresShell.cpp b/layout/base/PresShell.cpp
index 89f9729dcefc9..b9b637b11924a 100644
--- a/layout/base/PresShell.cpp
+++ b/layout/base/PresShell.cpp
@@ -2507,50 +2507,6 @@ PresShell::CompleteMove(bool aForward, bool aExtend) {
nsISelectionController::SCROLL_FOR_CARET_MOVE);
}
-static void DoCheckVisibility(nsPresContext* aPresContext, nsIContent* aNode,
- int16_t aStartOffset, int16_t aEndOffset,
- bool* aRetval) {
- nsIFrame* frame = aNode->GetPrimaryFrame();
- if (!frame) {
- // No frame to look at so it must not be visible.
- return;
- }
-
- // Start process now to go through all frames to find startOffset. Then check
- // chars after that to see if anything until EndOffset is visible.
- bool finished = false;
- frame->CheckVisibility(aPresContext, aStartOffset, aEndOffset, true,
- &finished, aRetval);
- // Don't worry about other return value.
-}
-
-NS_IMETHODIMP
-PresShell::CheckVisibility(nsINode* node, int16_t startOffset,
- int16_t EndOffset, bool* _retval) {
- if (!node || startOffset > EndOffset || !_retval || startOffset < 0 ||
- EndOffset < 0)
- return NS_ERROR_INVALID_ARG;
- *_retval = false; // initialize return parameter
- nsCOMPtr<nsIContent> content(do_QueryInterface(node));
- if (!content) return NS_ERROR_FAILURE;
-
- DoCheckVisibility(mPresContext, content, startOffset, EndOffset, _retval);
- return NS_OK;
-}
-
-nsresult PresShell::CheckVisibilityContent(nsIContent* aNode,
- int16_t aStartOffset,
- int16_t aEndOffset, bool* aRetval) {
- if (!aNode || aStartOffset > aEndOffset || !aRetval || aStartOffset < 0 ||
- aEndOffset < 0) {
- return NS_ERROR_INVALID_ARG;
- }
-
- *aRetval = false;
- DoCheckVisibility(mPresContext, aNode, aStartOffset, aEndOffset, aRetval);
- return NS_OK;
-}
-
// end implementations nsISelectionController
nsIFrame* PresShell::GetRootScrollFrame() const {
diff --git a/layout/base/PresShell.h b/layout/base/PresShell.h
index 5c14a34d088b9..0c02d76cfb6b0 100644
--- a/layout/base/PresShell.h
+++ b/layout/base/PresShell.h
@@ -1386,10 +1386,6 @@ class PresShell final : public nsStubDocumentObserver,
NS_IMETHOD CompleteScroll(bool aForward) override;
MOZ_CAN_RUN_SCRIPT NS_IMETHOD CompleteMove(bool aForward,
bool aExtend) override;
- NS_IMETHOD CheckVisibility(nsINode* node, int16_t startOffset,
- int16_t EndOffset, bool* _retval) override;
- nsresult CheckVisibilityContent(nsIContent* aNode, int16_t aStartOffset,
- int16_t aEndOffset, bool* aRetval) override;
// Notifies that the state of the document has changed.
void DocumentStatesChanged(dom::DocumentState);
diff --git a/layout/generic/nsIFrame.cpp b/layout/generic/nsIFrame.cpp
index 2d7a9ead526f2..62185e5bb2df6 100644
--- a/layout/generic/nsIFrame.cpp
+++ b/layout/generic/nsIFrame.cpp
@@ -9210,11 +9210,6 @@ bool nsIFrame::BreakWordBetweenPunctuation(const PeekWordState* aState,
return aState->mSeenNonPunctuationSinceWhitespace;
}
-nsresult nsIFrame::CheckVisibility(nsPresContext*, int32_t, int32_t, bool,
- bool*, bool*) {
- return NS_ERROR_NOT_IMPLEMENTED;
-}
-
std::pair<nsIFrame*, nsIFrame*> nsIFrame::GetContainingBlockForLine(
bool aLockScroll) const {
const nsIFrame* parentFrame = this;
diff --git a/layout/generic/nsIFrame.h b/layout/generic/nsIFrame.h
index eb127353c9532..91d5810037d8e 100644
--- a/layout/generic/nsIFrame.h
+++ b/layout/generic/nsIFrame.h
@@ -3925,28 +3925,6 @@ class nsIFrame : public nsQueryFrame {
nsDirection aDirection);
public:
- /**
- * Called to see if the children of the frame are visible from indexstart to
- * index end. This does not change any state. Returns true only if the indexes
- * are valid and any of the children are visible. For textframes this index
- * is the character index. If aStart = aEnd result will be false.
- *
- * @param aStart start index of first child from 0-N (number of children)
- *
- * @param aEnd end index of last child from 0-N
- *
- * @param aRecurse should this frame talk to siblings to get to the contents
- * other children?
- *
- * @param aFinished did this frame have the aEndIndex? or is there more work
- * to do
- *
- * @param _retval return value true or false. false = range is not rendered.
- */
- virtual nsresult CheckVisibility(nsPresContext* aContext, int32_t aStartIndex,
- int32_t aEndIndex, bool aRecurse,
- bool* aFinished, bool* _retval);
-
/**
* Called to tell a frame that one of its child frames is dirty (i.e.,
* has the NS_FRAME_IS_DIRTY *or* NS_FRAME_HAS_DIRTY_CHILDREN bit
diff --git a/layout/generic/nsTextFrame.cpp b/layout/generic/nsTextFrame.cpp
index 91dc6f2a7acd9..66d597a4cc017 100644
--- a/layout/generic/nsTextFrame.cpp
+++ b/layout/generic/nsTextFrame.cpp
@@ -8489,27 +8489,17 @@ nsIFrame::FrameSearchResult nsTextFrame::PeekOffsetWord(
return CONTINUE;
}
-// TODO this needs to be deCOMtaminated with the interface fixed in
-// nsIFrame.h, but we won't do that until the old textframe is gone.
-nsresult nsTextFrame::CheckVisibility(nsPresContext* aContext,
- int32_t aStartIndex, int32_t aEndIndex,
- bool aRecurse, bool* aFinished,
- bool* aRetval) {
- if (!aRetval) return NS_ERROR_NULL_POINTER;
-
+bool nsTextFrame::HasVisibleText() {
// Text in the range is visible if there is at least one character in the
// range that is not skipped and is mapped by this frame (which is the primary
// frame) or one of its continuations.
for (nsTextFrame* f = this; f; f = f->GetNextContinuation()) {
int32_t dummyOffset = 0;
if (f->PeekOffsetNoAmount(true, &dummyOffset) == FOUND) {
- *aRetval = true;
- return NS_OK;
+ return true;
}
}
-
- *aRetval = false;
- return NS_OK;
+ return false;
}
std::pair<int32_t, int32_t> nsTextFrame::GetOffsets() const {
diff --git a/layout/generic/nsTextFrame.h b/layout/generic/nsTextFrame.h
index 1f69ad8c30c73..72a9b976c772a 100644
--- a/layout/generic/nsTextFrame.h
+++ b/layout/generic/nsTextFrame.h
@@ -342,9 +342,8 @@ class nsTextFrame : public nsIFrame {
PeekWordState* aState,
bool aTrimSpaces) final;
- nsresult CheckVisibility(nsPresContext* aContext, int32_t aStartIndex,
- int32_t aEndIndex, bool aRecurse, bool* aFinished,
- bool* _retval) final;
+ // Helper method that editor code uses to test for visibility.
+ [[nodiscard]] bool HasVisibleText();
// Flags for aSetLengthFlags
enum { ALLOW_FRAME_CREATION_AND_DESTRUCTION = 0x01 };
--
GitLab