1946001 - `Selection::Extend` may cross anonymous subtree root boundary

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

With the patch for bug 1945711 and adding assertion, I got this assertion failure.

[Child 5458, Main Thread] WARNING: The caller cannot compare the position of the child of the common ancestor due to not in the child list of the common ancestor:
  textarea.div['content'].body.html.#document
    + div.textarea.div['content'].body.html.#document
: file /builds/worker/checkouts/gecko/dom/base/nsContentUtils.cpp:880
#01: nsContentUtils::ComparePointsWithIndices(nsINode const*, unsigned int, nsINode const*, unsigned int, nsContentUtils::ResizableNodeIndexCache<(unsigned long)100>*) [dom/base/nsContentUtils.cpp:3408]
#02: mozilla::RangeUtils::CompareNodeToRangeBoundaries<nsCOMPtr<nsINode>, nsCOMPtr<nsIContent>, nsCOMPtr<nsINode>, nsCOMPtr<nsIContent> >(nsINode*, mozilla::RangeBoundaryBase<nsCOMPtr<nsINode>, nsCOMPtr<nsIContent> > const&, mozilla::RangeBoundaryBase<nsCOMPtr<nsINode>, nsCOMPtr<nsIContent> > const&, bool*, bool*) [dom/base/RangeUtils.cpp:220]
#03: mozilla::RangeUtils::IsNodeContainedInRange(nsINode&, mozilla::dom::AbstractRange*) [dom/base/RangeUtils.cpp:127]
#04: mozilla::ContentSubtreeIterator::GetTopAncestorInRange(nsINode*) const [dom/base/ContentIterator.cpp:1255]
#05: mozilla::ContentSubtreeIterator::DetermineFirstContent() const [dom/base/ContentIterator.cpp:0]
#06: mozilla::ContentSubtreeIterator::InitWithRange() [dom/base/ContentIterator.cpp:1093]
#07: mozilla::ContentSubtreeIterator::InitWithAllowCrossShadowBoundary(mozilla::dom::AbstractRange*) [dom/base/ContentIterator.cpp:0]
#08: mozilla::dom::Selection::SelectFrames(nsPresContext*, mozilla::dom::AbstractRange&, bool) const [dom/base/Selection.cpp:1942]
#09: mozilla::dom::Selection::Extend(nsINode&, unsigned int, mozilla::ErrorResult&) [dom/base/Selection.cpp:0]
#10: mozilla::dom::Selection::Extend(nsINode*, unsigned int) [dom/base/Selection.cpp:3005]
#11: nsFrameSelection::TakeFocus(nsIContent&, unsigned int, unsigned int, mozilla::CaretAssociationHint, nsFrameSelection::FocusMode) [layout/generic/nsFrameSelection.cpp:1524]
#12: nsFrameSelection::HandleClick(nsIContent*, unsigned int, unsigned int, nsFrameSelection::FocusMode, mozilla::CaretAssociationHint) [layout/generic/nsFrameSelection.cpp:1267]
#13: nsIFrame::PeekBackwardAndForward(nsSelectionAmount, nsSelectionAmount, int, bool, unsigned int) [layout/generic/nsIFrame.cpp:5168]
#14: nsIFrame::SelectByTypeAtPoint(nsPresContext*, nsPoint const&, nsSelectionAmount, nsSelectionAmount, unsigned int) [layout/generic/nsIFrame.cpp:0]
#15: nsIFrame::HandleMultiplePress(nsPresContext*, mozilla::WidgetGUIEvent*, nsEventStatus*, bool) [layout/generic/nsIFrame.cpp:5094]
#16: nsIFrame::MoveCaretToEventPoint(nsPresContext*, mozilla::WidgetMouseEvent*, nsEventStatus*) [layout/generic/nsIFrame.cpp:4784]
#17: nsIFrame::HandleEvent(nsPresContext*, mozilla::WidgetGUIEvent*, nsEventStatus*) [layout/generic/nsIFrame.cpp:4434]

This may be related to bug 1943225. The root cause must be that nsIFrame::PeekBackwardAndForward crosses the anonymous subtree root element boundary, but Selection::Extend may allow to extend the range as crossing the boundary.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

I saw this failure with running layout/generic/test/test_bug1655135.html.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: Bug 1946001 - Fix some points which may cross independent selection boundary r=emilio!,jjaschke!

This patch fixes multiple points which may cross independent selection
boundaries which is element boundary of the native anonymous <div> for
editable content in text controls. The reason why I don't split this patch is,
fixing one of them leads another assertion failure. So, I couldn't pass all
tests with separated patches.

1. nsFrameTraversal should take Element instead of nsIFrame for the
   limiter because e.g., inline editing host like <span contenteditable> may
   have multiple frames if it's wrapped. Therefore, we should make it take
   an element as the ancestor limiter, and check it when getting parent frame
   or after getting previous or next frame.
2. nsIFrame::PeekBackwardAndForward may cross the boundary because it does not
   take the anonymous <div> element as ancestor limiter of the selection. It's
   currently used only for extending selection. Therefore, I rename it to make
   it clearer.
3. SelectionMovementUtils::GetPrevNextBidiLevel to take the ancestor limiter
   for calling nsIFrame::GetFrameFromDirection().
4. nsINode should have a method to return the nsFrameSelection instance
   which manages the selection in the node. This makes the check simpler, and
   this is not expensive. Then, for making it clearer, I rename
   TextControlElement::GetConstFrameSelection() to
   GetIndependentFrameSelection().
5. nsINode::GetSelectionRootContent() should have an option to return
   independent selection root when the node is its host for
   nsFrameSelection::ConstrainFrameAndPointToAnchorSubtree()
6. nsFrameSelection::ConstrainFrameAndPointToAnchorSubtree() should not
   retrieve independent selection root when the given frame is a text control
   frame.
7. RangeUtils should get parent with checking the independent selection
   boundaries.
8. Selection::Extend should assert if the destination is managed by its
   frame selection to detect a bug.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: Bug 1946001 - Rename methods which return an independent `nsFrameSelection` to `GetIndependentFrameSelection()` r=emilio!

The methods are now, GetConstFrameSelection() etc, but does not return
const nsFrameSelection*. I believe that GetIndependentFrameSelection() is
clearer than the old names.

And also this patch adds IsIndependentSelection() and
GetIndependentSelectionRootParentElement() to make some callers of
GetLimiter() easier to read.

For the consistency, GetLimiter() should be renamed too, but I already have
a patch doing it in bug 1954020 and it's bigger. Therefore, I leave it as-is
for now.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/30cfc2e67ec3
Rename methods which return an independent `nsFrameSelection` to `GetIndependentFrameSelection()` r=emilio
https://hg.mozilla.org/integration/autoland/rev/56e690d6db67
Fix some points which may cross independent selection boundary r=emilio,jjaschke

Comment 5

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/30cfc2e67ec3
https://hg.mozilla.org/mozilla-central/rev/56e690d6db67