Bug 1375825 - part2: ContentEventHandler::ExpandToClusterBoundary() should check the return value of nsTextFrame::PeekOffsetCharacter() r?jfkthame
ContentEventHandler::ExpandToClusterBoundary() doesn't check the return value of nsTextFrame::PeekOffsetCharacter(). Therefore, it may set its result to reversed offset. (e.g., when aForward is true and offset is 6, the result may be 5. When aForward is false and offset is 5, the result may be 6.)
For avoiding that, ContentEventHandler::ExpandToClusterBoundary() should check the result and only when it returns nsIFrame::FOUND, it should compute the proper offset.
On the other hand, it's too bad for ContentEventHandler that nsTextFrame::PeekOffsetCharacter() to return nsIFrame::CONTINUE_UNSELECTABLE when the user-select style is "all" because IME doesn't expect such cases.
Therefore, this patch adds additional argument to nsIFrame::PeekOffsetCharacter(), aOptions which is a struct containing bool members. The reason why it's not a bit mask enum is, such struct doesn't cause simple mistake at checking the value and the code is shorter. When mIgnoreUserStyleAll of it is true, this patch makes nsTextFrame not return nsIFrame::CONTINUE_UNSELECTABLE.
MozReview-Commit-ID: ACNNBTP92YZ
--- a/dom/events/ContentEventHandler.cpp
+++ b/dom/events/ContentEventHandler.cpp
@@ -934,41 +934,51 @@ ContentEventHandler::ExpandToClusterBoun
"offset is out of range.");
RefPtr<nsFrameSelection> fs = mPresShell->FrameSelection();
int32_t offsetInFrame;
CaretAssociationHint hint =
aForward ? CARET_ASSOCIATE_BEFORE : CARET_ASSOCIATE_AFTER;
nsIFrame* frame = fs->GetFrameForNodeOffset(aContent, int32_t(*aXPOffset),
hint, &offsetInFrame);
- if (!frame) {
- // This content doesn't have any frames, we only can check surrogate pair...
- const nsTextFragment* text = aContent->GetText();
- NS_ENSURE_TRUE(text, NS_ERROR_FAILURE);
- if (NS_IS_LOW_SURROGATE(text->CharAt(*aXPOffset)) &&
- NS_IS_HIGH_SURROGATE(text->CharAt(*aXPOffset - 1))) {
- *aXPOffset += aForward ? 1 : -1;
+ if (frame) {
+ int32_t startOffset, endOffset;
+ nsresult rv = frame->GetOffsets(startOffset, endOffset);
+ NS_ENSURE_SUCCESS(rv, rv);
+ if (*aXPOffset == static_cast<uint32_t>(startOffset) ||
+ *aXPOffset == static_cast<uint32_t>(endOffset)) {
+ return NS_OK;
+ }
+ if (!frame->IsTextFrame()) {
+ return NS_ERROR_FAILURE;
}
- return NS_OK;
+ nsTextFrame* textFrame = static_cast<nsTextFrame*>(frame);
+ int32_t newOffsetInFrame = *aXPOffset - startOffset;
+ newOffsetInFrame += aForward ? -1 : 1;
+ // PeekOffsetCharacter() should respect cluster but ignore user-select
+ // style. If it returns "FOUND", we should use the result. Otherwise,
+ // we shouldn't use the result because the offset was moved to reversed
+ // direction.
+ nsTextFrame::PeekOffsetCharacterOptions options;
+ options.mRespectClusters = true;
+ options.mIgnoreUserStyleAll = true;
+ if (textFrame->PeekOffsetCharacter(aForward, &newOffsetInFrame,
+ options) == nsIFrame::FOUND) {
+ *aXPOffset = startOffset + newOffsetInFrame;
+ return NS_OK;
+ }
}
- int32_t startOffset, endOffset;
- nsresult rv = frame->GetOffsets(startOffset, endOffset);
- NS_ENSURE_SUCCESS(rv, rv);
- if (*aXPOffset == static_cast<uint32_t>(startOffset) ||
- *aXPOffset == static_cast<uint32_t>(endOffset)) {
- return NS_OK;
+
+ // If the frame isn't available, we only can check surrogate pair...
+ const nsTextFragment* text = aContent->GetText();
+ NS_ENSURE_TRUE(text, NS_ERROR_FAILURE);
+ if (NS_IS_LOW_SURROGATE(text->CharAt(*aXPOffset)) &&
+ NS_IS_HIGH_SURROGATE(text->CharAt(*aXPOffset - 1))) {
+ *aXPOffset += aForward ? 1 : -1;
}
- if (!frame->IsTextFrame()) {
- return NS_ERROR_FAILURE;
- }
- nsTextFrame* textFrame = static_cast<nsTextFrame*>(frame);
- int32_t newOffsetInFrame = *aXPOffset - startOffset;
- newOffsetInFrame += aForward ? -1 : 1;
- textFrame->PeekOffsetCharacter(aForward, &newOffsetInFrame);
- *aXPOffset = startOffset + newOffsetInFrame;
return NS_OK;
}
nsresult
ContentEventHandler::SetRangeFromFlatTextOffset(nsRange* aRange,
uint32_t aOffset,
uint32_t aLength,
LineBreakType aLineBreakType,
--- a/layout/generic/BRFrame.cpp
+++ b/layout/generic/BRFrame.cpp
@@ -29,18 +29,20 @@ class BRFrame final : public nsFrame
public:
NS_DECL_FRAMEARENA_HELPERS(BRFrame)
friend nsIFrame* ::NS_NewBRFrame(nsIPresShell* aPresShell, nsStyleContext* aContext);
virtual ContentOffsets CalcContentOffsetsFromFramePoint(nsPoint aPoint) override;
virtual FrameSearchResult PeekOffsetNoAmount(bool aForward, int32_t* aOffset) override;
- virtual FrameSearchResult PeekOffsetCharacter(bool aForward, int32_t* aOffset,
- bool aRespectClusters = true) override;
+ virtual FrameSearchResult
+ PeekOffsetCharacter(bool aForward, int32_t* aOffset,
+ PeekOffsetCharacterOptions aOptions =
+ PeekOffsetCharacterOptions()) override;
virtual FrameSearchResult PeekOffsetWord(bool aForward, bool aWordSelectEatSpace,
bool aIsKeyboardSelect, int32_t* aOffset,
PeekWordState* aState) override;
virtual void Reflow(nsPresContext* aPresContext,
ReflowOutput& aDesiredSize,
const ReflowInput& aReflowInput,
nsReflowStatus& aStatus) override;
@@ -239,17 +241,17 @@ BRFrame::PeekOffsetNoAmount(bool aForwar
return FOUND;
}
// Otherwise, stop if we hit the beginning, continue (forward) if we hit the end.
return (startOffset == 0) ? FOUND : CONTINUE;
}
nsIFrame::FrameSearchResult
BRFrame::PeekOffsetCharacter(bool aForward, int32_t* aOffset,
- bool aRespectClusters)
+ PeekOffsetCharacterOptions aOptions)
{
NS_ASSERTION (aOffset && *aOffset <= 1, "aOffset out of range");
// Keep going. The actual line jumping will stop us.
return CONTINUE;
}
nsIFrame::FrameSearchResult
BRFrame::PeekOffsetWord(bool aForward, bool aWordSelectEatSpace, bool aIsKeyboardSelect,
--- a/layout/generic/nsAtomicContainerFrame.h
+++ b/layout/generic/nsAtomicContainerFrame.h
@@ -23,20 +23,22 @@ public:
NS_DECL_ABSTRACT_FRAME(nsAtomicContainerFrame)
// Bypass the nsContainerFrame/nsSplittableFrame impl of the following
// methods so we behave like a leaf frame.
FrameSearchResult PeekOffsetNoAmount(bool aForward, int32_t* aOffset) override
{
return nsFrame::PeekOffsetNoAmount(aForward, aOffset);
}
- FrameSearchResult PeekOffsetCharacter(bool aForward, int32_t* aOffset,
- bool aRespectClusters = true) override
+ FrameSearchResult
+ PeekOffsetCharacter(bool aForward, int32_t* aOffset,
+ PeekOffsetCharacterOptions aOptions =
+ PeekOffsetCharacterOptions()) override
{
- return nsFrame::PeekOffsetCharacter(aForward, aOffset, aRespectClusters);
+ return nsFrame::PeekOffsetCharacter(aForward, aOffset, aOptions);
}
nsSplittableType GetSplittableType() const override
{
return nsFrame::GetSplittableType();
}
protected:
nsAtomicContainerFrame(nsStyleContext* aContext, ClassID aID)
--- a/layout/generic/nsContainerFrame.cpp
+++ b/layout/generic/nsContainerFrame.cpp
@@ -397,18 +397,19 @@ nsIFrame::FrameSearchResult
nsContainerFrame::PeekOffsetNoAmount(bool aForward, int32_t* aOffset)
{
NS_ASSERTION (aOffset && *aOffset <= 1, "aOffset out of range");
// Don't allow the caret to stay in an empty (leaf) container frame.
return CONTINUE_EMPTY;
}
nsIFrame::FrameSearchResult
-nsContainerFrame::PeekOffsetCharacter(bool aForward, int32_t* aOffset,
- bool aRespectClusters)
+nsContainerFrame::PeekOffsetCharacter(
+ bool aForward, int32_t* aOffset,
+ PeekOffsetCharacterOptions aOptions)
{
NS_ASSERTION (aOffset && *aOffset <= 1, "aOffset out of range");
// Don't allow the caret to stay in an empty (leaf) container frame.
return CONTINUE_EMPTY;
}
/////////////////////////////////////////////////////////////////////////////
// Helper member functions
--- a/layout/generic/nsContainerFrame.h
+++ b/layout/generic/nsContainerFrame.h
@@ -58,18 +58,20 @@ public:
}
virtual const nsFrameList& GetChildList(ChildListID aList) const override;
virtual void GetChildLists(nsTArray<ChildList>* aLists) const override;
virtual void DestroyFrom(nsIFrame* aDestructRoot) override;
virtual void ChildIsDirty(nsIFrame* aChild) override;
virtual FrameSearchResult PeekOffsetNoAmount(bool aForward, int32_t* aOffset) override;
- virtual FrameSearchResult PeekOffsetCharacter(bool aForward, int32_t* aOffset,
- bool aRespectClusters = true) override;
+ virtual FrameSearchResult
+ PeekOffsetCharacter(bool aForward, int32_t* aOffset,
+ PeekOffsetCharacterOptions aOptions =
+ PeekOffsetCharacterOptions()) override;
virtual nsresult AttributeChanged(int32_t aNameSpaceID,
nsIAtom* aAttribute,
int32_t aModType) override;
#ifdef DEBUG_FRAME_DUMP
void List(FILE* out = stderr, const char* aPrefix = "", uint32_t aFlags = 0) const override;
#endif
--- a/layout/generic/nsFrame.cpp
+++ b/layout/generic/nsFrame.cpp
@@ -8020,21 +8020,24 @@ nsIFrame::PeekOffset(nsPeekOffsetStruct*
nsIFrame::FrameSearchResult peekSearchState = CONTINUE;
bool jumpedLine = false;
bool movedOverNonSelectableText = false;
while (peekSearchState != FOUND) {
bool movingInFrameDirection =
IsMovingInFrameDirection(current, aPos->mDirection, aPos->mVisual);
- if (eatingNonRenderableWS)
+ if (eatingNonRenderableWS) {
peekSearchState = current->PeekOffsetNoAmount(movingInFrameDirection, &offset);
- else
- peekSearchState = current->PeekOffsetCharacter(movingInFrameDirection, &offset,
- aPos->mAmount == eSelectCluster);
+ } else {
+ PeekOffsetCharacterOptions options;
+ options.mRespectClusters = aPos->mAmount == eSelectCluster;
+ peekSearchState = current->PeekOffsetCharacter(movingInFrameDirection,
+ &offset, options);
+ }
movedOverNonSelectableText |= (peekSearchState == CONTINUE_UNSELECTABLE);
if (peekSearchState != FOUND) {
bool movedOverNonSelectable = false;
result =
current->GetFrameFromDirection(aPos->mDirection, aPos->mVisual,
aPos->mJumpLines, aPos->mScrollViewStop,
@@ -8358,17 +8361,17 @@ nsFrame::PeekOffsetNoAmount(bool aForwar
{
NS_ASSERTION (aOffset && *aOffset <= 1, "aOffset out of range");
// Sure, we can stop right here.
return FOUND;
}
nsIFrame::FrameSearchResult
nsFrame::PeekOffsetCharacter(bool aForward, int32_t* aOffset,
- bool aRespectClusters)
+ PeekOffsetCharacterOptions aOptions)
{
NS_ASSERTION (aOffset && *aOffset <= 1, "aOffset out of range");
int32_t startOffset = *aOffset;
// A negative offset means "end of frame", which in our case means offset 1.
if (startOffset < 0)
startOffset = 1;
if (aForward == (startOffset == 0)) {
// We're before the frame and moving forward, or after it and moving backwards:
--- a/layout/generic/nsFrame.h
+++ b/layout/generic/nsFrame.h
@@ -206,18 +206,20 @@ public:
nsIFrame* GetNextInFlowVirtual() const override;
void SetNextInFlow(nsIFrame*) override;
nsresult GetSelectionController(nsPresContext *aPresContext,
nsISelectionController **aSelCon) override;
FrameSearchResult PeekOffsetNoAmount(bool aForward,
int32_t* aOffset) override;
- FrameSearchResult PeekOffsetCharacter(bool aForward, int32_t* aOffset,
- bool aRespectClusters = true) override;
+ FrameSearchResult
+ PeekOffsetCharacter(bool aForward, int32_t* aOffset,
+ PeekOffsetCharacterOptions aOptions =
+ PeekOffsetCharacterOptions()) override;
FrameSearchResult PeekOffsetWord(bool aForward, bool aWordSelectEatSpace,
bool aIsKeyboardSelect,
int32_t* aOffset,
PeekWordState *aState) override;
/**
* Check whether we should break at a boundary between punctuation and
* non-punctuation. Only call it at a punctuation boundary
* (i.e. exactly one of the previous and next characters are punctuation).
--- a/layout/generic/nsIFrame.h
+++ b/layout/generic/nsIFrame.h
@@ -662,16 +662,36 @@ public:
// try next frame for offset.
CONTINUE = 0x1,
// offset not found because the frame was empty of text.
CONTINUE_EMPTY = 0x2 | CONTINUE,
// offset not found because the frame didn't contain any text that could be selected.
CONTINUE_UNSELECTABLE = 0x4 | CONTINUE,
};
+ /**
+ * Options for PeekOffsetCharacter().
+ */
+ struct MOZ_STACK_CLASS PeekOffsetCharacterOptions
+ {
+ // Whether to restrict result to valid cursor locations (between grapheme
+ // clusters) - if this is included, maintains "normal" behavior, otherwise,
+ // used for selection by "code unit" (instead of "character")
+ bool mRespectClusters;
+ // Whether to check user-select style value - if this is included, checks
+ // if user-select is all, then, it may return CONTINUE_UNSELECTABLE.
+ bool mIgnoreUserStyleAll;
+
+ PeekOffsetCharacterOptions()
+ : mRespectClusters(true)
+ , mIgnoreUserStyleAll(false)
+ {
+ }
+ };
+
protected:
/**
* Return true if the frame is part of a Selection.
* Helper method to implement the public IsSelected() API.
*/
virtual bool IsFrameSelected() const;
/**
@@ -4112,26 +4132,29 @@ protected:
*/
virtual FrameSearchResult PeekOffsetNoAmount(bool aForward, int32_t* aOffset) = 0;
/**
* Search the frame for the next character
* @param aForward [in] Are we moving forward (or backward) in content order.
* @param aOffset [in/out] At what offset into the frame to start looking.
* on output - what offset was reached (whether or not we found a place to stop).
- * @param aRespectClusters [in] Whether to restrict result to valid cursor locations
- * (between grapheme clusters) - default TRUE maintains "normal" behavior,
- * FALSE is used for selection by "code unit" (instead of "character")
+ * @param aOptions [in] Options, see the comment in
+ * PeekOffsetCharacterOptions for the detail.
* @return STOP: An appropriate offset was found within this frame,
* and is given by aOffset.
* CONTINUE: Not found within this frame, need to try the next frame.
* see enum FrameSearchResult for more details.
*/
- virtual FrameSearchResult PeekOffsetCharacter(bool aForward, int32_t* aOffset,
- bool aRespectClusters = true) = 0;
+ virtual FrameSearchResult
+ PeekOffsetCharacter(bool aForward, int32_t* aOffset,
+ PeekOffsetCharacterOptions aOptions =
+ PeekOffsetCharacterOptions()) = 0;
+ static_assert(sizeof(PeekOffsetCharacterOptions) <= sizeof(intptr_t),
+ "aOptions should be changed to const reference");
/**
* Search the frame for the next word boundary
* @param aForward [in] Are we moving forward (or backward) in content order.
* @param aWordSelectEatSpace [in] true: look for non-whitespace following
* whitespace (in the direction of movement).
* false: look for whitespace following non-whitespace (in the
* direction of movement).
--- a/layout/generic/nsInlineFrame.cpp
+++ b/layout/generic/nsInlineFrame.cpp
@@ -165,17 +165,17 @@ nsInlineFrame::IsEmpty()
return false;
}
return true;
}
nsIFrame::FrameSearchResult
nsInlineFrame::PeekOffsetCharacter(bool aForward, int32_t* aOffset,
- bool aRespectClusters)
+ PeekOffsetCharacterOptions aOptions)
{
// Override the implementation in nsFrame, to skip empty inline frames
NS_ASSERTION (aOffset && *aOffset <= 1, "aOffset out of range");
int32_t startOffset = *aOffset;
if (startOffset < 0)
startOffset = 1;
if (aForward == (startOffset == 0)) {
// We're before the frame and moving forward, or after it and moving backwards:
--- a/layout/generic/nsInlineFrame.h
+++ b/layout/generic/nsInlineFrame.h
@@ -51,18 +51,20 @@ public:
}
virtual void InvalidateFrame(uint32_t aDisplayItemKey = 0) override;
virtual void InvalidateFrameWithRect(const nsRect& aRect, uint32_t aDisplayItemKey = 0) override;
virtual bool IsEmpty() override;
virtual bool IsSelfEmpty() override;
- virtual FrameSearchResult PeekOffsetCharacter(bool aForward, int32_t* aOffset,
- bool aRespectClusters = true) override;
+ virtual FrameSearchResult
+ PeekOffsetCharacter(bool aForward, int32_t* aOffset,
+ PeekOffsetCharacterOptions aOptions =
+ PeekOffsetCharacterOptions()) override;
virtual void DestroyFrom(nsIFrame* aDestructRoot) override;
virtual nsresult StealFrame(nsIFrame* aChild) override;
// nsIHTMLReflow overrides
virtual void AddInlineMinISize(gfxContext *aRenderingContext,
InlineMinISizeData *aData) override;
virtual void AddInlinePrefISize(gfxContext *aRenderingContext,
--- a/layout/generic/nsTextFrame.cpp
+++ b/layout/generic/nsTextFrame.cpp
@@ -7979,58 +7979,63 @@ IsAcceptableCaretPosition(const gfxSkipC
}
}
}
return true;
}
nsIFrame::FrameSearchResult
nsTextFrame::PeekOffsetCharacter(bool aForward, int32_t* aOffset,
- bool aRespectClusters)
+ PeekOffsetCharacterOptions aOptions)
{
int32_t contentLength = GetContentLength();
NS_ASSERTION(aOffset && *aOffset <= contentLength, "aOffset out of range");
- StyleUserSelect selectStyle;
- IsSelectable(&selectStyle);
- if (selectStyle == StyleUserSelect::All)
- return CONTINUE_UNSELECTABLE;
+ if (!aOptions.mIgnoreUserStyleAll) {
+ StyleUserSelect selectStyle;
+ IsSelectable(&selectStyle);
+ if (selectStyle == StyleUserSelect::All) {
+ return CONTINUE_UNSELECTABLE;
+ }
+ }
gfxSkipCharsIterator iter = EnsureTextRun(nsTextFrame::eInflated);
if (!mTextRun)
return CONTINUE_EMPTY;
TrimmedOffsets trimmed = GetTrimmedOffsets(mContent->GetText(), false);
// A negative offset means "end of frame".
int32_t startOffset = GetContentOffset() + (*aOffset < 0 ? contentLength : *aOffset);
if (!aForward) {
// If at the beginning of the line, look at the previous continuation
for (int32_t i = std::min(trimmed.GetEnd(), startOffset) - 1;
i >= trimmed.mStart; --i) {
iter.SetOriginalOffset(i);
- if (IsAcceptableCaretPosition(iter, aRespectClusters, mTextRun, this)) {
+ if (IsAcceptableCaretPosition(iter, aOptions.mRespectClusters, mTextRun,
+ this)) {
*aOffset = i - mContentOffset;
return FOUND;
}
}
*aOffset = 0;
} else {
// If we're at the end of a line, look at the next continuation
iter.SetOriginalOffset(startOffset);
if (startOffset <= trimmed.GetEnd() &&
!(startOffset < trimmed.GetEnd() &&
StyleText()->NewlineIsSignificant(this) &&
iter.GetSkippedOffset() < mTextRun->GetLength() &&
mTextRun->CharIsNewline(iter.GetSkippedOffset()))) {
for (int32_t i = startOffset + 1; i <= trimmed.GetEnd(); ++i) {
iter.SetOriginalOffset(i);
if (i == trimmed.GetEnd() ||
- IsAcceptableCaretPosition(iter, aRespectClusters, mTextRun, this)) {
+ IsAcceptableCaretPosition(iter, aOptions.mRespectClusters, mTextRun,
+ this)) {
*aOffset = i - mContentOffset;
return FOUND;
}
}
}
*aOffset = contentLength;
}
--- a/layout/generic/nsTextFrame.h
+++ b/layout/generic/nsTextFrame.h
@@ -183,19 +183,21 @@ public:
*/
void SetSelectedRange(uint32_t aStart,
uint32_t aEnd,
bool aSelected,
SelectionType aSelectionType);
FrameSearchResult PeekOffsetNoAmount(bool aForward,
int32_t* aOffset) override;
- FrameSearchResult PeekOffsetCharacter(bool aForward,
- int32_t* aOffset,
- bool aRespectClusters = true) override;
+ FrameSearchResult
+ PeekOffsetCharacter(bool aForward,
+ int32_t* aOffset,
+ PeekOffsetCharacterOptions aOptions =
+ PeekOffsetCharacterOptions()) override;
FrameSearchResult PeekOffsetWord(bool aForward,
bool aWordSelectEatSpace,
bool aIsKeyboardSelect,
int32_t* aOffset,
PeekWordState* aState) override;
nsresult CheckVisibility(nsPresContext* aContext,
int32_t aStartIndex,